From: Phillip Wood <phillip.wood123@gmail.com>
Date: Fri, 04 Nov 2022 14:42:25 GMT
Subject: Re: [PATCH 13/17] rebase: don't leak on "--abort"
Message-ID: <f9df4dfc-6024-deb0-5277-bab96c81a875@dunelm.org.uk>
In-Reply-To: <patch-13.17-fda9914b558-20221103T164632Z-avarab@gmail.com>

Hi Ævar

On 03/11/2022 17:06, Ævar Arnfjörð Bjarmason wrote:
> Fix a leak in the recent 6159e7add49 (rebase --abort: improve reflog
> message, 2022-10-12). Before that commit we'd strbuf_release() the
> reflog message we were formatting, but when that code was refactored
> to use "ropts.head_msg" the strbuf_release() was omitted.
> 
> Ideally the three users of "ropts" in cmd_rebase() should use
> different "ropts" variables, in practice they're completely separate,
> as this and the other user in the "switch" statement will "goto
> cleanup", which won't touch "ropts".
> 
> The third caller after the "switch" is then unreachable if we take
> these two branches, so all of them are getting a "{ 0 }" init'd
> "ropts".
> 
> So it's OK that we're leaving a stale pointer in "ropts.head_msg",
> cleaning it up was our responsibility, and it won't be used again.

This looks fine. One could argue that the leak is not a "real" leak as 
we're about to exit but the omission of strbuf_release() was 
unintentional and fix is nice and simple.

Thanks

Phillip

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/rebase.c          | 1 +
>   t/t7517-per-repo-email.sh | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 64aed11c85d..8a8b5e34786 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1320,6 +1320,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   		if (reset_head(the_repository, &ropts) < 0)
>   			die(_("could not move back to %s"),
>   			    oid_to_hex(&options.orig_head->object.oid));
> +		strbuf_release(&head_msg);
>   		remove_branch_state(the_repository, 0);
>   		ret = finish_rebase(&options);
>   		goto cleanup;
> diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
> index 163ae804685..efc6496e2b2 100755
> --- a/t/t7517-per-repo-email.sh
> +++ b/t/t7517-per-repo-email.sh
> @@ -9,6 +9,7 @@ test_description='per-repo forced setting of email address'
>   GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>   export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
>   
> +TEST_PASSES_SANITIZE_LEAK=true
>   . ./test-lib.sh
>   
>   test_expect_success 'setup a likely user.useConfigOnly use case' '