git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Elijah Newren via GitGitGadget <gitgitgadget@gmail.com>
Date
Jan 20, 2022, 07:47 UTC
Message-ID
<396325fac50365f2651b9447d227fc9ae025acef.1642664835.git.gitgitgadget@gmail.com>
In-Reply-To
<pull.1200.git.git.1642664835.gitgitgadget@gmail.com>
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
Previous: Elijah Newren via GitGitGadgetNext: Elijah Newren via GitGitGadget
Message 2 of 4 in “Fix some old memory leaks in merge-ort and builtin/merge”
  1. 0/2 Fix some old memory leaks in merge-ort and builtin/mergeElijah Newren via GitGitGadget, Jan 20, 2022
  2. 1/2 merge-ort: fix memory leak in merge_ort_internal()Elijah Newren via GitGitGadget, Jan 20, 2022
  3. 2/2 merge: fix memory leaks in cmd_merge()Elijah Newren via GitGitGadget, Jan 20, 2022
  4. Junio C HamanoJan 22, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.