Re: [PATCH 03/28] fetch-pack: fix leaking sought refs

[Patrick Steinhardt <ps@pks.im>]

On Wed, Sep 25, 2024 at 07:17:24PM +0200, René Scharfe wrote:
> Am 24.09.24 um 23:51 schrieb Jeff King:
> > From: Patrick Steinhardt <ps@pks.im>
> > diff --git a/transport.c b/transport.c
> > index 3c4714581f..1098bbd60e 100644
> > --- a/transport.c
> > +++ b/transport.c
> > @@ -414,7 +414,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  	struct git_transport_data *data = transport->data;
> >  	struct ref *refs = NULL;
> >  	struct fetch_pack_args args;
> > -	struct ref *refs_tmp = NULL;
> > +	struct ref *refs_tmp = NULL, **to_fetch_dup = NULL;
> >
> >  	memset(&args, 0, sizeof(args));
> >  	args.uploadpack = data->options.uploadpack;
> > @@ -477,6 +477,14 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  		goto cleanup;
> >  	}
> >
> > +	/*
> > +	 * Create a shallow copy of `sought` so that we can free all of its entries.
> > +	 * This is because `fetch_pack()` will modify the array to evict some
> > +	 * entries, but won't free those.
> > +	 */
> > +	DUP_ARRAY(to_fetch_dup, to_fetch, nr_heads);
> > +	to_fetch = to_fetch_dup;
> > +
> >  	refs = fetch_pack(&args, data->fd,
> >  			  refs_tmp ? refs_tmp : transport->remote_refs,
> >  			  to_fetch, nr_heads, &data->shallow,
> > @@ -500,6 +508,7 @@ static int fetch_refs_via_pack(struct transport *transport,
> >  		ret = -1;
> >  	data->conn = NULL;
> >
> > +	free(to_fetch_dup);
> 
> This makes a shallow copy and passes it to fetch_pack() and later to
> report_unmatched_refs().  It shields callers of fetch_refs_via_pack()
> from the effect of fetch_pack()'s NULLification.  This is what the
> commit message says would not work.  What am I missing?

`fetch_refs_via_pack()` is called via `transport_fetch_refs()`, which
constructs the array of refs to fetch ad-hoc and then discards it
immediately. So it doesn't ever inspect the modified array in the first
place, and consequently we can guard against the NULLification here.

This code is quite intricate :/

Patrick