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 <kevinbackhouse@xxxxxxxxxx> ## 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 <kevinbackhouse@xxxxxxxxxx> @@ 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
