Re: [PATCH 06/28] fetch-pack, send-pack: clean up shallow oid array

From: Jeff King <peff@peff.net>
Date: Sep 27, 2024, 03:45 UTC
Message-ID: <20240927034525.GA567395@coredump.intra.peff.net>
In-Reply-To: <ZvVmitkT-QUneqvb@pks.im>

On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote:

Show 15 quoted lines

> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index cfc6951d23..ef4143eef3 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
> >  	free_refs(fetched_refs);
> >  	free_refs(remote_refs);
> >  	list_objects_filter_release(&args.filter_options);
> > +	oid_array_clear(&shallow);
> >  	return ret;
> >  }
> 
> I wonder about the early exit path we have when `finish_connect()`
> returns non-zero. Should we make it go via the common cleanup path as
> well, or do we not care in the error case?

Yeah, I think it would technically be a leak if we hit that error case. But it's pretty unlikely in practice (you'd have to finish the fetch and then the "ssh" or "upload-pack" sub-process returns a non-zero exit code anyway).

I'd prefer not to deal with it here for two reasons:

  1. The same leak is true of all the existing cleanup. So it really is
     orthogonal to what this patch is doing, and should be a separate
     patch.

  2. I think cleaning up in top-level builtins like this is mostly
     cosmetic to appease the leak checker. In the real world the memory
     is going back to the OS when we return anyway. Even in a libified
     world, I don't think cmd_fetch_pack() is a reasonable entry point
     (we already have a more lib-ish fetch_pack() that is used by the
     transport code).

So I don't think it would be wrong to refactor the cleanup to trigger on that other early return. But I also suspect there are many such paths lurking (and will continue to lurk, even after we run clean with SANITIZE=leak) and I'm not sure how productive it is to hunt them down.

Show 14 quoted lines

> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 81fc96d423..c49fe6c53c 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
> >  	free_refs(remote_refs);
> >  	free_refs(local_refs);
> >  	refspec_clear(&rs);
> > +	oid_array_clear(&shallow);
> >  	return ret;
> >  }
> 
> We also have an early exit in this function when `match_push_refs()`
> returns non-zero.

And I think this is in the same boat.

-Peff