merge: fix leaking merge bases

When calling either the recursive or the ORT merge machineries we need
to provide a list of merge bases. The ownership of that parameter is
then implicitly transferred to the callee, which is somewhat fishy.
Furthermore, that list may leak in some cases where the merge machinery
runs into an error, thus causing a memory leak.

Refactor the code such that we stop transferring ownership. Instead, the
merge machinery will now create its own local copies of the passed in
list as required if they need to modify the list. Free the list at the
callsites as required.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

1 files changed, 30 insertions, 19 deletions

diff --git a/merge-recursive.c b/merge-recursive.c
index 832c8ef3f3..1ac0316cce 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -3633,15 +3633,16 @@ static int merge_trees_internal(struct merge_options *opt,
 static int merge_recursive_internal(struct merge_options *opt,
 				    struct commit *h1,
 				    struct commit *h2,
-				    struct commit_list *merge_bases,
+				    const struct commit_list *_merge_bases,
 				    struct commit **result)
 {
+	struct commit_list *merge_bases = copy_commit_list(_merge_bases);
 	struct commit_list *iter;
 	struct commit *merged_merge_bases;
 	struct tree *result_tree;
-	int clean;
 	const char *ancestor_name;
 	struct strbuf merge_base_abbrev = STRBUF_INIT;
+	int ret;
 
 	if (show(opt, 4)) {
 		output(opt, 4, _("Merging:"));
@@ -3651,8 +3652,10 @@ static int merge_recursive_internal(struct merge_options *opt,
 
 	if (!merge_bases) {
 		if (repo_get_merge_bases(the_repository, h1, h2,
-					 &merge_bases) < 0)
-			return -1;
+					 &merge_bases) < 0) {
+			ret = -1;
+			goto out;
+		}
 		merge_bases = reverse_commit_list(merge_bases);
 	}
 
@@ -3702,14 +3705,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) {
+			ret = -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) {
+			ret = err(opt, _("merge returned no commit"));
+			goto out;
+		}
 	}
 
 	/*
@@ -3726,17 +3733,16 @@ static int merge_recursive_internal(struct merge_options *opt,
 		repo_read_index(opt->repo);
 
 	opt->ancestor = ancestor_name;
-	clean = merge_trees_internal(opt,
-				     repo_get_commit_tree(opt->repo, h1),
-				     repo_get_commit_tree(opt->repo, h2),
-				     repo_get_commit_tree(opt->repo,
-							  merged_merge_bases),
-				     &result_tree);
-	strbuf_release(&merge_base_abbrev);
+	ret = merge_trees_internal(opt,
+				   repo_get_commit_tree(opt->repo, h1),
+				   repo_get_commit_tree(opt->repo, h2),
+				   repo_get_commit_tree(opt->repo,
+							merged_merge_bases),
+				   &result_tree);
 	opt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */
-	if (clean < 0) {
+	if (ret < 0) {
 		flush_output(opt);
-		return clean;
+		goto out;
 	}
 
 	if (opt->priv->call_depth) {
@@ -3745,7 +3751,11 @@ static int merge_recursive_internal(struct merge_options *opt,
 		commit_list_insert(h1, &(*result)->parents);
 		commit_list_insert(h2, &(*result)->parents->next);
 	}
-	return clean;
+
+out:
+	strbuf_release(&merge_base_abbrev);
+	free_commit_list(merge_bases);
+	return ret;
 }
 
 static int merge_start(struct merge_options *opt, struct tree *head)
@@ -3827,7 +3837,7 @@ int merge_trees(struct merge_options *opt,
 int merge_recursive(struct merge_options *opt,
 		    struct commit *h1,
 		    struct commit *h2,
-		    struct commit_list *merge_bases,
+		    const struct commit_list *merge_bases,
 		    struct commit **result)
 {
 	int clean;
@@ -3895,6 +3905,7 @@ int merge_recursive_generic(struct merge_options *opt,
 	repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);
 	clean = merge_recursive(opt, head_commit, next_commit, ca,
 				result);
+	free_commit_list(ca);
 	if (clean < 0) {
 		rollback_lock_file(&lock);
 		return clean;