Re: [PATCH] blame.c: fix garbled error message

From: Jeff King <peff@peff.net>
Date: Jan 12, 2015, 20:40 UTC
Message-ID: <20150112204025.GB9686@peff.net>
In-Reply-To: <xmqq4mrv95qt.fsf@gitster.dls.corp.google.com>

On Mon, Jan 12, 2015 at 11:08:26AM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> Lukas Fleischer <git@cryptocrack.de> writes:
> 
> > The helper functions prepare_final() and prepare_initial() return a
> > pointer to a string that is a member of an object in the revs->pending
> > array. This array is later rebuilt when running prepare_revision_walk()
> > which potentially transforms the pointer target into a bogus string. Fix
> > this by maintaining a copy of the original string.
> >
> > Signed-off-by: Lukas Fleischer <git@cryptocrack.de>
> > ---
> > The bug manifests when running `git blame HEAD^ -- nonexistent.file`.
> 
> Before 1da1e07c (clean up name allocation in prepare_revision_walk,
> 2014-10-15), these strings used to be non-volatile; they were instead
> leaked more or less deliberately.  But these days, these strings are
> cleared, so your patch is absolutely the right thing to do.
> 
> Thanks for catching and fixing.  This fix needs to go to the 2.2.x
> maintenance track.

Yeah, agreed. Sorry for not catching this as part of 1da1e07c.

I did a grep for 'pending.*name' to look at any other potential problem sites. It looks like blame is the only one that tries to retain a long-lived pointer to the name.

The other potentially interesting spot is that they are fed to the object callbacks from traverse_commit_list for tags. However, none of the callbacks saves it (and it would not make much sense to do so; they also receive broken-down filenames in the same way, so if they want to use it at all, they feed it through path_name() first, which makes a copy).

So I think Lukas's patch fixes everything (and his positioning of the strdup() calls is right where I would have put them).

-Peff