| Message ID | patch-13.17-fda9914b558-20221103T164632Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: use existing constructors & other trivia | expand |
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' '
On Fri, Nov 04 2022, Phillip Wood wrote: > 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. >> [...] >> --- 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; > > 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. What I've been targeting in this and past leak fix topics is roughly what valgrind calls[1] "definitely lost" and "indirectly lost", and which -fsanitize=leak calls "that thing I die on whan I run into it" :) So, you and I know we're "about to exit" as we're in cmd_rebase() and about to return to common-main.c etc, but it's harder to convince a compiler of that. Maybe I'm just touchy, sorry. It just feels like every time I submit a leak fix topic there's some rehash of the "why do this at all?" or "why this one?" discussion. To summarize (maybe too much for just this reply, but I hope to link back to this in the future): * Historically we've said "that's just a built-in, let the OS take care of it!" * I agree with that in principle. It's pretty pointless to free() things in cmd_*() in the abstract. We're "about to exit", even if valgrind etc. call it "definitely lost"[1]. * But, it would be nice to have git's APIs not leak, so we could call them in loops, replace some of our own sub-process invocations etc[2]. * If we had 100% test coverage for those libraries we could just test that, and get those leak-free, and ignore the built-ins. * We don't have that, and probably never will. E.g. "git log" and friends are *the* things that stress revision.[ch]. So to assert that the API is leak-free we really need to assert that its users are. Ditto for pretty much any other API we carry. * Still, we could just say "if something's alloc'd in cmd_*() don't care about it". Both LSAN and valgrind support "suppressions" to ignore certain functions, patterns etc. * But that doesn't really work either, as e.g. "struct rev_info" would seem to come from such a cmd_*() as far as ownership goes... * ..."but you could have an even more clever suppressions mechanism, and only ignore those things that are malloc'd by cmd_*(). But not e.g. a malloc in revision.c in a struct member in a struct that's on the stack in a cmd_*()", one might say. Yeah, you could do that, e.g. in this case we'll call strbuf_addf() which makes a beeline to malloc(). Sufficiently clever post-processing of leak stacktraces could probably make the distinction. But then you run into the problem of e.g. how the the log family and others use "mailmap". I.e. in that case (and many others) the cmd_*() will malloc(), but "hand it off" to the API, whose job we know it is to free() it. But we only know because of knowledge about the API, an automated system would care, still, it's not unworkable. We could go through those, list the exceptions etc. etc. That's a lot of context, but I think brings us up-to-date on this one-line change. So, could we do things differently here? Definitely. And if the codebase was in a state where e.g. builtin/*.c had ~1500 UNLEAK() already I'd probably try to extend that, rather than fighting that uphill battle. But that's not the case, we have around ~30 of them, fewer after this topic. By far the majority of builtin/* is caring about directly recahable leaks already. E.g. just above this context in another "case" arm we're doing the exact "variable on stack, use it, then release()", just with a "struct string_list". So I think this is the right direction to go in, both in this patch & in general. 1. https://valgrind.org/docs/manual/mc-manual.html#mc-manual.leaks 2. Even the assumption that cmd_*() is a one-off is shaky, e.g. for cases where we shell out to "git commit" we're close now to being able to just call cmd_commit(), but let's leave that aside for now). So, we're pretty close to cmd_*() being a funny API that happens to take strvec arguments, which is going to be the shortest path to eliminating sub-process invocation in many cases. I.e. as opposed to refactoring the cmd_*() into a "real" library, that takes a struct with options, doesn't call parse_options() etc.
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' '
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. 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(+)