@matheusbsbs / matheus-bagliano / commits / 44ec7c575f

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>

Patrick Steinhardt committed Jun 11, 2024 at 11:21 UTC 44ec7c575f914d77787a17cefd094e3c46b8b12b

17 files changed +54 -29

builtin/merge-tree.c

@@ -482,6 +482,7 @@ static int real_merge(struct merge_tree_options *o,

482 die(_("refusing to merge unrelated histories"));

483 merge_bases = reverse_commit_list(merge_bases);

484 merge_incore_recursive(&opt, merge_bases, parent1, parent2, &result);

485 + free_commit_list(merge_bases);

486 }

487

488 if (result.clean < 0)

builtin/merge.c

@@ -746,6 +746,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,

746 else

747 clean = merge_recursive(&o, head, remoteheads->item,

748 reversed, &result);

749 + free_commit_list(reversed);

750 +

751 if (clean < 0) {

752 rollback_lock_file(&lock);

753 return 2;

commit.c

+1 -1

@@ -680,7 +680,7 @@ unsigned commit_list_count(const struct commit_list *l)

680 return c;

681 }

682

683 -struct commit_list *copy_commit_list(struct commit_list *list)

683 +struct commit_list *copy_commit_list(const struct commit_list *list)

684 {

685 struct commit_list *head = NULL;

686 struct commit_list **pp = &head;

commit.h

+1 -1

@@ -181,7 +181,7 @@ struct commit_list *commit_list_insert_by_date(struct commit *item,

181 void commit_list_sort_by_date(struct commit_list **list);

182

183 /* Shallow copy of the input list */

184 -struct commit_list *copy_commit_list(struct commit_list *list);

184 +struct commit_list *copy_commit_list(const struct commit_list *list);

185

186 /* Modify list in-place to reverse it, returning new head; list will be tail */

187 struct commit_list *reverse_commit_list(struct commit_list *list);

log-tree.c

@@ -1047,6 +1047,7 @@ static int do_remerge_diff(struct rev_info *opt,

1047 log_tree_diff_flush(opt);

1048

1049 /* Cleanup */

1050 + free_commit_list(bases);

1051 cleanup_additional_headers(&opt->diffopt);

1052 strbuf_release(&parent1_desc);

1053 strbuf_release(&parent2_desc);

merge-ort-wrappers.c

+1 -1

@@ -48,7 +48,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,

48 int merge_ort_recursive(struct merge_options *opt,

49 struct commit *side1,

50 struct commit *side2,

51 - struct commit_list *merge_bases,

51 + const struct commit_list *merge_bases,

52 struct commit **result)

53 {

54 struct tree *head = repo_get_commit_tree(opt->repo, side1);

merge-ort-wrappers.h

+1 -1

@@ -19,7 +19,7 @@ int merge_ort_nonrecursive(struct merge_options *opt,

19 int merge_ort_recursive(struct merge_options *opt,

20 struct commit *h1,

21 struct commit *h2,

22 - struct commit_list *ancestors,

22 + const struct commit_list *ancestors,

23 struct commit **result);

25 #endif

merge-ort.c

+8 -4

@@ -5071,11 +5071,12 @@ redo:

5071 * Originally from merge_recursive_internal(); somewhat adapted, though.

5072 */

5073 static void merge_ort_internal(struct merge_options *opt,

5074 - struct commit_list *merge_bases,

5074 + const struct commit_list *_merge_bases,

5075 struct commit *h1,

5076 struct commit *h2,

5077 struct merge_result *result)

5078 {

5079 + struct commit_list *merge_bases = copy_commit_list(_merge_bases);

5080 struct commit *next;

5081 struct commit *merged_merge_bases;

5082 const char *ancestor_name;

@@ -5085,7 +5086,7 @@ static void merge_ort_internal(struct merge_options *opt,

5086 if (repo_get_merge_bases(the_repository, h1, h2,

5087 &merge_bases) < 0) {

5088 result->clean = -1;

5088 - return;

5089 + goto out;

5090 }

5091 /* See merge-ort.h:merge_incore_recursive() declaration NOTE */

5092 merge_bases = reverse_commit_list(merge_bases);

@@ -5129,7 +5130,7 @@ static void merge_ort_internal(struct merge_options *opt,

5130 opt->branch2 = "Temporary merge branch 2";

5131 merge_ort_internal(opt, NULL, prev, next, result);

5132 if (result->clean < 0)

5132 - return;

5133 + goto out;

5134 opt->branch1 = saved_b1;

5135 opt->branch2 = saved_b2;

5136 opt->priv->call_depth--;

@@ -5152,6 +5153,9 @@ static void merge_ort_internal(struct merge_options *opt,

5153 result);

5154 strbuf_release(&merge_base_abbrev);

5155 opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */

5156 +

5157 +out:

5158 + free_commit_list(merge_bases);

5159 }

5160

5161 void merge_incore_nonrecursive(struct merge_options *opt,

@@ -5181,7 +5185,7 @@ void merge_incore_nonrecursive(struct merge_options *opt,

5185 }

5186

5187 void merge_incore_recursive(struct merge_options *opt,

5184 - struct commit_list *merge_bases,

5188 + const struct commit_list *merge_bases,

5189 struct commit *side1,

5190 struct commit *side2,

5191 struct merge_result *result)

merge-ort.h

+1 -1

@@ -59,7 +59,7 @@ struct merge_result {

59 * first", 2006-08-09)

60 */

61 void merge_incore_recursive(struct merge_options *opt,

62 - struct commit_list *merge_bases,

62 + const struct commit_list *merge_bases,

63 struct commit *side1,

64 struct commit *side2,

65 struct merge_result *result);

merge-recursive.c

+30 -19

@@ -3633,15 +3633,16 @@ static int merge_trees_internal(struct merge_options *opt,

3633 static int merge_recursive_internal(struct merge_options *opt,

3634 struct commit *h1,

3635 struct commit *h2,

3636 - struct commit_list *merge_bases,

3636 + const struct commit_list *_merge_bases,

3637 struct commit **result)

3638 {

3639 + struct commit_list *merge_bases = copy_commit_list(_merge_bases);

3640 struct commit_list *iter;

3641 struct commit *merged_merge_bases;

3642 struct tree *result_tree;

3642 - int clean;

3643 const char *ancestor_name;

3644 struct strbuf merge_base_abbrev = STRBUF_INIT;

3645 + int ret;

3646

3647 if (show(opt, 4)) {

3648 output(opt, 4, _("Merging:"));

@@ -3651,8 +3652,10 @@ static int merge_recursive_internal(struct merge_options *opt,

3652

3653 if (!merge_bases) {

3654 if (repo_get_merge_bases(the_repository, h1, h2,

3654 - &merge_bases) < 0)

3655 - return -1;

3655 + &merge_bases) < 0) {

3656 + ret = -1;

3657 + goto out;

3658 + }

3659 merge_bases = reverse_commit_list(merge_bases);

3660 }

3661

@@ -3702,14 +3705,18 @@ static int merge_recursive_internal(struct merge_options *opt,

3705 opt->branch1 = "Temporary merge branch 1";

3706 opt->branch2 = "Temporary merge branch 2";

3707 if (merge_recursive_internal(opt, merged_merge_bases, iter->item,

3705 - NULL, &merged_merge_bases) < 0)

3706 - return -1;

3708 + NULL, &merged_merge_bases) < 0) {

3709 + ret = -1;

3710 + goto out;

3711 + }

3712 opt->branch1 = saved_b1;

3713 opt->branch2 = saved_b2;

3714 opt->priv->call_depth--;

3715

3711 - if (!merged_merge_bases)

3712 - return err(opt, _("merge returned no commit"));

3716 + if (!merged_merge_bases) {

3717 + ret = err(opt, _("merge returned no commit"));

3718 + goto out;

3719 + }

3720 }

3721

3722 /*

@@ -3726,17 +3733,16 @@ static int merge_recursive_internal(struct merge_options *opt,

3733 repo_read_index(opt->repo);

3734

3735 opt->ancestor = ancestor_name;

3729 - clean = merge_trees_internal(opt,

3730 - repo_get_commit_tree(opt->repo, h1),

3731 - repo_get_commit_tree(opt->repo, h2),

3732 - repo_get_commit_tree(opt->repo,

3733 - merged_merge_bases),

3734 - &result_tree);

3735 - strbuf_release(&merge_base_abbrev);

3736 + ret = merge_trees_internal(opt,

3737 + repo_get_commit_tree(opt->repo, h1),

3738 + repo_get_commit_tree(opt->repo, h2),

3739 + repo_get_commit_tree(opt->repo,

3740 + merged_merge_bases),

3741 + &result_tree);

3742 opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */

3737 - if (clean < 0) {

3743 + if (ret < 0) {

3744 flush_output(opt);

3739 - return clean;

3745 + goto out;

3746 }

3747

3748 if (opt->priv->call_depth) {

@@ -3745,7 +3751,11 @@ static int merge_recursive_internal(struct merge_options *opt,

3751 commit_list_insert(h1, &(*result)->parents);

3752 commit_list_insert(h2, &(*result)->parents->next);

3753 }

3748 - return clean;

3754 +

3755 +out:

3756 + strbuf_release(&merge_base_abbrev);

3757 + free_commit_list(merge_bases);

3758 + return ret;

3759 }

3760

3761 static int merge_start(struct merge_options *opt, struct tree *head)

@@ -3827,7 +3837,7 @@ int merge_trees(struct merge_options *opt,

3837 int merge_recursive(struct merge_options *opt,

3838 struct commit *h1,

3839 struct commit *h2,

3830 - struct commit_list *merge_bases,

3840 + const struct commit_list *merge_bases,

3841 struct commit **result)

3842 {

3843 int clean;

@@ -3895,6 +3905,7 @@ int merge_recursive_generic(struct merge_options *opt,

3905 repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR);

3906 clean = merge_recursive(opt, head_commit, next_commit, ca,

3907 result);

3908 + free_commit_list(ca);

3909 if (clean < 0) {

3910 rollback_lock_file(&lock);

3911 return clean;

merge-recursive.h

+1 -1

@@ -104,7 +104,7 @@ int merge_trees(struct merge_options *opt,

104 int merge_recursive(struct merge_options *opt,

105 struct commit *h1,

106 struct commit *h2,

107 - struct commit_list *merge_bases,

107 + const struct commit_list *merge_bases,

108 struct commit **result);

109

110 /*

sequencer.c

@@ -4315,6 +4315,7 @@ leave_merge:

4315 strbuf_release(&ref_name);

4316 rollback_lock_file(&lock);

4317 free_commit_list(to_merge);

4318 + free_commit_list(bases);

4319 return ret;

4320 }

4321

t/t3430-rebase-merges.sh

@@ -21,6 +21,7 @@ Initial setup:

21 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

22 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

24 +TEST_PASSES_SANITIZE_LEAK=true

25 . ./test-lib.sh

26 . "$TEST_DIRECTORY"/lib-rebase.sh

27 . "$TEST_DIRECTORY"/lib-log-graph.sh

t/t6402-merge-rename.sh

@@ -4,6 +4,7 @@ test_description='Merge-recursive merging renames'

4 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

5 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

7 +TEST_PASSES_SANITIZE_LEAK=true

8 . ./test-lib.sh

10 modify () {

t/t6430-merge-recursive.sh

@@ -5,6 +5,7 @@ test_description='merge-recursive backend test'

5 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

6 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

8 +TEST_PASSES_SANITIZE_LEAK=true

9 . ./test-lib.sh

10 . "$TEST_DIRECTORY"/lib-merge.sh

t/t6436-merge-overwrite.sh

@@ -7,6 +7,7 @@ Do not overwrite changes.'

7 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

8 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

10 +TEST_PASSES_SANITIZE_LEAK=true

11 . ./test-lib.sh

13 test_expect_success 'setup' '

t/t7611-merge-abort.sh

@@ -25,6 +25,7 @@ Next, test git merge --abort with the following variables:

25 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main

26 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

28 +TEST_PASSES_SANITIZE_LEAK=true

29 . ./test-lib.sh

31 test_expect_success 'setup' '