Re: [PATCH 19/20] transport: fix leaking arguments when fetching from bundle

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 22, 2024, 08:19 UTC
Message-ID: <Zsb0qU-G4kdlxrWa@tanuki>
In-Reply-To: <xmqqseuxyck4.fsf@gitster.g>

On Wed, Aug 21, 2024 at 11:07:39AM -0700, Junio C Hamano wrote:

Show 35 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In `fetch_refs_from_bundle()` we assemble a vector of arguments to pass
> > to `unbundle()`, but never free it. Fix this leak.
> >
> > This memory leak gets hit in t5510, but fixing it isn't sufficient to
> > make the whole test suite pass.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  transport.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/transport.c b/transport.c
> > index f0672fdc505..da639d3bff0 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -189,6 +189,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
> >  		       &extra_index_pack_args,
> >  		       fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0);
> >  	transport->hash_algo = data->header.hash_algo;
> > +
> > +	strvec_clear(&extra_index_pack_args);
> >  	return ret;
> >  }
> 
> Hmph.  unbundle() has this:
> 
> 	if (extra_index_pack_args) {
> 		strvec_pushv(&ip.args, extra_index_pack_args->v);
> 		strvec_clear(extra_index_pack_args);
> 	}
> 
> so while I think this patch would not hurt at all, do we need this
> patch?

Yes we do, because there is an early return in case `verify_bundle()` fails. I didn't notice that we have it in `unbundle()` though.

> The other caller of unbundle() that passes the extra_index_pack_args
> is cmd_bundle_unbundle() and it does not do anything after it is
> done.

I'd argue it's bad practice to have `unbundle()` clear the caller provided args for us and somewhat surprising. While one way to fix this would be to have a common exit path where we always free them. But I'd much rather make it the responsibility of the caller itself to free them.

I'll adapt the code in this spirit.

Patrick