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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 6, 2024, 15:44 UTC
Message-ID: <xmqqcymlzmec.fsf@gitster.g>
In-Reply-To: <ZrHAu0wfipR6CShS@tanuki>

Patrick Steinhardt <ps@pks.im> writes:

Show 9 quoted lines

>> 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 :)

Yes, we had "what is release, clear, and free?" discussion recently.

Show 5 quoted lines

>> 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.

I am not short of (other) things to do, and expect that I will not touching this for a while, but in case somebody finds #leftoverbits here, I'll leave a note here.

There are "hidden" freeing that we have to adjust, if we were to follow through this approach. For example, those free()'s added in the patch in the message that started the thread are introduing double free---after the strmap_for_each_entry() loop, a strmap_clear() callis done with free_values=1. If we freed inside ref_store_release(), we'd need to adjust the call to strmap_clear() to pass free_values=0 to compensate.