Skip to content

Commit a59b8dd

Browse files
newrengitster
authored andcommitted
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 <[email protected]> Signed-off-by: Elijah Newren <[email protected]> Signed-off-by: Junio C Hamano <[email protected]>
1 parent 297ca89 commit a59b8dd

File tree

1 file changed

+5
-5
lines changed

1 file changed

+5
-5
lines changed

merge-ort.c

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -4575,7 +4575,7 @@ static void merge_ort_internal(struct merge_options *opt,
45754575
struct commit *h2,
45764576
struct merge_result *result)
45774577
{
4578-
struct commit_list *iter;
4578+
struct commit *next;
45794579
struct commit *merged_merge_bases;
45804580
const char *ancestor_name;
45814581
struct strbuf merge_base_abbrev = STRBUF_INIT;
@@ -4604,7 +4604,8 @@ static void merge_ort_internal(struct merge_options *opt,
46044604
ancestor_name = merge_base_abbrev.buf;
46054605
}
46064606

4607-
for (iter = merge_bases; iter; iter = iter->next) {
4607+
for (next = pop_commit(&merge_bases); next;
4608+
next = pop_commit(&merge_bases)) {
46084609
const char *saved_b1, *saved_b2;
46094610
struct commit *prev = merged_merge_bases;
46104611

@@ -4621,7 +4622,7 @@ static void merge_ort_internal(struct merge_options *opt,
46214622
saved_b2 = opt->branch2;
46224623
opt->branch1 = "Temporary merge branch 1";
46234624
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);
46254626
if (result->clean < 0)
46264627
return;
46274628
opt->branch1 = saved_b1;
@@ -4632,8 +4633,7 @@ static void merge_ort_internal(struct merge_options *opt,
46324633
result->tree,
46334634
"merged tree");
46344635
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);
46374637

46384638
clear_or_reinit_internal_opts(opt->priv, 1);
46394639
}

0 commit comments

Comments
 (0)