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 1082d919fd1ca99e91168fda923d839f73907095..dab2fdc2a6ba1eb2debfc6f797413c8474b9f4b4 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 682c6ed868e8fb95c69d00ddc8c7883450e212da..1120a6e2f855790e5ccdc46d10da1e4ad7490473 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 f674eca32051581db5daf89914da444644a93e92..1386b12df11d1eee6c56444848aab153d4d9fefc 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 442e50ff2450a111bfbb9819943a0aca76338d67..acabb05785713032a6e00e6a34d741ea32a2ca97 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 41416de4e3fbf130982455070f9118c0bcd1e469..91b96d83a011510c6c7ad02a2bdf0c39a66fcfd2 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 4acedf3c3386d7c9c2e57eebabddb1fe9ec27331..d6f61359965b490a3453956e1ee4bb9c8f1a87e5 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 0c4c57adbb8c3b2a579550e970c06ac41fa9c550..90af1f69c550383e548a2bd491af1aaf09619aba 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 eaede6cead9442995ef2f0ea2b449bceeaf0bcb7..8ed8a4c9dcbf6430a584c494ef9761a86ca1555e 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 ce56ec1a7802c5eac51d404e03220b83e52ebe1c..6af97c0828b811cf64a925c7f17f0baa337e7c7a 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 832c8ef3f34daaaae0523a131d5c1dfd4cb38b0f..1ac0316cce4af6a6e41ce7f4365e701417d0e632 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 839eb6436e48944b7df75eb82660c025ee0384e0..3136c7cc2dfb47435e0a34dedfb753ca008215a4 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 20807ea7e58785273e7c2ad8455979abbfc8c9fc..131443c24275ead45ae8330d707c19e14240ae07 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 59b5d6b6f276c367862df92f847db37906715004..36ca126bcdf9a5b68d883bb97bd1d5eadfab613b 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 2738b50c2a9e01aab9e3248e666ddb91be9c3bcf..729aac9842d1f7679da2c187bbb857f85ec3a131 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 ca15e6dd6da94bc1bac8286e9e36a196cde7a797..555f00f78a19053c49b9c625471461621e4f103a 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 4f4376421e7da28e22f58258061bf58f3a8bc0c8..ccc620477d494b20588ba87bf83e9fb73a90539d 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 d6975ca48df3d4710940a4b5a040c0598b0d0628..992a8f98749baecbe109f8b839d270aecfb77fe8 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' '