[PATCH 0/2] Fix some old memory leaks in merge-ort and builtin/merge

[PATCH 0/2] Fix some old memory leaks in merge-ort and builtin/merge

[Elijah Newren via GitGitGadget]

This series fixes three memory leaks.

The first comes from a report at [1]. It's a leak in merge-ort that
pre-dates the remerge-diff topic (technically traceable back 15.5 years ago
across merge-recursive.c history) and is triggerable from a variety of
testcases. I think I may have overlooked it previously just because there's
other leaks in revision walking and this looks like one of those.

The next two leaks were ones in builtin/merge.c found while fixing the above
leak; they are fixed together in the second patch.

[1] https://lore.kernel.org/git/220119.86pmonn2mb.gmgdl@evledraar.gmail.com/

Elijah Newren (2):
  merge-ort: fix memory leak in merge_ort_internal()
  merge: fix memory leaks in cmd_merge()

 builtin/merge.c |  6 +++++-
 merge-ort.c     | 10 +++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)


base-commit: af4e5f569bc89f356eb34a9373d7f82aca6faa8a
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1200%2Fnewren%2Fmerge-leaks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1200/newren/merge-leaks-v1
Pull-Request: https://github.com/git/git/pull/1200
-- 
gitgitgadget

[PATCH 1/2] merge-ort: fix memory leak in merge_ort_internal()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

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>
---
 merge-ort.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/merge-ort.c b/merge-ort.c
index c3197970219..83a89f9f4c4 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4575,7 +4575,7 @@ static void merge_ort_internal(struct merge_options *opt,
 			       struct commit *h2,
 			       struct merge_result *result)
 {
-	struct commit_list *iter;
+	struct commit *next;
 	struct commit *merged_merge_bases;
 	const char *ancestor_name;
 	struct strbuf merge_base_abbrev = STRBUF_INIT;
@@ -4604,7 +4604,8 @@ static void merge_ort_internal(struct merge_options *opt,
 		ancestor_name = merge_base_abbrev.buf;
 	}
 
-	for (iter = merge_bases; iter; iter = iter->next) {
+	for (next = pop_commit(&merge_bases); next;
+	     next = pop_commit(&merge_bases)) {
 		const char *saved_b1, *saved_b2;
 		struct commit *prev = merged_merge_bases;
 
@@ -4621,7 +4622,7 @@ static void merge_ort_internal(struct merge_options *opt,
 		saved_b2 = opt->branch2;
 		opt->branch1 = "Temporary merge branch 1";
 		opt->branch2 = "Temporary merge branch 2";
-		merge_ort_internal(opt, NULL, prev, iter->item, result);
+		merge_ort_internal(opt, NULL, prev, next, result);
 		if (result->clean < 0)
 			return;
 		opt->branch1 = saved_b1;
@@ -4632,8 +4633,7 @@ static void merge_ort_internal(struct merge_options *opt,
 							 result->tree,
 							 "merged tree");
 		commit_list_insert(prev, &merged_merge_bases->parents);
-		commit_list_insert(iter->item,
-				   &merged_merge_bases->parents->next);
+		commit_list_insert(next, &merged_merge_bases->parents->next);
 
 		clear_or_reinit_internal_opts(opt->priv, 1);
 	}
-- 
gitgitgadget

[PATCH 2/2] merge: fix memory leaks in cmd_merge()

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

There were two commit_lists created in cmd_merge() that were only
conditionally free()'d.  Add a quick conditional call to
free_commit_list() for each of them at the end of the function.

Testing this commit against t6404 under valgrind shows that this patch
fixes the following two leaks:

    16 bytes in 1 blocks are definitely lost in loss record 16 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 0x47FC93: reduce_parents (merge.c:1114)
       by 0x4801EE: collect_parents (merge.c:1214)
       by 0x480B56: cmd_merge (merge.c:1465)
       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)

    8 (16 direct, 32 indirect) bytes in 1 blocks are definitely lost in \
    loss record 61 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 0x52A8F2: commit_list_insert_by_date (commit.c:620)
       by 0x5270AC: get_merge_bases_many_0 (commit-reach.c:413)
       by 0x52716C: repo_get_merge_bases (commit-reach.c:438)
       by 0x480E5A: cmd_merge (merge.c:1520)
       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)

There are still 3 leaks in chdir_notify_register() after this, but
chdir_notify_register() has been brought up on the list before and folks
were not a fan of fixing those, so I'm not touching them.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 builtin/merge.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 74e53cf20a7..bd8fff9b223 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1273,7 +1273,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
-	struct commit_list *remoteheads, *p;
+	struct commit_list *remoteheads = NULL, *p;
 	void *branch_to_free;
 	int orig_argc = argc;
 
@@ -1752,6 +1752,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts();
 
 done:
+	if (!automerge_was_ok) {
+		free_commit_list(common);
+		free_commit_list(remoteheads);
+	}
 	strbuf_release(&buf);
 	free(branch_to_free);
 	return ret;
-- 
gitgitgadget

Re: [PATCH 2/2] merge: fix memory leaks in cmd_merge()

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

>  done:
> +	if (!automerge_was_ok) {
> +		free_commit_list(common);
> +		free_commit_list(remoteheads);
> +	}

I wondered what happens upon a successful automerge.

We call finish_automerge() and come here, and I do see a call to
free_commit_list(common) in finish_automerge(), but the way
remoteheads is used looked a bit iffy.

In finish_automerge(), we use a temporary variable parents to point
remoteheads at it, and conditionally prepend the current HEAD at the
beginning of the parents list.  This is passed to commit_tree(),
which does pop_commit() to consume all commits on the list.

So after commit_tree() returns, all commit_list instances on
remoteheads list, and possibly the one finish_automerge() prepended
for the current HEAD, are all consumed, and there is no need to
call, and it would be wrong to call, free_commit_list(), at this
point.

So, I agree that this conditional freeing is correct.  It was just
it was a bit hard to see.

Thanks.