{"thread":{"id":"58019","subject":"[PATCH 00/11] Coverity fixes","startedAt":"2022-06-15T23:35:55Z","lastAt":"2022-06-17T21:47:48Z","messageCount":38,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King","Ævar Arnfjörð Bjarmason","Glen Choo","René Scharfe","Taylor Blau","Tom Levy"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"457326","messageId":"pull.1264.git.1655336146.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":null,"subject":"[PATCH 00/11] Coverity fixes","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:34Z","receivedAt":"2022-06-15T23:35:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The release window is a fine time to concentrate on bug fixes, and Coverity\ncan help identify bugs. These are only a couple of fixes, Coverity reported\nmany more, but most of the reports seemed either bogus or harmless (such as\nresource leaks in test helpers).\n\nJohannes Schindelin (11):\n mingw: avoid accessing uninitialized memory in `is_executable()`\n fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`\n submodule--helper: avoid memory leak in `update_submodule()`\n get_oid_with_context_1(): avoid use-after-free\n submodule-config: avoid memory leak\n pack-redundant: avoid using uninitialized memory\n submodule--helper: avoid memory leak when fetching submodules\n read_index_from(): avoid memory leak\n pack-mtimes: avoid closing a bogus file descriptor\n relative_url(): fix incorrect condition\n bug_fl(): add missing `va_end()` call\n\n builtin/pack-redundant.c | 1 +\n builtin/submodule--helper.c | 3 +++\n fsmonitor-settings.c | 8 ++++++--\n object-name.c | 6 ++++--\n pack-mtimes.c | 3 ++-\n read-cache.c | 6 +++---\n remote.c | 2 +-\n run-command.c | 2 +-\n submodule-config.c | 8 ++++----\n usage.c | 1 +\n 10 files changed, 26 insertions(+), 14 deletions(-)\n\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1264\n-- \ngitgitgadget\n"},{"id":"457327","messageId":"679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:35Z","receivedAt":"2022-06-15T23:35:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nOn Windows, we open files we suspect might be scripts, read the first\ntwo bytes, and see whether they indicate a hash-bang line. We do not\ninitialize the byte _after_ those two bytes, therefore `strcmp()` is\ninappropriate here.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n run-command.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 14f17830f51..2ba38850b4d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -154,7 +154,7 @@ int is_executable(const char *name)\n \t\tn = read(fd, buf, 2);\n \t\tif (n == 2)\n \t\t\t/* look for a she-bang */\n-\t\t\tif (!strcmp(buf, \"#!\"))\n+\t\t\tif (!memcmp(buf, \"#!\", 2))\n \t\t\t\tst.st_mode |= S_IXUSR;\n \t\tclose(fd);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"457328","messageId":"6d409ad45867aa5193c15828614e865e53ac0c57.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:36Z","receivedAt":"2022-06-15T23:35:58Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n fsmonitor-settings.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/fsmonitor-settings.c b/fsmonitor-settings.c\nindex 658cb79da01..464424a1e92 100644\n--- a/fsmonitor-settings.c\n+++ b/fsmonitor-settings.c\n@@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r,\n \tcase FSMONITOR_REASON_OK:\n \t\tgoto done;\n \n-\tcase FSMONITOR_REASON_BARE:\n+\tcase FSMONITOR_REASON_BARE: {\n+\t\tchar *cwd = xgetcwd();\n+\n \t\tstrbuf_addf(&msg,\n \t\t\t _(\"bare repository '%s' is incompatible with fsmonitor\"),\n-\t\t\t xgetcwd());\n+\t\t\t cwd);\n+\t\tfree(cwd);\n \t\tgoto done;\n+\t}\n \n \tcase FSMONITOR_REASON_ERROR:\n \t\tstrbuf_addf(&msg,\n-- \ngitgitgadget\n\n"},{"id":"457329","messageId":"877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:37Z","receivedAt":"2022-06-15T23:36:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/submodule--helper.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 5c77dfcffee..d7b8004b933 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)\n \n \t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n \t\t\t\t\t\t\t\t update_data->prefix);\n+\t\tfree(prefixed_path);\n+\n \t\tnext.prefix = NULL;\n \t\toidcpy(&next.oid, null_oid());\n \t\toidcpy(&next.suboid, null_oid());\n-- \ngitgitgadget\n\n"},{"id":"457330","messageId":"cf6bcdb43e5b4abab464c30a914d64dc8e7a9925.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 04/11] get_oid_with_context_1(): avoid use-after-free","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:38Z","receivedAt":"2022-06-15T23:36:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 561287d342cc (object-name: reject trees found in the index,\n2022-04-26), we changed the code to release the memory stored under the\n`new_path` variable a bit earlier.\n\nHowever, at that point the variable `cp` still points to `new_path` (if\nnon-`NULL`), and we _then_ pass that pointer to\n`reject_tree_in_index()`.\n\nLet's make sure that the pointer still points to allocated memory by\nmoving the original `free()` call back where it was, and adding another\none in the early return code path.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n object-name.c | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/object-name.c b/object-name.c\nindex 4d2746574cd..228c12a554c 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1971,12 +1971,14 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t memcmp(ce->name, cp, namelen))\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n-\t\t\t\tfree(new_path);\n \t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n-\t\t\t\t\t\t\t stage, prefix, cp))\n+\t\t\t\t\t\t\t stage, prefix, cp)) {\n+\t\t\t\t\tfree(new_path);\n \t\t\t\t\treturn -1;\n+\t\t\t\t}\n \t\t\t\toidcpy(oid, &ce->oid);\n \t\t\t\toc->mode = ce->ce_mode;\n+\t\t\t\tfree(new_path);\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t\tpos++;\n-- \ngitgitgadget\n\n"},{"id":"457331","messageId":"591166e07d87fdb5efc2769d3e2963e3f0412720.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 05/11] submodule-config: avoid memory leak","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:39Z","receivedAt":"2022-06-15T23:36:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 961b130d20c9 (branch: add --recurse-submodules option for branch\ncreation, 2022-01-28), a funny pattern was introduced where first some\nstruct is `xmalloc()`ed, then we resize an array whose element type is\nthe same struct, and then the first struct's contents are copied into\nthe last element of that array.\n\nCrucially, the `xmalloc()`ed memory never gets released.\n\nLet's avoid that memory leak and that memory allocation dance altogether\nby first reallocating the array, then using a pointer to the last array\nelement to go forward.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n submodule-config.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/submodule-config.c b/submodule-config.c\nindex ce3beaf5d4f..51ecbe901ec 100644\n--- a/submodule-config.c\n+++ b/submodule-config.c\n@@ -756,7 +756,10 @@ static void traverse_tree_submodules(struct repository *r,\n \n \t\tif (S_ISGITLINK(name_entry->mode) &&\n \t\t is_tree_submodule_active(r, root_tree, tree_path)) {\n-\t\t\tst_entry = xmalloc(sizeof(*st_entry));\n+\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n+\t\t\t\t out->entry_alloc);\n+\t\t\tst_entry = &out->entries[out->entry_nr++];\n+\n \t\t\tst_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));\n \t\t\t*st_entry->name_entry = *name_entry;\n \t\t\tst_entry->submodule =\n@@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,\n \t\t\t\t\t\troot_tree))\n \t\t\t\tFREE_AND_NULL(st_entry->repo);\n \n-\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n-\t\t\t\t out->entry_alloc);\n-\t\t\tout->entries[out->entry_nr++] = *st_entry;\n \t\t} else if (S_ISDIR(name_entry->mode))\n \t\t\ttraverse_tree_submodules(r, root_tree, tree_path,\n \t\t\t\t\t\t &name_entry->oid, out);\n-- \ngitgitgadget\n\n"},{"id":"457332","messageId":"bc29a9710e3a22e6d660098c4f201f3bfecc54ea.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 06/11] pack-redundant: avoid using uninitialized memory","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:40Z","receivedAt":"2022-06-15T23:36:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the `add_pack()` function, we forgot to initialize the field `next`,\nwhich could potentially lead to readin uninitialized memory later.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/pack-redundant.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex ed9b9013a5f..1f7da1f68b6 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p)\n \t}\n \tl.all_objects_size = l.remaining_objects->size;\n \tl.unique_objects = NULL;\n+\tl.next = NULL;\n \tif (p->pack_local)\n \t\treturn pack_list_insert(&local_packs, &l);\n \telse\n-- \ngitgitgadget\n\n"},{"id":"457333","messageId":"ad3f23a9f0c2c92ac95406fcadc51eec1a9861ff.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:41Z","receivedAt":"2022-06-15T23:36:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn c51f8f94e5b3 (submodule--helper: run update procedures from C,\n2021-08-24), we added code that first obtains the default remote, and\nthen adds that to a `strvec`.\n\nHowever, we never released the default remote's memory.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/submodule--helper.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d7b8004b933..9a8248ffe6a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str\n \t\tchar *hex = oid_to_hex(oid);\n \t\tchar *remote = get_default_remote();\n \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n+\t\tfree(remote);\n \t}\n \n \treturn run_command(&cp);\n-- \ngitgitgadget\n\n"},{"id":"457334","messageId":"a3f79b3e034a3a982b3be7835062493bfdf29ee5.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 08/11] read_index_from(): avoid memory leak","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:42Z","receivedAt":"2022-06-15T23:36:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 998330ac2e7c (read-cache: look for shared index files next to the\nindex, too, 2021-08-26), we added code that allocates memory to store\nthe base path of a shared index, but we never released that memory.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n read-cache.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex e61af3a3d4d..76f372ff917 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2473,15 +2473,15 @@ int read_index_from(struct index_state *istate, const char *path,\n \t\t\t\t the_repository, \"%s\", base_path);\n \tif (!ret) {\n \t\tchar *path_copy = xstrdup(path);\n-\t\tconst char *base_path2 = xstrfmt(\"%s/sharedindex.%s\",\n-\t\t\t\t\t\t dirname(path_copy),\n-\t\t\t\t\t\t base_oid_hex);\n+\t\tchar *base_path2 = xstrfmt(\"%s/sharedindex.%s\",\n+\t\t\t\t\t dirname(path_copy), base_oid_hex);\n \t\tfree(path_copy);\n \t\ttrace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n \t\t\t\t\t the_repository, \"%s\", base_path2);\n \t\tret = do_read_index(split_index->base, base_path2, 1);\n \t\ttrace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n \t\t\t\t\t the_repository, \"%s\", base_path2);\n+\t\tfree(base_path2);\n \t}\n \tif (!oideq(&split_index->base_oid, &split_index->base->oid))\n \t\tdie(_(\"broken index, expect %s in %s, got %s\"),\n-- \ngitgitgadget\n\n"},{"id":"457335","messageId":"b176e5764f990fbf8c132ae1563027531bd8cc9f.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:43Z","receivedAt":"2022-06-15T23:36:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 94cd775a6c52 (pack-mtimes: support reading .mtimes files,\n2022-05-20), code was added to close the file descriptor corresponding\nto the mtimes file.\n\nHowever, it is possible that opening that file failed, in which case we\nare closing a file descriptor with the value `-1`. Let's guard that\n`close()` call.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n pack-mtimes.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/pack-mtimes.c b/pack-mtimes.c\nindex 0e0aafdcb06..0f9785fc5e4 100644\n--- a/pack-mtimes.c\n+++ b/pack-mtimes.c\n@@ -89,7 +89,8 @@ cleanup:\n \t\t*data_p = data;\n \t}\n \n-\tclose(fd);\n+\tif (fd >= 0)\n+\t\tclose(fd);\n \treturn ret;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"457336","messageId":"0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 10/11] relative_url(): fix incorrect condition","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:44Z","receivedAt":"2022-06-15T23:36:20Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 63e95beb085c (submodule: port resolve_relative_url from shell to C,\n2016-04-15), we added a loop over `url` where we are looking for `../`\nor `./` components.\n\nThe loop condition we used is the pointer `url` itself, which is clearly\nnot what we wanted.\n\nPointed out by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n remote.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9b9bbfe80ec..bded6acbfe8 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,\n \t * When the url starts with '../', remove that and the\n \t * last directory in remoteurl.\n \t */\n-\twhile (url) {\n+\twhile (*url) {\n \t\tif (starts_with_dot_dot_slash_native(url)) {\n \t\t\turl += 3;\n \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n-- \ngitgitgadget\n\n"},{"id":"457337","messageId":"d674aefa78bdb6d255e40af2f308abf8a87a593a.1655336146.git.gitgitgadget@gmail.com","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"[PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-15T23:35:45Z","receivedAt":"2022-06-15T23:36:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nAccording to the manual:\n\n\tEach invocation of va_copy() must be matched by a corresponding\n\tinvocation of va_end() in the same function.\n\nNote: There is another instance of `va_copy()` in `usage.c` that is\nmissing a `va_end()` call, in `BUG_vfl()`. It does not matter there,\nthough, because that function either `exit()`s or `abort()`s, anyway.\n\nReported by Coverity.\n\nSigned-off-by: Johannes Schindelin \n---\n usage.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/usage.c b/usage.c\nindex 79900d0287f..4849eb75785 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -343,6 +343,7 @@ void bug_fl(const char *file, int line, const char *fmt, ...)\n \tBUG_vfl_common(file, line, fmt, ap);\n \tva_end(ap);\n \ttrace2_cmd_error_va(fmt, cp);\n+\tva_end(cp);\n }\n \n #ifdef SUPPRESS_ANNOTATED_LEAKS\n-- \ngitgitgadget\n"},{"id":"457343","messageId":"xmqqo7ytxnge.fsf@gitster.g","threadId":"58019","inReplyTo":"pull.1264.git.1655336146.gitgitgadget@gmail.com","subject":"Re: [PATCH 00/11] Coverity fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:05:21Z","receivedAt":"2022-06-16T04:05:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> The release window is a fine time to concentrate on bug fixes,\n\nWell, it certainly is better than random shiny new features or\nrefactoring for the sake of refactoring, but finding and fixing a\nbug that has been with us since forever without anybody noticing is\nnot all that ultra-urgent, either ;-)\n\nIt is a good time to concentrate on finding and fixing regressions.\n\n> and Coverity\n> can help identify bugs. These are only a couple of fixes, Coverity reported\n> many more, but most of the reports seemed either bogus or harmless (such as\n> resource leaks in test helpers).\n\nThanks.\n\n>\n> Johannes Schindelin (11):\n> mingw: avoid accessing uninitialized memory in `is_executable()`\n> fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`\n> submodule--helper: avoid memory leak in `update_submodule()`\n> get_oid_with_context_1(): avoid use-after-free\n> submodule-config: avoid memory leak\n> pack-redundant: avoid using uninitialized memory\n> submodule--helper: avoid memory leak when fetching submodules\n> read_index_from(): avoid memory leak\n> pack-mtimes: avoid closing a bogus file descriptor\n> relative_url(): fix incorrect condition\n> bug_fl(): add missing `va_end()` call\n>\n> builtin/pack-redundant.c | 1 +\n> builtin/submodule--helper.c | 3 +++\n> fsmonitor-settings.c | 8 ++++++--\n> object-name.c | 6 ++++--\n> pack-mtimes.c | 3 ++-\n> read-cache.c | 6 +++---\n> remote.c | 2 +-\n> run-command.c | 2 +-\n> submodule-config.c | 8 ++++----\n> usage.c | 1 +\n> 10 files changed, 26 insertions(+), 14 deletions(-)\n>\n>\n> base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1264%2Fdscho%2Fcoverity-fixes-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1264/dscho/coverity-fixes-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1264\n"},{"id":"457344","messageId":"xmqqk09hxndi.fsf@gitster.g","threadId":"58019","inReplyTo":"679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:07:05Z","receivedAt":"2022-06-16T04:07:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> On Windows, we open files we suspect might be scripts, read the first\n> two bytes, and see whether they indicate a hash-bang line. We do not\n> initialize the byte _after_ those two bytes, therefore `strcmp()` is\n> inappropriate here.\n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> run-command.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nThis has been with us since cc3b7a97 (Windows: Make 'git help -a'\nwork., 2008-01-14) and apparently nobody made loud enough noises\nto make us aware since then.\n\nThe fix is trivially correct, of course.\n\nWill queue.\n\n\n>\n> diff --git a/run-command.c b/run-command.c\n> index 14f17830f51..2ba38850b4d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -154,7 +154,7 @@ int is_executable(const char *name)\n> \t\tn = read(fd, buf, 2);\n> \t\tif (n == 2)\n> \t\t\t/* look for a she-bang */\n> -\t\t\tif (!strcmp(buf, \"#!\"))\n> +\t\t\tif (!memcmp(buf, \"#!\", 2))\n> \t\t\t\tst.st_mode |= S_IXUSR;\n> \t\tclose(fd);\n> \t}\n"},{"id":"457345","messageId":"xmqqfsk5xn8n.fsf@gitster.g","threadId":"58019","inReplyTo":"6d409ad45867aa5193c15828614e865e53ac0c57.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 02/11] fsmonitor: avoid memory leak in `fsm_settings__get_incompatible_msg()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:10:00Z","receivedAt":"2022-06-16T04:10:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> fsmonitor-settings.c | 8 ++++++--\n> 1 file changed, 6 insertions(+), 2 deletions(-)\n>\n> diff --git a/fsmonitor-settings.c b/fsmonitor-settings.c\n> index 658cb79da01..464424a1e92 100644\n> --- a/fsmonitor-settings.c\n> +++ b/fsmonitor-settings.c\n> @@ -202,11 +202,15 @@ char *fsm_settings__get_incompatible_msg(const struct repository *r,\n> \tcase FSMONITOR_REASON_OK:\n> \t\tgoto done;\n\nObviously correct, but the placement of these ...\n\n> -\tcase FSMONITOR_REASON_BARE:\n> +\tcase FSMONITOR_REASON_BARE: {\n> +\t\tchar *cwd = xgetcwd();\n> +\n> \t\tstrbuf_addf(&msg,\n> \t\t\t _(\"bare repository '%s' is incompatible with fsmonitor\"),\n> -\t\t\t xgetcwd());\n> +\t\t\t cwd);\n> +\t\tfree(cwd);\n> \t\tgoto done;\n> +\t}\n\n... braces are misleading and confusing.\n\n> \n> \tcase FSMONITOR_REASON_ERROR:\n> \t\tstrbuf_addf(&msg,\n"},{"id":"457346","messageId":"xmqq35g5xmmv.fsf@gitster.g","threadId":"58019","inReplyTo":"877a45867ae368bf9e053caedcb6cf421e02344d.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:23:04Z","receivedAt":"2022-06-16T04:23:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/submodule--helper.c | 2 ++\n> 1 file changed, 2 insertions(+)\n\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 5c77dfcffee..d7b8004b933 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)\n> \n> \t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n> \t\t\t\t\t\t\t\t update_data->prefix);\n> +\t\tfree(prefixed_path);\n> +\n\nThis function has two very similar code block that computes\nprefixed_path depending on the same condition, and one frees the\nvariable correctly while the other one (i.e. this one) forgets to do\nso, which is irritating to see.\n\nPerhaps the whole \"we have update_data structure, in which\nrecursive_prefix, sm_path and prefix members in it; please set the\ndisplaypath member based on these values\" should become a helper\nfunction, e.g.\n\n\tstatic const char *displaypath_from_update_data(struct update_data *u)\n\t{\n\t\tchar *pp, *ret;\n\n\t\tif (u->recursive_prefix)\n\t\t\tpp = xstrfmt(\"%s%s\", u->recursive_prefix, u->sm_path);\n\t\telse\n\t\t\tpp = xstrdup(u->sm_path);\n\n\t\tret = get_submodule_displaypath(pp, u->prefix);\n\t\tfree(pp);\n\t\treturn ret;\n\t}\n\nto avoid duplicated computation.\n\nBut the whole thing may become moot, as there seems to be a move to\nget rid of submodule--helper.c altogether?\n\nI'll refrain from touching this patch and instead redirect it to\nGlen; perhaps removal of submodule--helper.c involves moving the\ncode here to another file or something, in which case it is far\neasier if I outsource that to somebody who is actually working on\nthe file ;-)\n\nThanks.\n\n> \t\tnext.prefix = NULL;\n> \t\toidcpy(&next.oid, null_oid());\n> \t\toidcpy(&next.suboid, null_oid());\n"},{"id":"457347","messageId":"xmqqy1xxw7rc.fsf@gitster.g","threadId":"58019","inReplyTo":"cf6bcdb43e5b4abab464c30a914d64dc8e7a9925.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 04/11] get_oid_with_context_1(): avoid use-after-free","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:29:43Z","receivedAt":"2022-06-16T04:29:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> \t\t\tif (ce_stage(ce) == stage) {\n> -\t\t\t\tfree(new_path);\n> \t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n> -\t\t\t\t\t\t\t stage, prefix, cp))\n> +\t\t\t\t\t\t\t stage, prefix, cp)) {\n> +\t\t\t\t\tfree(new_path);\n> \t\t\t\t\treturn -1;\n> +\t\t\t\t}\n> \t\t\t\toidcpy(oid, &ce->oid);\n> \t\t\t\toc->mode = ce->ce_mode;\n> +\t\t\t\tfree(new_path);\n> \t\t\t\treturn 0;\n> \t\t\t}\n\nHmph. Without the \"lets make sure we do not leak in the error code\npath\", it would have been no brainer to avoid this bug in the\noriginal version. Of course the postimage of the above hunk is\ncorrect, but with extra free() sprinkled, it became ugly to the eye.\n\nI wonder if the following is easier to follow.\n\nThanks.\n\n object-name.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git c/object-name.c w/object-name.c\nindex 4d2746574c..57188db7b0 100644\n--- c/object-name.c\n+++ w/object-name.c\n@@ -1971,13 +1971,16 @@ static enum get_oid_result get_oid_with_context_1(struct repository *repo,\n \t\t\t memcmp(ce->name, cp, namelen))\n \t\t\t\tbreak;\n \t\t\tif (ce_stage(ce) == stage) {\n+\t\t\t\tint ret = -1;\n+\n+\t\t\t\tif (!reject_tree_in_index(repo, only_to_die, ce,\n+\t\t\t\t\t\t\t stage, prefix, cp)) {\n+\t\t\t\t\toidcpy(oid, &ce->oid);\n+\t\t\t\t\toc->mode = ce->ce_mode;\n+\t\t\t\t\tret = 0;\n+\t\t\t\t}\n \t\t\t\tfree(new_path);\n-\t\t\t\tif (reject_tree_in_index(repo, only_to_die, ce,\n-\t\t\t\t\t\t\t stage, prefix, cp))\n-\t\t\t\t\treturn -1;\n-\t\t\t\toidcpy(oid, &ce->oid);\n-\t\t\t\toc->mode = ce->ce_mode;\n-\t\t\t\treturn 0;\n+\t\t\t\treturn ret;\n \t\t\t}\n \t\t\tpos++;\n \t\t}\n"},{"id":"457348","messageId":"xmqqmtedw7fm.fsf@gitster.g","threadId":"58019","inReplyTo":"591166e07d87fdb5efc2769d3e2963e3f0412720.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 05/11] submodule-config: avoid memory leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:36:45Z","receivedAt":"2022-06-16T04:36:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 961b130d20c9 (branch: add --recurse-submodules option for branch\n> creation, 2022-01-28), a funny pattern was introduced where first some\n> struct is `xmalloc()`ed, then we resize an array whose element type is\n> the same struct, and then the first struct's contents are copied into\n> the last element of that array.\n\nSigh. The original is butt ugly, with this strange pattern and\nstructure assignments etc. I wonder how something like this slipped\nthrough our reviews.\n\nI wonder if it would help for me to stop trusting reviews by less\nexperienced reviewers too much, and instead give sanity checks to\nmore patches myself from now on, but I certainly cannot afford the\ntime and my mental health to do so for all the patches X-<.\n\nWill queue.\n\n> \t\tif (S_ISGITLINK(name_entry->mode) &&\n> \t\t is_tree_submodule_active(r, root_tree, tree_path)) {\n> -\t\t\tst_entry = xmalloc(sizeof(*st_entry));\n> +\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n> +\t\t\t\t out->entry_alloc);\n> +\t\t\tst_entry = &out->entries[out->entry_nr++];\n> +\n> \t\t\tst_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));\n> \t\t\t*st_entry->name_entry = *name_entry;\n> \t\t\tst_entry->submodule =\n> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,\n> \t\t\t\t\t\troot_tree))\n> \t\t\t\tFREE_AND_NULL(st_entry->repo);\n> \n> -\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n> -\t\t\t\t out->entry_alloc);\n> -\t\t\tout->entries[out->entry_nr++] = *st_entry;\n> \t\t} else if (S_ISDIR(name_entry->mode))\n> \t\t\ttraverse_tree_submodules(r, root_tree, tree_path,\n> \t\t\t\t\t\t &name_entry->oid, out);\n"},{"id":"457349","messageId":"xmqqilp1w6oi.fsf@gitster.g","threadId":"58019","inReplyTo":"bc29a9710e3a22e6d660098c4f201f3bfecc54ea.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 06/11] pack-redundant: avoid using uninitialized memory","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:53:01Z","receivedAt":"2022-06-16T04:53:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In the `add_pack()` function, we forgot to initialize the field `next`,\n> which could potentially lead to readin uninitialized memory later.\n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/pack-redundant.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\n> index ed9b9013a5f..1f7da1f68b6 100644\n> --- a/builtin/pack-redundant.c\n> +++ b/builtin/pack-redundant.c\n> @@ -526,6 +526,7 @@ static struct pack_list * add_pack(struct packed_git *p)\n> \t}\n> \tl.all_objects_size = l.remaining_objects->size;\n> \tl.unique_objects = NULL;\n> +\tl.next = NULL;\n> \tif (p->pack_local)\n> \t\treturn pack_list_insert(&local_packs, &l);\n> \telse\n\t\treturn pack_list_insert(&altodb_packs, &l);\n\n\nThe pack_list_insert reads like so:\n\nstatic inline struct pack_list * pack_list_insert(struct pack_list **pl,\n\t\t\t\t\t struct pack_list *entry)\n{\n\tstruct pack_list *p = xmalloc(sizeof(struct pack_list));\n\tmemcpy(p, entry, sizeof(struct pack_list));\n\tp->next = *pl;\n\t*pl = p;\n\treturn p;\n}\n\n\nIOW, we prepare members of \"l\", pass it as \"entry\" to pack_list_insert(),\nbut the helper function allocates a new piece of memory, copies \"l\"\nto it, and then sets the .next member of it to a reasonable value.\n\nAnd after that, the \"l\" that was merely used as a template goes out\nof scope.\n\nSo I think this is a false positive. memcpy(p) does copy the\nuninitialized garbage that is held in entry->next to p->next, but it\nis overwritten with a reasonable address immediately after that.\n\n\n"},{"id":"457350","messageId":"Yqq3O5hykBecoVKQ@coredump.intra.peff.net","threadId":"58019","inReplyTo":"d674aefa78bdb6d255e40af2f308abf8a87a593a.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-16T04:53:15Z","receivedAt":"2022-06-16T04:53:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n> \n> According to the manual:\n> \n> \tEach invocation of va_copy() must be matched by a corresponding\n> \tinvocation of va_end() in the same function.\n> \n> Note: There is another instance of `va_copy()` in `usage.c` that is\n> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,\n> though, because that function either `exit()`s or `abort()`s, anyway.\n> \n> Reported by Coverity.\n\nThis was introduced by the recent 0cc05b044f (usage.c: add a non-fatal\nbug() function to go with BUG(), 2022-06-02). But there's a much worse\nbug in the same function. The code introduced by that patch does:\n\n va_list ap, cp;\n [...]\n va_copy(cp, ap);\n va_start(ap, fmt);\n\nSo \"cp\" is copied from \"ap\" before we have actually initialized \"ap\".\nIt's surprising that this works at all. The two lines should be flipped.\n\nIMHO, since we're in the actual varargs function itself, it would be\nsimpler to just bracket each use with start/end, rather than copying,\nlike:\n\ndiff --git a/usage.c b/usage.c\nindex 79900d0287..56e29d6cd6 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)\n int bug_called_must_BUG;\n void bug_fl(const char *file, int line, const char *fmt, ...)\n {\n-\tva_list ap, cp;\n+\tva_list ap;\n \n \tbug_called_must_BUG = 1;\n \n-\tva_copy(cp, ap);\n \tva_start(ap, fmt);\n \tBUG_vfl_common(file, line, fmt, ap);\n \tva_end(ap);\n-\ttrace2_cmd_error_va(fmt, cp);\n+\n+\tva_start(ap, fmt);\n+\ttrace2_cmd_error_va(fmt, ap);\n+\tva_end(ap);\n }\n \n #ifdef SUPPRESS_ANNOTATED_LEAKS\n\nbut I am happy with any solution that is correct. :)\n\n-Peff\n"},{"id":"457351","messageId":"xmqqedzpw6k8.fsf@gitster.g","threadId":"58019","inReplyTo":"ad3f23a9f0c2c92ac95406fcadc51eec1a9861ff.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 07/11] submodule--helper: avoid memory leak when fetching submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T04:55:35Z","receivedAt":"2022-06-16T04:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In c51f8f94e5b3 (submodule--helper: run update procedures from C,\n> 2021-08-24), we added code that first obtains the default remote, and\n> then adds that to a `strvec`.\n>\n> However, we never released the default remote's memory.\n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> builtin/submodule--helper.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d7b8004b933..9a8248ffe6a 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2208,6 +2208,7 @@ static int fetch_in_submodule(const char *module_path, int depth, int quiet, str\n> \t\tchar *hex = oid_to_hex(oid);\n> \t\tchar *remote = get_default_remote();\n> \t\tstrvec_pushl(&cp.args, remote, hex, NULL);\n> +\t\tfree(remote);\n\nTrivially correct. Will queue.\n\n> \t}\n> \n> \treturn run_command(&cp);\n"},{"id":"457352","messageId":"xmqq8rpxw6ch.fsf@gitster.g","threadId":"58019","inReplyTo":"Yqq3O5hykBecoVKQ@coredump.intra.peff.net","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T05:00:14Z","receivedAt":"2022-06-16T05:00:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>\n>> From: Johannes Schindelin \n>> \n>> According to the manual:\n>> \n>> \tEach invocation of va_copy() must be matched by a corresponding\n>> \tinvocation of va_end() in the same function.\n>> \n>> Note: There is another instance of `va_copy()` in `usage.c` that is\n>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,\n>> though, because that function either `exit()`s or `abort()`s, anyway.\n>> \n>> Reported by Coverity.\n>\n> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal\n> bug() function to go with BUG(), 2022-06-02). But there's a much worse\n> bug in the same function. The code introduced by that patch does:\n>\n> va_list ap, cp;\n> [...]\n> va_copy(cp, ap);\n> va_start(ap, fmt);\n>\n> So \"cp\" is copied from \"ap\" before we have actually initialized \"ap\".\n> It's surprising that this works at all. The two lines should be flipped.\n\nYes, it is surprising. Perhaps it is not working at all.\n\nThanks for an extra set of eyeballs.\n\n> IMHO, since we're in the actual varargs function itself, it would be\n> simpler to just bracket each use with start/end, rather than copying,\n> like:\n>\n> diff --git a/usage.c b/usage.c\n> index 79900d0287..56e29d6cd6 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)\n> int bug_called_must_BUG;\n> void bug_fl(const char *file, int line, const char *fmt, ...)\n> {\n> -\tva_list ap, cp;\n> +\tva_list ap;\n> \n> \tbug_called_must_BUG = 1;\n> \n> -\tva_copy(cp, ap);\n> \tva_start(ap, fmt);\n> \tBUG_vfl_common(file, line, fmt, ap);\n> \tva_end(ap);\n> -\ttrace2_cmd_error_va(fmt, cp);\n> +\n> +\tva_start(ap, fmt);\n> +\ttrace2_cmd_error_va(fmt, ap);\n> +\tva_end(ap);\n> }\n> \n> #ifdef SUPPRESS_ANNOTATED_LEAKS\n>\n> but I am happy with any solution that is correct. :)\n>\n> -Peff\n"},{"id":"457353","messageId":"xmqq4k0lw68q.fsf@gitster.g","threadId":"58019","inReplyTo":"0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] relative_url(): fix incorrect condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T05:02:29Z","receivedAt":"2022-06-16T05:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,\n> 2016-04-15), we added a loop over `url` where we are looking for `../`\n> or `./` components.\n>\n> The loop condition we used is the pointer `url` itself, which is clearly\n> not what we wanted.\n\n8/11, 9/11, and 10/11 are trivially correct. Will queue.\n\nThanks.\n\n\n>\n> Pointed out by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 9b9bbfe80ec..bded6acbfe8 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,\n> \t * When the url starts with '../', remove that and the\n> \t * last directory in remoteurl.\n> \t */\n> -\twhile (url) {\n> +\twhile (*url) {\n> \t\tif (starts_with_dot_dot_slash_native(url)) {\n> \t\t\turl += 3;\n> \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n"},{"id":"457368","messageId":"220616.86o7yswyek.gmgdl@evledraar.gmail.com","threadId":"58019","inReplyTo":"xmqq8rpxw6ch.fsf@gitster.g","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-16T13:02:03Z","receivedAt":"2022-06-16T13:06:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 15 2022, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> On Wed, Jun 15, 2022 at 11:35:45PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>>\n>>> From: Johannes Schindelin \n>>> \n>>> According to the manual:\n>>> \n>>> \tEach invocation of va_copy() must be matched by a corresponding\n>>> \tinvocation of va_end() in the same function.\n>>> \n>>> Note: There is another instance of `va_copy()` in `usage.c` that is\n>>> missing a `va_end()` call, in `BUG_vfl()`. It does not matter there,\n>>> though, because that function either `exit()`s or `abort()`s, anyway.\n>>> \n>>> Reported by Coverity.\n>>\n>> This was introduced by the recent 0cc05b044f (usage.c: add a non-fatal\n>> bug() function to go with BUG(), 2022-06-02). But there's a much worse\n>> bug in the same function. The code introduced by that patch does:\n>>\n>> va_list ap, cp;\n>> [...]\n>> va_copy(cp, ap);\n>> va_start(ap, fmt);\n>>\n>> So \"cp\" is copied from \"ap\" before we have actually initialized \"ap\".\n>> It's surprising that this works at all. The two lines should be flipped.\n>\n> Yes, it is surprising. Perhaps it is not working at all.\n>\n> Thanks for an extra set of eyeballs.\n\nIt's \"working\" now in the sense that we run this code, and even test\nthat trace2 output specifically, see the tests in 0cc05b044fd (usage.c:\nadd a non-fatal bug() function to go with BUG(), 2022-06-02).\n\nBut obviously that's a bad use of the varargs API, I just don't know how\nwe've been getting away with it in practice, sorry about that.\n\nThe fix Peff's got here LGTM. I can (re)submit it with\nformat-patch+send-email after giving it a commit message describing the\nissue if you'd like, but the change would be the same.\n\nI'm also happy for you to pick up the upthread directly, but I don't see\nthat you did that already in one of the integration branches (but\nperhaps I missed one).\n\nJust let me know, but I didn't want to re-submit this without asking, in\ncase I'd step on any toes (or submit something in duplicate).\n"},{"id":"457374","messageId":"220616.86k09gwx6i.gmgdl@evledraar.gmail.com","threadId":"58019","inReplyTo":"0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] relative_url(): fix incorrect condition","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-16T13:09:38Z","receivedAt":"2022-06-16T13:33:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jun 15 2022, Johannes Schindelin via GitGitGadget wrote:\n\n> From: Johannes Schindelin \n>\n> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,\n> 2016-04-15), we added a loop over `url` where we are looking for `../`\n> or `./` components.\n>\n> The loop condition we used is the pointer `url` itself, which is clearly\n> not what we wanted.\n\nClearly, but having looked at this I think it's useful to note that\ncoverity must be (I don't know what it actually emitted) be complaining\nabout the \"url\" condition being useless, i.e. it's the same as a \"while\n(1)\", not that we run off the end of the string.\n\nI.e. due to the way those start_with*() functions work we would abort\nearly if we were running off th end of the string.\n\n> Pointed out by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 9b9bbfe80ec..bded6acbfe8 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,\n> \t * When the url starts with '../', remove that and the\n> \t * last directory in remoteurl.\n> \t */\n> -\twhile (url) {\n> +\twhile (*url) {\n> \t\tif (starts_with_dot_dot_slash_native(url)) {\n> \t\t\turl += 3;\n> \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n\nWhich I tested with this:\n\t\n\tdiff --git a/remote.c b/remote.c\n\tindex 9b9bbfe80ec..e049bbb791c 100644\n\t--- a/remote.c\n\t+++ b/remote.c\n\t@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,\n\t \t * When the url starts with '../', remove that and the\n\t \t * last directory in remoteurl.\n\t \t */\n\t-\twhile (url) {\n\t+\twhile (1) {\n\t \t\tif (starts_with_dot_dot_slash_native(url)) {\n\t \t\t\turl += 3;\n\t \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n\t-\t\t} else if (starts_with_dot_slash_native(url))\n\t+\t\t} else if (starts_with_dot_slash_native(url)) {\n\t \t\t\turl += 2;\n\t-\t\telse\n\t-\t\t\tbreak;\n\t+\t\t} else if (!*url) {\n\t+\t\t\tBUG(\"ran off the end of our url?\");\n\t+\t\t} else {\n\t+\t\t\tbreak; \n\t+\t\t}\n\t \t}\n\t \tstrbuf_reset(&sb);\n\t \tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n\t\nWhich will fail e.g. on this test in t3420-rebase-autostash.sh:\n\n\t+ git submodule add ./ sub\n\tBUG: remote.c:2856: ran off the end of our url?\n\tAborted\n\nI worried a bit about this since we released this with v2.9.0, so in all\nthis time we haven't been infinitely looping on this case, or at least\nhaven't had any reports about that.\n\nSo if we hadn't been catching this in starts_with_*() I wouldn't be\nconfident that this was the correct fix, yes it's almost certainly not\nwhat not what was intended, but if we change it to that are we confident\nthat the side-effects are going to be what we want?\n\nBut in this case I'm pretty sure that the behavior before/after will be\nthe same, and that the only change will be to make coverity happier, and\nthe code less confusing.\n\n"},{"id":"457385","messageId":"xmqqwndgv9vv.fsf@gitster.g","threadId":"58019","inReplyTo":"0bf70e65d2c9e187203a77088ff0f7d18510caca.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 10/11] relative_url(): fix incorrect condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T16:41:24Z","receivedAt":"2022-06-16T16:41:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 63e95beb085c (submodule: port resolve_relative_url from shell to C,\n> 2016-04-15), we added a loop over `url` where we are looking for `../`\n> or `./` components.\n>\n> The loop condition we used is the pointer `url` itself, which is clearly\n> not what we wanted.\n>\n> Pointed out by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> remote.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 9b9bbfe80ec..bded6acbfe8 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -2846,7 +2846,7 @@ char *relative_url(const char *remote_url, const char *url,\n> \t * When the url starts with '../', remove that and the\n> \t * last directory in remoteurl.\n> \t */\n> -\twhile (url) {\n> +\twhile (*url) {\n> \t\tif (starts_with_dot_dot_slash_native(url)) {\n> \t\t\turl += 3;\n> \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n\t\t} else if (starts_with_dot_slash_native(url))\n\t\t\turl += 2;\n\t\telse\n\t\t\tbreak;\n\t}\n\nLooking at the \"problematic\" code again, the original is bad in the\nsense that it is utterly misleading, but not quite \"incorrect\", I\nwould think.\n\nIn other words, what the original wanted to write is an infinite\nloop that uses an explicit \"break\" to terminate the iteration, so we\nshould have written \"while (1)\" in the first place.\n\nWe know that earlier part of the function already depends on url\nbeing not NULL, and nothing in the loop makes it NULL. *url that is\nNUL would make the two starts_with_frotz() tests fail and causes the\nloop to terminate by hitting \"break\", so while it is correct to\ncheck *url, it is wasting cycles.\n\nBut this patch is not wrong per-se, so I still have it in my tree\n;-)\n\nThanks.\n"},{"id":"457390","messageId":"kl6lsfo44hvd.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58019","inReplyTo":"xmqq35g5xmmv.fsf@gitster.g","subject":"Re: [PATCH 03/11] submodule--helper: avoid memory leak in `update_submodule()`","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-16T17:51:02Z","receivedAt":"2022-06-16T17:51:07Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n>> From: Johannes Schindelin \n>>\n>> Reported by Coverity.\n>>\n>> Signed-off-by: Johannes Schindelin \n>> ---\n>> builtin/submodule--helper.c | 2 ++\n>> 1 file changed, 2 insertions(+)\n>\n>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 5c77dfcffee..d7b8004b933 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2512,6 +2512,8 @@ static int update_submodule(struct update_data *update_data)\n>> \n>> \t\tnext.recursive_prefix = get_submodule_displaypath(prefixed_path,\n>> \t\t\t\t\t\t\t\t update_data->prefix);\n>> +\t\tfree(prefixed_path);\n>> +\n>\n> This function has two very similar code block that computes\n> prefixed_path depending on the same condition, and one frees the\n> variable correctly while the other one (i.e. this one) forgets to do\n> so, which is irritating to see.\n\nGood eye, I hadn't noticed this.\n\nIn this case, we're computing the same thing twice in the function -\nfirst unconditionally, then conditionally. So we can actually just reuse\nthe first result by doing something like\n\n\t\tnext.recursive_prefix = xstrfmt(\"%s/\", update_data->displaypath);\n\n(note that we need to append a \"/\" at the end)\n\n> Perhaps the whole \"we have update_data structure, in which\n> recursive_prefix, sm_path and prefix members in it; please set the\n> displaypath member based on these values\" should become a helper\n> function, e.g.\n>\n> \tstatic const char *displaypath_from_update_data(struct update_data *u)\n> \t{\n> \t\tchar *pp, *ret;\n>\n> \t\tif (u->recursive_prefix)\n> \t\t\tpp = xstrfmt(\"%s%s\", u->recursive_prefix, u->sm_path);\n> \t\telse\n> \t\t\tpp = xstrdup(u->sm_path);\n>\n> \t\tret = get_submodule_displaypath(pp, u->prefix);\n> \t\tfree(pp);\n> \t\treturn ret;\n> \t}\n>\n> to avoid duplicated computation.\n\nEven better, I recently noticed that .recursive_prefix can probably just\nbe replaced with \"--super-prefix\", and that will let us dispatch to\nget_submodule_displaypath() without this extra dance here.\n\nI'll flesh that out into a series and send it soon.\n\n> But the whole thing may become moot, as there seems to be a move to\n> get rid of submodule--helper.c altogether?\n>\n>\n> I'll refrain from touching this patch and instead redirect it to\n> Glen; perhaps removal of submodule--helper.c involves moving the\n> code here to another file or something, in which case it is far\n> easier if I outsource that to somebody who is actually working on\n> the file ;-)\n\ncc-ed Ævar, who knows even more than I do :)\n\ngit-submodule.sh is definitely going away soon \\o/ but I don't think we\nhave plans to get rid of submodule--helper.c just yet, so I think we\nshould probably still keep this patch.\n\n>\n> Thanks.\n>\n>> \t\tnext.prefix = NULL;\n>> \t\toidcpy(&next.oid, null_oid());\n>> \t\toidcpy(&next.suboid, null_oid());\n"},{"id":"457391","messageId":"xmqqr13otrv6.fsf@gitster.g","threadId":"58019","inReplyTo":"220616.86k09gwx6i.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 10/11] relative_url(): fix incorrect condition","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T17:55:57Z","receivedAt":"2022-06-16T17:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> -\twhile (url) {\n>> +\twhile (*url) {\n>> \t\tif (starts_with_dot_dot_slash_native(url)) {\n>> \t\t\turl += 3;\n>> \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n>\n> Which I tested with this:\n> \t\n> \tdiff --git a/remote.c b/remote.c\n> \tindex 9b9bbfe80ec..e049bbb791c 100644\n> \t--- a/remote.c\n> \t+++ b/remote.c\n> \t@@ -2846,14 +2846,17 @@ char *relative_url(const char *remote_url, const char *url,\n> \t \t * When the url starts with '../', remove that and the\n> \t \t * last directory in remoteurl.\n> \t \t */\n> \t-\twhile (url) {\n> \t+\twhile (1) {\n> \t \t\tif (starts_with_dot_dot_slash_native(url)) {\n> \t \t\t\turl += 3;\n> \t \t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n> \t-\t\t} else if (starts_with_dot_slash_native(url))\n> \t+\t\t} else if (starts_with_dot_slash_native(url)) {\n> \t \t\t\turl += 2;\n> \t-\t\telse\n> \t-\t\t\tbreak;\n> \t+\t\t} else if (!*url) {\n> \t+\t\t\tBUG(\"ran off the end of our url?\");\n> \t+\t\t} else {\n> \t+\t\t\tbreak; \n> \t+\t\t}\n> \t \t}\n> \t \tstrbuf_reset(&sb);\n> \t \tstrbuf_addf(&sb, \"%s%s%s\", remoteurl, colonsep ? \":\" : \"/\", url);\n> \t\n> Which will fail e.g. on this test in t3420-rebase-autostash.sh:\n>\n> \t+ git submodule add ./ sub\n> \tBUG: remote.c:2856: ran off the end of our url?\n> \tAborted\n\nSorry, but I am confused. I do not see the point of that test\nbefore the BUG.\n\nI agree that this loop over url is \"we want to loop forever, and\nstop immediately when url does not start with ../ or ./\" infinite\nloop, and it is better written with \"while(1)\". If you do not make\nany other changes that confuses me, we'll get this:\n\n-\twhile (*url) {\n+\twhile (1) {\n\t\tif (starts_with_dot_dot_slash_native(url)) {\n\t\t\turl += 3;\n\t\t\tcolonsep |= chop_last_dir(&remoteurl, is_relative);\n\t\t} else if (starts_with_dot_slash_native(url))\n\t\t\turl += 2;\n\t\telse\n\t\t\tbreak;\n\t}\n\nWe know url upon entry is not NULL. starts_with_dot_*() ends up\ncalling dir.c:path_match_flags() and the first thing it does is to\nreturn if the byte is not '.'. So the difference is whether you\nleave the loop with the \"while\" condition, or you fail the two\nstarts_with_dot_*() calls and hit the \"break\" at the end in the\nloop.\n\nIOW, running of the end of the URL is not a BUG, is it?\n\n"},{"id":"457392","messageId":"xmqqk09gtriq.fsf@gitster.g","threadId":"58019","inReplyTo":"220616.86o7yswyek.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T18:03:25Z","receivedAt":"2022-06-16T18:03:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But obviously that's a bad use of the varargs API, I just don't know how\n> we've been getting away with it in practice, sorry about that.\n\nExactly. We three all expressed our surprises on why it \"works\".\nNobody offered an explanation, though, which leaves us in suspense\n;-)\n\n> The fix Peff's got here LGTM. I can (re)submit it with\n> format-patch+send-email after giving it a commit message describing the\n> issue if you'd like, but the change would be the same.\n\nYup, I think the code change there looks the most sensible.\n\nThanks, all.\n"},{"id":"457393","messageId":"kl6lpmj84h18.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58019","inReplyTo":"xmqqmtedw7fm.fsf@gitster.g","subject":"Re: [PATCH 05/11] submodule-config: avoid memory leak","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-06-16T18:09:07Z","receivedAt":"2022-06-16T18:09:11Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Junio C Hamano writes:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n>> From: Johannes Schindelin \n>>\n>> In 961b130d20c9 (branch: add --recurse-submodules option for branch\n>> creation, 2022-01-28), a funny pattern was introduced where first some\n>> struct is `xmalloc()`ed, then we resize an array whose element type is\n>> the same struct, and then the first struct's contents are copied into\n>> the last element of that array.\n>\n> Sigh. The original is butt ugly, with this strange pattern and\n> structure assignments etc. I wonder how something like this slipped\n> through our reviews.\n\nGah, I have no excuses for bad code I've already written, I can only\nstrive to write better code next time.\n\nThanks Johannes for spotting and fixing this.\n\n> I wonder if it would help for me to stop trusting reviews by less\n> experienced reviewers too much, and instead give sanity checks to\n> more patches myself from now on, but I certainly cannot afford the\n> time and my mental health to do so for all the patches X-<.\n\nI seem to recall that this was reviewed by fairly experienced folks. I'm\nguessing it slipped through due to reviewer fatigue, which might be a\ngood argument for adding more tooling to lighten reviewer load/patch up\nthe gaps.\n\n>\n> Will queue.\n>\n>> \t\tif (S_ISGITLINK(name_entry->mode) &&\n>> \t\t is_tree_submodule_active(r, root_tree, tree_path)) {\n>> -\t\t\tst_entry = xmalloc(sizeof(*st_entry));\n>> +\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n>> +\t\t\t\t out->entry_alloc);\n>> +\t\t\tst_entry = &out->entries[out->entry_nr++];\n>> +\n>> \t\t\tst_entry->name_entry = xmalloc(sizeof(*st_entry->name_entry));\n>> \t\t\t*st_entry->name_entry = *name_entry;\n>> \t\t\tst_entry->submodule =\n>> @@ -766,9 +769,6 @@ static void traverse_tree_submodules(struct repository *r,\n>> \t\t\t\t\t\troot_tree))\n>> \t\t\t\tFREE_AND_NULL(st_entry->repo);\n>> \n>> -\t\t\tALLOC_GROW(out->entries, out->entry_nr + 1,\n>> -\t\t\t\t out->entry_alloc);\n>> -\t\t\tout->entries[out->entry_nr++] = *st_entry;\n>> \t\t} else if (S_ISDIR(name_entry->mode))\n>> \t\t\ttraverse_tree_submodules(r, root_tree, tree_path,\n>> \t\t\t\t\t\t &name_entry->oid, out);\n"},{"id":"457400","messageId":"YquCaE+Vw9P/fybU@coredump.intra.peff.net","threadId":"58019","inReplyTo":"xmqqk09gtriq.fsf@gitster.g","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-16T19:20:08Z","receivedAt":"2022-06-16T19:21:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2022 at 11:03:25AM -0700, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> \n> > But obviously that's a bad use of the varargs API, I just don't know how\n> > we've been getting away with it in practice, sorry about that.\n> \n> Exactly. We three all expressed our surprises on why it \"works\".\n> Nobody offered an explanation, though, which leaves us in suspense\n> ;-)\n\nBeing the curious sort, I ran it in a debugger. And indeed, \"cp\" is\nfilled with uninitialized garbage. The reason it works is that the test\ncalls bug() with a format string that does not contain any placeholders.\nAnd thus our eventual call to vsnprintf() does not ever look at \"cp\" at\nall!\n\n> > The fix Peff's got here LGTM. I can (re)submit it with\n> > format-patch+send-email after giving it a commit message describing the\n> > issue if you'd like, but the change would be the same.\n> \n> Yup, I think the code change there looks the most sensible.\n\nI'll wrap it up with a commit message and modify the test to be more\nthorough.\n\n-Peff\n"},{"id":"457401","messageId":"98ff788a-d621-cf3f-aed5-66ae763b381f@web.de","threadId":"58019","inReplyTo":"679ea7421f73ce41515aca549982233f88bcefef.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-06-16T19:53:28Z","receivedAt":"2022-06-16T19:53:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget:\n> From: Johannes Schindelin \n>\n> On Windows, we open files we suspect might be scripts, read the first\n> two bytes, and see whether they indicate a hash-bang line. We do not\n> initialize the byte _after_ those two bytes, therefore `strcmp()` is\n> inappropriate here.\n\nHmm, but buf _is_ initialized fully? Line 149:\n\n char buf[3] = { 0 };\n\nC99 §6.7.8 21: \"If there are [...] fewer characters in a string literal\nused to initialize an array of known size than there are elements in the\narray, the remainder of the aggregate shall be initialized implicitly\nthe same as objects that have static storage duration.\"\n\n>\n> Reported by Coverity.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> run-command.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 14f17830f51..2ba38850b4d 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -154,7 +154,7 @@ int is_executable(const char *name)\n> \t\tn = read(fd, buf, 2);\n> \t\tif (n == 2)\n> \t\t\t/* look for a she-bang */\n> -\t\t\tif (!strcmp(buf, \"#!\"))\n> +\t\t\tif (!memcmp(buf, \"#!\", 2))\n> \t\t\t\tst.st_mode |= S_IXUSR;\n> \t\tclose(fd);\n> \t}\n\n"},{"id":"457402","messageId":"YquMyakxYnU6mI5a@coredump.intra.peff.net","threadId":"58019","inReplyTo":"YquCaE+Vw9P/fybU@coredump.intra.peff.net","subject":"[PATCH] bug_fl(): correctly initialize trace2 va_list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-06-16T20:04:25Z","receivedAt":"2022-06-16T20:04:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 16, 2022 at 03:20:08PM -0400, Jeff King wrote:\n\n> > > The fix Peff's got here LGTM. I can (re)submit it with\n> > > format-patch+send-email after giving it a commit message describing the\n> > > issue if you'd like, but the change would be the same.\n> > \n> > Yup, I think the code change there looks the most sensible.\n> \n> I'll wrap it up with a commit message and modify the test to be more\n> thorough.\n\nHere it is. This can replace Johannes's patch 11.\n\n-- >8 --\nSubject: bug_fl(): correctly initialize trace2 va_list\n\nThe code added 0cc05b044f (usage.c: add a non-fatal bug() function to go\nwith BUG(), 2022-06-02) sets up two va_list variables: one to output to\nstderr, and one to trace2. But the order of initialization is wrong:\n\n va_list ap, cp;\n va_copy(cp, ap);\n va_start(ap, fmt);\n\nWe copy the contents of \"ap\" into \"cp\" before it is initialized, meaning\nit is full of garbage. The two should be swapped.\n\nHowever, there's another bug, noticed by Johannes Schindelin: we forget\nto call va_end() for the copy. So instead of just fixing the copy's\ninitialization, let's do two separate start/end pairs. This is allowed\nby the standard, and we don't need to use copy here since we have access\nto the original varargs. Matching the pairs with the calls makes it more\nobvious that everything is being done correctly.\n\nNote that we do call bug_fl() in the tests, but it didn't trigger this\nproblem because our format string doesn't have any placeholders. So even\nthough we were passing a garbage va_list through the stack, nobody ever\nneeded to look at it. We can easily adjust one of the trace2 tests to\ntrigger this, both for bug() and for BUG(). The latter isn't broken, but\nit's nice to exercise both a bit more. Without the fix in this patch\n(but with the test change), the bug() case causes a segfault.\n\nSigned-off-by: Jeff King \n---\n t/helper/test-trace2.c | 4 ++--\n usage.c | 8 +++++---\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex 180c7f53f3..a714130ece 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -224,8 +224,8 @@ static int ut_009bug_BUG(int argc, const char **argv)\n \n static int ut_010bug_BUG(int argc, const char **argv)\n {\n-\tbug(\"a bug message\");\n-\tBUG(\"a BUG message\");\n+\tbug(\"a %s message\", \"bug\");\n+\tBUG(\"a %s message\", \"BUG\");\n }\n \n /*\ndiff --git a/usage.c b/usage.c\nindex 79900d0287..56e29d6cd6 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -334,15 +334,17 @@ NORETURN void BUG_fl(const char *file, int line, const char *fmt, ...)\n int bug_called_must_BUG;\n void bug_fl(const char *file, int line, const char *fmt, ...)\n {\n-\tva_list ap, cp;\n+\tva_list ap;\n \n \tbug_called_must_BUG = 1;\n \n-\tva_copy(cp, ap);\n \tva_start(ap, fmt);\n \tBUG_vfl_common(file, line, fmt, ap);\n \tva_end(ap);\n-\ttrace2_cmd_error_va(fmt, cp);\n+\n+\tva_start(ap, fmt);\n+\ttrace2_cmd_error_va(fmt, ap);\n+\tva_end(ap);\n }\n \n #ifdef SUPPRESS_ANNOTATED_LEAKS\n-- \n2.37.0.rc0.352.g10876ef154\n\n"},{"id":"457404","messageId":"xmqqr13os71k.fsf@gitster.g","threadId":"58019","inReplyTo":"YquCaE+Vw9P/fybU@coredump.intra.peff.net","subject":"Re: [PATCH 11/11] bug_fl(): add missing `va_end()` call","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T20:11:03Z","receivedAt":"2022-06-16T20:11:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Being the curious sort, I ran it in a debugger. And indeed, \"cp\" is\n> filled with uninitialized garbage. The reason it works is that the test\n> calls bug() with a format string that does not contain any placeholders.\n> And thus our eventual call to vsnprintf() does not ever look at \"cp\" at\n> all!\n\n;-) Thanks.\n"},{"id":"457405","messageId":"xmqqmtecs6ws.fsf@gitster.g","threadId":"58019","inReplyTo":"98ff788a-d621-cf3f-aed5-66ae763b381f@web.de","subject":"Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T20:13:55Z","receivedAt":"2022-06-16T20:14:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 16.06.22 um 01:35 schrieb Johannes Schindelin via GitGitGadget:\n>> From: Johannes Schindelin \n>>\n>> On Windows, we open files we suspect might be scripts, read the first\n>> two bytes, and see whether they indicate a hash-bang line. We do not\n>> initialize the byte _after_ those two bytes, therefore `strcmp()` is\n>> inappropriate here.\n>\n> Hmm, but buf _is_ initialized fully? Line 149:\n>\n> char buf[3] = { 0 };\n\nAhh, yeah, that changes the landscape quite a bit.\n\nWe explicitly ask to read 2 bytes and look at the buf[] when read\nsays it read 2 bytes, so this is another false positive X-<.\n\n>> diff --git a/run-command.c b/run-command.c\n>> index 14f17830f51..2ba38850b4d 100644\n>> --- a/run-command.c\n>> +++ b/run-command.c\n>> @@ -154,7 +154,7 @@ int is_executable(const char *name)\n>> \t\tn = read(fd, buf, 2);\n>> \t\tif (n == 2)\n>> \t\t\t/* look for a she-bang */\n>> -\t\t\tif (!strcmp(buf, \"#!\"))\n>> +\t\t\tif (!memcmp(buf, \"#!\", 2))\n>> \t\t\t\tst.st_mode |= S_IXUSR;\n>> \t\tclose(fd);\n>> \t}\n\nWe can update the variable to\n\n\tchar buf[2];\n\nto match the updated code, I guess. The fewer bytes we use on\nstack, the better ;-).\n\n"},{"id":"457406","messageId":"xmqqilp0s6lk.fsf@gitster.g","threadId":"58019","inReplyTo":"xmqqmtecs6ws.fsf@gitster.g","subject":"Re: [PATCH 01/11] mingw: avoid accessing uninitialized memory in `is_executable()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-16T20:20:39Z","receivedAt":"2022-06-16T20:20:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Hmm, but buf _is_ initialized fully? Line 149:\n>>\n>> char buf[3] = { 0 };\n>\n> Ahh, yeah, that changes the landscape quite a bit.\n\nJust for reference, this piece of code has been correct ever since\nit was introduced to help.c by cc3b7a97 (Windows: Make 'git help -a'\nwork., 2008-01-14). With a larger context, we can see buf[3] that\nis NUL filled, which receives 2 bytes by read(), and strcmp() does\nlook semantically more correct than memcmp(), even though there\nprobably shouldn't be any correctness or performance difference.\n\nSo, strcmp -> memcmp in this case is a strict dis-improvement. I\nmerged it already to 'next', but I haven't pushed the result out, so\nI'll redo 'next' before I push the result out.\n\nThanks.\n\n"},{"id":"457408","messageId":"YquV9oYaAPeiJec1@nand.local","threadId":"58019","inReplyTo":"b176e5764f990fbf8c132ae1563027531bd8cc9f.1655336146.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 09/11] pack-mtimes: avoid closing a bogus file descriptor","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-16T20:43:34Z","receivedAt":"2022-06-16T20:43:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Jun 15, 2022 at 11:35:43PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n>\n> In 94cd775a6c52 (pack-mtimes: support reading .mtimes files,\n> 2022-05-20), code was added to close the file descriptor corresponding\n> to the mtimes file.\n>\n> However, it is possible that opening that file failed, in which case we\n> are closing a file descriptor with the value `-1`. Let's guard that\n> `close()` call.\n\nNice catch. I agree with your assessment and fix. Thanks for spotting\nand fixing!\n\nThanks,\nTaylor\n"},{"id":"457464","messageId":"20220617212736.1951-1-tomlevy93@gmail.com","threadId":"58019","inReplyTo":"a3f79b3e034a3a982b3be7835062493bfdf29ee5.1655336146.git.gitgitgadget@gmail.com","subject":"[PATCH 08/11] read_index_from(): avoid memory leak","fromName":"Tom Levy","fromEmail":"tomlevy93@gmail.com","sentAt":"2022-06-17T21:27:36Z","receivedAt":"2022-06-17T21:47:48Z","isPatch":true,"sender":{"key":"tomlevy93@gmail.com","avatar":null},"body":"Hi,\n\nI think there is another problem with this code: later on it refers to\nthe 'base_path' variable, but I think it should refer to 'base_path2'\nif the first path did not exist.\n\nHow about the following patch instead (or in addition)?\n\nRegards,\nTom Levy\n\n\n-- >8 --\n\nSubject: [PATCH] read_index_from(): use second path consistently, avoid memory\n leak\n\nIn 998330ac2e7c (read-cache: look for shared index files next to the\nindex, too, 2021-08-26), we added code that allocates memory to store\nthe path of a second shared base index, but we never released that\nmemory. Also, later code (e.g. the call to freshen_shared_index())\ncontinued to use the first path even when the second path was\nselected.\n\nModify the code to store the second path in the original variable\n(after freeing the first path), so that the later code will use the\nsecond path (and also free it).\n\nThe memory leak was reported by Coverity.\n\nCo-authored-by: Johannes Schindelin \nSigned-off-by: Tom Levy \n---\n read-cache.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 4df97e185e..48f2b9aa9d 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -2459,15 +2459,15 @@ int read_index_from(struct index_state *istate, const char *path,\n the_repository, \"%s\", base_path);\n if (!ret) {\n char *path_copy = xstrdup(path);\n- const char *base_path2 = xstrfmt(\"%s/sharedindex.%s\",\n- dirname(path_copy),\n- base_oid_hex);\n+ free(base_path);\n+ base_path = xstrfmt(\"%s/sharedindex.%s\", dirname(path_copy),\n+ base_oid_hex);\n free(path_copy);\n trace2_region_enter_printf(\"index\", \"shared/do_read_index\",\n- the_repository, \"%s\", base_path2);\n- ret = do_read_index(split_index->base, base_path2, 1);\n+ the_repository, \"%s\", base_path);\n+ ret = do_read_index(split_index->base, base_path, 1);\n trace2_region_leave_printf(\"index\", \"shared/do_read_index\",\n- the_repository, \"%s\", base_path2);\n+ the_repository, \"%s\", base_path);\n }\n if (!oideq(&split_index->base_oid, &split_index->base->oid))\n die(_(\"broken index, expect %s in %s, got %s\"),\n--\n2.30.2\n\n"}]}