Thread (1 message) 1 message, 1 author, 2024-08-06

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

From: Junio C Hamano <hidden>
Date: 2024-08-06 15:44:30

Patrick Steinhardt [off-list ref] writes:
quoted
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.
quoted
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.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help