# [PATCH 00/11] Coverity fixes 38 messages from 2022-06-15 to 2022-06-17. Participants: Johannes Schindelin via GitGitGadget, Junio C Hamano, Jeff King, Ævar Arnfjörð Bjarmason, Glen Choo, René Scharfe, Taylor Blau, Tom Levy. Thread: https://gitlist.dev/t/58019 ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 00/11] Coverity fixes Message-ID: URL: https://gitlist.dev/e/pull.1264.git.1655336146.gitgitgadget%40gmail.com ``` The release window is a fine time to concentrate on bug fixes, and Coverity can help identify bugs. These are only a couple of fixes, Coverity reported many more, but most of the reports seemed either bogus or harmless (such as resource leaks in test helpers). Johannes Schindelin (11): mingw: avoid accessing uninitialized memory in `is_executable()` fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()` submodule--helper: avoid memory leak in `update_submodule()` get_oid_with_context_1(): avoid use-after-free submodule-config: avoid memory leak pack-redundant: avoid using uninitialized memory submodule--helper: avoid memory leak when fetching submodules read_index_from(): avoid memory leak pack-mtimes: avoid closing a bogus file descriptor relative_url(): fix incorrect condition bug_fl(): add missing `va_end()` call builtin/pack-redundant.c | 1 + builtin/submodule--helper.c | 3 +++ fsmonitor-settings.c | 8 ++++++-- object-name.c | 6 ++++-- pack-mtimes.c | 3 ++- read-cache.c | 6 +++--- remote.c | 2 +- run-command.c | 2 +- submodule-config.c | 8 ++++---- usage.c | 1 + 10 files changed, 26 insertions(+), 14 deletions(-) base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1264 -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()` Message-ID: <679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin On Windows, we open files we suspect might be scripts, read the first two bytes, and see whether they indicate a hash-bang line. We do not initialize the byte _after_ those two bytes, therefore `strcmp()` is inappropriate here. Reported by Coverity. Signed-off-by: Johannes Schindelin --- run-command.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/run-command.c b/run-command.c index 14f17830f51..2ba38850b4d 100644 --- a/run-command.c +++ b/run-command.c @@ -154,7 +154,7 @@ int is_executable(const char *name) n = read(fd, buf, 2); if (n == 2) /* look for a she-bang */ - if (!strcmp(buf, "#!")) + if (!memcmp(buf, "#!", 2)) st.st_mode |= S_IXUSR; close(fd); } -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()` Message-ID: <6d409ad45867aa5193c15828614e865e53ac0c57.1655336146.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/6d409ad45867aa5193c15828614e865e53ac0c57.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Reported by Coverity. Signed-off-by: Johannes Schindelin --- fsmonitor-settings.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c index 658cb79da01..464424a1e92 100644 --- a/fsmonitor-settings.c +++ b/fsmonitor-settings.c @@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r, case FSMONITOR_REASON_OK: goto done; - case FSMONITOR_REASON_BARE: + case FSMONITOR_REASON_BARE: { + char *cwd = xgetcwd(); + strbuf_addf(&msg, _("bare repository '%s' is incompatible with fsmonitor"), - xgetcwd()); + cwd); + free(cwd); goto done; + } case FSMONITOR_REASON_ERROR: strbuf_addf(&msg, -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()` Message-ID: <877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin Reported by Coverity. Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 5c77dfcffee..d7b8004b933 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) next.recursive_prefix = get_submodule_displaypath(prefixed_path, update_data->prefix); + free(prefixed_path); + next.prefix = NULL; oidcpy(&next.oid, null_oid()); oidcpy(&next.suboid, null_oid()); -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 04/11] get_oid_with_context_1(): avoid use-after-free Message-ID: URL: https://gitlist.dev/e/cf6bcdb43e5b4abab464c30a914d64dc8e7a9925.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 561287d342cc (object-name: reject trees found in the index, 2022-04-26), we changed the code to release the memory stored under the `new_path` variable a bit earlier. However, at that point the variable `cp` still points to `new_path` (if non-`NULL`), and we _then_ pass that pointer to `reject_tree_in_index()`. Let's make sure that the pointer still points to allocated memory by moving the original `free()` call back where it was, and adding another one in the early return code path. Reported by Coverity. Signed-off-by: Johannes Schindelin --- object-name.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/object-name.c b/object-name.c index 4d2746574cd..228c12a554c 100644 --- a/object-name.c +++ b/object-name.c @@ -1971,12 +1971,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, memcmp(ce->name, cp, namelen)) break; if (ce_stage(ce) == stage) { - free(new_path); if (reject_tree_in_index(repo, only_to_die, ce, - stage, prefix, cp)) + stage, prefix, cp)) { + free(new_path); return -1; + } oidcpy(oid, &ce->oid); oc->mode = ce->ce_mode; + free(new_path); return 0; } pos++; -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 05/11] submodule-config: avoid memory leak Message-ID: <591166e07d87fdb5efc2769d3e2963e3f0412720.1655336146.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/591166e07d87fdb5efc2769d3e2963e3f0412720.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 961b130d20c9 (branch: add --recurse-submodules option for branch creation, 2022-01-28), a funny pattern was introduced where first some struct is `xmalloc()`ed, then we resize an array whose element type is the same struct, and then the first struct's contents are copied into the last element of that array. Crucially, the `xmalloc()`ed memory never gets released. Let's avoid that memory leak and that memory allocation dance altogether by first reallocating the array, then using a pointer to the last array element to go forward. Reported by Coverity. Signed-off-by: Johannes Schindelin --- submodule-config.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/submodule-config.c b/submodule-config.c index ce3beaf5d4f..51ecbe901ec 100644 --- a/submodule-config.c +++ b/submodule-config.c @@ -756,7 +756,10 @@ static void traverse_tree_submodules(struct repository *r, if (S_ISGITLINK(name_entry->mode) && is_tree_submodule_active(r, root_tree, tree_path)) { - st_entry = xmalloc(sizeof(*st_entry)); + ALLOC_GROW(out->entries, out->entry_nr + 1, + out->entry_alloc); + st_entry = &out->entries[out->entry_nr++]; + st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry)); *st_entry->name_entry = *name_entry; st_entry->submodule = @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r, root_tree)) FREE_AND_NULL(st_entry->repo); - ALLOC_GROW(out->entries, out->entry_nr + 1, - out->entry_alloc); - out->entries[out->entry_nr++] = *st_entry; } else if (S_ISDIR(name_entry->mode)) traverse_tree_submodules(r, root_tree, tree_path, &name_entry->oid, out); -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 06/11] pack-redundant: avoid using uninitialized memory Message-ID: URL: https://gitlist.dev/e/bc29a9710e3a22e6d660098c4f201f3bfecc54ea.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In the `add_pack()` function, we forgot to initialize the field `next`, which could potentially lead to readin uninitialized memory later. Reported by Coverity. Signed-off-by: Johannes Schindelin --- builtin/pack-redundant.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c index ed9b9013a5f..1f7da1f68b6 100644 --- a/builtin/pack-redundant.c +++ b/builtin/pack-redundant.c @@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p) } l.all_objects_size = l.remaining_objects->size; l.unique_objects = NULL; + l.next = NULL; if (p->pack_local) return pack_list_insert(&local_packs, &l); else -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules Message-ID: URL: https://gitlist.dev/e/ad3f23a9f0c2c92ac95406fcadc51eec1a9861ff.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In c51f8f94e5b3 (submodule--helper: run update procedures from C, 2021-08-24), we added code that first obtains the default remote, and then adds that to a `strvec`. However, we never released the default remote's memory. Reported by Coverity. Signed-off-by: Johannes Schindelin --- builtin/submodule--helper.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index d7b8004b933..9a8248ffe6a 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str char *hex = oid_to_hex(oid); char *remote = get_default_remote(); strvec_pushl(&cp.args, remote, hex, NULL); + free(remote); } return run_command(&cp); -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 08/11] read_index_from(): avoid memory leak Message-ID: URL: https://gitlist.dev/e/a3f79b3e034a3a982b3be7835062493bfdf29ee5.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 998330ac2e7c (read-cache: look for shared index files next to the index, too, 2021-08-26), we added code that allocates memory to store the base path of a shared index, but we never released that memory. Reported by Coverity. Signed-off-by: Johannes Schindelin --- read-cache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/read-cache.c b/read-cache.c index e61af3a3d4d..76f372ff917 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2473,15 +2473,15 @@ int read_index_from(struct index_state *istate, const char *path, the_repository, "%s", base_path); if (!ret) { char *path_copy = xstrdup(path); - const char *base_path2 = xstrfmt("%s/sharedindex.%s", - dirname(path_copy), - base_oid_hex); + char *base_path2 = xstrfmt("%s/sharedindex.%s", + dirname(path_copy), base_oid_hex); free(path_copy); trace2_region_enter_printf("index", "shared/do_read_index", the_repository, "%s", base_path2); ret = do_read_index(split_index->base, base_path2, 1); trace2_region_leave_printf("index", "shared/do_read_index", the_repository, "%s", base_path2); + free(base_path2); } if (!oideq(&split_index->base_oid, &split_index->base->oid)) die(_("broken index, expect %s in %s, got %s"), -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor Message-ID: URL: https://gitlist.dev/e/b176e5764f990fbf8c132ae1563027531bd8cc9f.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 94cd775a6c52 (pack-mtimes: support reading .mtimes files, 2022-05-20), code was added to close the file descriptor corresponding to the mtimes file. However, it is possible that opening that file failed, in which case we are closing a file descriptor with the value `-1`. Let's guard that `close()` call. Reported by Coverity. Signed-off-by: Johannes Schindelin --- pack-mtimes.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pack-mtimes.c b/pack-mtimes.c index 0e0aafdcb06..0f9785fc5e4 100644 --- a/pack-mtimes.c +++ b/pack-mtimes.c @@ -89,7 +89,8 @@ cleanup: *data_p = data; } - close(fd); + if (fd >= 0) + close(fd); return ret; } -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 10/11] relative_url(): fix incorrect condition Message-ID: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin In 63e95beb085c (submodule: port resolve_relative_url from shell to C, 2016-04-15), we added a loop over `url` where we are looking for `../` or `./` components. The loop condition we used is the pointer `url` itself, which is clearly not what we wanted. Pointed out by Coverity. Signed-off-by: Johannes Schindelin --- remote.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/remote.c b/remote.c index 9b9bbfe80ec..bded6acbfe8 100644 --- a/remote.c +++ b/remote.c @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url, * When the url starts with '../', remove that and the * last directory in remoteurl. */ - while (url) { + while (*url) { if (starts_with_dot_dot_slash_native(url)) { url += 3; colonsep |= chop_last_dir(&remoteurl, is_relative); -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-15 23:35 Subject: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/d674aefa78bdb6d255e40af2f308abf8a87a593a.1655336146.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin According to the manual: Each invocation of va_copy() must be matched by a corresponding invocation of va_end() in the same function. Note: There is another instance of `va_copy()` in `usage.c` that is missing a `va_end()` call, in `BUG_vfl()`. It does not matter there, though, because that function either `exit()`s or `abort()`s, anyway. Reported by Coverity. Signed-off-by: Johannes Schindelin --- usage.c | 1 + 1 file changed, 1 insertion(+) diff --git a/usage.c b/usage.c index 79900d0287f..4849eb75785 100644 --- a/usage.c +++ b/usage.c @@ -343,6 +343,7 @@ void bug_fl(const char *file, int line, const char *fmt, ...) BUG_vfl_common(file, line, fmt, ap); va_end(ap); trace2_cmd_error_va(fmt, cp); + va_end(cp); } #ifdef SUPPRESS_ANNOTATED_LEAKS -- gitgitgadget ``` ## Junio C Hamano, 2022-06-16 04:05 Subject: Re: [PATCH 00/11] Coverity fixes Message-ID: URL: https://gitlist.dev/e/xmqqo7ytxnge.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > The release window is a fine time to concentrate on bug fixes, Well, it certainly is better than random shiny new features or refactoring for the sake of refactoring, but finding and fixing a bug that has been with us since forever without anybody noticing is not all that ultra-urgent, either ;-) It is a good time to concentrate on finding and fixing regressions. > and Coverity > can help identify bugs. These are only a couple of fixes, Coverity reported > many more, but most of the reports seemed either bogus or harmless (such as > resource leaks in test helpers). Thanks. > > Johannes Schindelin (11): > mingw: avoid accessing uninitialized memory in `is_executable()` > fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()` > submodule--helper: avoid memory leak in `update_submodule()` > get_oid_with_context_1(): avoid use-after-free > submodule-config: avoid memory leak > pack-redundant: avoid using uninitialized memory > submodule--helper: avoid memory leak when fetching submodules > read_index_from(): avoid memory leak > pack-mtimes: avoid closing a bogus file descriptor > relative_url(): fix incorrect condition > bug_fl(): add missing `va_end()` call > > builtin/pack-redundant.c | 1 + > builtin/submodule--helper.c | 3 +++ > fsmonitor-settings.c | 8 ++++++-- > object-name.c | 6 ++++-- > pack-mtimes.c | 3 ++- > read-cache.c | 6 +++--- > remote.c | 2 +- > run-command.c | 2 +- > submodule-config.c | 8 ++++---- > usage.c | 1 + > 10 files changed, 26 insertions(+), 14 deletions(-) > > > base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1264 ``` ## Junio C Hamano, 2022-06-16 04:07 Subject: Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()` Message-ID: URL: https://gitlist.dev/e/xmqqk09hxndi.fsf%40gitster.g In-Reply-To: <679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > On Windows, we open files we suspect might be scripts, read the first > two bytes, and see whether they indicate a hash-bang line. We do not > initialize the byte _after_ those two bytes, therefore `strcmp()` is > inappropriate here. > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > run-command.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) This has been with us since cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14) and apparently nobody made loud enough noises to make us aware since then. The fix is trivially correct, of course. Will queue. > > diff --git a/run-command.c b/run-command.c > index 14f17830f51..2ba38850b4d 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -154,7 +154,7 @@ int is_executable(const char *name) > n = read(fd, buf, 2); > if (n == 2) > /* look for a she-bang */ > - if (!strcmp(buf, "#!")) > + if (!memcmp(buf, "#!", 2)) > st.st_mode |= S_IXUSR; > close(fd); > } ``` ## Junio C Hamano, 2022-06-16 04:10 Subject: Re: [PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()` Message-ID: URL: https://gitlist.dev/e/xmqqfsk5xn8n.fsf%40gitster.g In-Reply-To: <6d409ad45867aa5193c15828614e865e53ac0c57.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > fsmonitor-settings.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c > index 658cb79da01..464424a1e92 100644 > --- a/fsmonitor-settings.c > +++ b/fsmonitor-settings.c > @@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r, > case FSMONITOR_REASON_OK: > goto done; Obviously correct, but the placement of these ... > - case FSMONITOR_REASON_BARE: > + case FSMONITOR_REASON_BARE: { > + char *cwd = xgetcwd(); > + > strbuf_addf(&msg, > _("bare repository '%s' is incompatible with fsmonitor"), > - xgetcwd()); > + cwd); > + free(cwd); > goto done; > + } ... braces are misleading and confusing. > > case FSMONITOR_REASON_ERROR: > strbuf_addf(&msg, ``` ## Junio C Hamano, 2022-06-16 04:23 Subject: Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()` Message-ID: URL: https://gitlist.dev/e/xmqq35g5xmmv.fsf%40gitster.g In-Reply-To: <877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > builtin/submodule--helper.c | 2 ++ > 1 file changed, 2 insertions(+) > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 5c77dfcffee..d7b8004b933 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) > > next.recursive_prefix = get_submodule_displaypath(prefixed_path, > update_data->prefix); > + free(prefixed_path); > + This function has two very similar code block that computes prefixed_path depending on the same condition, and one frees the variable correctly while the other one (i.e. this one) forgets to do so, which is irritating to see. Perhaps the whole "we have update_data structure, in which recursive_prefix, sm_path and prefix members in it; please set the displaypath member based on these values" should become a helper function, e.g. static const char *displaypath_from_update_data(struct update_data *u) { char *pp, *ret; if (u->recursive_prefix) pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path); else pp = xstrdup(u->sm_path); ret = get_submodule_displaypath(pp, u->prefix); free(pp); return ret; } to avoid duplicated computation. But the whole thing may become moot, as there seems to be a move to get rid of submodule--helper.c altogether? I'll refrain from touching this patch and instead redirect it to Glen; perhaps removal of submodule--helper.c involves moving the code here to another file or something, in which case it is far easier if I outsource that to somebody who is actually working on the file ;-) Thanks. > next.prefix = NULL; > oidcpy(&next.oid, null_oid()); > oidcpy(&next.suboid, null_oid()); ``` ## Junio C Hamano, 2022-06-16 04:29 Subject: Re: [PATCH 04/11] get_oid_with_context_1(): avoid use-after-free Message-ID: URL: https://gitlist.dev/e/xmqqy1xxw7rc.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > if (ce_stage(ce) == stage) { > - free(new_path); > if (reject_tree_in_index(repo, only_to_die, ce, > - stage, prefix, cp)) > + stage, prefix, cp)) { > + free(new_path); > return -1; > + } > oidcpy(oid, &ce->oid); > oc->mode = ce->ce_mode; > + free(new_path); > return 0; > } Hmph. Without the "lets make sure we do not leak in the error code path", it would have been no brainer to avoid this bug in the original version. Of course the postimage of the above hunk is correct, but with extra free() sprinkled, it became ugly to the eye. I wonder if the following is easier to follow. Thanks. object-name.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git c/object-name.c w/object-name.c index 4d2746574c..57188db7b0 100644 --- c/object-name.c +++ w/object-name.c @@ -1971,13 +1971,16 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo, memcmp(ce->name, cp, namelen)) break; if (ce_stage(ce) == stage) { + int ret = -1; + + if (!reject_tree_in_index(repo, only_to_die, ce, + stage, prefix, cp)) { + oidcpy(oid, &ce->oid); + oc->mode = ce->ce_mode; + ret = 0; + } free(new_path); - if (reject_tree_in_index(repo, only_to_die, ce, - stage, prefix, cp)) - return -1; - oidcpy(oid, &ce->oid); - oc->mode = ce->ce_mode; - return 0; + return ret; } pos++; } ``` ## Junio C Hamano, 2022-06-16 04:36 Subject: Re: [PATCH 05/11] submodule-config: avoid memory leak Message-ID: URL: https://gitlist.dev/e/xmqqmtedw7fm.fsf%40gitster.g In-Reply-To: <591166e07d87fdb5efc2769d3e2963e3f0412720.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > In 961b130d20c9 (branch: add --recurse-submodules option for branch > creation, 2022-01-28), a funny pattern was introduced where first some > struct is `xmalloc()`ed, then we resize an array whose element type is > the same struct, and then the first struct's contents are copied into > the last element of that array. Sigh. The original is butt ugly, with this strange pattern and structure assignments etc. I wonder how something like this slipped through our reviews. I wonder if it would help for me to stop trusting reviews by less experienced reviewers too much, and instead give sanity checks to more patches myself from now on, but I certainly cannot afford the time and my mental health to do so for all the patches X-<. Will queue. > if (S_ISGITLINK(name_entry->mode) && > is_tree_submodule_active(r, root_tree, tree_path)) { > - st_entry = xmalloc(sizeof(*st_entry)); > + ALLOC_GROW(out->entries, out->entry_nr + 1, > + out->entry_alloc); > + st_entry = &out->entries[out->entry_nr++]; > + > st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry)); > *st_entry->name_entry = *name_entry; > st_entry->submodule = > @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r, > root_tree)) > FREE_AND_NULL(st_entry->repo); > > - ALLOC_GROW(out->entries, out->entry_nr + 1, > - out->entry_alloc); > - out->entries[out->entry_nr++] = *st_entry; > } else if (S_ISDIR(name_entry->mode)) > traverse_tree_submodules(r, root_tree, tree_path, > &name_entry->oid, out); ``` ## Junio C Hamano, 2022-06-16 04:53 Subject: Re: [PATCH 06/11] pack-redundant: avoid using uninitialized memory Message-ID: URL: https://gitlist.dev/e/xmqqilp1w6oi.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > In the `add_pack()` function, we forgot to initialize the field `next`, > which could potentially lead to readin uninitialized memory later. > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > builtin/pack-redundant.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c > index ed9b9013a5f..1f7da1f68b6 100644 > --- a/builtin/pack-redundant.c > +++ b/builtin/pack-redundant.c > @@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p) > } > l.all_objects_size = l.remaining_objects->size; > l.unique_objects = NULL; > + l.next = NULL; > if (p->pack_local) > return pack_list_insert(&local_packs, &l); > else return pack_list_insert(&altodb_packs, &l); The pack_list_insert reads like so: static inline struct pack_list * pack_list_insert(struct pack_list **pl, struct pack_list *entry) { struct pack_list *p = xmalloc(sizeof(struct pack_list)); memcpy(p, entry, sizeof(struct pack_list)); p->next = *pl; *pl = p; return p; } IOW, we prepare members of "l", pass it as "entry" to pack_list_insert(), but the helper function allocates a new piece of memory, copies "l" to it, and then sets the .next member of it to a reasonable value. And after that, the "l" that was merely used as a template goes out of scope. So I think this is a false positive. memcpy(p) does copy the uninitialized garbage that is held in entry->next to p->next, but it is overwritten with a reasonable address immediately after that. ``` ## Jeff King, 2022-06-16 04:53 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/Yqq3O5hykBecoVKQ%40coredump.intra.peff.net In-Reply-To: ``` On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > According to the manual: > > Each invocation of va_copy() must be matched by a corresponding > invocation of va_end() in the same function. > > Note: There is another instance of `va_copy()` in `usage.c` that is > missing a `va_end()` call, in `BUG_vfl()`. It does not matter there, > though, because that function either `exit()`s or `abort()`s, anyway. > > Reported by Coverity. This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02). But there's a much worse bug in the same function. The code introduced by that patch does: va_list ap, cp; [...] va_copy(cp, ap); va_start(ap, fmt); So "cp" is copied from "ap" before we have actually initialized "ap". It's surprising that this works at all. The two lines should be flipped. IMHO, since we're in the actual varargs function itself, it would be simpler to just bracket each use with start/end, rather than copying, like: diff --git a/usage.c b/usage.c index 79900d0287..56e29d6cd6 100644 --- a/usage.c +++ b/usage.c @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) int bug_called_must_BUG; void bug_fl(const char *file, int line, const char *fmt, ...) { - va_list ap, cp; + va_list ap; bug_called_must_BUG = 1; - va_copy(cp, ap); va_start(ap, fmt); BUG_vfl_common(file, line, fmt, ap); va_end(ap); - trace2_cmd_error_va(fmt, cp); + + va_start(ap, fmt); + trace2_cmd_error_va(fmt, ap); + va_end(ap); } #ifdef SUPPRESS_ANNOTATED_LEAKS but I am happy with any solution that is correct. :) -Peff ``` ## Junio C Hamano, 2022-06-16 04:55 Subject: Re: [PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules Message-ID: URL: https://gitlist.dev/e/xmqqedzpw6k8.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > In c51f8f94e5b3 (submodule--helper: run update procedures from C, > 2021-08-24), we added code that first obtains the default remote, and > then adds that to a `strvec`. > > However, we never released the default remote's memory. > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > builtin/submodule--helper.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index d7b8004b933..9a8248ffe6a 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str > char *hex = oid_to_hex(oid); > char *remote = get_default_remote(); > strvec_pushl(&cp.args, remote, hex, NULL); > + free(remote); Trivially correct. Will queue. > } > > return run_command(&cp); ``` ## Junio C Hamano, 2022-06-16 05:00 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/xmqq8rpxw6ch.fsf%40gitster.g In-Reply-To: ``` Jeff King writes: > On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote: > >> From: Johannes Schindelin >> >> According to the manual: >> >> Each invocation of va_copy() must be matched by a corresponding >> invocation of va_end() in the same function. >> >> Note: There is another instance of `va_copy()` in `usage.c` that is >> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there, >> though, because that function either `exit()`s or `abort()`s, anyway. >> >> Reported by Coverity. > > This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal > bug() function to go with BUG(), 2022-06-02). But there's a much worse > bug in the same function. The code introduced by that patch does: > > va_list ap, cp; > [...] > va_copy(cp, ap); > va_start(ap, fmt); > > So "cp" is copied from "ap" before we have actually initialized "ap". > It's surprising that this works at all. The two lines should be flipped. Yes, it is surprising. Perhaps it is not working at all. Thanks for an extra set of eyeballs. > IMHO, since we're in the actual varargs function itself, it would be > simpler to just bracket each use with start/end, rather than copying, > like: > > diff --git a/usage.c b/usage.c > index 79900d0287..56e29d6cd6 100644 > --- a/usage.c > +++ b/usage.c > @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) > int bug_called_must_BUG; > void bug_fl(const char *file, int line, const char *fmt, ...) > { > - va_list ap, cp; > + va_list ap; > > bug_called_must_BUG = 1; > > - va_copy(cp, ap); > va_start(ap, fmt); > BUG_vfl_common(file, line, fmt, ap); > va_end(ap); > - trace2_cmd_error_va(fmt, cp); > + > + va_start(ap, fmt); > + trace2_cmd_error_va(fmt, ap); > + va_end(ap); > } > > #ifdef SUPPRESS_ANNOTATED_LEAKS > > but I am happy with any solution that is correct. :) > > -Peff ``` ## Junio C Hamano, 2022-06-16 05:02 Subject: Re: [PATCH 10/11] relative_url(): fix incorrect condition Message-ID: URL: https://gitlist.dev/e/xmqq4k0lw68q.fsf%40gitster.g In-Reply-To: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > In 63e95beb085c (submodule: port resolve_relative_url from shell to C, > 2016-04-15), we added a loop over `url` where we are looking for `../` > or `./` components. > > The loop condition we used is the pointer `url` itself, which is clearly > not what we wanted. 8/11, 9/11, and 10/11 are trivially correct. Will queue. Thanks. > > Pointed out by Coverity. > > Signed-off-by: Johannes Schindelin > --- > remote.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/remote.c b/remote.c > index 9b9bbfe80ec..bded6acbfe8 100644 > --- a/remote.c > +++ b/remote.c > @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url, > * When the url starts with '../', remove that and the > * last directory in remoteurl. > */ > - while (url) { > + while (*url) { > if (starts_with_dot_dot_slash_native(url)) { > url += 3; > colonsep |= chop_last_dir(&remoteurl, is_relative); ``` ## Ævar Arnfjörð Bjarmason, 2022-06-16 13:02 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: <220616.86o7yswyek.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220616.86o7yswyek.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Wed, Jun 15 2022, Junio C Hamano wrote: > Jeff King writes: > >> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote: >> >>> From: Johannes Schindelin >>> >>> According to the manual: >>> >>> Each invocation of va_copy() must be matched by a corresponding >>> invocation of va_end() in the same function. >>> >>> Note: There is another instance of `va_copy()` in `usage.c` that is >>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there, >>> though, because that function either `exit()`s or `abort()`s, anyway. >>> >>> Reported by Coverity. >> >> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal >> bug() function to go with BUG(), 2022-06-02). But there's a much worse >> bug in the same function. The code introduced by that patch does: >> >> va_list ap, cp; >> [...] >> va_copy(cp, ap); >> va_start(ap, fmt); >> >> So "cp" is copied from "ap" before we have actually initialized "ap". >> It's surprising that this works at all. The two lines should be flipped. > > Yes, it is surprising. Perhaps it is not working at all. > > Thanks for an extra set of eyeballs. It's "working" now in the sense that we run this code, and even test that trace2 output specifically, see the tests in 0cc05b044fd (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02). But obviously that's a bad use of the varargs API, I just don't know how we've been getting away with it in practice, sorry about that. The fix Peff's got here LGTM. I can (re)submit it with format-patch+send-email after giving it a commit message describing the issue if you'd like, but the change would be the same. I'm also happy for you to pick up the upthread directly, but I don't see that you did that already in one of the integration branches (but perhaps I missed one). Just let me know, but I didn't want to re-submit this without asking, in case I'd step on any toes (or submit something in duplicate). ``` ## Ævar Arnfjörð Bjarmason, 2022-06-16 13:09 Subject: Re: [PATCH 10/11] relative_url(): fix incorrect condition Message-ID: <220616.86k09gwx6i.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220616.86k09gwx6i.gmgdl%40evledraar.gmail.com In-Reply-To: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com> ``` On Wed, Jun 15 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > In 63e95beb085c (submodule: port resolve_relative_url from shell to C, > 2016-04-15), we added a loop over `url` where we are looking for `../` > or `./` components. > > The loop condition we used is the pointer `url` itself, which is clearly > not what we wanted. Clearly, but having looked at this I think it's useful to note that coverity must be (I don't know what it actually emitted) be complaining about the "url" condition being useless, i.e. it's the same as a "while (1)", not that we run off the end of the string. I.e. due to the way those start_with*() functions work we would abort early if we were running off th end of the string. > Pointed out by Coverity. > > Signed-off-by: Johannes Schindelin > --- > remote.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/remote.c b/remote.c > index 9b9bbfe80ec..bded6acbfe8 100644 > --- a/remote.c > +++ b/remote.c > @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url, > * When the url starts with '../', remove that and the > * last directory in remoteurl. > */ > - while (url) { > + while (*url) { > if (starts_with_dot_dot_slash_native(url)) { > url += 3; > colonsep |= chop_last_dir(&remoteurl, is_relative); Which I tested with this: diff --git a/remote.c b/remote.c index 9b9bbfe80ec..e049bbb791c 100644 --- a/remote.c +++ b/remote.c @@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url, * When the url starts with '../', remove that and the * last directory in remoteurl. */ - while (url) { + while (1) { if (starts_with_dot_dot_slash_native(url)) { url += 3; colonsep |= chop_last_dir(&remoteurl, is_relative); - } else if (starts_with_dot_slash_native(url)) + } else if (starts_with_dot_slash_native(url)) { url += 2; - else - break; + } else if (!*url) { + BUG("ran off the end of our url?"); + } else { + break; + } } strbuf_reset(&sb); strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url); Which will fail e.g. on this test in t3420-rebase-autostash.sh: + git submodule add ./ sub BUG: remote.c:2856: ran off the end of our url? Aborted I worried a bit about this since we released this with v2.9.0, so in all this time we haven't been infinitely looping on this case, or at least haven't had any reports about that. So if we hadn't been catching this in starts_with_*() I wouldn't be confident that this was the correct fix, yes it's almost certainly not what not what was intended, but if we change it to that are we confident that the side-effects are going to be what we want? But in this case I'm pretty sure that the behavior before/after will be the same, and that the only change will be to make coverity happier, and the code less confusing. ``` ## Junio C Hamano, 2022-06-16 16:41 Subject: Re: [PATCH 10/11] relative_url(): fix incorrect condition Message-ID: URL: https://gitlist.dev/e/xmqqwndgv9vv.fsf%40gitster.g In-Reply-To: <0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > From: Johannes Schindelin > > In 63e95beb085c (submodule: port resolve_relative_url from shell to C, > 2016-04-15), we added a loop over `url` where we are looking for `../` > or `./` components. > > The loop condition we used is the pointer `url` itself, which is clearly > not what we wanted. > > Pointed out by Coverity. > > Signed-off-by: Johannes Schindelin > --- > remote.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/remote.c b/remote.c > index 9b9bbfe80ec..bded6acbfe8 100644 > --- a/remote.c > +++ b/remote.c > @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url, > * When the url starts with '../', remove that and the > * last directory in remoteurl. > */ > - while (url) { > + while (*url) { > if (starts_with_dot_dot_slash_native(url)) { > url += 3; > colonsep |= chop_last_dir(&remoteurl, is_relative); } else if (starts_with_dot_slash_native(url)) url += 2; else break; } Looking at the "problematic" code again, the original is bad in the sense that it is utterly misleading, but not quite "incorrect", I would think. In other words, what the original wanted to write is an infinite loop that uses an explicit "break" to terminate the iteration, so we should have written "while (1)" in the first place. We know that earlier part of the function already depends on url being not NULL, and nothing in the loop makes it NULL. *url that is NUL would make the two starts_with_frotz() tests fail and causes the loop to terminate by hitting "break", so while it is correct to check *url, it is wasting cycles. But this patch is not wrong per-se, so I still have it in my tree ;-) Thanks. ``` ## Glen Choo, 2022-06-16 17:51 Subject: Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()` Message-ID: URL: https://gitlist.dev/e/kl6lsfo44hvd.fsf%40chooglen-macbookpro.roam.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > "Johannes Schindelin via GitGitGadget" > writes: > >> From: Johannes Schindelin >> >> Reported by Coverity. >> >> Signed-off-by: Johannes Schindelin >> --- >> builtin/submodule--helper.c | 2 ++ >> 1 file changed, 2 insertions(+) > > >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 5c77dfcffee..d7b8004b933 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data) >> >> next.recursive_prefix = get_submodule_displaypath(prefixed_path, >> update_data->prefix); >> + free(prefixed_path); >> + > > This function has two very similar code block that computes > prefixed_path depending on the same condition, and one frees the > variable correctly while the other one (i.e. this one) forgets to do > so, which is irritating to see. Good eye, I hadn't noticed this. In this case, we're computing the same thing twice in the function - first unconditionally, then conditionally. So we can actually just reuse the first result by doing something like next.recursive_prefix = xstrfmt("%s/", update_data->displaypath); (note that we need to append a "/" at the end) > Perhaps the whole "we have update_data structure, in which > recursive_prefix, sm_path and prefix members in it; please set the > displaypath member based on these values" should become a helper > function, e.g. > > static const char *displaypath_from_update_data(struct update_data *u) > { > char *pp, *ret; > > if (u->recursive_prefix) > pp = xstrfmt("%s%s", u->recursive_prefix, u->sm_path); > else > pp = xstrdup(u->sm_path); > > ret = get_submodule_displaypath(pp, u->prefix); > free(pp); > return ret; > } > > to avoid duplicated computation. Even better, I recently noticed that .recursive_prefix can probably just be replaced with "--super-prefix", and that will let us dispatch to get_submodule_displaypath() without this extra dance here. I'll flesh that out into a series and send it soon. > But the whole thing may become moot, as there seems to be a move to > get rid of submodule--helper.c altogether? > > > I'll refrain from touching this patch and instead redirect it to > Glen; perhaps removal of submodule--helper.c involves moving the > code here to another file or something, in which case it is far > easier if I outsource that to somebody who is actually working on > the file ;-) cc-ed Ævar, who knows even more than I do :) git-submodule.sh is definitely going away soon \o/ but I don't think we have plans to get rid of submodule--helper.c just yet, so I think we should probably still keep this patch. > > Thanks. > >> next.prefix = NULL; >> oidcpy(&next.oid, null_oid()); >> oidcpy(&next.suboid, null_oid()); ``` ## Junio C Hamano, 2022-06-16 17:55 Subject: Re: [PATCH 10/11] relative_url(): fix incorrect condition Message-ID: URL: https://gitlist.dev/e/xmqqr13otrv6.fsf%40gitster.g In-Reply-To: <220616.86k09gwx6i.gmgdl@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: >> - while (url) { >> + while (*url) { >> if (starts_with_dot_dot_slash_native(url)) { >> url += 3; >> colonsep |= chop_last_dir(&remoteurl, is_relative); > > Which I tested with this: > > diff --git a/remote.c b/remote.c > index 9b9bbfe80ec..e049bbb791c 100644 > --- a/remote.c > +++ b/remote.c > @@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url, > * When the url starts with '../', remove that and the > * last directory in remoteurl. > */ > - while (url) { > + while (1) { > if (starts_with_dot_dot_slash_native(url)) { > url += 3; > colonsep |= chop_last_dir(&remoteurl, is_relative); > - } else if (starts_with_dot_slash_native(url)) > + } else if (starts_with_dot_slash_native(url)) { > url += 2; > - else > - break; > + } else if (!*url) { > + BUG("ran off the end of our url?"); > + } else { > + break; > + } > } > strbuf_reset(&sb); > strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url); > > Which will fail e.g. on this test in t3420-rebase-autostash.sh: > > + git submodule add ./ sub > BUG: remote.c:2856: ran off the end of our url? > Aborted Sorry, but I am confused. I do not see the point of that test before the BUG. I agree that this loop over url is "we want to loop forever, and stop immediately when url does not start with ../ or ./" infinite loop, and it is better written with "while(1)". If you do not make any other changes that confuses me, we'll get this: - while (*url) { + while (1) { if (starts_with_dot_dot_slash_native(url)) { url += 3; colonsep |= chop_last_dir(&remoteurl, is_relative); } else if (starts_with_dot_slash_native(url)) url += 2; else break; } We know url upon entry is not NULL. starts_with_dot_*() ends up calling dir.c:path_match_flags() and the first thing it does is to return if the byte is not '.'. So the difference is whether you leave the loop with the "while" condition, or you fail the two starts_with_dot_*() calls and hit the "break" at the end in the loop. IOW, running of the end of the URL is not a BUG, is it? ``` ## Junio C Hamano, 2022-06-16 18:03 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/xmqqk09gtriq.fsf%40gitster.g In-Reply-To: <220616.86o7yswyek.gmgdl@evledraar.gmail.com> ``` Ævar Arnfjörð Bjarmason writes: > But obviously that's a bad use of the varargs API, I just don't know how > we've been getting away with it in practice, sorry about that. Exactly. We three all expressed our surprises on why it "works". Nobody offered an explanation, though, which leaves us in suspense ;-) > The fix Peff's got here LGTM. I can (re)submit it with > format-patch+send-email after giving it a commit message describing the > issue if you'd like, but the change would be the same. Yup, I think the code change there looks the most sensible. Thanks, all. ``` ## Glen Choo, 2022-06-16 18:09 Subject: Re: [PATCH 05/11] submodule-config: avoid memory leak Message-ID: URL: https://gitlist.dev/e/kl6lpmj84h18.fsf%40chooglen-macbookpro.roam.corp.google.com In-Reply-To: ``` Junio C Hamano writes: > "Johannes Schindelin via GitGitGadget" > writes: > >> From: Johannes Schindelin >> >> In 961b130d20c9 (branch: add --recurse-submodules option for branch >> creation, 2022-01-28), a funny pattern was introduced where first some >> struct is `xmalloc()`ed, then we resize an array whose element type is >> the same struct, and then the first struct's contents are copied into >> the last element of that array. > > Sigh. The original is butt ugly, with this strange pattern and > structure assignments etc. I wonder how something like this slipped > through our reviews. Gah, I have no excuses for bad code I've already written, I can only strive to write better code next time. Thanks Johannes for spotting and fixing this. > I wonder if it would help for me to stop trusting reviews by less > experienced reviewers too much, and instead give sanity checks to > more patches myself from now on, but I certainly cannot afford the > time and my mental health to do so for all the patches X-<. I seem to recall that this was reviewed by fairly experienced folks. I'm guessing it slipped through due to reviewer fatigue, which might be a good argument for adding more tooling to lighten reviewer load/patch up the gaps. > > Will queue. > >> if (S_ISGITLINK(name_entry->mode) && >> is_tree_submodule_active(r, root_tree, tree_path)) { >> - st_entry = xmalloc(sizeof(*st_entry)); >> + ALLOC_GROW(out->entries, out->entry_nr + 1, >> + out->entry_alloc); >> + st_entry = &out->entries[out->entry_nr++]; >> + >> st_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry)); >> *st_entry->name_entry = *name_entry; >> st_entry->submodule = >> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r, >> root_tree)) >> FREE_AND_NULL(st_entry->repo); >> >> - ALLOC_GROW(out->entries, out->entry_nr + 1, >> - out->entry_alloc); >> - out->entries[out->entry_nr++] = *st_entry; >> } else if (S_ISDIR(name_entry->mode)) >> traverse_tree_submodules(r, root_tree, tree_path, >> &name_entry->oid, out); ``` ## Jeff King, 2022-06-16 19:20 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/YquCaE%2BVw9P%2FfybU%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Jun 16, 2022 at 11:03:25AM -0700, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > But obviously that's a bad use of the varargs API, I just don't know how > > we've been getting away with it in practice, sorry about that. > > Exactly. We three all expressed our surprises on why it "works". > Nobody offered an explanation, though, which leaves us in suspense > ;-) Being the curious sort, I ran it in a debugger. And indeed, "cp" is filled with uninitialized garbage. The reason it works is that the test calls bug() with a format string that does not contain any placeholders. And thus our eventual call to vsnprintf() does not ever look at "cp" at all! > > The fix Peff's got here LGTM. I can (re)submit it with > > format-patch+send-email after giving it a commit message describing the > > issue if you'd like, but the change would be the same. > > Yup, I think the code change there looks the most sensible. I'll wrap it up with a commit message and modify the test to be more thorough. -Peff ``` ## René Scharfe, 2022-06-16 19:53 Subject: Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()` Message-ID: <98ff788a-d621-cf3f-aed5-66ae763b381f@web.de> URL: https://gitlist.dev/e/98ff788a-d621-cf3f-aed5-66ae763b381f%40web.de In-Reply-To: <679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com> ``` Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget: > From: Johannes Schindelin > > On Windows, we open files we suspect might be scripts, read the first > two bytes, and see whether they indicate a hash-bang line. We do not > initialize the byte _after_ those two bytes, therefore `strcmp()` is > inappropriate here. Hmm, but buf _is_ initialized fully? Line 149: char buf[3] = { 0 }; C99 §6.7.8 21: "If there are [...] fewer characters in a string literal used to initialize an array of known size than there are elements in the array, the remainder of the aggregate shall be initialized implicitly the same as objects that have static storage duration." > > Reported by Coverity. > > Signed-off-by: Johannes Schindelin > --- > run-command.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/run-command.c b/run-command.c > index 14f17830f51..2ba38850b4d 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -154,7 +154,7 @@ int is_executable(const char *name) > n = read(fd, buf, 2); > if (n == 2) > /* look for a she-bang */ > - if (!strcmp(buf, "#!")) > + if (!memcmp(buf, "#!", 2)) > st.st_mode |= S_IXUSR; > close(fd); > } ``` ## Jeff King, 2022-06-16 20:04 Subject: [PATCH] bug_fl(): correctly initialize trace2 va_list Message-ID: URL: https://gitlist.dev/e/YquMyakxYnU6mI5a%40coredump.intra.peff.net In-Reply-To: ``` On Thu, Jun 16, 2022 at 03:20:08PM -0400, Jeff King wrote: > > > The fix Peff's got here LGTM. I can (re)submit it with > > > format-patch+send-email after giving it a commit message describing the > > > issue if you'd like, but the change would be the same. > > > > Yup, I think the code change there looks the most sensible. > > I'll wrap it up with a commit message and modify the test to be more > thorough. Here it is. This can replace Johannes's patch 11. -- >8 -- Subject: bug_fl(): correctly initialize trace2 va_list The code added 0cc05b044f (usage.c: add a non-fatal bug() function to go with BUG(), 2022-06-02) sets up two va_list variables: one to output to stderr, and one to trace2. But the order of initialization is wrong: va_list ap, cp; va_copy(cp, ap); va_start(ap, fmt); We copy the contents of "ap" into "cp" before it is initialized, meaning it is full of garbage. The two should be swapped. However, there's another bug, noticed by Johannes Schindelin: we forget to call va_end() for the copy. So instead of just fixing the copy's initialization, let's do two separate start/end pairs. This is allowed by the standard, and we don't need to use copy here since we have access to the original varargs. Matching the pairs with the calls makes it more obvious that everything is being done correctly. Note that we do call bug_fl() in the tests, but it didn't trigger this problem because our format string doesn't have any placeholders. So even though we were passing a garbage va_list through the stack, nobody ever needed to look at it. We can easily adjust one of the trace2 tests to trigger this, both for bug() and for BUG(). The latter isn't broken, but it's nice to exercise both a bit more. Without the fix in this patch (but with the test change), the bug() case causes a segfault. Signed-off-by: Jeff King --- t/helper/test-trace2.c | 4 ++-- usage.c | 8 +++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c index 180c7f53f3..a714130ece 100644 --- a/t/helper/test-trace2.c +++ b/t/helper/test-trace2.c @@ -224,8 +224,8 @@ static int ut_009bug_BUG(int argc, const char **argv) static int ut_010bug_BUG(int argc, const char **argv) { - bug("a bug message"); - BUG("a BUG message"); + bug("a %s message", "bug"); + BUG("a %s message", "BUG"); } /* diff --git a/usage.c b/usage.c index 79900d0287..56e29d6cd6 100644 --- a/usage.c +++ b/usage.c @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...) int bug_called_must_BUG; void bug_fl(const char *file, int line, const char *fmt, ...) { - va_list ap, cp; + va_list ap; bug_called_must_BUG = 1; - va_copy(cp, ap); va_start(ap, fmt); BUG_vfl_common(file, line, fmt, ap); va_end(ap); - trace2_cmd_error_va(fmt, cp); + + va_start(ap, fmt); + trace2_cmd_error_va(fmt, ap); + va_end(ap); } #ifdef SUPPRESS_ANNOTATED_LEAKS -- 2.37.0.rc0.352.g10876ef154 ``` ## Junio C Hamano, 2022-06-16 20:11 Subject: Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call Message-ID: URL: https://gitlist.dev/e/xmqqr13os71k.fsf%40gitster.g In-Reply-To: ``` Jeff King writes: > Being the curious sort, I ran it in a debugger. And indeed, "cp" is > filled with uninitialized garbage. The reason it works is that the test > calls bug() with a format string that does not contain any placeholders. > And thus our eventual call to vsnprintf() does not ever look at "cp" at > all! ;-) Thanks. ``` ## Junio C Hamano, 2022-06-16 20:13 Subject: Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()` Message-ID: URL: https://gitlist.dev/e/xmqqmtecs6ws.fsf%40gitster.g In-Reply-To: <98ff788a-d621-cf3f-aed5-66ae763b381f@web.de> ``` René Scharfe writes: > Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget: >> From: Johannes Schindelin >> >> On Windows, we open files we suspect might be scripts, read the first >> two bytes, and see whether they indicate a hash-bang line. We do not >> initialize the byte _after_ those two bytes, therefore `strcmp()` is >> inappropriate here. > > Hmm, but buf _is_ initialized fully? Line 149: > > char buf[3] = { 0 }; Ahh, yeah, that changes the landscape quite a bit. We explicitly ask to read 2 bytes and look at the buf[] when read says it read 2 bytes, so this is another false positive X-<. >> diff --git a/run-command.c b/run-command.c >> index 14f17830f51..2ba38850b4d 100644 >> --- a/run-command.c >> +++ b/run-command.c >> @@ -154,7 +154,7 @@ int is_executable(const char *name) >> n = read(fd, buf, 2); >> if (n == 2) >> /* look for a she-bang */ >> - if (!strcmp(buf, "#!")) >> + if (!memcmp(buf, "#!", 2)) >> st.st_mode |= S_IXUSR; >> close(fd); >> } We can update the variable to char buf[2]; to match the updated code, I guess. The fewer bytes we use on stack, the better ;-). ``` ## Junio C Hamano, 2022-06-16 20:20 Subject: Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()` Message-ID: URL: https://gitlist.dev/e/xmqqilp0s6lk.fsf%40gitster.g In-Reply-To: ``` Junio C Hamano writes: >> Hmm, but buf _is_ initialized fully? Line 149: >> >> char buf[3] = { 0 }; > > Ahh, yeah, that changes the landscape quite a bit. Just for reference, this piece of code has been correct ever since it was introduced to help.c by cc3b7a97 (Windows: Make 'git help -a' work., 2008-01-14). With a larger context, we can see buf[3] that is NUL filled, which receives 2 bytes by read(), and strcmp() does look semantically more correct than memcmp(), even though there probably shouldn't be any correctness or performance difference. So, strcmp -> memcmp in this case is a strict dis-improvement. I merged it already to 'next', but I haven't pushed the result out, so I'll redo 'next' before I push the result out. Thanks. ``` ## Taylor Blau, 2022-06-16 20:43 Subject: Re: [PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor Message-ID: URL: https://gitlist.dev/e/YquV9oYaAPeiJec1%40nand.local In-Reply-To: ``` On Wed, Jun 15, 2022 at 11:35:43PM +0000, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > In 94cd775a6c52 (pack-mtimes: support reading .mtimes files, > 2022-05-20), code was added to close the file descriptor corresponding > to the mtimes file. > > However, it is possible that opening that file failed, in which case we > are closing a file descriptor with the value `-1`. Let's guard that > `close()` call. Nice catch. I agree with your assessment and fix. Thanks for spotting and fixing! Thanks, Taylor ``` ## Tom Levy, 2022-06-17 21:27 Subject: [PATCH 08/11] read_index_from(): avoid memory leak Message-ID: <20220617212736.1951-1-tomlevy93@gmail.com> URL: https://gitlist.dev/e/20220617212736.1951-1-tomlevy93%40gmail.com In-Reply-To: ``` Hi, I think there is another problem with this code: later on it refers to the 'base_path' variable, but I think it should refer to 'base_path2' if the first path did not exist. How about the following patch instead (or in addition)? Regards, Tom Levy -- >8 -- Subject: [PATCH] read_index_from(): use second path consistently, avoid memory leak In 998330ac2e7c (read-cache: look for shared index files next to the index, too, 2021-08-26), we added code that allocates memory to store the path of a second shared base index, but we never released that memory. Also, later code (e.g. the call to freshen_shared_index()) continued to use the first path even when the second path was selected. Modify the code to store the second path in the original variable (after freeing the first path), so that the later code will use the second path (and also free it). The memory leak was reported by Coverity. Co-authored-by: Johannes Schindelin Signed-off-by: Tom Levy --- read-cache.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/read-cache.c b/read-cache.c index 4df97e185e..48f2b9aa9d 100644 --- a/read-cache.c +++ b/read-cache.c @@ -2459,15 +2459,15 @@ int read_index_from(struct index_state *istate, const char *path, the_repository, "%s", base_path); if (!ret) { char *path_copy = xstrdup(path); - const char *base_path2 = xstrfmt("%s/sharedindex.%s", - dirname(path_copy), - base_oid_hex); + free(base_path); + base_path = xstrfmt("%s/sharedindex.%s", dirname(path_copy), + base_oid_hex); free(path_copy); trace2_region_enter_printf("index", "shared/do_read_index", - the_repository, "%s", base_path2); - ret = do_read_index(split_index->base, base_path2, 1); + the_repository, "%s", base_path); + ret = do_read_index(split_index->base, base_path, 1); trace2_region_leave_printf("index", "shared/do_read_index", - the_repository, "%s", base_path2); + the_repository, "%s", base_path); } if (!oideq(&split_index->base_oid, &split_index->base->oid)) die(_("broken index, expect %s in %s, got %s"), -- 2.30.2 ```