{"thread":{"id":"60126","subject":"[PATCH] This fixes a minor memory leak (detected by LeakSanitizer) in git merge.","startedAt":"2023-08-18T20:05:09Z","lastAt":"2023-09-12T15:06:22Z","messageCount":8,"participants":["Kevin Backhouse via GitGitGadget","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"480788","messageId":"pull.1577.git.1692389061490.gitgitgadget@gmail.com","threadId":"60126","inReplyTo":null,"subject":"[PATCH] This fixes a minor memory leak (detected by LeakSanitizer) in git merge.","fromName":"Kevin Backhouse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-18T20:04:21Z","receivedAt":"2023-08-18T20:05:09Z","isPatch":true,"sender":{"key":"name:Kevin Backhouse","avatar":null},"body":"From: Kevin Backhouse <kevinbackhouse@github.com>\n\nTo reproduce (with an ASAN build):\n\n```\nmkdir test\ncd test\ngit init\necho x > x.txt\ngit add .\ngit commit -m \"WIP\"\ngit checkout -b dev\necho y > x.txt\ngit add .\ngit commit -m \"WIP\"\ngit checkout main\necho z > x.txt\ngit add .\ngit commit -m \"WIP\"\necho a > x.txt\ngit add .\ngit merge dev\n```\n\nThe fix is to call free_commit_list(merge_bases) when an error occurs.\n\nSigned-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n---\n    This fixes a minor memory leak (detected by LeakSanitizer) in git merge\n    \n    To reproduce (with an ASAN build):\n    \n    mkdir test\n    cd test\n    git init\n    echo x > x.txt\n    git add .\n    git commit -m \"WIP\"\n    git checkout -b dev\n    echo y > x.txt\n    git add .\n    git commit -m \"WIP\"\n    git checkout main\n    echo z > x.txt\n    git add .\n    git commit -m \"WIP\"\n    echo a > x.txt\n    git add .\n    git merge dev\n    \n    \n    The fix is to call free_commit_list(merge_bases) when an error occurs.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1577%2Fkevinbackhouse%2Ffree-merge-bases-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1577/kevinbackhouse/free-merge-bases-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1577\n\n merge-ort-wrappers.c | 4 +++-\n merge-ort.c          | 4 +++-\n 2 files changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\nindex 4acedf3c338..aeb56c9970c 100644\n--- a/merge-ort-wrappers.c\n+++ b/merge-ort-wrappers.c\n@@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,\n \tstruct tree *head = repo_get_commit_tree(opt->repo, side1);\n \tstruct merge_result tmp;\n \n-\tif (unclean(opt, head))\n+\tif (unclean(opt, head)) {\n+\t\tfree_commit_list(merge_bases);\n \t\treturn -1;\n+\t}\n \n \tmemset(&tmp, 0, sizeof(tmp));\n \tmerge_incore_recursive(opt, merge_bases, side1, side2, &tmp);\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8631c997002..a0eb91fb011 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\topt->branch1 = \"Temporary merge branch 1\";\n \t\topt->branch2 = \"Temporary merge branch 2\";\n \t\tmerge_ort_internal(opt, NULL, prev, next, result);\n-\t\tif (result->clean < 0)\n+\t\tif (result->clean < 0) {\n+\t\t\tfree_commit_list(merge_bases);\n \t\t\treturn;\n+\t\t}\n \t\topt->branch1 = saved_b1;\n \t\topt->branch2 = saved_b2;\n \t\topt->priv->call_depth--;\n\nbase-commit: f9972720e9a405e4f6924a7cde0ed5880687f4d0\n-- \ngitgitgadget\n"},{"id":"480791","messageId":"xmqqo7j4t4dm.fsf@gitster.g","threadId":"60126","inReplyTo":"pull.1577.git.1692389061490.gitgitgadget@gmail.com","subject":"Re: [PATCH] This fixes a minor memory leak (detected by LeakSanitizer) in git merge.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-18T21:41:41Z","receivedAt":"2023-08-18T21:42:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kevin Backhouse via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Kevin Backhouse <kevinbackhouse@github.com>\n>\n> To reproduce (with an ASAN build):\n>\n> ```\n> mkdir test\n> cd test\n> git init\n> echo x > x.txt\n> git add .\n> git commit -m \"WIP\"\n> git checkout -b dev\n> echo y > x.txt\n> git add .\n> git commit -m \"WIP\"\n> git checkout main\n> echo z > x.txt\n> git add .\n> git commit -m \"WIP\"\n> echo a > x.txt\n> git add .\n> git merge dev\n> ```\n\nWe'd rather not to see the above in the proposed log message; can't\nwe add (a variation of) it to our test suite?\n\n> The fix is to call free_commit_list(merge_bases) when an error occurs.\n\nWe usually have the description of what the problem is and give an\nanalysis on why/how it happens, before presenting a solution.  Write\nit more like:\n\n    The caller of merge_ort_recursive() expects the commit list\n    passed in as the merge_bases parameter to be fully consumed by\n    the function and does not free it when the function returns.  In\n    normal cases, the commit list does get consumed, but when the\n    function returns early upon encountering an error, it forgets to\n    clean it up.\n\n    Fix this by freeing the list in the code paths for error returns.\n\n>  merge-ort-wrappers.c | 4 +++-\n>  merge-ort.c          | 4 +++-\n\nThese two places and their fixes seem OK, but I have to wonder if\nthese are complete fixes.\n\n> diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\n> index 4acedf3c338..aeb56c9970c 100644\n> --- a/merge-ort-wrappers.c\n> +++ b/merge-ort-wrappers.c\n> @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,\n>  \tstruct tree *head = repo_get_commit_tree(opt->repo, side1);\n>  \tstruct merge_result tmp;\n>  \n> -\tif (unclean(opt, head))\n> +\tif (unclean(opt, head)) {\n> +\t\tfree_commit_list(merge_bases);\n>  \t\treturn -1;\n> +\t}\n>  \n>  \tmemset(&tmp, 0, sizeof(tmp));\n>  \tmerge_incore_recursive(opt, merge_bases, side1, side2, &tmp);\n\nThe function before this hunk appears to have very similar code\nstructure.  Does it need the same fix, or if not why not?\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 8631c997002..a0eb91fb011 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,\n>  \t\topt->branch1 = \"Temporary merge branch 1\";\n>  \t\topt->branch2 = \"Temporary merge branch 2\";\n>  \t\tmerge_ort_internal(opt, NULL, prev, next, result);\n> -\t\tif (result->clean < 0)\n> +\t\tif (result->clean < 0) {\n> +\t\t\tfree_commit_list(merge_bases);\n>  \t\t\treturn;\n> +\t\t}\n\nBefore this function, there is a comment that this came from another\nfunction and it seems to still have a very similar code structure.\nDoes the other function need the same fix, or if not why not?\n\nThanks.\n"},{"id":"480990","messageId":"f940104a7814bd0b1e45f1230b2cb4b0be305699.1692886365.git.gitgitgadget@gmail.com","threadId":"60126","inReplyTo":"pull.1577.v2.git.1692886365.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] Regression test for https://github.com/gitgitgadget/git/pull/1577","fromName":"Kevin Backhouse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-24T14:12:44Z","receivedAt":"2023-08-24T14:13:34Z","isPatch":true,"sender":{"key":"name:Kevin Backhouse","avatar":null},"body":"From: Kevin Backhouse <kevinbackhouse@github.com>\n\nSigned-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n---\n t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 40 insertions(+)\n create mode 100755 t/t9904-merge-leak.sh\n\ndiff --git a/t/t9904-merge-leak.sh b/t/t9904-merge-leak.sh\nnew file mode 100755\nindex 00000000000..09a4474fd73\n--- /dev/null\n+++ b/t/t9904-merge-leak.sh\n@@ -0,0 +1,40 @@\n+#!/bin/sh\n+#\n+\n+test_description='regression test for memory leak in git merge'\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./lib-bash.sh\n+\n+# test-lib.sh disables LeakSanitizer by default, but we want it enabled\n+# for this test\n+ASAN_OPTIONS=\n+export ASAN_OPTIONS\n+\n+. \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n+\n+test_expect_success 'Merge fails due to local changes' '\n+\tgit init &&\n+\techo x > x.txt &&\n+\tgit add . &&\n+\tgit commit -m \"WIP\" &&\n+\tgit checkout -b dev &&\n+\techo y > x.txt &&\n+\tgit add . &&\n+\tgit commit -m \"WIP\" &&\n+\tgit checkout main &&\n+\techo z > x.txt &&\n+\tgit add . &&\n+\tgit commit -m \"WIP\" &&\n+\techo a > x.txt &&\n+\tgit add . &&\n+\techo \"error: ''Your local changes to the following files would be overwritten by merge:''\" >expected &&\n+\techo \"  x.txt\" >>expected &&\n+\techo \"Merge with strategy ort failed.\" >>expected &&\n+\ttest_must_fail git merge -s ort dev 2>actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_done\n-- \ngitgitgadget\n\n"},{"id":"480991","messageId":"353e1960b4466912c508733e5a7eb80884cd9bdd.1692886365.git.gitgitgadget@gmail.com","threadId":"60126","inReplyTo":"pull.1577.v2.git.1692886365.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] Fix minor memory leak found by LeakSanitizer.","fromName":"Kevin Backhouse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-24T14:12:45Z","receivedAt":"2023-08-24T14:13:35Z","isPatch":true,"sender":{"key":"name:Kevin Backhouse","avatar":null},"body":"From: Kevin Backhouse <kevinbackhouse@github.com>\n\nThe callers of merge_recursive() and merge_ort_recursive() expects the\ncommit list passed in as the merge_bases parameter to be fully\nconsumed by the function and does not free it when the function\nreturns.  In normal cases, the commit list does get consumed, but when\nthe function returns early upon encountering an error, it forgets to\nclean it up.\n\nFix this by freeing the list in the code paths for error returns.\n\nSigned-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n---\n merge-ort-wrappers.c |  4 +++-\n merge-ort.c          |  4 +++-\n merge-recursive.c    | 32 ++++++++++++++++++++++----------\n 3 files changed, 28 insertions(+), 12 deletions(-)\n\ndiff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\nindex 4acedf3c338..aeb56c9970c 100644\n--- a/merge-ort-wrappers.c\n+++ b/merge-ort-wrappers.c\n@@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,\n \tstruct tree *head = repo_get_commit_tree(opt->repo, side1);\n \tstruct merge_result tmp;\n \n-\tif (unclean(opt, head))\n+\tif (unclean(opt, head)) {\n+\t\tfree_commit_list(merge_bases);\n \t\treturn -1;\n+\t}\n \n \tmemset(&tmp, 0, sizeof(tmp));\n \tmerge_incore_recursive(opt, merge_bases, side1, side2, &tmp);\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 8631c997002..a0eb91fb011 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,\n \t\topt->branch1 = \"Temporary merge branch 1\";\n \t\topt->branch2 = \"Temporary merge branch 2\";\n \t\tmerge_ort_internal(opt, NULL, prev, next, result);\n-\t\tif (result->clean < 0)\n+\t\tif (result->clean < 0) {\n+\t\t\tfree_commit_list(merge_bases);\n \t\t\treturn;\n+\t\t}\n \t\topt->branch1 = saved_b1;\n \t\topt->branch2 = saved_b2;\n \t\topt->priv->call_depth--;\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 6a4081bb0f5..49e54d3722f 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -3652,14 +3652,18 @@ static int merge_recursive_internal(struct merge_options *opt,\n \t\topt->branch1 = \"Temporary merge branch 1\";\n \t\topt->branch2 = \"Temporary merge branch 2\";\n \t\tif (merge_recursive_internal(opt, merged_merge_bases, iter->item,\n-\t\t\t\t\t     NULL, &merged_merge_bases) < 0)\n-\t\t\treturn -1;\n+\t\t\t\t\t     NULL, &merged_merge_bases) < 0) {\n+\t\t\tclean = -1;\n+\t\t\tgoto out;\n+\t\t}\n \t\topt->branch1 = saved_b1;\n \t\topt->branch2 = saved_b2;\n \t\topt->priv->call_depth--;\n \n-\t\tif (!merged_merge_bases)\n-\t\t\treturn err(opt, _(\"merge returned no commit\"));\n+\t\tif (!merged_merge_bases) {\n+\t\t\tclean = err(opt, _(\"merge returned no commit\"));\n+\t\t\tgoto out;\n+\t\t}\n \t}\n \n \t/*\n@@ -3682,8 +3686,11 @@ static int merge_recursive_internal(struct merge_options *opt,\n \t\t\t\t     repo_get_commit_tree(opt->repo,\n \t\t\t\t\t\t\t  merged_merge_bases),\n \t\t\t\t     &result_tree);\n+\n+out:\n \tstrbuf_release(&merge_base_abbrev);\n \topt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */\n+\tfree_commit_list(merge_bases);\n \tif (clean < 0) {\n \t\tflush_output(opt);\n \t\treturn clean;\n@@ -3729,6 +3736,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \tassert(!opt->record_conflict_msgs_as_headers);\n \tassert(!opt->msg_header_prefix);\n \n+\tCALLOC_ARRAY(opt->priv, 1);\n+\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n+\n \t/* Sanity check on repo state; index must match head */\n \tif (repo_index_has_changes(opt->repo, head, &sb)) {\n \t\terr(opt, _(\"Your local changes to the following files would be overwritten by merge:\\n  %s\"),\n@@ -3737,16 +3747,13 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n \t\treturn -1;\n \t}\n \n-\tCALLOC_ARRAY(opt->priv, 1);\n-\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n \treturn 0;\n }\n \n static void merge_finalize(struct merge_options *opt)\n {\n \tflush_output(opt);\n-\tif (!opt->priv->call_depth && opt->buffer_output < 2)\n-\t\tstrbuf_release(&opt->obuf);\n+\tstrbuf_release(&opt->obuf);\n \tif (show(opt, 2))\n \t\tdiff_warn_rename_limit(\"merge.renamelimit\",\n \t\t\t\t       opt->priv->needed_rename_limit, 0);\n@@ -3763,8 +3770,10 @@ int merge_trees(struct merge_options *opt,\n \n \tassert(opt->ancestor != NULL);\n \n-\tif (merge_start(opt, head))\n+\tif (merge_start(opt, head)) {\n+\t\tmerge_finalize(opt);\n \t\treturn -1;\n+\t}\n \tclean = merge_trees_internal(opt, head, merge, merge_base, &ignored);\n \tmerge_finalize(opt);\n \n@@ -3785,8 +3794,11 @@ int merge_recursive(struct merge_options *opt,\n \tprepare_repo_settings(opt->repo);\n \topt->repo->settings.command_requires_full_index = 1;\n \n-\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))\n+\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) {\n+\t\tfree_commit_list(merge_bases);\n+\t\tmerge_finalize(opt);\n \t\treturn -1;\n+\t}\n \tclean = merge_recursive_internal(opt, h1, h2, merge_bases, result);\n \tmerge_finalize(opt);\n \n-- \ngitgitgadget\n"},{"id":"480992","messageId":"pull.1577.v2.git.1692886365.gitgitgadget@gmail.com","threadId":"60126","inReplyTo":"pull.1577.git.1692389061490.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] This fixes a minor memory leak (detected by LeakSanitizer) in git merge","fromName":"Kevin Backhouse via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-08-24T14:12:43Z","receivedAt":"2023-08-24T14:13:35Z","isPatch":true,"sender":{"key":"name:Kevin Backhouse","avatar":null},"body":"Hi Junio,\n\nThank you for your comments. As you suggested, I have added similar fixes in\nmerge-recursive.c and updated the commit message. I have also added a test.\n\nThanks,\n\nKev\n\nKevin Backhouse (2):\n  Regression test for https://github.com/gitgitgadget/git/pull/1577\n  Fix minor memory leak found by LeakSanitizer.\n\n merge-ort-wrappers.c  |  4 +++-\n merge-ort.c           |  4 +++-\n merge-recursive.c     | 32 ++++++++++++++++++++++----------\n t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 68 insertions(+), 12 deletions(-)\n create mode 100755 t/t9904-merge-leak.sh\n\n\nbase-commit: f9972720e9a405e4f6924a7cde0ed5880687f4d0\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1577%2Fkevinbackhouse%2Ffree-merge-bases-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1577/kevinbackhouse/free-merge-bases-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1577\n\nRange-diff vs v1:\n\n -:  ----------- > 1:  f940104a781 Regression test for https://github.com/gitgitgadget/git/pull/1577\n 1:  64b00e4448d ! 2:  353e1960b44 This fixes a minor memory leak (detected by LeakSanitizer) in git merge.\n     @@ Metadata\n      Author: Kevin Backhouse <kevinbackhouse@github.com>\n      \n       ## Commit message ##\n     -    This fixes a minor memory leak (detected by LeakSanitizer) in git merge.\n     +    Fix minor memory leak found by LeakSanitizer.\n      \n     -    To reproduce (with an ASAN build):\n     +    The callers of merge_recursive() and merge_ort_recursive() expects the\n     +    commit list passed in as the merge_bases parameter to be fully\n     +    consumed by the function and does not free it when the function\n     +    returns.  In normal cases, the commit list does get consumed, but when\n     +    the function returns early upon encountering an error, it forgets to\n     +    clean it up.\n      \n     -    ```\n     -    mkdir test\n     -    cd test\n     -    git init\n     -    echo x > x.txt\n     -    git add .\n     -    git commit -m \"WIP\"\n     -    git checkout -b dev\n     -    echo y > x.txt\n     -    git add .\n     -    git commit -m \"WIP\"\n     -    git checkout main\n     -    echo z > x.txt\n     -    git add .\n     -    git commit -m \"WIP\"\n     -    echo a > x.txt\n     -    git add .\n     -    git merge dev\n     -    ```\n     -\n     -    The fix is to call free_commit_list(merge_bases) when an error occurs.\n     +    Fix this by freeing the list in the code paths for error returns.\n      \n          Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n      \n     @@ merge-ort.c: static void merge_ort_internal(struct merge_options *opt,\n       \t\topt->branch1 = saved_b1;\n       \t\topt->branch2 = saved_b2;\n       \t\topt->priv->call_depth--;\n     +\n     + ## merge-recursive.c ##\n     +@@ merge-recursive.c: static int merge_recursive_internal(struct merge_options *opt,\n     + \t\topt->branch1 = \"Temporary merge branch 1\";\n     + \t\topt->branch2 = \"Temporary merge branch 2\";\n     + \t\tif (merge_recursive_internal(opt, merged_merge_bases, iter->item,\n     +-\t\t\t\t\t     NULL, &merged_merge_bases) < 0)\n     +-\t\t\treturn -1;\n     ++\t\t\t\t\t     NULL, &merged_merge_bases) < 0) {\n     ++\t\t\tclean = -1;\n     ++\t\t\tgoto out;\n     ++\t\t}\n     + \t\topt->branch1 = saved_b1;\n     + \t\topt->branch2 = saved_b2;\n     + \t\topt->priv->call_depth--;\n     + \n     +-\t\tif (!merged_merge_bases)\n     +-\t\t\treturn err(opt, _(\"merge returned no commit\"));\n     ++\t\tif (!merged_merge_bases) {\n     ++\t\t\tclean = err(opt, _(\"merge returned no commit\"));\n     ++\t\t\tgoto out;\n     ++\t\t}\n     + \t}\n     + \n     + \t/*\n     +@@ merge-recursive.c: static int merge_recursive_internal(struct merge_options *opt,\n     + \t\t\t\t     repo_get_commit_tree(opt->repo,\n     + \t\t\t\t\t\t\t  merged_merge_bases),\n     + \t\t\t\t     &result_tree);\n     ++\n     ++out:\n     + \tstrbuf_release(&merge_base_abbrev);\n     + \topt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */\n     ++\tfree_commit_list(merge_bases);\n     + \tif (clean < 0) {\n     + \t\tflush_output(opt);\n     + \t\treturn clean;\n     +@@ merge-recursive.c: static int merge_start(struct merge_options *opt, struct tree *head)\n     + \tassert(!opt->record_conflict_msgs_as_headers);\n     + \tassert(!opt->msg_header_prefix);\n     + \n     ++\tCALLOC_ARRAY(opt->priv, 1);\n     ++\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n     ++\n     + \t/* Sanity check on repo state; index must match head */\n     + \tif (repo_index_has_changes(opt->repo, head, &sb)) {\n     + \t\terr(opt, _(\"Your local changes to the following files would be overwritten by merge:\\n  %s\"),\n     +@@ merge-recursive.c: static int merge_start(struct merge_options *opt, struct tree *head)\n     + \t\treturn -1;\n     + \t}\n     + \n     +-\tCALLOC_ARRAY(opt->priv, 1);\n     +-\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n     + \treturn 0;\n     + }\n     + \n     + static void merge_finalize(struct merge_options *opt)\n     + {\n     + \tflush_output(opt);\n     +-\tif (!opt->priv->call_depth && opt->buffer_output < 2)\n     +-\t\tstrbuf_release(&opt->obuf);\n     ++\tstrbuf_release(&opt->obuf);\n     + \tif (show(opt, 2))\n     + \t\tdiff_warn_rename_limit(\"merge.renamelimit\",\n     + \t\t\t\t       opt->priv->needed_rename_limit, 0);\n     +@@ merge-recursive.c: int merge_trees(struct merge_options *opt,\n     + \n     + \tassert(opt->ancestor != NULL);\n     + \n     +-\tif (merge_start(opt, head))\n     ++\tif (merge_start(opt, head)) {\n     ++\t\tmerge_finalize(opt);\n     + \t\treturn -1;\n     ++\t}\n     + \tclean = merge_trees_internal(opt, head, merge, merge_base, &ignored);\n     + \tmerge_finalize(opt);\n     + \n     +@@ merge-recursive.c: int merge_recursive(struct merge_options *opt,\n     + \tprepare_repo_settings(opt->repo);\n     + \topt->repo->settings.command_requires_full_index = 1;\n     + \n     +-\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))\n     ++\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) {\n     ++\t\tfree_commit_list(merge_bases);\n     ++\t\tmerge_finalize(opt);\n     + \t\treturn -1;\n     ++\t}\n     + \tclean = merge_recursive_internal(opt, h1, h2, merge_bases, result);\n     + \tmerge_finalize(opt);\n     + \n\n-- \ngitgitgadget\n"},{"id":"480996","messageId":"xmqqh6oocw5h.fsf@gitster.g","threadId":"60126","inReplyTo":"f940104a7814bd0b1e45f1230b2cb4b0be305699.1692886365.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] Regression test for https://github.com/gitgitgadget/git/pull/1577","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-24T15:11:54Z","receivedAt":"2023-08-24T15:12:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kevin Backhouse via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Subject: Re: [PATCH v2 1/2] Regression test for https://github.com/gitgitgadget/git/pull/1577\n\nWe try to come up with titles that are helpful to readers when seen\nin \"git shortlog --since=6.months --no-merges\", and the above does\nnot exactly it.\n\n> From: Kevin Backhouse <kevinbackhouse@github.com>\n>\n> Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n> ---\n>  t/t9904-merge-leak.sh | 40 ++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 40 insertions(+)\n>  create mode 100755 t/t9904-merge-leak.sh\n>\n> diff --git a/t/t9904-merge-leak.sh b/t/t9904-merge-leak.sh\n> new file mode 100755\n> index 00000000000..09a4474fd73\n> --- /dev/null\n> +++ b/t/t9904-merge-leak.sh\n> @@ -0,0 +1,40 @@\n> +#!/bin/sh\n> +#\n> +\n> +test_description='regression test for memory leak in git merge'\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./lib-bash.sh\n> +\n> +# test-lib.sh disables LeakSanitizer by default, but we want it enabled\n> +# for this test\n> +ASAN_OPTIONS=\n> +export ASAN_OPTIONS\n\nYou do not want to do this.\n\nWe have CI jobs that run everybody under asan, ubsan etc., so it is\nsufficient and much more preferrable to just add a reproduction\nrecipe to an _existing_ test that is about \"git merge\" (or if we\nhave \"ort\" specific one, \"git merge -s ort\").  Of course they would\nnot fail in jobs that do not enable asan, and that is expected and\nperfectly OK.\n\nAlso, please check Documentation/CodingGuidelines for shell style\nissues.\n\n> +. \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n\nIs this about testing prompts, or does the bug/leak appear only when\nthe prompt support is in use?  Could you explain why this is needed?\n\n> +test_expect_success 'Merge fails due to local changes' '\n> +\tgit init &&\n> +\techo x > x.txt &&\n> +\tgit add . &&\n> +\tgit commit -m \"WIP\" &&\n> +\tgit checkout -b dev &&\n> +\techo y > x.txt &&\n> +\tgit add . &&\n> +\tgit commit -m \"WIP\" &&\n> +\tgit checkout main &&\n> +\techo z > x.txt &&\n> +\tgit add . &&\n> +\tgit commit -m \"WIP\" &&\n> +\techo a > x.txt &&\n> +\tgit add . &&\n> +\techo \"error: ''Your local changes to the following files would be overwritten by merge:''\" >expected &&\n> +\techo \"  x.txt\" >>expected &&\n> +\techo \"Merge with strategy ort failed.\" >>expected &&\n> +\ttest_must_fail git merge -s ort dev 2>actual &&\n> +\ttest_cmp expected actual\n> +'\n\nIf this 1/2 adds a new test that is expected to fail without leak\nfix, which has to wait until 2/2, it breaks the bisection.  In this\ncase, since it will be a simple addition to an existing test script,\nhaving both tests and code changes in a single patch is the most\nappropriate.\n\nThank you for working on this.\n\n\n> +\n> +test_done\n"},{"id":"480998","messageId":"xmqqv8d4bfhw.fsf@gitster.g","threadId":"60126","inReplyTo":"353e1960b4466912c508733e5a7eb80884cd9bdd.1692886365.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] Fix minor memory leak found by LeakSanitizer.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-24T15:56:59Z","receivedAt":"2023-08-24T15:57:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kevin Backhouse via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Subject: Re: [PATCH v2 2/2] Fix minor memory leak found by LeakSanitizer.\n\nContinuing the review for the previous step, perhaps \n\n\tSubject: [PATCH] merge: free list of merge bases upon failure\n\nor something?\n\n> From: Kevin Backhouse <kevinbackhouse@github.com>\n>\n> The callers of merge_recursive() and merge_ort_recursive() expects the\n\n\"expects\" -> \"expect\"\n\n> commit list passed in as the merge_bases parameter to be fully\n> consumed by the function and does not free it when the function\n\n\"does not\" -> \"do not\".\n\n> returns.  In normal cases, the commit list does get consumed, but when\n> the function returns early upon encountering an error, it forgets to\n> clean it up.\n>\n> Fix this by freeing the list in the code paths for error returns.\n>\n> Signed-off-by: Kevin Backhouse <kevinbackhouse@github.com>\n> ---\n\nWell written to be understandable.  Nicely done.\n\n>  merge-ort-wrappers.c |  4 +++-\n>  merge-ort.c          |  4 +++-\n>  merge-recursive.c    | 32 ++++++++++++++++++++++----------\n>  3 files changed, 28 insertions(+), 12 deletions(-)\n>\n> diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\n> index 4acedf3c338..aeb56c9970c 100644\n> --- a/merge-ort-wrappers.c\n> +++ b/merge-ort-wrappers.c\n> @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,\n>  \tstruct tree *head = repo_get_commit_tree(opt->repo, side1);\n>  \tstruct merge_result tmp;\n>  \n> -\tif (unclean(opt, head))\n> +\tif (unclean(opt, head)) {\n> +\t\tfree_commit_list(merge_bases);\n>  \t\treturn -1;\n> +\t}\n\nOK.\n\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 8631c997002..a0eb91fb011 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,\n>  \t\topt->branch1 = \"Temporary merge branch 1\";\n>  \t\topt->branch2 = \"Temporary merge branch 2\";\n>  \t\tmerge_ort_internal(opt, NULL, prev, next, result);\n> -\t\tif (result->clean < 0)\n> +\t\tif (result->clean < 0) {\n> +\t\t\tfree_commit_list(merge_bases);\n>  \t\t\treturn;\n> +\t\t}\n\nOK.\n\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index 6a4081bb0f5..49e54d3722f 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -3652,14 +3652,18 @@ static int merge_recursive_internal(struct merge_options *opt,\n>  \t\topt->branch1 = \"Temporary merge branch 1\";\n>  \t\topt->branch2 = \"Temporary merge branch 2\";\n>  \t\tif (merge_recursive_internal(opt, merged_merge_bases, iter->item,\n> -\t\t\t\t\t     NULL, &merged_merge_bases) < 0)\n> -\t\t\treturn -1;\n> +\t\t\t\t\t     NULL, &merged_merge_bases) < 0) {\n> +\t\t\tclean = -1;\n> +\t\t\tgoto out;\n> +\t\t}\n>  \t\topt->branch1 = saved_b1;\n>  \t\topt->branch2 = saved_b2;\n>  \t\topt->priv->call_depth--;\n>  \n> -\t\tif (!merged_merge_bases)\n> -\t\t\treturn err(opt, _(\"merge returned no commit\"));\n> +\t\tif (!merged_merge_bases) {\n> +\t\t\tclean = err(opt, _(\"merge returned no commit\"));\n> +\t\t\tgoto out;\n> +\t\t}\n>  \t}\n>  \n>  \t/*\n> @@ -3682,8 +3686,11 @@ static int merge_recursive_internal(struct merge_options *opt,\n>  \t\t\t\t     repo_get_commit_tree(opt->repo,\n>  \t\t\t\t\t\t\t  merged_merge_bases),\n>  \t\t\t\t     &result_tree);\n> +\n> +out:\n>  \tstrbuf_release(&merge_base_abbrev);\n>  \topt->ancestor = NULL;  /* avoid accidental re-use of opt->ancestor */\n> +\tfree_commit_list(merge_bases);\n>  \tif (clean < 0) {\n>  \t\tflush_output(opt);\n>  \t\treturn clean;\n\nHmph, so the proposed log message made it sound like the merge_bases\nlist is consumed fully in the normal non-error case, but even the\nnormal case was leaky on the \"-s recursive\" side?  Or was the\nrecursive side was OK and the caller had different expectations, in\nwhich case we may be breaking them, but you poked at these codepaths\nlong enough to produce this patch, so I doubt it.  The proposed log\nmessage needs to be updated to explain the findings on this side,\ntoo, if the situation is different from the \"ort\" side.\n\n> @@ -3729,6 +3736,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n>  \tassert(!opt->record_conflict_msgs_as_headers);\n>  \tassert(!opt->msg_header_prefix);\n>  \n> +\tCALLOC_ARRAY(opt->priv, 1);\n> +\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n\nThis move, what it does, why it is needed, and what breaks without\nit, is not explained in the proposed log message.\n\n>  \t/* Sanity check on repo state; index must match head */\n>  \tif (repo_index_has_changes(opt->repo, head, &sb)) {\n>  \t\terr(opt, _(\"Your local changes to the following files would be overwritten by merge:\\n  %s\"),\n> @@ -3737,16 +3747,13 @@ static int merge_start(struct merge_options *opt, struct tree *head)\n>  \t\treturn -1;\n>  \t}\n>  \n> -\tCALLOC_ARRAY(opt->priv, 1);\n> -\tstring_list_init_dup(&opt->priv->df_conflict_file_set);\n>  \treturn 0;\n>  }\n\n>  static void merge_finalize(struct merge_options *opt)\n>  {\n>  \tflush_output(opt);\n> -\tif (!opt->priv->call_depth && opt->buffer_output < 2)\n> -\t\tstrbuf_release(&opt->obuf);\n> +\tstrbuf_release(&opt->obuf);\n\nDitto.  Unconditional release here may help the new caller in\nmerge_trees() that failed merge_start(), but is the change safe for\nother existing callers and if so why/how?\n\nIn any case, this needs a review by somebody more familiar with the\nrecursive backend machinery than myself.  Any takers?\n\n>  \tif (show(opt, 2))\n>  \t\tdiff_warn_rename_limit(\"merge.renamelimit\",\n>  \t\t\t\t       opt->priv->needed_rename_limit, 0);\n> @@ -3763,8 +3770,10 @@ int merge_trees(struct merge_options *opt,\n>  \n>  \tassert(opt->ancestor != NULL);\n>  \n> -\tif (merge_start(opt, head))\n> +\tif (merge_start(opt, head)) {\n> +\t\tmerge_finalize(opt);\n>  \t\treturn -1;\n> +\t}\n>  \tclean = merge_trees_internal(opt, head, merge, merge_base, &ignored);\n>  \tmerge_finalize(opt);\n>  \n> @@ -3785,8 +3794,11 @@ int merge_recursive(struct merge_options *opt,\n>  \tprepare_repo_settings(opt->repo);\n>  \topt->repo->settings.command_requires_full_index = 1;\n>  \n> -\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1)))\n> +\tif (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) {\n> +\t\tfree_commit_list(merge_bases);\n> +\t\tmerge_finalize(opt);\n>  \t\treturn -1;\n> +\t}\n\nI suspect that the way leaks happen is different between \"ort\" and\n\"recursive\", and what is in the proposed log message may have been\nthe right description of the problem back when the patch was only\nabout fixing \"ort\" but no longer is sufficient now that we also fix\nthe \"recursive\" side.\n\n>  \tclean = merge_recursive_internal(opt, h1, h2, merge_bases, result);\n>  \tmerge_finalize(opt);\n\nHmph, but this does expect merge_bases is consumed in normal\ncodepath.  Now I am confused, sorry.\n\nThanks for working on this.\n"},{"id":"481768","messageId":"CABPp-BGnjfNEROw0tbvk5vCeFeiS_5COH+_u7r5ZeXX1epP0wQ@mail.gmail.com","threadId":"60126","inReplyTo":"xmqqo7j4t4dm.fsf@gitster.g","subject":"Re: [PATCH] This fixes a minor memory leak (detected by LeakSanitizer) in git merge.","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-09-12T15:06:04Z","receivedAt":"2023-09-12T15:06:22Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Aug 18, 2023 at 2:41 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Kevin Backhouse via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n> > From: Kevin Backhouse <kevinbackhouse@github.com>\n> >\n> > To reproduce (with an ASAN build):\n> >\n> > ```\n> > mkdir test\n> > cd test\n> > git init\n> > echo x > x.txt\n> > git add .\n> > git commit -m \"WIP\"\n> > git checkout -b dev\n> > echo y > x.txt\n> > git add .\n> > git commit -m \"WIP\"\n> > git checkout main\n> > echo z > x.txt\n> > git add .\n> > git commit -m \"WIP\"\n> > echo a > x.txt\n> > git add .\n> > git merge dev\n> > ```\n>\n> We'd rather not to see the above in the proposed log message; can't\n> we add (a variation of) it to our test suite?\n>\n> > The fix is to call free_commit_list(merge_bases) when an error occurs.\n>\n> We usually have the description of what the problem is and give an\n> analysis on why/how it happens, before presenting a solution.  Write\n> it more like:\n>\n>     The caller of merge_ort_recursive() expects the commit list\n>     passed in as the merge_bases parameter to be fully consumed by\n>     the function and does not free it when the function returns.  In\n>     normal cases, the commit list does get consumed, but when the\n>     function returns early upon encountering an error, it forgets to\n>     clean it up.\n>\n>     Fix this by freeing the list in the code paths for error returns.\n>\n> >  merge-ort-wrappers.c | 4 +++-\n> >  merge-ort.c          | 4 +++-\n>\n> These two places and their fixes seem OK, but I have to wonder if\n> these are complete fixes.\n>\n> > diff --git a/merge-ort-wrappers.c b/merge-ort-wrappers.c\n> > index 4acedf3c338..aeb56c9970c 100644\n> > --- a/merge-ort-wrappers.c\n> > +++ b/merge-ort-wrappers.c\n> > @@ -54,8 +54,10 @@ int merge_ort_recursive(struct merge_options *opt,\n> >       struct tree *head = repo_get_commit_tree(opt->repo, side1);\n> >       struct merge_result tmp;\n> >\n> > -     if (unclean(opt, head))\n> > +     if (unclean(opt, head)) {\n> > +             free_commit_list(merge_bases);\n> >               return -1;\n> > +     }\n> >\n> >       memset(&tmp, 0, sizeof(tmp));\n> >       merge_incore_recursive(opt, merge_bases, side1, side2, &tmp);\n>\n> The function before this hunk appears to have very similar code\n> structure.  Does it need the same fix, or if not why not?\n>\n> > diff --git a/merge-ort.c b/merge-ort.c\n> > index 8631c997002..a0eb91fb011 100644\n> > --- a/merge-ort.c\n> > +++ b/merge-ort.c\n> > @@ -5070,8 +5070,10 @@ static void merge_ort_internal(struct merge_options *opt,\n> >               opt->branch1 = \"Temporary merge branch 1\";\n> >               opt->branch2 = \"Temporary merge branch 2\";\n> >               merge_ort_internal(opt, NULL, prev, next, result);\n> > -             if (result->clean < 0)\n> > +             if (result->clean < 0) {\n> > +                     free_commit_list(merge_bases);\n> >                       return;\n> > +             }\n>\n> Before this function, there is a comment that this came from another\n> function and it seems to still have a very similar code structure.\n> Does the other function need the same fix, or if not why not?\n\nThe other function would need a more involved fix, which would\nbasically involve porting a59b8dd94f (merge-ort: fix memory leak in\nmerge_ort_internal(), 2022-01-20) to merge-recursive as a preparatory\nstep.  This particular cleanup cannot be ported in its current form to\nmerge-recursive.c until then.\n"}]}