Re: git log - crash and core dump

From: Jeff King <peff@peff.net>
Date: Apr 17, 2013, 06:39 UTC
Message-ID: <20130417063942.GA27703@sigill.intra.peff.net>
In-Reply-To: <7vli8hhgmn.fsf@alter.siamese.dyndns.org>

On Tue, Apr 16, 2013 at 10:26:40PM -0700, Junio C Hamano wrote:

Show 22 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> >
> >> How about making split_ident_line() a bit friendlier be letting it
> >> provide the epoch as default time stamp instead of NULL?  
> >
> > Two knee-jerk concerns I have without going back to the callers:
> >
> >  * Would that "0" ever be given to the approxidate parser, which
> >    rejects ancient dates in numbers-since-epoch format without @
> >    prefix?
> >
> >  * Does any existing caller use the NULL as a sign to see the input
> >    was without date and act on that information?
> 
> I looked at all the callers (there aren't that many), and none of
> them did "Do this on a person-only ident, and do that on an ident
> with timestamp".  So for the callers that ignore timestamp, your
> patch will be a no-op, and for others that assume there is a
> timestamp, it will turn a crash/segv into output with funny
> timestamp.

What about sane_ident_split in builtin/commit.c? It explicitly rejects a NULL date. The logic in determine_author_info is a little hard to follow (it assembles the ident line with fmt_ident and then reparses it), but I believe it should be catching a bogus line from "commit -c", or from GIT_AUTHOR_DATE in the environment.

As a side note, when determine_author_info sees a bogus ident, it appears to just silently ignore it, which is probably a bad thing. Shouldn't we by complaining? Or am I mis-reading the code?

-Peff