@matheusbsbs / matheus-bagliano / commits / a59b8dd94f

merge-ort: fix memory leak in merge_ort_internal()

The documentation for merge_incore_recursive(), modelled after merge_recursive(), notes that merge_bases will be consumed (emptied) so make a copy if you need it However, in merge_ort_internal() (which merge_incore_recursive() calls), it runs merged_merge_bases = pop_commit(&merge_bases); ... for (iter = merge_bases; iter; iter = iter->next) { ... } In other words, it only consumes the *first* entry of merge_bases, and the rest it iterates through. If it iterated through all of them, the caller could be responsible for free'ing the memory. If it consumed all of them, the current documentation would be correct and the callers would need to do nothing. The current middle ground makes it impossible for callers to avoid memory leaks, since any attempt to use the merge_bases it passes in would result in a use-after-free. It turns out this part of the code was copied from merge-recursive.c, which has had the same bug for 15.5 years. However, since we are trying to keep merge-recursive.c stable as we sunset it, let's just fix the leak in in merge_ort_internal() by having it actually consume all the elements of the merge_bases commit_list. Testing this commit against t6404 (the first testcase specifically about recursive merges) under valgrind shows that this patch fixes the following leak: 32 (16 direct, 16 indirect) bytes in 1 blocks are definitely lost \ in loss record 49 of 126 at 0x484086F: malloc (vg_replace_malloc.c:380) by 0x69FFEB: do_xmalloc (wrapper.c:41) by 0x6A0073: xmalloc (wrapper.c:62) by 0x52A72D: commit_list_insert (commit.c:556) by 0x47EC86: try_merge_strategy (merge.c:751) by 0x48143B: cmd_merge (merge.c:1679) by 0x40686E: run_builtin (git.c:464) by 0x406C51: handle_builtin (git.c:716) by 0x406E96: run_argv (git.c:783) by 0x40730A: cmd_main (git.c:914) by 0x4E7DFA: main (common-main.c:56) Reported-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Elijah Newren committed Jan 20, 2022 at 07:47 UTC a59b8dd94f0e24f56e1d4b4b1f537d0e7144e30c

1 file changed +5 -5

merge-ort.c

+5 -5

@@ -4575,7 +4575,7 @@ static void merge_ort_internal(struct merge_options *opt,

4575 struct commit *h2,

4576 struct merge_result *result)

4577 {

4578 - struct commit_list *iter;

4578 + struct commit *next;

4579 struct commit *merged_merge_bases;

4580 const char *ancestor_name;

4581 struct strbuf merge_base_abbrev = STRBUF_INIT;

@@ -4604,7 +4604,8 @@ static void merge_ort_internal(struct merge_options *opt,

4604 ancestor_name = merge_base_abbrev.buf;

4605 }

4606

4607 - for (iter = merge_bases; iter; iter = iter->next) {

4607 + for (next = pop_commit(&merge_bases); next;

4608 + next = pop_commit(&merge_bases)) {

4609 const char *saved_b1, *saved_b2;

4610 struct commit *prev = merged_merge_bases;

4611

@@ -4621,7 +4622,7 @@ static void merge_ort_internal(struct merge_options *opt,

4622 saved_b2 = opt->branch2;

4623 opt->branch1 = "Temporary merge branch 1";

4624 opt->branch2 = "Temporary merge branch 2";

4624 - merge_ort_internal(opt, NULL, prev, iter->item, result);

4625 + merge_ort_internal(opt, NULL, prev, next, result);

4626 if (result->clean < 0)

4627 return;

4628 opt->branch1 = saved_b1;

@@ -4632,8 +4633,7 @@ static void merge_ort_internal(struct merge_options *opt,

4633 result->tree,

4634 "merged tree");

4635 commit_list_insert(prev, &merged_merge_bases->parents);

4635 - commit_list_insert(iter->item,

4636 - &merged_merge_bases->parents->next);

4636 + commit_list_insert(next, &merged_merge_bases->parents->next);

4637

4638 clear_or_reinit_internal_opts(opt->priv, 1);

4639 }