{"thread":{"id":"57275","subject":"[PATCH 0/2] Fix some old memory leaks in merge-ort and builtin/merge","startedAt":"2022-01-20T07:47:20Z","lastAt":"2022-01-22T00:05:23Z","messageCount":4,"participants":["Elijah Newren via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"446538","messageId":"pull.1200.git.git.1642664835.gitgitgadget@gmail.com","threadId":"57275","inReplyTo":null,"subject":"[PATCH 0/2] Fix some old memory leaks in merge-ort and builtin/merge","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-20T07:47:13Z","receivedAt":"2022-01-20T07:47:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"This series fixes three memory leaks.\n\nThe first comes from a report at [1]. It's a leak in merge-ort that\npre-dates the remerge-diff topic (technically traceable back 15.5 years ago\nacross merge-recursive.c history) and is triggerable from a variety of\ntestcases. I think I may have overlooked it previously just because there's\nother leaks in revision walking and this looks like one of those.\n\nThe next two leaks were ones in builtin/merge.c found while fixing the above\nleak; they are fixed together in the second patch.\n\n[1] https://lore.kernel.org/git/220119.86pmonn2mb.gmgdl@evledraar.gmail.com/\n\nElijah Newren (2):\n merge-ort: fix memory leak in merge_ort_internal()\n merge: fix memory leaks in cmd_merge()\n\n builtin/merge.c | 6 +++++-\n merge-ort.c | 10 +++++-----\n 2 files changed, 10 insertions(+), 6 deletions(-)\n\n\nbase-commit: af4e5f569bc89f356eb34a9373d7f82aca6faa8a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1200%2Fnewren%2Fmerge-leaks-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1200/newren/merge-leaks-v1\nPull-Request: https://github.com/git/git/pull/1200\n-- \ngitgitgadget\n"},{"id":"446539","messageId":"396325fac50365f2651b9447d227fc9ae025acef.1642664835.git.gitgitgadget@gmail.com","threadId":"57275","inReplyTo":"pull.1200.git.git.1642664835.gitgitgadget@gmail.com","subject":"[PATCH 1/2] merge-ort: fix memory leak in merge_ort_internal()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-20T07:47:14Z","receivedAt":"2022-01-20T07:47:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe documentation for merge_incore_recursive(), modelled after\nmerge_recursive(), notes that\n\n merge_bases will be consumed (emptied) so make a copy if you need it\n\nHowever, in merge_ort_internal() (which merge_incore_recursive() calls),\nit runs\n\n merged_merge_bases = pop_commit(&merge_bases);\n ...\n for (iter = merge_bases; iter; iter = iter->next) {\n ...\n }\n\nIn other words, it only consumes the *first* entry of merge_bases, and\nthe rest it iterates through. If it iterated through all of them, the\ncaller could be responsible for free'ing the memory. If it consumed all\nof them, the current documentation would be correct and the callers\nwould need to do nothing. The current middle ground makes it impossible\nfor callers to avoid memory leaks, since any attempt to use the\nmerge_bases it passes in would result in a use-after-free.\n\nIt turns out this part of the code was copied from merge-recursive.c,\nwhich has had the same bug for 15.5 years. However, since we are trying\nto keep merge-recursive.c stable as we sunset it, let's just fix the\nleak in in merge_ort_internal() by having it actually consume all the\nelements of the merge_bases commit_list.\n\nTesting this commit against t6404 (the first testcase specifically\nabout recursive merges) under valgrind shows that this patch fixes\nthe following leak:\n\n 32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost \\\n in loss record 49 of 126\n at 0x484086F: malloc (vg_replace_malloc.c:380)\n by 0x69FFEB: do_xmalloc (wrapper.c:41)\n by 0x6A0073: xmalloc (wrapper.c:62)\n by 0x52A72D: commit_list_insert (commit.c:556)\n by 0x47EC86: try_merge_strategy (merge.c:751)\n by 0x48143B: cmd_merge (merge.c:1679)\n by 0x40686E: run_builtin (git.c:464)\n by 0x406C51: handle_builtin (git.c:716)\n by 0x406E96: run_argv (git.c:783)\n by 0x40730A: cmd_main (git.c:914)\n by 0x4E7DFA: main (common-main.c:56)\n\nReported-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Elijah Newren \n---\n merge-ort.c | 10 +++++-----\n 1 file changed, 5 insertions(+), 5 deletions(-)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex c3197970219..83a89f9f4c4 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -4575,7 +4575,7 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\t\t struct commit *h2,\n \t\t\t struct merge_result *result)\n {\n-\tstruct commit_list *iter;\n+\tstruct commit *next;\n \tstruct commit *merged_merge_bases;\n \tconst char *ancestor_name;\n \tstruct strbuf merge_base_abbrev = STRBUF_INIT;\n@@ -4604,7 +4604,8 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\tancestor_name = merge_base_abbrev.buf;\n \t}\n \n-\tfor (iter = merge_bases; iter; iter = iter->next) {\n+\tfor (next = pop_commit(&merge_bases); next;\n+\t next = pop_commit(&merge_bases)) {\n \t\tconst char *saved_b1, *saved_b2;\n \t\tstruct commit *prev = merged_merge_bases;\n \n@@ -4621,7 +4622,7 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\tsaved_b2 = opt->branch2;\n \t\topt->branch1 = \"Temporary merge branch 1\";\n \t\topt->branch2 = \"Temporary merge branch 2\";\n-\t\tmerge_ort_internal(opt, NULL, prev, iter->item, result);\n+\t\tmerge_ort_internal(opt, NULL, prev, next, result);\n \t\tif (result->clean < 0)\n \t\t\treturn;\n \t\topt->branch1 = saved_b1;\n@@ -4632,8 +4633,7 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\t\t\t\t\t\t result->tree,\n \t\t\t\t\t\t\t \"merged tree\");\n \t\tcommit_list_insert(prev, &merged_merge_bases->parents);\n-\t\tcommit_list_insert(iter->item,\n-\t\t\t\t &merged_merge_bases->parents->next);\n+\t\tcommit_list_insert(next, &merged_merge_bases->parents->next);\n \n \t\tclear_or_reinit_internal_opts(opt->priv, 1);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"446540","messageId":"c6522d6449f9323edbbd9329b2859368492aabd3.1642664835.git.gitgitgadget@gmail.com","threadId":"57275","inReplyTo":"pull.1200.git.git.1642664835.gitgitgadget@gmail.com","subject":"[PATCH 2/2] merge: fix memory leaks in cmd_merge()","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-01-20T07:47:15Z","receivedAt":"2022-01-20T07:47:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere were two commit_lists created in cmd_merge() that were only\nconditionally free()'d. Add a quick conditional call to\nfree_commit_list() for each of them at the end of the function.\n\nTesting this commit against t6404 under valgrind shows that this patch\nfixes the following two leaks:\n\n 16 bytes in 1 blocks are definitely lost in loss record 16 of 126\n at 0x484086F: malloc (vg_replace_malloc.c:380)\n by 0x69FFEB: do_xmalloc (wrapper.c:41)\n by 0x6A0073: xmalloc (wrapper.c:62)\n by 0x52A72D: commit_list_insert (commit.c:556)\n by 0x47FC93: reduce_parents (merge.c:1114)\n by 0x4801EE: collect_parents (merge.c:1214)\n by 0x480B56: cmd_merge (merge.c:1465)\n by 0x40686E: run_builtin (git.c:464)\n by 0x406C51: handle_builtin (git.c:716)\n by 0x406E96: run_argv (git.c:783)\n by 0x40730A: cmd_main (git.c:914)\n by 0x4E7DFA: main (common-main.c:56)\n\n 8 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost in \\\n loss record 61 of 126\n at 0x484086F: malloc (vg_replace_malloc.c:380)\n by 0x69FFEB: do_xmalloc (wrapper.c:41)\n by 0x6A0073: xmalloc (wrapper.c:62)\n by 0x52A72D: commit_list_insert (commit.c:556)\n by 0x52A8F2: commit_list_insert_by_date (commit.c:620)\n by 0x5270AC: get_merge_bases_many_0 (commit-reach.c:413)\n by 0x52716C: repo_get_merge_bases (commit-reach.c:438)\n by 0x480E5A: cmd_merge (merge.c:1520)\n by 0x40686E: run_builtin (git.c:464)\n by 0x406C51: handle_builtin (git.c:716)\n by 0x406E96: run_argv (git.c:783)\n by 0x40730A: cmd_main (git.c:914)\n\nThere are still 3 leaks in chdir_notify_register() after this, but\nchdir_notify_register() has been brought up on the list before and folks\nwere not a fan of fixing those, so I'm not touching them.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 74e53cf20a7..bd8fff9b223 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1273,7 +1273,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n-\tstruct commit_list *remoteheads, *p;\n+\tstruct commit_list *remoteheads = NULL, *p;\n \tvoid *branch_to_free;\n \tint orig_argc = argc;\n \n@@ -1752,6 +1752,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = suggest_conflicts();\n \n done:\n+\tif (!automerge_was_ok) {\n+\t\tfree_commit_list(common);\n+\t\tfree_commit_list(remoteheads);\n+\t}\n \tstrbuf_release(&buf);\n \tfree(branch_to_free);\n \treturn ret;\n-- \ngitgitgadget\n"},{"id":"446663","messageId":"xmqqczkkeitd.fsf@gitster.g","threadId":"57275","inReplyTo":"c6522d6449f9323edbbd9329b2859368492aabd3.1642664835.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] merge: fix memory leaks in cmd_merge()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-22T00:05:18Z","receivedAt":"2022-01-22T00:05:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> done:\n> +\tif (!automerge_was_ok) {\n> +\t\tfree_commit_list(common);\n> +\t\tfree_commit_list(remoteheads);\n> +\t}\n\nI wondered what happens upon a successful automerge.\n\nWe call finish_automerge() and come here, and I do see a call to\nfree_commit_list(common) in finish_automerge(), but the way\nremoteheads is used looked a bit iffy.\n\nIn finish_automerge(), we use a temporary variable parents to point\nremoteheads at it, and conditionally prepend the current HEAD at the\nbeginning of the parents list. This is passed to commit_tree(),\nwhich does pop_commit() to consume all commits on the list.\n\nSo after commit_tree() returns, all commit_list instances on\nremoteheads list, and possibly the one finish_automerge() prepended\nfor the current HEAD, are all consumed, and there is no need to\ncall, and it would be wrong to call, free_commit_list(), at this\npoint.\n\nSo, I agree that this conditional freeing is correct. It was just\nit was a bit hard to see.\n\nThanks.\n\n\n"}]}