{"thread":{"id":"57443","subject":"[PATCH 0/2] Fix a couple small leaks in merge-ort","startedAt":"2022-02-19T17:10:08Z","lastAt":"2022-06-01T10:26:00Z","messageCount":15,"participants":["Elijah Newren via GitGitGadget","Ævar Arnfjörð Bjarmason","Elijah Newren","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"448864","messageId":"pull.1152.git.1645290601.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":null,"subject":"[PATCH 0/2] Fix a couple small leaks in merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-19T17:09:58Z","receivedAt":"2022-02-19T17:10:08Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Off-list, Ævar reported a few small leaks in merge-ort to me that I missed\npreviously. Here's a couple fixes.\n\nElijah Newren (2):\n merge-ort: fix small memory leak in detect_and_process_renames()\n merge-ort: fix small memory leak in unique_path()\n\n merge-ort.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\n\nbase-commit: e2ac9141e64e2cd3e690d1b5fc848949827c09b4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1152%2Fnewren%2Fmerge-ort-leak-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1152/newren/merge-ort-leak-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1152\n-- \ngitgitgadget\n"},{"id":"448865","messageId":"f0308de28e49fb9bb1239fdcbc839097f5afa62a.1645290601.git.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":"pull.1152.git.1645290601.gitgitgadget@gmail.com","subject":"[PATCH 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-19T17:09:59Z","receivedAt":"2022-02-19T17:10:10Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ndetect_and_process_renames() detects renames on both sides of history\nand then combines these into a single diff_queue_struct. The combined\ndiff_queue_struct needs to be able to hold the renames found on either\nside, and since it knows the (maximum) size it needs, it pre-emptively\ngrows the array to the appropriate size:\n\n\tALLOC_GROW(combined.queue,\n\t\t renames->pairs[1].nr + renames->pairs[2].nr,\n\t\t combined.alloc);\n\nIt then collects the items from each side:\n\n\tcollect_renames(opt, &combined, MERGE_SIDE1, ...)\n\tcollect_renames(opt, &combined, MERGE_SIDE2, ...)\n\nNote, though, that collect_renames() sometimes determines that some\npairs are unnecessary and does not include them in the combined array.\nWhen it is done, detect_and_process_renames() frees this memory:\n\n\tif (combined.nr) {\n ...\n\t\tfree(combined.queue);\n }\n\nThe problem is that sometimes even when there are pairs, none of them are\nnecessary. Instead of checking combined.nr, we should check\ncombined.alloc. Doing so fixes the following memory leak, as reported\nby valgrind:\n\n==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n==PID== at 0xADDRESS: malloc\n==PID== by 0xADDRESS: realloc\n==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n==PID== by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)\n==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)\n==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n==PID== by 0xADDRESS: cmd_merge (merge.c:1676)\n==PID== by 0xADDRESS: run_builtin (git.c:461)\n==PID== by 0xADDRESS: handle_builtin (git.c:713)\n==PID== by 0xADDRESS: run_argv (git.c:780)\n==PID== by 0xADDRESS: cmd_main (git.c:911)\n==PID== by 0xADDRESS: main (common-main.c:52)\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d85b1cd99e9..4f5abc558c5 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3175,7 +3175,7 @@ simple_cleanup:\n \t\tfree(renames->pairs[s].queue);\n \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n \t}\n-\tif (combined.nr) {\n+\tif (combined.alloc) {\n \t\tint i;\n \t\tfor (i = 0; i < combined.nr; i++)\n \t\t\tpool_diff_free_filepair(&opt->priv->pool,\n-- \ngitgitgadget\n\n"},{"id":"448866","messageId":"73bc1e5c5dffbe9c132ea786dd414ef2159967e3.1645290601.git.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":"pull.1152.git.1645290601.gitgitgadget@gmail.com","subject":"[PATCH 2/2] merge-ort: fix small memory leak in unique_path()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-19T17:10:00Z","receivedAt":"2022-02-19T17:10:11Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe struct strmap paths member of merge_options_internal is perhaps the\nmost central data structure to all of merge-ort. Because all the paths\ninvolved in the merge need to be kept until the merge is complete, this\n\"paths\" data structure traditionally took responsibility for owning all\nthe allocated paths. When the merge is over, those paths were free()d\nas part of free()ing this strmap.\n\nIn commit 6697ee01b5d3 (merge-ort: switch our strmaps over to using\nmemory pools, 2021-07-30), we changed the allocations for pathnames to\ncome from a memory pool. That meant the ownership changed slightly;\nthere were no individual free() calls to make, instead the memory pool\nowned all those paths and they were free()d all at once.\n\nUnfortunately unique_path() was written presuming the pre-memory-pool\nmodel, and allocated a path on the heap and left it in the strmap for\nlater free()ing. Modify it to return a path allocated from the memory\npool instead.\n\nNote that there's one instance -- in record_conflicted_index_entries()\n-- where the returned string from unique_path() was only used very\ntemporarily and thus had been immediately free()'d. This codepath was\nassociated with an ugly skip-worktree workaround that has since been\nbetter fixed by the in-flight en/present-despite-skipped topic. This\nworkaround probably makes sense to excise once that topic merges down,\nbut for now, just remove the immediate free() and allow the returned\nstring to be free()d when the memory pool is released.\n\nThis fixes the following memory leak as reported by valgrind:\n\n==PID== 65 bytes in 1 blocks are definitely lost in loss record 79 of 134\n==PID== at 0xADDRESS: malloc\n==PID== by 0xADDRESS: realloc\n==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n==PID== by 0xADDRESS: strbuf_grow (strbuf.c:98)\n==PID== by 0xADDRESS: strbuf_vaddf (strbuf.c:394)\n==PID== by 0xADDRESS: strbuf_addf (strbuf.c:335)\n==PID== by 0xADDRESS: unique_path (merge-ort.c:733)\n==PID== by 0xADDRESS: process_entry (merge-ort.c:3678)\n==PID== by 0xADDRESS: process_entries (merge-ort.c:4037)\n==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4621)\n==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 4f5abc558c5..40ae4dc4e92 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -722,13 +722,15 @@ static void add_flattened_path(struct strbuf *out, const char *s)\n \t\t\tout->buf[i] = '_';\n }\n \n-static char *unique_path(struct strmap *existing_paths,\n+static char *unique_path(struct merge_options *opt,\n \t\t\t const char *path,\n \t\t\t const char *branch)\n {\n+\tchar *ret = NULL;\n \tstruct strbuf newpath = STRBUF_INIT;\n \tint suffix = 0;\n \tsize_t base_len;\n+\tstruct strmap *existing_paths = &opt->priv->paths;\n \n \tstrbuf_addf(&newpath, \"%s~\", path);\n \tadd_flattened_path(&newpath, branch);\n@@ -739,7 +741,11 @@ static char *unique_path(struct strmap *existing_paths,\n \t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n \t}\n \n-\treturn strbuf_detach(&newpath, NULL);\n+\t/* Track the new path in our memory pool */\n+\tret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);\n+\tmemcpy(ret, newpath.buf, newpath.len + 1);\n+\tstrbuf_release(&newpath);\n+\treturn ret;\n }\n \n /*** Function Grouping: functions related to collect_merge_info() ***/\n@@ -3679,7 +3685,7 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tdf_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;\n \t\tbranch = (df_file_index == 1) ? opt->branch1 : opt->branch2;\n-\t\tpath = unique_path(&opt->priv->paths, path, branch);\n+\t\tpath = unique_path(opt, path, branch);\n \t\tstrmap_put(&opt->priv->paths, path, new_ci);\n \n \t\tpath_msg(opt, path, 0,\n@@ -3804,14 +3810,12 @@ static void process_entry(struct merge_options *opt,\n \t\t\t/* Insert entries into opt->priv_paths */\n \t\t\tassert(rename_a || rename_b);\n \t\t\tif (rename_a) {\n-\t\t\t\ta_path = unique_path(&opt->priv->paths,\n-\t\t\t\t\t\t path, opt->branch1);\n+\t\t\t\ta_path = unique_path(opt, path, opt->branch1);\n \t\t\t\tstrmap_put(&opt->priv->paths, a_path, ci);\n \t\t\t}\n \n \t\t\tif (rename_b)\n-\t\t\t\tb_path = unique_path(&opt->priv->paths,\n-\t\t\t\t\t\t path, opt->branch2);\n+\t\t\t\tb_path = unique_path(opt, path, opt->branch2);\n \t\t\telse\n \t\t\t\tb_path = path;\n \t\t\tstrmap_put(&opt->priv->paths, b_path, new_ci);\n@@ -4199,7 +4203,7 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n \t\t\t\tstruct stat st;\n \n \t\t\t\tif (!lstat(path, &st)) {\n-\t\t\t\t\tchar *new_name = unique_path(&opt->priv->paths,\n+\t\t\t\t\tchar *new_name = unique_path(opt,\n \t\t\t\t\t\t\t\t path,\n \t\t\t\t\t\t\t\t \"cruft\");\n \n@@ -4207,7 +4211,6 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n \t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n \t\t\t\t\t\t path, new_name);\n \t\t\t\t\terrs |= rename(path, new_name);\n-\t\t\t\t\tfree(new_name);\n \t\t\t\t}\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n \t\t\t}\n-- \ngitgitgadget\n"},{"id":"448869","messageId":"220219.86sfsecyy6.gmgdl@evledraar.gmail.com","threadId":"57443","inReplyTo":"f0308de28e49fb9bb1239fdcbc839097f5afa62a.1645290601.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T21:44:19Z","receivedAt":"2022-02-19T21:45:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> detect_and_process_renames() detects renames on both sides of history\n> and then combines these into a single diff_queue_struct. The combined\n> diff_queue_struct needs to be able to hold the renames found on either\n> side, and since it knows the (maximum) size it needs, it pre-emptively\n> grows the array to the appropriate size:\n>\n> \tALLOC_GROW(combined.queue,\n> \t\t renames->pairs[1].nr + renames->pairs[2].nr,\n> \t\t combined.alloc);\n>\n> It then collects the items from each side:\n>\n> \tcollect_renames(opt, &combined, MERGE_SIDE1, ...)\n> \tcollect_renames(opt, &combined, MERGE_SIDE2, ...)\n>\n> Note, though, that collect_renames() sometimes determines that some\n> pairs are unnecessary and does not include them in the combined array.\n> When it is done, detect_and_process_renames() frees this memory:\n>\n> \tif (combined.nr) {\n> ...\n> \t\tfree(combined.queue);\n> }\n>\n> The problem is that sometimes even when there are pairs, none of them are\n> necessary. Instead of checking combined.nr, we should check\n> combined.alloc. Doing so fixes the following memory leak, as reported\n> by valgrind:\n>\n> ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n> ==PID== at 0xADDRESS: malloc\n> ==PID== by 0xADDRESS: realloc\n> ==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n> ==PID== by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)\n> ==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)\n> ==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n> ==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n> ==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n> ==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n> ==PID== by 0xADDRESS: cmd_merge (merge.c:1676)\n> ==PID== by 0xADDRESS: run_builtin (git.c:461)\n> ==PID== by 0xADDRESS: handle_builtin (git.c:713)\n> ==PID== by 0xADDRESS: run_argv (git.c:780)\n> ==PID== by 0xADDRESS: cmd_main (git.c:911)\n> ==PID== by 0xADDRESS: main (common-main.c:52)\n>\n> Reported-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index d85b1cd99e9..4f5abc558c5 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3175,7 +3175,7 @@ simple_cleanup:\n> \t\tfree(renames->pairs[s].queue);\n> \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n> \t}\n> -\tif (combined.nr) {\n> +\tif (combined.alloc) {\n> \t\tint i;\n> \t\tfor (i = 0; i < combined.nr; i++)\n> \t\t\tpool_diff_free_filepair(&opt->priv->pool,\n\nThis looks correct in that it'll work, but I still don't get why the\npre-image or post-image is going through this indirection of checking\n\"alloc\" at all. Wouldn't this be correct & more straightforward (the\nmemset part is optional, just did it for good measure)?:\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 40ae4dc4e92..a01f28586a1 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3092,12 +3092,12 @@ static int detect_and_process_renames(struct merge_options *opt,\n \t\t\t\t struct tree *side1,\n \t\t\t\t struct tree *side2)\n {\n-\tstruct diff_queue_struct combined;\n+\tstruct diff_queue_struct combined = { 0 };\n \tstruct rename_info *renames = &opt->priv->renames;\n \tint need_dir_renames, s, clean = 1;\n \tunsigned detection_run = 0;\n+\tint i;\n \n-\tmemset(&combined, 0, sizeof(combined));\n \tif (!possible_renames(renames))\n \t\tgoto cleanup;\n \n@@ -3181,13 +3181,10 @@ static int detect_and_process_renames(struct merge_options *opt,\n \t\tfree(renames->pairs[s].queue);\n \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n \t}\n-\tif (combined.alloc) {\n-\t\tint i;\n-\t\tfor (i = 0; i < combined.nr; i++)\n-\t\t\tpool_diff_free_filepair(&opt->priv->pool,\n-\t\t\t\t\t\tcombined.queue[i]);\n-\t\tfree(combined.queue);\n-\t}\n+\n+\tfor (i = 0; i < combined.nr; i++)\n+\t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n+\tfree(combined.queue);\n \n \treturn clean;\n }\n"},{"id":"448870","messageId":"220219.86o832cwup.gmgdl@evledraar.gmail.com","threadId":"57443","inReplyTo":"73bc1e5c5dffbe9c132ea786dd414ef2159967e3.1645290601.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix small memory leak in unique_path()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-19T22:22:44Z","receivedAt":"2022-02-19T22:31:16Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> The struct strmap paths member of merge_options_internal is perhaps the\n> most central data structure to all of merge-ort. Because all the paths\n> involved in the merge need to be kept until the merge is complete, this\n> \"paths\" data structure traditionally took responsibility for owning all\n> the allocated paths. When the merge is over, those paths were free()d\n> as part of free()ing this strmap.\n>\n> In commit 6697ee01b5d3 (merge-ort: switch our strmaps over to using\n> memory pools, 2021-07-30), we changed the allocations for pathnames to\n> come from a memory pool. That meant the ownership changed slightly;\n> there were no individual free() calls to make, instead the memory pool\n> owned all those paths and they were free()d all at once.\n>\n> Unfortunately unique_path() was written presuming the pre-memory-pool\n> model, and allocated a path on the heap and left it in the strmap for\n> later free()ing. Modify it to return a path allocated from the memory\n> pool instead.\n\nThis seems like a rather obvious fix to the leak, as the other side\nwasn't ready to have the detached strbuf handed to it, and instead is\nassuming everything is mempools.\n\nThe downside is a bit of heap churn here since you malloc() & use the\nstrbuf just to ask for that size from the mempool, and then free() the\nstrbuf (of course we had that before, we just weren't free-ing).\n\nSo this is just an aside & I have no idea if it's worth it, but FWIW you\ncan have your cake & eat it too here memory-allocation wise and avoid\nthe strbuf allocation entirely, and just use your mem-pool.\n\nLike this:\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 40ae4dc4e92..1111916d5cb 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -731,6 +731,16 @@ static char *unique_path(struct merge_options *opt,\n \tint suffix = 0;\n \tsize_t base_len;\n \tstruct strmap *existing_paths = &opt->priv->paths;\n+\t/*\n+\t * pre-size path + ~ + branch + _%d + \"\\0\". Hopefully 6 digits\n+\t * of suffix is enough for everyone?\n+\t */\n+\tconst size_t max_suffix = 6;\n+\tconst size_t expected_len = strlen(path) + 1 + strlen(branch) + 1 +\n+\t\tmax_suffix + 1;\n+\n+\tret = mem_pool_alloc(&opt->priv->pool, expected_len);\n+\tstrbuf_attach(&newpath, ret, 0, expected_len);\n \n \tstrbuf_addf(&newpath, \"%s~\", path);\n \tadd_flattened_path(&newpath, branch);\n@@ -741,10 +751,10 @@ static char *unique_path(struct merge_options *opt,\n \t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n \t}\n \n-\t/* Track the new path in our memory pool */\n-\tret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);\n-\tmemcpy(ret, newpath.buf, newpath.len + 1);\n-\tstrbuf_release(&newpath);\n+\tif (newpath.alloc > expected_len)\n+\t\tBUG(\"we assumed too much thinking '%s~%s' would fit in %lu, ended up %lu ('%s')\",\n+\t\t path, branch, expected_len, newpath.alloc, newpath.buf);\n+\n \treturn ret;\n }\n \n\nA bit nasty for sure, but if you're willing to BUG() out if we ever go\nabove 999999 suffix tries or whatever (which would be trivial to add to\nthe loop there) it's rather straightforward.\n\nI.e. we know the size of the buffer ahead of time, except for that loop\nthat'll add \"_%d\" to the end, and that can be made bounded.\n\nObviously your solution's a lot simpler, so I think this is only\nsomething you should consider if you think it matters for the\nperformance numbers linked to from 6697ee01b5d3. I'm not familiar enough\nwith merge-ort.c to know if it is in this case, or if this would be\npointless micro-optimization on a non-hot codepath.\n\n"},{"id":"448872","messageId":"CABPp-BEfMVFYaN=17Hp5GgSLbhDnS5YOx2PBtme39xioQgBXTQ@mail.gmail.com","threadId":"57443","inReplyTo":"220219.86sfsecyy6.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-20T00:26:49Z","receivedAt":"2022-02-20T00:27:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Feb 19, 2022 at 1:45 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > detect_and_process_renames() detects renames on both sides of history\n> > and then combines these into a single diff_queue_struct. The combined\n> > diff_queue_struct needs to be able to hold the renames found on either\n> > side, and since it knows the (maximum) size it needs, it pre-emptively\n> > grows the array to the appropriate size:\n> >\n> > ALLOC_GROW(combined.queue,\n> > renames->pairs[1].nr + renames->pairs[2].nr,\n> > combined.alloc);\n> >\n> > It then collects the items from each side:\n> >\n> > collect_renames(opt, &combined, MERGE_SIDE1, ...)\n> > collect_renames(opt, &combined, MERGE_SIDE2, ...)\n> >\n> > Note, though, that collect_renames() sometimes determines that some\n> > pairs are unnecessary and does not include them in the combined array.\n> > When it is done, detect_and_process_renames() frees this memory:\n> >\n> > if (combined.nr) {\n> > ...\n> > free(combined.queue);\n> > }\n> >\n> > The problem is that sometimes even when there are pairs, none of them are\n> > necessary. Instead of checking combined.nr, we should check\n> > combined.alloc. Doing so fixes the following memory leak, as reported\n> > by valgrind:\n> >\n> > ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n> > ==PID== at 0xADDRESS: malloc\n> > ==PID== by 0xADDRESS: realloc\n> > ==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n> > ==PID== by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)\n> > ==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)\n> > ==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n> > ==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n> > ==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n> > ==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n> > ==PID== by 0xADDRESS: cmd_merge (merge.c:1676)\n> > ==PID== by 0xADDRESS: run_builtin (git.c:461)\n> > ==PID== by 0xADDRESS: handle_builtin (git.c:713)\n> > ==PID== by 0xADDRESS: run_argv (git.c:780)\n> > ==PID== by 0xADDRESS: cmd_main (git.c:911)\n> > ==PID== by 0xADDRESS: main (common-main.c:52)\n> >\n> > Reported-by: Ævar Arnfjörð Bjarmason \n> > Signed-off-by: Elijah Newren \n> > ---\n> > merge-ort.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index d85b1cd99e9..4f5abc558c5 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3175,7 +3175,7 @@ simple_cleanup:\n> > free(renames->pairs[s].queue);\n> > DIFF_QUEUE_CLEAR(&renames->pairs[s]);\n> > }\n> > - if (combined.nr) {\n> > + if (combined.alloc) {\n> > int i;\n> > for (i = 0; i < combined.nr; i++)\n> > pool_diff_free_filepair(&opt->priv->pool,\n>\n> This looks correct in that it'll work, but I still don't get why the\n> pre-image or post-image is going through this indirection of checking\n> \"alloc\" at all. Wouldn't this be correct & more straightforward (the\n> memset part is optional, just did it for good measure)?:\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 40ae4dc4e92..a01f28586a1 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3092,12 +3092,12 @@ static int detect_and_process_renames(struct merge_options *opt,\n> struct tree *side1,\n> struct tree *side2)\n> {\n> - struct diff_queue_struct combined;\n> + struct diff_queue_struct combined = { 0 };\n> struct rename_info *renames = &opt->priv->renames;\n> int need_dir_renames, s, clean = 1;\n> unsigned detection_run = 0;\n> + int i;\n>\n> - memset(&combined, 0, sizeof(combined));\n> if (!possible_renames(renames))\n> goto cleanup;\n>\n> @@ -3181,13 +3181,10 @@ static int detect_and_process_renames(struct merge_options *opt,\n> free(renames->pairs[s].queue);\n> DIFF_QUEUE_CLEAR(&renames->pairs[s]);\n> }\n> - if (combined.alloc) {\n> - int i;\n> - for (i = 0; i < combined.nr; i++)\n> - pool_diff_free_filepair(&opt->priv->pool,\n> - combined.queue[i]);\n> - free(combined.queue);\n> - }\n> +\n> + for (i = 0; i < combined.nr; i++)\n> + pool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n> + free(combined.queue);\n>\n> return clean;\n> }\n\nHmm, good point; I like your version better. I'll change it, thanks.\n"},{"id":"448873","messageId":"CABPp-BEpSEmndTHOLrdTGmcf_+5uoR6_7fy58KXXktV4tcTXpA@mail.gmail.com","threadId":"57443","inReplyTo":"220219.86o832cwup.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 2/2] merge-ort: fix small memory leak in unique_path()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-20T00:37:50Z","receivedAt":"2022-02-20T00:38:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Feb 19, 2022 at 2:31 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Sat, Feb 19 2022, Elijah Newren via GitGitGadget wrote:\n>\n> > From: Elijah Newren \n> >\n> > The struct strmap paths member of merge_options_internal is perhaps the\n> > most central data structure to all of merge-ort. Because all the paths\n> > involved in the merge need to be kept until the merge is complete, this\n> > \"paths\" data structure traditionally took responsibility for owning all\n> > the allocated paths. When the merge is over, those paths were free()d\n> > as part of free()ing this strmap.\n> >\n> > In commit 6697ee01b5d3 (merge-ort: switch our strmaps over to using\n> > memory pools, 2021-07-30), we changed the allocations for pathnames to\n> > come from a memory pool. That meant the ownership changed slightly;\n> > there were no individual free() calls to make, instead the memory pool\n> > owned all those paths and they were free()d all at once.\n> >\n> > Unfortunately unique_path() was written presuming the pre-memory-pool\n> > model, and allocated a path on the heap and left it in the strmap for\n> > later free()ing. Modify it to return a path allocated from the memory\n> > pool instead.\n>\n> This seems like a rather obvious fix to the leak, as the other side\n> wasn't ready to have the detached strbuf handed to it, and instead is\n> assuming everything is mempools.\n>\n> The downside is a bit of heap churn here since you malloc() & use the\n> strbuf just to ask for that size from the mempool, and then free() the\n> strbuf (of course we had that before, we just weren't free-ing).\n>\n> So this is just an aside & I have no idea if it's worth it, but FWIW you\n> can have your cake & eat it too here memory-allocation wise and avoid\n> the strbuf allocation entirely, and just use your mem-pool.\n>\n> Like this:\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 40ae4dc4e92..1111916d5cb 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -731,6 +731,16 @@ static char *unique_path(struct merge_options *opt,\n> int suffix = 0;\n> size_t base_len;\n> struct strmap *existing_paths = &opt->priv->paths;\n> + /*\n> + * pre-size path + ~ + branch + _%d + \"\\0\". Hopefully 6 digits\n> + * of suffix is enough for everyone?\n> + */\n> + const size_t max_suffix = 6;\n> + const size_t expected_len = strlen(path) + 1 + strlen(branch) + 1 +\n> + max_suffix + 1;\n> +\n> + ret = mem_pool_alloc(&opt->priv->pool, expected_len);\n> + strbuf_attach(&newpath, ret, 0, expected_len);\n>\n> strbuf_addf(&newpath, \"%s~\", path);\n> add_flattened_path(&newpath, branch);\n> @@ -741,10 +751,10 @@ static char *unique_path(struct merge_options *opt,\n> strbuf_addf(&newpath, \"_%d\", suffix++);\n> }\n>\n> - /* Track the new path in our memory pool */\n> - ret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);\n> - memcpy(ret, newpath.buf, newpath.len + 1);\n> - strbuf_release(&newpath);\n> + if (newpath.alloc > expected_len)\n> + BUG(\"we assumed too much thinking '%s~%s' would fit in %lu, ended up %lu ('%s')\",\n> + path, branch, expected_len, newpath.alloc, newpath.buf);\n> +\n> return ret;\n> }\n>\n>\n> A bit nasty for sure, but if you're willing to BUG() out if we ever go\n> above 999999 suffix tries or whatever (which would be trivial to add to\n> the loop there) it's rather straightforward.\n>\n> I.e. we know the size of the buffer ahead of time, except for that loop\n> that'll add \"_%d\" to the end, and that can be made bounded.\n>\n> Obviously your solution's a lot simpler, so I think this is only\n> something you should consider if you think it matters for the\n> performance numbers linked to from 6697ee01b5d3. I'm not familiar enough\n> with merge-ort.c to know if it is in this case, or if this would be\n> pointless micro-optimization on a non-hot codepath.\n\nThat's an interesting idea, but it's a micro-optimization on a very\ncold path. You need to either have a D/F conflict that persists or a\nmode-type conflict (e.g. an add/add conflict with symlink vs. file\ntypes). Those tend to be quite rare; in fact, the testcase with the\nperformance numbers in 6697ee01b5d3 didn't have any of these and never\neven triggered this codepath (otherwise I would have caught the leak\nin my earlier leak testing). So I think simple and robust makes more\nsense here.\n"},{"id":"448875","messageId":"pull.1152.v2.git.1645320591.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":"pull.1152.git.1645290601.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] Fix a couple small leaks in merge-ort","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-20T01:29:49Z","receivedAt":"2022-02-20T01:29:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Off-list, Ævar reported a few small leaks in merge-ort to me that I missed\npreviously. Here's a couple fixes.\n\nChanges since v1:\n\n * Simplified patch 1 a bit as per Ævar's suggestion\n\nElijah Newren (2):\n merge-ort: fix small memory leak in detect_and_process_renames()\n merge-ort: fix small memory leak in unique_path()\n\n merge-ort.c | 36 +++++++++++++++++-------------------\n 1 file changed, 17 insertions(+), 19 deletions(-)\n\n\nbase-commit: e2ac9141e64e2cd3e690d1b5fc848949827c09b4\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1152%2Fnewren%2Fmerge-ort-leak-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1152/newren/merge-ort-leak-fixes-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1152\n\nRange-diff vs v1:\n\n 1: f0308de28e4 ! 1: f1f7fc97fe2 merge-ort: fix small memory leak in detect_and_process_renames()\n @@ Commit message\n free(combined.queue);\n }\n \n - The problem is that sometimes even when there are pairs, none of them are\n - necessary. Instead of checking combined.nr, we should check\n - combined.alloc. Doing so fixes the following memory leak, as reported\n - by valgrind:\n + The problem is that sometimes even when there are pairs, none of them\n + are necessary. Instead of checking combined.nr, just remove the\n + if-check; free() knows to skip NULL pointers. This change fixes the\n + following memory leak, as reported by valgrind:\n \n ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n ==PID== at 0xADDRESS: malloc\n @@ Commit message\n Signed-off-by: Elijah Newren \n \n ## merge-ort.c ##\n +@@ merge-ort.c: static int detect_and_process_renames(struct merge_options *opt,\n + \t\t\t\t struct tree *side1,\n + \t\t\t\t struct tree *side2)\n + {\n +-\tstruct diff_queue_struct combined;\n ++\tstruct diff_queue_struct combined = { 0 };\n + \tstruct rename_info *renames = &opt->priv->renames;\n +-\tint need_dir_renames, s, clean = 1;\n ++\tint need_dir_renames, s, i, clean = 1;\n + \tunsigned detection_run = 0;\n + \n +-\tmemset(&combined, 0, sizeof(combined));\n + \tif (!possible_renames(renames))\n + \t\tgoto cleanup;\n + \n @@ merge-ort.c: simple_cleanup:\n \t\tfree(renames->pairs[s].queue);\n \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n \t}\n -\tif (combined.nr) {\n -+\tif (combined.alloc) {\n - \t\tint i;\n - \t\tfor (i = 0; i < combined.nr; i++)\n - \t\t\tpool_diff_free_filepair(&opt->priv->pool,\n +-\t\tint i;\n +-\t\tfor (i = 0; i < combined.nr; i++)\n +-\t\t\tpool_diff_free_filepair(&opt->priv->pool,\n +-\t\t\t\t\t\tcombined.queue[i]);\n +-\t\tfree(combined.queue);\n +-\t}\n ++\tfor (i = 0; i < combined.nr; i++)\n ++\t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n ++\tfree(combined.queue);\n + \n + \treturn clean;\n + }\n 2: 73bc1e5c5df = 2: 69fb932c21d merge-ort: fix small memory leak in unique_path()\n\n-- \ngitgitgadget\n"},{"id":"448874","messageId":"f1f7fc97fe2fe5079365bb91c71fb7033378995d.1645320592.git.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":"pull.1152.v2.git.1645320591.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-20T01:29:50Z","receivedAt":"2022-02-20T01:29:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\ndetect_and_process_renames() detects renames on both sides of history\nand then combines these into a single diff_queue_struct. The combined\ndiff_queue_struct needs to be able to hold the renames found on either\nside, and since it knows the (maximum) size it needs, it pre-emptively\ngrows the array to the appropriate size:\n\n\tALLOC_GROW(combined.queue,\n\t\t renames->pairs[1].nr + renames->pairs[2].nr,\n\t\t combined.alloc);\n\nIt then collects the items from each side:\n\n\tcollect_renames(opt, &combined, MERGE_SIDE1, ...)\n\tcollect_renames(opt, &combined, MERGE_SIDE2, ...)\n\nNote, though, that collect_renames() sometimes determines that some\npairs are unnecessary and does not include them in the combined array.\nWhen it is done, detect_and_process_renames() frees this memory:\n\n\tif (combined.nr) {\n ...\n\t\tfree(combined.queue);\n }\n\nThe problem is that sometimes even when there are pairs, none of them\nare necessary. Instead of checking combined.nr, just remove the\nif-check; free() knows to skip NULL pointers. This change fixes the\nfollowing memory leak, as reported by valgrind:\n\n==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n==PID== at 0xADDRESS: malloc\n==PID== by 0xADDRESS: realloc\n==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n==PID== by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)\n==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)\n==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n==PID== by 0xADDRESS: cmd_merge (merge.c:1676)\n==PID== by 0xADDRESS: run_builtin (git.c:461)\n==PID== by 0xADDRESS: handle_builtin (git.c:713)\n==PID== by 0xADDRESS: run_argv (git.c:780)\n==PID== by 0xADDRESS: cmd_main (git.c:911)\n==PID== by 0xADDRESS: main (common-main.c:52)\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 15 +++++----------\n 1 file changed, 5 insertions(+), 10 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex d85b1cd99e9..3d7f9feb6f7 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,\n \t\t\t\t struct tree *side1,\n \t\t\t\t struct tree *side2)\n {\n-\tstruct diff_queue_struct combined;\n+\tstruct diff_queue_struct combined = { 0 };\n \tstruct rename_info *renames = &opt->priv->renames;\n-\tint need_dir_renames, s, clean = 1;\n+\tint need_dir_renames, s, i, clean = 1;\n \tunsigned detection_run = 0;\n \n-\tmemset(&combined, 0, sizeof(combined));\n \tif (!possible_renames(renames))\n \t\tgoto cleanup;\n \n@@ -3175,13 +3174,9 @@ simple_cleanup:\n \t\tfree(renames->pairs[s].queue);\n \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n \t}\n-\tif (combined.nr) {\n-\t\tint i;\n-\t\tfor (i = 0; i < combined.nr; i++)\n-\t\t\tpool_diff_free_filepair(&opt->priv->pool,\n-\t\t\t\t\t\tcombined.queue[i]);\n-\t\tfree(combined.queue);\n-\t}\n+\tfor (i = 0; i < combined.nr; i++)\n+\t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n+\tfree(combined.queue);\n \n \treturn clean;\n }\n-- \ngitgitgadget\n\n"},{"id":"448876","messageId":"69fb932c21dc88f6522e9ca98ef283e714496039.1645320592.git.gitgitgadget@gmail.com","threadId":"57443","inReplyTo":"pull.1152.v2.git.1645320591.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] merge-ort: fix small memory leak in unique_path()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-20T01:29:51Z","receivedAt":"2022-02-20T01:30:07Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe struct strmap paths member of merge_options_internal is perhaps the\nmost central data structure to all of merge-ort. Because all the paths\ninvolved in the merge need to be kept until the merge is complete, this\n\"paths\" data structure traditionally took responsibility for owning all\nthe allocated paths. When the merge is over, those paths were free()d\nas part of free()ing this strmap.\n\nIn commit 6697ee01b5d3 (merge-ort: switch our strmaps over to using\nmemory pools, 2021-07-30), we changed the allocations for pathnames to\ncome from a memory pool. That meant the ownership changed slightly;\nthere were no individual free() calls to make, instead the memory pool\nowned all those paths and they were free()d all at once.\n\nUnfortunately unique_path() was written presuming the pre-memory-pool\nmodel, and allocated a path on the heap and left it in the strmap for\nlater free()ing. Modify it to return a path allocated from the memory\npool instead.\n\nNote that there's one instance -- in record_conflicted_index_entries()\n-- where the returned string from unique_path() was only used very\ntemporarily and thus had been immediately free()'d. This codepath was\nassociated with an ugly skip-worktree workaround that has since been\nbetter fixed by the in-flight en/present-despite-skipped topic. This\nworkaround probably makes sense to excise once that topic merges down,\nbut for now, just remove the immediate free() and allow the returned\nstring to be free()d when the memory pool is released.\n\nThis fixes the following memory leak as reported by valgrind:\n\n==PID== 65 bytes in 1 blocks are definitely lost in loss record 79 of 134\n==PID== at 0xADDRESS: malloc\n==PID== by 0xADDRESS: realloc\n==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n==PID== by 0xADDRESS: strbuf_grow (strbuf.c:98)\n==PID== by 0xADDRESS: strbuf_vaddf (strbuf.c:394)\n==PID== by 0xADDRESS: strbuf_addf (strbuf.c:335)\n==PID== by 0xADDRESS: unique_path (merge-ort.c:733)\n==PID== by 0xADDRESS: process_entry (merge-ort.c:3678)\n==PID== by 0xADDRESS: process_entries (merge-ort.c:4037)\n==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4621)\n==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 21 ++++++++++++---------\n 1 file changed, 12 insertions(+), 9 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 3d7f9feb6f7..71134d51502 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -722,13 +722,15 @@ static void add_flattened_path(struct strbuf *out, const char *s)\n \t\t\tout->buf[i] = '_';\n }\n \n-static char *unique_path(struct strmap *existing_paths,\n+static char *unique_path(struct merge_options *opt,\n \t\t\t const char *path,\n \t\t\t const char *branch)\n {\n+\tchar *ret = NULL;\n \tstruct strbuf newpath = STRBUF_INIT;\n \tint suffix = 0;\n \tsize_t base_len;\n+\tstruct strmap *existing_paths = &opt->priv->paths;\n \n \tstrbuf_addf(&newpath, \"%s~\", path);\n \tadd_flattened_path(&newpath, branch);\n@@ -739,7 +741,11 @@ static char *unique_path(struct strmap *existing_paths,\n \t\tstrbuf_addf(&newpath, \"_%d\", suffix++);\n \t}\n \n-\treturn strbuf_detach(&newpath, NULL);\n+\t/* Track the new path in our memory pool */\n+\tret = mem_pool_alloc(&opt->priv->pool, newpath.len + 1);\n+\tmemcpy(ret, newpath.buf, newpath.len + 1);\n+\tstrbuf_release(&newpath);\n+\treturn ret;\n }\n \n /*** Function Grouping: functions related to collect_merge_info() ***/\n@@ -3674,7 +3680,7 @@ static void process_entry(struct merge_options *opt,\n \t\t */\n \t\tdf_file_index = (ci->dirmask & (1 << 1)) ? 2 : 1;\n \t\tbranch = (df_file_index == 1) ? opt->branch1 : opt->branch2;\n-\t\tpath = unique_path(&opt->priv->paths, path, branch);\n+\t\tpath = unique_path(opt, path, branch);\n \t\tstrmap_put(&opt->priv->paths, path, new_ci);\n \n \t\tpath_msg(opt, path, 0,\n@@ -3799,14 +3805,12 @@ static void process_entry(struct merge_options *opt,\n \t\t\t/* Insert entries into opt->priv_paths */\n \t\t\tassert(rename_a || rename_b);\n \t\t\tif (rename_a) {\n-\t\t\t\ta_path = unique_path(&opt->priv->paths,\n-\t\t\t\t\t\t path, opt->branch1);\n+\t\t\t\ta_path = unique_path(opt, path, opt->branch1);\n \t\t\t\tstrmap_put(&opt->priv->paths, a_path, ci);\n \t\t\t}\n \n \t\t\tif (rename_b)\n-\t\t\t\tb_path = unique_path(&opt->priv->paths,\n-\t\t\t\t\t\t path, opt->branch2);\n+\t\t\t\tb_path = unique_path(opt, path, opt->branch2);\n \t\t\telse\n \t\t\t\tb_path = path;\n \t\t\tstrmap_put(&opt->priv->paths, b_path, new_ci);\n@@ -4194,7 +4198,7 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n \t\t\t\tstruct stat st;\n \n \t\t\t\tif (!lstat(path, &st)) {\n-\t\t\t\t\tchar *new_name = unique_path(&opt->priv->paths,\n+\t\t\t\t\tchar *new_name = unique_path(opt,\n \t\t\t\t\t\t\t\t path,\n \t\t\t\t\t\t\t\t \"cruft\");\n \n@@ -4202,7 +4206,6 @@ static int record_conflicted_index_entries(struct merge_options *opt)\n \t\t\t\t\t\t _(\"Note: %s not up to date and in way of checking out conflicted version; old copy renamed to %s\"),\n \t\t\t\t\t\t path, new_name);\n \t\t\t\t\terrs |= rename(path, new_name);\n-\t\t\t\t\tfree(new_name);\n \t\t\t\t}\n \t\t\t\terrs |= checkout_entry(ce, &state, NULL, NULL);\n \t\t\t}\n-- \ngitgitgadget\n"},{"id":"448943","messageId":"YhL6i3x1l4qwQJg+@nand.local","threadId":"57443","inReplyTo":"f1f7fc97fe2fe5079365bb91c71fb7033378995d.1645320592.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T02:35:55Z","receivedAt":"2022-02-21T02:35:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Feb 20, 2022 at 01:29:50AM +0000, Elijah Newren via GitGitGadget wrote:\n> merge-ort.c | 15 +++++----------\n> 1 file changed, 5 insertions(+), 10 deletions(-)\n\nBoth versions of this patch look good to me (in fact, I appreciated\nseeing both v1 and v2, since v1 makes it more obvious what is changing,\nbut v2 does the whole thing in a little bit of a cleaner way).\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index d85b1cd99e9..3d7f9feb6f7 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \t\t\t\t struct tree *side1,\n> \t\t\t\t struct tree *side2)\n> {\n> -\tstruct diff_queue_struct combined;\n> +\tstruct diff_queue_struct combined = { 0 };\n> \tstruct rename_info *renames = &opt->priv->renames;\n> -\tint need_dir_renames, s, clean = 1;\n> +\tint need_dir_renames, s, i, clean = 1;\n\nAnd this entire patch looks good to me, but I did wonder about why \"i\"\nis an int here. Shouldn't it be a size_t instead? Looking at\ndiff_queue_struct's definition, both \"alloc\" and \"nr\" are signed ints,\nwhen they should almost certainly be unsigned to avoid overflow.\n\nThanks,\nTaylor\n"},{"id":"448944","messageId":"YhL8YXgRdCJV/ReP@nand.local","threadId":"57443","inReplyTo":"pull.1152.v2.git.1645320591.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] Fix a couple small leaks in merge-ort","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-21T02:43:45Z","receivedAt":"2022-02-21T02:43:52Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Feb 20, 2022 at 01:29:49AM +0000, Elijah Newren via GitGitGadget wrote:\n> Off-list, Ævar reported a few small leaks in merge-ort to me that I missed\n> previously. Here's a couple fixes.\n>\n> Changes since v1:\n>\n> * Simplified patch 1 a bit as per Ævar's suggestion\n\nThanks; both look good. I left a comment about an issue unrelated to\nthis series on the first patch. But this looks ready to go to my eyes.\n\nThanks,\nTaylor\n"},{"id":"449228","messageId":"CABPp-BFO0dz6t2ouC0+40Z6oDnGc32GKDHUpeFfh7C_Vpccerg@mail.gmail.com","threadId":"57443","inReplyTo":"YhL6i3x1l4qwQJg+@nand.local","subject":"Re: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-02-23T07:57:47Z","receivedAt":"2022-02-23T07:58:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Feb 20, 2022 at 6:35 PM Taylor Blau wrote:\n>\n> On Sun, Feb 20, 2022 at 01:29:50AM +0000, Elijah Newren via GitGitGadget wrote:\n> > merge-ort.c | 15 +++++----------\n> > 1 file changed, 5 insertions(+), 10 deletions(-)\n>\n> Both versions of this patch look good to me (in fact, I appreciated\n> seeing both v1 and v2, since v1 makes it more obvious what is changing,\n> but v2 does the whole thing in a little bit of a cleaner way).\n\nThanks for taking a look!\n\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index d85b1cd99e9..3d7f9feb6f7 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,\n> > struct tree *side1,\n> > struct tree *side2)\n> > {\n> > - struct diff_queue_struct combined;\n> > + struct diff_queue_struct combined = { 0 };\n> > struct rename_info *renames = &opt->priv->renames;\n> > - int need_dir_renames, s, clean = 1;\n> > + int need_dir_renames, s, i, clean = 1;\n>\n> And this entire patch looks good to me, but I did wonder about why \"i\"\n> is an int here. Shouldn't it be a size_t instead? Looking at\n> diff_queue_struct's definition, both \"alloc\" and \"nr\" are signed ints,\n> when they should almost certainly be unsigned to avoid overflow.\n\nYou may be right, but I'm not sure we're too worried right now about\nfolks having billions of paths involved in renames; such a repo would\nmake even the Microsoft monorepos look miniscule. ;-)\n"},{"id":"456439","messageId":"220601.86h7541yqj.gmgdl@evledraar.gmail.com","threadId":"57443","inReplyTo":"f1f7fc97fe2fe5079365bb91c71fb7033378995d.1645320592.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T10:00:25Z","receivedAt":"2022-06-01T10:11:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 20 2022, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n>\n> detect_and_process_renames() detects renames on both sides of history\n> and then combines these into a single diff_queue_struct. The combined\n> diff_queue_struct needs to be able to hold the renames found on either\n> side, and since it knows the (maximum) size it needs, it pre-emptively\n> grows the array to the appropriate size:\n>\n> \tALLOC_GROW(combined.queue,\n> \t\t renames->pairs[1].nr + renames->pairs[2].nr,\n> \t\t combined.alloc);\n>\n> It then collects the items from each side:\n>\n> \tcollect_renames(opt, &combined, MERGE_SIDE1, ...)\n> \tcollect_renames(opt, &combined, MERGE_SIDE2, ...)\n>\n> Note, though, that collect_renames() sometimes determines that some\n> pairs are unnecessary and does not include them in the combined array.\n> When it is done, detect_and_process_renames() frees this memory:\n>\n> \tif (combined.nr) {\n> ...\n> \t\tfree(combined.queue);\n> }\n>\n> The problem is that sometimes even when there are pairs, none of them\n> are necessary. Instead of checking combined.nr, just remove the\n> if-check; free() knows to skip NULL pointers. This change fixes the\n> following memory leak, as reported by valgrind:\n>\n> ==PID== 192 bytes in 1 blocks are definitely lost in loss record 107 of 134\n> ==PID== at 0xADDRESS: malloc\n> ==PID== by 0xADDRESS: realloc\n> ==PID== by 0xADDRESS: xrealloc (wrapper.c:126)\n> ==PID== by 0xADDRESS: detect_and_process_renames (merge-ort.c:3134)\n> ==PID== by 0xADDRESS: merge_ort_nonrecursive_internal (merge-ort.c:4610)\n> ==PID== by 0xADDRESS: merge_ort_internal (merge-ort.c:4709)\n> ==PID== by 0xADDRESS: merge_incore_recursive (merge-ort.c:4760)\n> ==PID== by 0xADDRESS: merge_ort_recursive (merge-ort-wrappers.c:57)\n> ==PID== by 0xADDRESS: try_merge_strategy (merge.c:753)\n> ==PID== by 0xADDRESS: cmd_merge (merge.c:1676)\n> ==PID== by 0xADDRESS: run_builtin (git.c:461)\n> ==PID== by 0xADDRESS: handle_builtin (git.c:713)\n> ==PID== by 0xADDRESS: run_argv (git.c:780)\n> ==PID== by 0xADDRESS: cmd_main (git.c:911)\n> ==PID== by 0xADDRESS: main (common-main.c:52)\n>\n> Reported-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: Elijah Newren \n> ---\n> merge-ort.c | 15 +++++----------\n> 1 file changed, 5 insertions(+), 10 deletions(-)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index d85b1cd99e9..3d7f9feb6f7 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \t\t\t\t struct tree *side1,\n> \t\t\t\t struct tree *side2)\n> {\n> -\tstruct diff_queue_struct combined;\n> +\tstruct diff_queue_struct combined = { 0 };\n> \tstruct rename_info *renames = &opt->priv->renames;\n> -\tint need_dir_renames, s, clean = 1;\n> +\tint need_dir_renames, s, i, clean = 1;\n> \tunsigned detection_run = 0;\n> \n> -\tmemset(&combined, 0, sizeof(combined));\n> \tif (!possible_renames(renames))\n> \t\tgoto cleanup;\n> \n> @@ -3175,13 +3174,9 @@ simple_cleanup:\n> \t\tfree(renames->pairs[s].queue);\n> \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n> \t}\n> -\tif (combined.nr) {\n> -\t\tint i;\n> -\t\tfor (i = 0; i < combined.nr; i++)\n> -\t\t\tpool_diff_free_filepair(&opt->priv->pool,\n> -\t\t\t\t\t\tcombined.queue[i]);\n> -\t\tfree(combined.queue);\n> -\t}\n> +\tfor (i = 0; i < combined.nr; i++)\n> +\t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n> +\tfree(combined.queue);\n> \n> \treturn clean;\n> }\n\n"},{"id":"456440","messageId":"220601.86czfs1xzi.gmgdl@evledraar.gmail.com","threadId":"57443","inReplyTo":"f1f7fc97fe2fe5079365bb91c71fb7033378995d.1645320592.git.gitgitgadget@gmail.com","subject":"Flaky SANITIZE=leak test \"regression\" in v2.36.0 (was: [PATCH v2 1/2] merge-ort: fix small memory leak in detect_and_process_renames())","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-01T10:09:45Z","receivedAt":"2022-06-01T10:26:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Feb 20 2022, Elijah Newren via GitGitGadget wrote:\n\n[I fat-fingered an empty E-Mail reply to this earlier in\nhttps://lore.kernel.org/git/220601.86h7541yqj.gmgdl@evledraar.gmail.com/,\nsorry!]\n\n> From: Elijah Newren \n>\n> detect_and_process_renames() detects renames on both sides of history\n> and then combines these into a single diff_queue_struct. The combined\n> diff_queue_struct needs to be able to hold the renames found on either\n> side, and since it knows the (maximum) size it needs, it pre-emptively\n> grows the array to the appropriate size:\n> [...]\n> diff --git a/merge-ort.c b/merge-ort.c\n> index d85b1cd99e9..3d7f9feb6f7 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -3086,12 +3086,11 @@ static int detect_and_process_renames(struct merge_options *opt,\n> \t\t\t\t struct tree *side1,\n> \t\t\t\t struct tree *side2)\n> {\n> -\tstruct diff_queue_struct combined;\n> +\tstruct diff_queue_struct combined = { 0 };\n> \tstruct rename_info *renames = &opt->priv->renames;\n> -\tint need_dir_renames, s, clean = 1;\n> +\tint need_dir_renames, s, i, clean = 1;\n> \tunsigned detection_run = 0;\n> \n> -\tmemset(&combined, 0, sizeof(combined));\n> \tif (!possible_renames(renames))\n> \t\tgoto cleanup;\n> \n> @@ -3175,13 +3174,9 @@ simple_cleanup:\n> \t\tfree(renames->pairs[s].queue);\n> \t\tDIFF_QUEUE_CLEAR(&renames->pairs[s]);\n> \t}\n> -\tif (combined.nr) {\n> -\t\tint i;\n> -\t\tfor (i = 0; i < combined.nr; i++)\n> -\t\t\tpool_diff_free_filepair(&opt->priv->pool,\n> -\t\t\t\t\t\tcombined.queue[i]);\n> -\t\tfree(combined.queue);\n> -\t}\n> +\tfor (i = 0; i < combined.nr; i++)\n> +\t\tpool_diff_free_filepair(&opt->priv->pool, combined.queue[i]);\n> +\tfree(combined.queue);\n> \n> \treturn clean;\n> }\n\nI haven't been able to find whether this actually causes anything bad,\nbut when I started digging into SANITIZE=leak failures I found that this\nchange made t6435-merge-sparse.sh flaky in the v2.36.0 release when\ncompiled with SANITIZE=leak.\n\nI.e. it \"should\" fail, but with 8d60e9d2010 (merge-ort: fix small memory\nleak in detect_and_process_renames(), 2022-02-20) it will sometimes\nsucceed.\n\nI bisected it between 2.35.0..2.36.0 with:\n\n\tgit bisect run sh -c 'make SANITIZE=leak && (cd t && ! (for i in $(seq 1 20); do ./t6435-merge-sparse.sh >/dev/null && echo $? || echo $?; done | grep -q 0))'\n\nI.e. \"fail if we succeed\" (there's some redundancy in the ad-hoc\none-liner). Manually debugging it with:\n\t\n\tdiff --git a/dir.c b/dir.c\n\tindex d91295f2bcd..3e860769c26 100644\n\t--- a/dir.c\n\t+++ b/dir.c\n\t@@ -878,6 +878,8 @@ void add_pattern(const char *string, const char *base,\n\t \tunsigned flags;\n\t \tint nowildcardlen;\n\t \n\t+\tfprintf(stderr, \"adding pattern %s\\n\", string);\n\t+\n\t \tparse_path_pattern(&string, &patternlen, &flags, &nowildcardlen);\n\t \tif (flags & PATTERN_FLAG_MUSTBEDIR) {\n\t \t\tFLEXPTR_ALLOC_MEM(pattern, pattern, string, patternlen);\n\nTurns up this odd case, i.e. we \"should\" fail at the end of the \"setup\"\nin this bit of test code:\n\n\t[...]\n test_commit_this ours &&\n git config core.sparseCheckout true &&\n echo \"/checked-out\" >.git/info/sparse-checkout &&\n git reset --hard &&\n test_must_fail git merge their\n\nHere we leak in \"git reset\", so we \"should\" get a leak like:\n\t\n\t[...]\t\n\t+ git tag ours\n\t+ git config core.sparseCheckout true\n\t+ echo /checked-out\n\t+ git reset --hard\n\tadding pattern /checked-out\n\tadding pattern /checked-out\n\tadding pattern /checked-out\n\tHEAD is now at 3dfe889 ours\n\t\n\t=================================================================\n\t==25385==ERROR: LeakSanitizer: detected memory leaks\n\t\n\tIndirect leak of 512 byte(s) in 1 object(s) allocated from:\n\t #0 0x7f9652ef50aa in __interceptor_calloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:90\n\t #1 0x6ab6dc in xcalloc /home/avar/g/git/wrapper.c:140\n\t #2 0x58e5ac in alloc_table /home/avar/g/git/hashmap.c:79\n\t #3 0x58e8e9 in hashmap_init /home/avar/g/git/hashmap.c:168\n\t #4 0x569808 in add_patterns_from_buffer /home/avar/g/git/dir.c:1136\n\t #5 0x5697a1 in add_patterns /home/avar/g/git/dir.c:1124\n\t #6 0x56996a in add_patterns_from_file_to_list /home/avar/g/git/dir.c:1164\n\t #7 0x56e0b2 in get_sparse_checkout_patterns /home/avar/g/git/dir.c:3273\n\t #8 0x56a240 in init_sparse_checkout_patterns /home/avar/g/git/dir.c:1451\n\t[...]\n\t\nThat's consistent with what I see before, but sometimes it'll succeed\nlike this:\n\n\t[...]\t\t\n\t+ git tag ours\n\t+ git config core.sparseCheckout true\n\t+ echo /checked-out\n\t+ git reset --hard\n\tadding pattern /checked-out \n\tHEAD is now at 3dfe889 ours \n [...]\n\nI.e. for some reason the same \"reset --hard\" now adds one, not three\npatterns (which before were all identical).\n\nNow, the \"flaky success\" with SANITIZE=leak does appear to be new in\n8d60e9d2010, but before that running this in a loop reveals that the 2nd\ntest sometimes succeeds, so perhaps the underlying \"issue\" isn't new.\n\nI say \"issue\" because I haven't dug enough to see if this has any impact\non anything, and the failure I discovered doesn't per-se matter now.\n\nBut perhaps this observed indeterminism is pointing the way to some\ndeeper bug here? Or maybe it isn't...\n\t\n"}]}