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@xxxxxxxxx>
---
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' '
