This fixes a minor memory leak (detected by LeakSanitizer) in git merge.

From: Kevin Backhouse <[email protected]>

To reproduce (with an ASAN build):

```
mkdir test
cd test
git init
echo x > x.txt
git add .
git commit -m "WIP"
git checkout -b dev
echo y > x.txt
git add .
git commit -m "WIP"
git checkout main
echo z > x.txt
git add .
git commit -m "WIP"
echo a > x.txt
git add .
git merge dev
```

The fix is to call free_commit_list(merge_bases) when an error occurs.

---
This fixes a minor memory leak (detected by LeakSanitizer) in git merge

To reproduce (with an ASAN build):

mkdir test
cd test
git init
echo x > x.txt
git add .
git commit -m "WIP"
git checkout -b dev
echo y > x.txt
git add .
git commit -m "WIP"
git checkout main
echo z > x.txt
git add .
git commit -m "WIP"
echo a > x.txt
git add .
git merge dev

The fix is to call free_commit_list(merge_bases) when an error occurs.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1577%2Fkevinbackhouse%2Ffree-merge-bases-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1577/kevinbackhouse/free-merge-bases-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1577

 merge-ort-wrappers.c | 4 +++-
 merge-ort.c          | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

index 4acedf3c338..aeb56c9970c 100644
@@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,
 	struct tree *head = repo_get_commit_tree(opt->repo, side1);
 	struct merge_result tmp;
 
-	if (unclean(opt, head))
+	if (unclean(opt, head)) {
+		free_commit_list(merge_bases);
 		return -1;
+	}
 
 	memset(&tmp, 0, sizeof(tmp));
 	merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);

index 8631c997002..a0eb91fb011 100644
@@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,
 		opt->branch1 = "Temporary merge branch 1";
 		opt->branch2 = "Temporary merge branch 2";
 		merge_ort_internal(opt, NULL, prev, next, result);
-		if (result->clean < 0)
+		if (result->clean < 0) {
+			free_commit_list(merge_bases);
 			return;
+		}
 		opt->branch1 = saved_b1;
 		opt->branch2 = saved_b2;
 		opt->priv->call_depth--;

base-commit: f9972720e9a405e4f6924a7cde0ed5880687f4d0

gitgitgadget

We'd rather not to see the above in the proposed log message; can't we add (a variation of) it to our test suite?

We usually have the description of what the problem is and give an analysis on why/how it happens, before presenting a solution. Write it more like:

The caller of merge_ort_recursive() expects the commit list passed in as the merge_bases parameter to be fully consumed by the function and does not free it when the function returns. In normal cases, the commit list does get consumed, but when the function returns early upon encountering an error, it forgets to clean it up.

Fix this by freeing the list in the code paths for error returns.

These two places and their fixes seem OK, but I have to wonder if these are complete fixes.

From: Kevin Backhouse <[email protected]>

---
 t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100755 t/t9904-merge-leak.sh

new file mode 100755
index 00000000000..09a4474fd73
@@ -0,0 +1,40 @@
+#!/bin/sh
+#
+
+test_description='regression test for memory leak in git merge'
+
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
+. ./lib-bash.sh
+
+# test-lib.sh disables LeakSanitizer by default, but we want it enabled
+# for this test
+ASAN_OPTIONS=
+export ASAN_OPTIONS
+
+. "$GIT_BUILD_DIR/contrib/completion/git-prompt.sh"
+
+test_expect_success 'Merge fails due to local changes' '
+	git init &&
+	echo x > x.txt &&
+	git add . &&
+	git commit -m "WIP" &&
+	git checkout -b dev &&
+	echo y > x.txt &&
+	git add . &&
+	git commit -m "WIP" &&
+	git checkout main &&
+	echo z > x.txt &&
+	git add . &&
+	git commit -m "WIP" &&
+	echo a > x.txt &&
+	git add . &&
+	echo "error: ''Your local changes to the following files would be overwritten by merge:''" >expected &&
+	echo "  x.txt" >>expected &&
+	echo "Merge with strategy ort failed." >>expected &&
+	test_must_fail git merge -s ort dev 2>actual &&
+	test_cmp expected actual
+'
+
+test_done

gitgitgadget

Hi Junio,

Thank you for your comments. As you suggested, I have added similar fixes in merge-recursive.c and updated the commit message. I have also added a test.

Thanks,

Kev

Kevin Backhouse (2):
Regression test for https://github.com/gitgitgadget/git/pull/1577
Fix minor memory leak found by LeakSanitizer.

 merge-ort-wrappers.c  |  4 +++-
 merge-ort.c           |  4 +++-
 merge-recursive.c     | 32 ++++++++++++++++++++++----------
 t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+), 12 deletions(-)
 create mode 100755 t/t9904-merge-leak.sh

base-commit: f9972720e9a405e4f6924a7cde0ed5880687f4d0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1577%2Fkevinbackhouse%2Ffree-merge-bases-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1577/kevinbackhouse/free-merge-bases-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1577

