Re: [PATCH 09/28] transport-helper: fix strbuf leak in push_refs_with_push()
- From
Jeff King <peff@peff.net>
- Date
- Sep 27, 2024, 03:49 UTC
- Message-ID
- <20240927034948.GC567395@coredump.intra.peff.net>
- In-Reply-To
- <ZvVmllrUvOC3pGJy@pks.im>
On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote:
Show 18 quoted lines
> 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