[v2,0/2] This fixes a minor memory leak (detected by LeakSanitizer) in git merge

Message

Derrick Stolee via GitGitGadget Aug. 24, 2023, 2:12 p.m. UTC

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 <[e-mail]>
      
       ## 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.
      
          Signed-off-by: Kevin Backhouse <[e-mail]>
      
     @@ 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);
     +