Range-diff vs v1:

 -:  ----------- > 1:  f940104a781 Regression test for https://github.com/gitgitgadget/git/pull/1577
 1:  64b00e4448d ! 2:  353e1960b44 This fixes a minor memory leak (detected by LeakSanitizer) in git merge.

@@ Metadata
Author: Kevin Backhouse <[email protected]>

## Commit message ##
- This fixes a minor memory leak (detected by LeakSanitizer) in git merge.
+ Fix minor memory leak found by LeakSanitizer.

- To reproduce (with an ASAN build):
+ The callers of merge_recursive() and merge_ort_recursive() expects the
+ commit list passed in as the merge_bases parameter to be fully
+ consumed by the function and does not free it when the function
+ returns. In normal cases, the commit list does get consumed, but when
+ the function returns early upon encountering an error, it forgets to
+ clean it up.

- ```
- mkdir test
- cd test
- git init
- echo x > x.txt
- git add .
- git commit -m "WIP"
- git checkout -b dev
- echo y > x.txt
- git add .
- git commit -m "WIP"
- git checkout main
- echo z > x.txt
- git add .
- git commit -m "WIP"
- echo a > x.txt
- git add .
- git merge dev
- ```
-
- The fix is to call free_commit_list(merge_bases) when an error occurs.
+ Fix this by freeing the list in the code paths for error returns.

@@ merge-ort.c: static void merge_ort_internal(struct merge_options *opt,
opt->branch1 = saved_b1;
opt->branch2 = saved_b2;
opt->priv->call_depth--;
+
+ ## merge-recursive.c ##
+@@ merge-recursive.c: static int merge_recursive_internal(struct merge_options *opt,
+ opt->branch1 = "Temporary merge branch 1";
+ opt->branch2 = "Temporary merge branch 2";
+ if (merge_recursive_internal(opt, merged_merge_bases, iter->item,
+- NULL, &merged_merge_bases) < 0)
+- return -1;
++ NULL, &merged_merge_bases) < 0) {
++ clean = -1;
++ goto out;
++ }
+ opt->branch1 = saved_b1;
+ opt->branch2 = saved_b2;
+ opt->priv->call_depth--;
+
+- if (!merged_merge_bases)
+- return err(opt, _("merge returned no commit"));
++ if (!merged_merge_bases) {
++ clean = err(opt, _("merge returned no commit"));
++ goto out;
++ }
+ }
+
+ /*
+@@ merge-recursive.c: static int merge_recursive_internal(struct merge_options *opt,
+ repo_get_commit_tree(opt->repo,
+ merged_merge_bases),
+ &result_tree);
++
++out:
+ strbuf_release(&merge_base_abbrev);
+ opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */
++ free_commit_list(merge_bases);
+ if (clean < 0) {
+ flush_output(opt);
+ return clean;
+@@ merge-recursive.c: static int merge_start(struct merge_options *opt, struct tree *head)
+ assert(!opt->record_conflict_msgs_as_headers);
+ assert(!opt->msg_header_prefix);
+
++ CALLOC_ARRAY(opt->priv, 1);
++ string_list_init_dup(&opt->priv->df_conflict_file_set);
++
+ /* Sanity check on repo state; index must match head */
+ if (repo_index_has_changes(opt->repo, head, &sb)) {
+ err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"),
+@@ merge-recursive.c: static int merge_start(struct merge_options *opt, struct tree *head)
+ return -1;
+ }
+
+- CALLOC_ARRAY(opt->priv, 1);
+- string_list_init_dup(&opt->priv->df_conflict_file_set);
+ return 0;
+ }
+
+ static void merge_finalize(struct merge_options *opt)
+ {
+ flush_output(opt);
+- if (!opt->priv->call_depth && opt->buffer_output < 2)
+- strbuf_release(&opt->obuf);
++ strbuf_release(&opt->obuf);
+ if (show(opt, 2))
+ diff_warn_rename_limit("merge.renamelimit",
+ opt->priv->needed_rename_limit, 0);
+@@ merge-recursive.c: int merge_trees(struct merge_options *opt,
+
+ assert(opt->ancestor != NULL);
+
+- if (merge_start(opt, head))
++ if (merge_start(opt, head)) {
++ merge_finalize(opt);
+ return -1;
++ }
+ clean = merge_trees_internal(opt, head, merge, merge_base, &ignored);
+ merge_finalize(opt);
+
+@@ merge-recursive.c: int merge_recursive(struct merge_options *opt,
+ prepare_repo_settings(opt->repo);
+ opt->repo->settings.command_requires_full_index = 1;
+
+- if (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))
++ if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) {
++ free_commit_list(merge_bases);
++ merge_finalize(opt);
+ return -1;
++ }
+ clean = merge_recursive_internal(opt, h1, h2, merge_bases, result);
+ merge_finalize(opt);
+

gitgitgadget

From: Kevin Backhouse <[email protected]>

The callers of merge_recursive() and merge_ort_recursive() expects the commit list passed in as the merge_bases parameter to be fully consumed by the function and does not free it when the function returns. In normal cases, the commit list does get consumed, but when the function returns early upon encountering an error, it forgets to clean it up.

Fix this by freeing the list in the code paths for error returns.

---
 merge-ort-wrappers.c |  4 +++-
 merge-ort.c          |  4 +++-
 merge-recursive.c    | 32 ++++++++++++++++++++++----------
 3 files changed, 28 insertions(+), 12 deletions(-)

index 4acedf3c338..aeb56c9970c 100644
@@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,
 	struct tree *head = repo_get_commit_tree(opt->repo, side1);
 	struct merge_result tmp;
 
-	if (unclean(opt, head))
+	if (unclean(opt, head)) {
+		free_commit_list(merge_bases);
 		return -1;
+	}
 
 	memset(&tmp, 0, sizeof(tmp));
 	merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);

