| Message ID | 20240924215634.GI1143820@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e00e1cff0d845a66c58180d75c33be109a958ba3 |
| Headers | show |
| Series | leak fixes for http fetch/push | expand |
On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote: > diff --git a/transport-helper.c b/transport-helper.c > index c688967b8c..9c8abd8eca 100644 > --- a/transport-helper.c > +++ b/transport-helper.c > @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport, > if (atomic) { > reject_atomic_push(remote_refs, mirror); > string_list_clear(&cas_options, 0); > + strbuf_release(&buf); > return 0; > } else > continue; What's not visible here is that a few lines further down we have another early return where we don't clear `buf`. But that exit is conditioned on `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we know that `buf.len == 0` only when it hasn't ever be allocated. So this LGTM. Patrick
On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote: > On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote: > > diff --git a/transport-helper.c b/transport-helper.c > > index c688967b8c..9c8abd8eca 100644 > > --- a/transport-helper.c > > +++ b/transport-helper.c > > @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport, > > if (atomic) { > > reject_atomic_push(remote_refs, mirror); > > string_list_clear(&cas_options, 0); > > + strbuf_release(&buf); > > return 0; > > } else > > continue; > > What's not visible here is that a few lines further down we have another > early return where we don't clear `buf`. But that exit is conditioned on > `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we > know that `buf.len == 0` only when it hasn't ever be allocated. Yeah, I noticed that, too, and came to the same conclusion. As you note, it's _possible_ to have an allocated but zero-length strbuf, but I don't think it happens here. So it's OK in practice. If we were going to refactor, I think it would make sense to do it with a cleanup label to cover both of these early returns, plus the cleanup at the end. I was mostly just going for minimal changes where possible. -Peff
diff --git a/t/t5541-http-push-smart.sh b/t/t5541-http-push-smart.sh index 71428f3d5c..3ad514bbd4 100755 --- a/t/t5541-http-push-smart.sh +++ b/t/t5541-http-push-smart.sh @@ -7,6 +7,7 @@ test_description='test smart pushing over http via http-backend' GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh ROOT_PATH="$PWD" diff --git a/transport-helper.c b/transport-helper.c index c688967b8c..9c8abd8eca 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport, if (atomic) { reject_atomic_push(remote_refs, mirror); string_list_clear(&cas_options, 0); + strbuf_release(&buf); return 0; } else continue;
We loop over the refs to push, building up a strbuf with the set of "push" directives to send to the remote helper. But if the atomic-push flag is set and we hit a rejected ref, we'll bail from the function early. We clean up most things, but forgot to release the strbuf. Fixing this lets us mark t5541 as leak-free. Signed-off-by: Jeff King <peff@peff.net> --- Arguably this whole function could benefit from a cleanup goto, but it's a little non-trivial. So I stuck with simple-and-stupid. t/t5541-http-push-smart.sh | 1 + transport-helper.c | 1 + 2 files changed, 2 insertions(+)