Re: [PATCH] repository: prevent memory leak when releasing ref stores

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2530 bytes --]

On Mon, Aug 05, 2024 at 10:24:10AM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > However, I am fuzzy on the existing uses in the backend
> > implementation.  For example:
> >
> >         static void files_ref_store_release(struct ref_store *ref_store)
> >         {
> >                 struct files_ref_store *refs = files_downcast(ref_store, 0, "release");
> >                 free_ref_cache(refs->loose);
> >                 free(refs->gitcommondir);
> >                 ref_store_release(refs->packed_ref_store);
> >         }
> >
> > The packed-ref-store is "released" here, as part of "releasing" the
> > files-ref-store that uses it as a fallback backend.  The caller of
> > files_ref_store_release() is refs.c:ref_store_release()
> >
> >         void ref_store_release(struct ref_store *ref_store)
> >         {
> >                 ref_store->be->release(ref_store);
> >                 free(ref_store->gitdir);
> >         }
> >
> > So if you have a files based ref store, when you are done you'd be
> > calling ref_store_release() on it, releasing the resources held by
> > the files_ref_store structure, but I do not know who frees the
> > packed_ref_store allocated by files_ref_store_init().  Perhaps it is
> > already leaking?  If that is the case then an API update like I
> > suggested above would make even more sense to make it less likely
> > for such a leak to be added to the system in the future, I suspect.
> 
> Ahh, that was the leak that you plugged in a separate patch.
> 
> So it does point us in the other direction to redefine _release with
> a different behaviour that releases the resource held by the
> structure, and frees the structure itself.
> 
> Something along the following line (caution: totally untested) that
> allows your two patches to become empty, and also allows a few
> callers to lose their existing explicit free()s immediately after
> they call _release(), perhaps?

I don't really know whether it's worth the churn, but if somebody wants
to pull through with this I'm game :) But: if we are going to do this,
we should rename the function to be called `ref_store_free()` instead of
`ref_store_release()` according to our recent coding style update :)

> If this were to become a real patch, I think debug backend should
> learn to use the same _downcast() to become more like the real ones
> before it happens in a preliminary clean-up patch.

That certainly wouldn't hurt, yeah.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]