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

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