Re: [PATCH] rebase: use child_process_clear() to clean

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> 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.

> 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.


> 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