On Mon, Sep 12, 2016 at 12:10:13PM -0700, Junio C Hamano wrote:
> Jeff King <p...@peff.net> writes:
> > I happened to notice today that this topic needs a minor tweak:
> > -- >8 --
> > Subject: [PATCH] add_delta_base_cache: use list_for_each_safe
> > We may remove elements from the list while we are iterating,
> > which requires using a second temporary pointer. Otherwise
> > stepping to the next element of the list might involve
> > looking at freed memory (which generally works in practice,
> > as we _just_ freed it, but of course is wrong to rely on;
> > valgrind notices it).
> I failed to notice it, too. Thanks.
After staring at this, I was wondering how the _original_ ever worked.
Because the problem is in the linked-list code, which I did not really
change (I switched it to LIST_HEAD(), but the code is equivalent).
The answer is that in the original, there was no free() in the original
code when we released an entry; it just went back to the (static) pool.
So the bug is in the conversion to hashmap, where we start allocating
(and freeing) the entries individually.