Re: [PATCH 13/17] rebase: don't leak on "--abort"
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Nov 4, 2022, 14:42 UTC
- 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:
Show 16 quoted lines
> 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
Show 30 quoted lines
> 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' '