{"thread":{"id":"66081","subject":"[PATCH] merge: fix leak with merge.defaultToUpstream","startedAt":"2026-07-28T13:00:31Z","lastAt":"2026-07-28T18:12:30Z","messageCount":3,"participants":["Toon Claes","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"549123","messageId":"20260728-toon-fix-merge-leak-v1-1-d6bd161a3acf@iotcl.com","threadId":"66081","inReplyTo":null,"subject":"[PATCH] merge: fix leak with merge.defaultToUpstream","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2026-07-28T13:00:04Z","receivedAt":"2026-07-28T13:00:31Z","isPatch":true,"body":"By default the setting 'merge.defaultToUpstream' for git-merge(1) is set\nto 'true', which means when `git merge` is invoked with no arguments it\nmerges the upstream branch configured for the current branch.\n\nWith this configuration set to 'true', setup_with_upstream() is called.\nThat function allocates an array of arguments and hands it back to\ncmd_merge() via its `argv` parameter. This array is never freed, so\ncmd_merge() leaks it on every invocation.\n\nTrack the allocated array in a separate variable and free it at the end.\n\nThe leak has been present since 93e535a5b7 (merge: merge with the\ndefault upstream branch without argument, 2011-03-24). Although the leak\nsanitizer was enabled for tests in fc1ddf42af (t: remove\nTEST_PASSES_SANITIZE_LEAK annotations, 2024-11-21), it went unnoticed\nbecause no test calls `git merge` without arguments, exercising the\ndefault-to-upstream path. Add such a test in t7600, which fails under\nthe leak sanitizer without this fix.\n\nSigned-off-by: Toon Claes <toon@iotcl.com>\n---\nI ran into this leak while running `yay`[1] (I use Arch btw). `yay` uses\nGit to fetch packages from source, and it happens to call `git merge`\nwithout any revision to merge (it only passes the options `--no-edit`\nand `--ff`). Because I have Git with the leak sanitizer enabled in my\n$PATH, my `yay` tripped on a leak.\n\nThis series fixes the leak and adds a test to reproduce it.\n\n[1]: https://github.com/Jguer/yay\n---\n builtin/merge.c  |  7 +++++--\n t/t7600-merge.sh | 17 +++++++++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 58d1b7bb07..5b4eb23a83 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1373,7 +1373,7 @@ int cmd_merge(int argc,\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list *remoteheads = NULL, *p;\n-\tvoid *branch_to_free;\n+\tvoid *branch_to_free, *argv_to_free = NULL;\n \tint orig_argc = argc;\n \tint merge_log_config = -1;\n \n@@ -1517,8 +1517,10 @@ int cmd_merge(int argc,\n \t\toption_commit = 1;\n \n \tif (!argc) {\n-\t\tif (default_to_upstream)\n+\t\tif (default_to_upstream) {\n \t\t\targc = setup_with_upstream(&argv);\n+\t\t\targv_to_free = argv;\n+\t\t}\n \t\telse\n \t\t\tdie(_(\"No commit specified and merge.defaultToUpstream not set.\"));\n \t} else if (argc == 1 && !strcmp(argv[0], \"-\")) {\n@@ -1880,6 +1882,7 @@ int cmd_merge(int argc,\n \t}\n \tstrbuf_release(&buf);\n \tfree(branch_to_free);\n+\tfree(argv_to_free);\n \tfree(pull_twohead);\n \tfree(pull_octopus);\n \tdiscard_index(the_repository->index);\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 7f2a1db16d..e31d261f9d 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -1166,4 +1166,21 @@ test_expect_success 'suggested names are not ambiguous' '\n \ttest_grep remotes/origin/not-local stderr\n '\n \n+test_expect_success 'merge with no argument defaults to upstream' '\n+\ttest_when_finished \"rm -rf upstream downstream\" &&\n+\tgit init upstream &&\n+\t(\n+\t\tcd upstream &&\n+\t\ttest_commit one &&\n+\t\ttest_commit two\n+\t) &&\n+\tgit clone upstream downstream &&\n+\t(\n+\t\tcd downstream &&\n+\t\tgit reset --hard HEAD^ &&\n+\t\tgit merge &&\n+\t\ttest_cmp_rev origin/main HEAD\n+\t)\n+'\n+\n test_done\n\n---\nbase-commit: 13c7afec212fc97ce257d15601659314c6673d6c\nchange-id: 20260728-toon-fix-merge-leak-6d3bc5af2082\n\n"},{"id":"549137","messageId":"20260728151959.GC41931@coredump.intra.peff.net","threadId":"66081","inReplyTo":"20260728-toon-fix-merge-leak-v1-1-d6bd161a3acf@iotcl.com","subject":"Re: [PATCH] merge: fix leak with merge.defaultToUpstream","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2026-07-28T15:19:59Z","receivedAt":"2026-07-28T15:20:00Z","isPatch":true,"body":"On Tue, Jul 28, 2026 at 03:00:04PM +0200, Toon Claes wrote:\n\n> The leak has been present since 93e535a5b7 (merge: merge with the\n> default upstream branch without argument, 2011-03-24). Although the leak\n> sanitizer was enabled for tests in fc1ddf42af (t: remove\n> TEST_PASSES_SANITIZE_LEAK annotations, 2024-11-21), it went unnoticed\n> because no test calls `git merge` without arguments, exercising the\n> default-to-upstream path. Add such a test in t7600, which fails under\n> the leak sanitizer without this fix.\n\nWow, I'm surprised we didn't cover this case in the test suite.\nIncreasing coverage is good.\n\n> @@ -1517,8 +1517,10 @@ int cmd_merge(int argc,\n>  \t\toption_commit = 1;\n>  \n>  \tif (!argc) {\n> -\t\tif (default_to_upstream)\n> +\t\tif (default_to_upstream) {\n>  \t\t\targc = setup_with_upstream(&argv);\n> +\t\t\targv_to_free = argv;\n> +\t\t}\n\nThe fix looks correct to me. This whole argv-juggling is pretty gross\n(especially the part below which overwrites argv[0]!). I suspect using a\nseparate strvec to hold the heads would be cleaner, but it is probably\nnot worth anybody's time to micro-polish this.\n\n-Peff\n"},{"id":"549159","messageId":"xmqqfr13dlv8.fsf@gitster.g","threadId":"66081","inReplyTo":"20260728151959.GC41931@coredump.intra.peff.net","subject":"Re: [PATCH] merge: fix leak with merge.defaultToUpstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-28T18:12:27Z","receivedAt":"2026-07-28T18:12:30Z","isPatch":true,"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 28, 2026 at 03:00:04PM +0200, Toon Claes wrote:\n>\n>> The leak has been present since 93e535a5b7 (merge: merge with the\n>> default upstream branch without argument, 2011-03-24). Although the leak\n>> sanitizer was enabled for tests in fc1ddf42af (t: remove\n>> TEST_PASSES_SANITIZE_LEAK annotations, 2024-11-21), it went unnoticed\n>> because no test calls `git merge` without arguments, exercising the\n>> default-to-upstream path. Add such a test in t7600, which fails under\n>> the leak sanitizer without this fix.\n>\n> Wow, I'm surprised we didn't cover this case in the test suite.\n> Increasing coverage is good.\n>\n>> @@ -1517,8 +1517,10 @@ int cmd_merge(int argc,\n>>  \t\toption_commit = 1;\n>>  \n>>  \tif (!argc) {\n>> -\t\tif (default_to_upstream)\n>> +\t\tif (default_to_upstream) {\n>>  \t\t\targc = setup_with_upstream(&argv);\n>> +\t\t\targv_to_free = argv;\n>> +\t\t}\n>\n> The fix looks correct to me. This whole argv-juggling is pretty gross\n> (especially the part below which overwrites argv[0]!). I suspect using a\n> separate strvec to hold the heads would be cleaner, but it is probably\n> not worth anybody's time to micro-polish this.\n\nThanks for heading off my OCD before it showed ;-).\n\nAnd of course, thanks Toon for noticing and fixing.\n\nWill queue.\n\n\n"}]}