Re: [PATCH] rebase: use child_process_clear() to clean
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 22, 2024, 17:21 UTC
- Message-ID
- <xmqqedc218to.fsf@gitster.g>
- In-Reply-To
- <20240322103502.GA2045297@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> In the run_am() function, we set up a child_process struct to run > "git-am", allocating memory for its args and env strvecs. These are > normally cleaned up when we call run_command(). But if we encounter > certain errors, we exit the function early and try to clean up ourselves > by clearing the am.args field. This leaks the "env" strvec. > > We should use child_process_clear() instead, which covers both. And more > importantly, it future proofs us against the struct ever growing more > allocated fields.
When 21853626 (built-in rebase: call `git am` directly, 2019-01-18) started using run_command() API to drive "am", there already was child_process API and child_process_clear() did clear both .args and .env_array members but we used argv_array_clear() only to clear am.args, leaking am.env_array.
Show 7 quoted lines
> These are unlikely errors to happen in practice, so they don't actually > trigger the leak sanitizer in the tests. But we can add a new test which > does exercise one of the paths (and fails SANITIZE=leak without this > patch). > ... > Not sure if the test is overkill. It really does nothing useful in a > non-leak-checking build.
I wonder what are in .env array at this point, though, but that is mere curiosity and not a problem with this patch.
Thanks. Will queue.
Show 18 quoted lines
> diff --git a/t/t3438-rebase-broken-files.sh b/t/t3438-rebase-broken-files.sh > index c614c4f2e4..821f08e5af 100755 > --- a/t/t3438-rebase-broken-files.sh > +++ b/t/t3438-rebase-broken-files.sh > @@ -58,4 +58,13 @@ test_expect_success 'unknown key in author-script' ' > check_resolve_fails > ' > > +test_expect_success POSIXPERM,SANITY 'unwritable rebased-patches does not leak' ' > + >.git/rebased-patches && > + chmod a-w .git/rebased-patches && > + > + git checkout -b side HEAD^ && > + test_commit unrelated && > + test_must_fail git rebase --apply --onto tmp HEAD^ > +' > + > test_done