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>

diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 1082d91..dab2fdc 100644 (file)
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -482,6 +482,7 @@ static int real_merge(struct merge_tree_options *o,
                        die(_("refusing to merge unrelated histories"));
                merge_bases = reverse_commit_list(merge_bases);
                merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);
+               free_commit_list(merge_bases);
        }
 
        if (result.clean < 0)

diff --git a/builtin/merge.c b/builtin/merge.c
index 682c6ed..1120a6e 100644 (file)
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -746,6 +746,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,
                else
                        clean = merge_recursive(&o, head, remoteheads->item,
                                                reversed, &result);
+               free_commit_list(reversed);
+
                if (clean < 0) {
                        rollback_lock_file(&lock);
                        return 2;

diff --git a/commit.c b/commit.c
index f674eca..1386b12 100644 (file)
--- a/commit.c
+++ b/commit.c
@@ -680,7 +680,7 @@ unsigned commit_list_count(const struct commit_list *l)
        return c;
 }
 
-struct commit_list *copy_commit_list(struct commit_list *list)
+struct commit_list *copy_commit_list(const struct commit_list *list)
 {
        struct commit_list *head = NULL;
        struct commit_list **pp = &head;

diff --git a/commit.h b/commit.h
index 442e50f..acabb05 100644 (file)
--- a/commit.h
+++ b/commit.h
@@ -181,7 +181,7 @@ struct commit_list *commit_list_insert_by_date(struct commit *item,
 void commit_list_sort_by_date(struct commit_list **list);
 
 /* Shallow copy of the input list */
-struct commit_list *copy_commit_list(struct commit_list *list);
+struct commit_list *copy_commit_list(const struct commit_list *list);
 
 /* Modify list in-place to reverse it, returning new head; list will be tail */
 struct commit_list *reverse_commit_list(struct commit_list *list);

diff --git a/log-tree.c b/log-tree.c
index 41416de..91b96d8 100644 (file)
--- a/log-tree.c
+++ b/log-tree.c
@@ -1047,6 +1047,7 @@ static int do_remerge_diff(struct rev_info *opt,
        log_tree_diff_flush(opt);
 
        /* Cleanup */
+       free_commit_list(bases);
        cleanup_additional_headers(&opt->diffopt);
        strbuf_release(&parent1_desc);
        strbuf_release(&parent2_desc);

diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c
index 4acedf3..d6f6135 100644 (file)
--- a/merge-ort-wrappers.c
+++ b/merge-ort-wrappers.c
@@ -48,7 +48,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,
 int merge_ort_recursive(struct merge_options *opt,
                        struct commit *side1,
                        struct commit *side2,
-                       struct commit_list *merge_bases,
+                       const struct commit_list *merge_bases,
                        struct commit **result)
 {
        struct tree *head = repo_get_commit_tree(opt->repo, side1);

diff --git a/merge-ort-wrappers.h b/merge-ort-wrappers.h
index 0c4c57a..90af1f6 100644 (file)
--- a/merge-ort-wrappers.h
+++ b/merge-ort-wrappers.h
@@ -19,7 +19,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,
 int merge_ort_recursive(struct merge_options *opt,
                        struct commit *h1,
                        struct commit *h2,
-                       struct commit_list *ancestors,
+                       const struct commit_list *ancestors,
                        struct commit **result);
 
 #endif

diff --git a/merge-ort.c b/merge-ort.c
index eaede6c..8ed8a4c 100644 (file)
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -5071,11 +5071,12 @@ redo:
  * Originally from merge_recursive_internal(); somewhat adapted, though.
  */
 static void merge_ort_internal(struct merge_options *opt,
-                              struct commit_list *merge_bases,
+                              const struct commit_list *_merge_bases,
                               struct commit *h1,
                               struct commit *h2,
                               struct merge_result *result)
 {
+       struct commit_list *merge_bases = copy_commit_list(_merge_bases);
        struct commit *next;
        struct commit *merged_merge_bases;
        const char *ancestor_name;
@@ -5085,7 +5086,7 @@ static void merge_ort_internal(struct merge_options *opt,
                if (repo_get_merge_bases(the_repository, h1, h2,
                                         &merge_bases) < 0) {
                        result->clean = -1;
-                       return;
+                       goto out;
                }
                /* See merge-ort.h:merge_incore_recursive() declaration NOTE */
                merge_bases = reverse_commit_list(merge_bases);
@@ -5129,7 +5130,7 @@ static void merge_ort_internal(struct merge_options *opt,
                opt->branch2 = "Temporary merge branch 2";
                merge_ort_internal(opt, NULL, prev, next, result);
                if (result->clean < 0)
-                       return;
+                       goto out;
                opt->branch1 = saved_b1;
                opt->branch2 = saved_b2;
                opt->priv->call_depth--;
@@ -5152,6 +5153,9 @@ static void merge_ort_internal(struct merge_options *opt,
                                        result);
        strbuf_release(&merge_base_abbrev);
        opt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */
+
+out:
+       free_commit_list(merge_bases);
 }
 
 void merge_incore_nonrecursive(struct merge_options *opt,
@@ -5181,7 +5185,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,
 }
 
 void merge_incore_recursive(struct merge_options *opt,
-                           struct commit_list *merge_bases,
+                           const struct commit_list *merge_bases,
                            struct commit *side1,
                            struct commit *side2,
                            struct merge_result *result)

diff --git a/merge-ort.h b/merge-ort.h
index ce56ec1..6af97c0 100644 (file)
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -59,7 +59,7 @@ struct merge_result {
  *           first", 2006-08-09)
  */
 void merge_incore_recursive(struct merge_options *opt,
-                           struct commit_list *merge_bases,
+                           const struct commit_list *merge_bases,
                            struct commit *side1,
                            struct commit *side2,
                            struct merge_result *result);

diff --git a/merge-recursive.c b/merge-recursive.c
index 832c8ef..1ac0316 100644 (file)
--- 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;

diff --git a/merge-recursive.h b/merge-recursive.h
index 839eb64..3136c7c 100644 (file)
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -104,7 +104,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);
 
 /*

diff --git a/sequencer.c b/sequencer.c
index 20807ea..131443c 100644 (file)
--- a/sequencer.c
+++ b/sequencer.c
@@ -4315,6 +4315,7 @@ leave_merge:
        strbuf_release(&ref_name);
        rollback_lock_file(&lock);
        free_commit_list(to_merge);
+       free_commit_list(bases);
        return ret;
 }
 

diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index 59b5d6b..36ca126 100755 (executable)
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -21,6 +21,7 @@ Initial setup:
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-rebase.sh
 . "$TEST_DIRECTORY"/lib-log-graph.sh

diff --git a/t/t6402-merge-rename.sh b/t/t6402-merge-rename.sh
index 2738b50..729aac9 100755 (executable)
--- a/t/t6402-merge-rename.sh
+++ b/t/t6402-merge-rename.sh
@@ -4,6 +4,7 @@ test_description='Merge-recursive merging renames'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 modify () {

diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh
index ca15e6d..555f00f 100755 (executable)
--- a/t/t6430-merge-recursive.sh
+++ b/t/t6430-merge-recursive.sh
@@ -5,6 +5,7 @@ test_description='merge-recursive backend test'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-merge.sh
 

diff --git a/t/t6436-merge-overwrite.sh b/t/t6436-merge-overwrite.sh
index 4f43764..ccc6204 100755 (executable)
--- a/t/t6436-merge-overwrite.sh
+++ b/t/t6436-merge-overwrite.sh
@@ -7,6 +7,7 @@ Do not overwrite changes.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '

diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index d6975ca..992a8f9 100755 (executable)
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -25,6 +25,7 @@ Next, test git merge --abort with the following variables:
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '