{"thread":{"id":"61178","subject":"[PATCH] rebase: use child_process_clear() to clean","startedAt":"2024-03-22T10:35:05Z","lastAt":"2024-03-25T18:44:44Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"491231","messageId":"20240322103502.GA2045297@coredump.intra.peff.net","threadId":"61178","inReplyTo":null,"subject":"[PATCH] rebase: use child_process_clear() to clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-22T10:35:02Z","receivedAt":"2024-03-22T10:35:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In the run_am() function, we set up a child_process struct to run\n\"git-am\", allocating memory for its args and env strvecs. These are\nnormally cleaned up when we call run_command(). But if we encounter\ncertain errors, we exit the function early and try to clean up ourselves\nby clearing the am.args field. This leaks the \"env\" strvec.\n\nWe should use child_process_clear() instead, which covers both. And more\nimportantly, it future proofs us against the struct ever growing more\nallocated fields.\n\nThese are unlikely errors to happen in practice, so they don't actually\ntrigger the leak sanitizer in the tests. But we can add a new test which\ndoes exercise one of the paths (and fails SANITIZE=leak without this\npatch).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI noticed this when investigating a different leak, since that other one\ncaused format-patch to crash, hitting one of the error paths. ;)\n\nThe results of \"git grep 'strvec_clear.*\\.args'\" imply the problem\ndoesn't exist elsewhere (arguably the hit in test-trace2.c should use\nchild_process_clear() to set a good example, but it's already a bit of\nan oddball use of the struct).\n\nNot sure if the test is overkill. It really does nothing useful in a\nnon-leak-checking build.\n\n builtin/rebase.c               | 6 +++---\n t/t3438-rebase-broken-files.sh | 9 +++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e444ab102d..b9d0fb3269 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -610,7 +610,7 @@ static int run_am(struct rebase_options *opts)\n \t\tstatus = error_errno(_(\"could not open '%s' for writing\"),\n \t\t\t\t     rebased_patches);\n \t\tfree(rebased_patches);\n-\t\tstrvec_clear(&am.args);\n+\t\tchild_process_clear(&am);\n \t\treturn status;\n \t}\n \n@@ -638,7 +638,7 @@ static int run_am(struct rebase_options *opts)\n \t\tstruct reset_head_opts ropts = { 0 };\n \t\tunlink(rebased_patches);\n \t\tfree(rebased_patches);\n-\t\tstrvec_clear(&am.args);\n+\t\tchild_process_clear(&am);\n \n \t\tropts.oid = &opts->orig_head->object.oid;\n \t\tropts.branch = opts->head_name;\n@@ -659,7 +659,7 @@ static int run_am(struct rebase_options *opts)\n \t\tstatus = error_errno(_(\"could not open '%s' for reading\"),\n \t\t\t\t     rebased_patches);\n \t\tfree(rebased_patches);\n-\t\tstrvec_clear(&am.args);\n+\t\tchild_process_clear(&am);\n \t\treturn status;\n \t}\n \ndiff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\nindex c614c4f2e4..821f08e5af 100755\n--- a/t/t3438-rebase-broken-files.sh\n+++ b/t/t3438-rebase-broken-files.sh\n@@ -58,4 +58,13 @@ test_expect_success 'unknown key in author-script' '\n \tcheck_resolve_fails\n '\n \n+test_expect_success POSIXPERM,SANITY 'unwritable rebased-patches does not leak' '\n+\t>.git/rebased-patches &&\n+\tchmod a-w .git/rebased-patches &&\n+\n+\tgit checkout -b side HEAD^ &&\n+\ttest_commit unrelated &&\n+\ttest_must_fail git rebase --apply --onto tmp HEAD^\n+'\n+\n test_done\n-- \n2.44.0.682.g01e1dab148\n"},{"id":"491255","messageId":"xmqqedc218to.fsf@gitster.g","threadId":"61178","inReplyTo":"20240322103502.GA2045297@coredump.intra.peff.net","subject":"Re: [PATCH] rebase: use child_process_clear() to clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-22T17:21:23Z","receivedAt":"2024-03-22T17:21:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> In the run_am() function, we set up a child_process struct to run\n> \"git-am\", allocating memory for its args and env strvecs. These are\n> normally cleaned up when we call run_command(). But if we encounter\n> certain errors, we exit the function early and try to clean up ourselves\n> by clearing the am.args field. This leaks the \"env\" strvec.\n>\n> We should use child_process_clear() instead, which covers both. And more\n> importantly, it future proofs us against the struct ever growing more\n> allocated fields.\n\nWhen 21853626 (built-in rebase: call `git am` directly, 2019-01-18)\nstarted using run_command() API to drive \"am\", there already was\nchild_process API and child_process_clear() did clear both .args and\n.env_array members but we used argv_array_clear() only to clear\nam.args, leaking am.env_array.\n\n> These are unlikely errors to happen in practice, so they don't actually\n> trigger the leak sanitizer in the tests. But we can add a new test which\n> does exercise one of the paths (and fails SANITIZE=leak without this\n> patch).\n> ...\n> Not sure if the test is overkill. It really does nothing useful in a\n> non-leak-checking build.\n\nI wonder what are in .env array at this point, though, but that is\nmere curiosity and not a problem with this patch.\n\nThanks.  Will queue.\n\n\n> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh\n> index c614c4f2e4..821f08e5af 100755\n> --- a/t/t3438-rebase-broken-files.sh\n> +++ b/t/t3438-rebase-broken-files.sh\n> @@ -58,4 +58,13 @@ test_expect_success 'unknown key in author-script' '\n>  \tcheck_resolve_fails\n>  '\n>  \n> +test_expect_success POSIXPERM,SANITY 'unwritable rebased-patches does not leak' '\n> +\t>.git/rebased-patches &&\n> +\tchmod a-w .git/rebased-patches &&\n> +\n> +\tgit checkout -b side HEAD^ &&\n> +\ttest_commit unrelated &&\n> +\ttest_must_fail git rebase --apply --onto tmp HEAD^\n> +'\n> +\n>  test_done\n"},{"id":"491285","messageId":"20240322233656.GB2049941@coredump.intra.peff.net","threadId":"61178","inReplyTo":"xmqqedc218to.fsf@gitster.g","subject":"Re: [PATCH] rebase: use child_process_clear() to clean","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-03-22T23:36:56Z","receivedAt":"2024-03-22T23:36:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 22, 2024 at 10:21:23AM -0700, Junio C Hamano wrote:\n\n> > We should use child_process_clear() instead, which covers both. And more\n> > importantly, it future proofs us against the struct ever growing more\n> > allocated fields.\n> \n> When 21853626 (built-in rebase: call `git am` directly, 2019-01-18)\n> started using run_command() API to drive \"am\", there already was\n> child_process API and child_process_clear() did clear both .args and\n> .env_array members but we used argv_array_clear() only to clear\n> am.args, leaking am.env_array.\n\nAh, interesting. I didn't bother to dig into the history. Back then,\nthough, I think that \"args\" was the only allocated thing. The env call\nwas added much later in be0d29d301 (rebase --apply: make reflog messages\nmatch rebase --merge, 2022-10-12).\n\n> I wonder what are in .env array at this point, though, but that is\n> mere curiosity and not a problem with this patch.\n\nIt's $GIT_REFLOG_ACTION for the sub-process.\n\n-Peff\n"},{"id":"491488","messageId":"xmqqedbykv6u.fsf@gitster.g","threadId":"61178","inReplyTo":"20240322233656.GB2049941@coredump.intra.peff.net","subject":"Re: [PATCH] rebase: use child_process_clear() to clean","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-25T18:44:41Z","receivedAt":"2024-03-25T18:44:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> I wonder what are in .env array at this point, though, but that is\n>> mere curiosity and not a problem with this patch.\n>\n> It's $GIT_REFLOG_ACTION for the sub-process.\n\nAh, OK.\n"}]}