index 8631c997002..a0eb91fb011 100644
@@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,
 		opt->branch1 = "Temporary merge branch 1";
 		opt->branch2 = "Temporary merge branch 2";
 		merge_ort_internal(opt, NULL, prev, next, result);
-		if (result->clean < 0)
+		if (result->clean < 0) {
+			free_commit_list(merge_bases);
 			return;
+		}
 		opt->branch1 = saved_b1;
 		opt->branch2 = saved_b2;
 		opt->priv->call_depth--;

index 6a4081bb0f5..49e54d3722f 100644
@@ -3652,14 +3652,18 @@ static int merge_recursive_internal(struct merge_options *opt,
 		opt->branch1 = "Temporary merge branch 1";
 		opt->branch2 = "Temporary merge branch 2";
 		if (merge_recursive_internal(opt, merged_merge_bases, iter->item,
-					     NULL, &merged_merge_bases) < 0)
-			return -1;
+					     NULL, &merged_merge_bases) < 0) {
+			clean = -1;
+			goto out;
+		}
 		opt->branch1 = saved_b1;
 		opt->branch2 = saved_b2;
 		opt->priv->call_depth--;
 
-		if (!merged_merge_bases)
-			return err(opt, _("merge returned no commit"));
+		if (!merged_merge_bases) {
+			clean = err(opt, _("merge returned no commit"));
+			goto out;
+		}
 	}
 
 	/*

@@ -3682,8 +3686,11 @@ static int merge_recursive_internal(struct merge_options *opt,
 				     repo_get_commit_tree(opt->repo,
 							  merged_merge_bases),
 				     &result_tree);
+
+out:
 	strbuf_release(&merge_base_abbrev);
 	opt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */
+	free_commit_list(merge_bases);
 	if (clean < 0) {
 		flush_output(opt);
 		return clean;

@@ -3729,6 +3736,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 	assert(!opt->record_conflict_msgs_as_headers);
 	assert(!opt->msg_header_prefix);
 
+	CALLOC_ARRAY(opt->priv, 1);
+	string_list_init_dup(&opt->priv->df_conflict_file_set);
+
 	/* Sanity check on repo state; index must match head */
 	if (repo_index_has_changes(opt->repo, head, &sb)) {
 		err(opt, _("Your local changes to the following files would be overwritten by merge:\n  %s"),

@@ -3737,16 +3747,13 @@ static int merge_start(struct merge_options *opt, struct tree *head)
 		return -1;
 	}
 
-	CALLOC_ARRAY(opt->priv, 1);
-	string_list_init_dup(&opt->priv->df_conflict_file_set);
 	return 0;
 }
 
 static void merge_finalize(struct merge_options *opt)
 {
 	flush_output(opt);
-	if (!opt->priv->call_depth && opt->buffer_output < 2)
-		strbuf_release(&opt->obuf);
+	strbuf_release(&opt->obuf);
 	if (show(opt, 2))
 		diff_warn_rename_limit("merge.renamelimit",
 				       opt->priv->needed_rename_limit, 0);

@@ -3763,8 +3770,10 @@ int merge_trees(struct merge_options *opt,
 
 	assert(opt->ancestor != NULL);
 
-	if (merge_start(opt, head))
+	if (merge_start(opt, head)) {
+		merge_finalize(opt);
 		return -1;
+	}
 	clean = merge_trees_internal(opt, head, merge, merge_base, &ignored);
 	merge_finalize(opt);
 

@@ -3785,8 +3794,11 @@ int merge_recursive(struct merge_options *opt,
 	prepare_repo_settings(opt->repo);
 	opt->repo->settings.command_requires_full_index = 1;
 
-	if (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))
+	if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) {
+		free_commit_list(merge_bases);
+		merge_finalize(opt);
 		return -1;
+	}
 	clean = merge_recursive_internal(opt, h1, h2, merge_bases, result);
 	merge_finalize(opt);
 

gitgitgadget

We try to come up with titles that are helpful to readers when seen in "git shortlog --since=6.months --no-merges", and the above does not exactly it.

Continuing the review for the previous step, perhaps

Subject: [PATCH] merge: free list of merge bases upon failure

or something?

"expects" -> "expect"

"does not" -> "do not".

Well written to be understandable. Nicely done.

The other function would need a more involved fix, which would basically involve porting a59b8dd94f (merge-ort: fix memory leak in merge_ort_internal(), 2022-01-20) to merge-recursive as a preparatory step. This particular cleanup cannot be ported in its current form to merge-recursive.c until then.