Re: What's cooking in git.git (Sep 2016, #03; Fri, 9)

From: Jeff King <peff@peff.net>
Date: Sep 12, 2016, 21:46 UTC
Message-ID: <20160912214611.wz6uduf33os3iprq@sigill.intra.peff.net>
In-Reply-To: <xmqqlgyx2aui.fsf@gitster.mtv.corp.google.com>

On Mon, Sep 12, 2016 at 12:10:13PM -0700, Junio C Hamano wrote:

Show 15 quoted lines

> Jeff King <peff@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.

-Peff