Re: Crashes while trying to show tag objects with bad timestamps

From: Jeff King <peff@peff.net>
Date: Feb 25, 2013, 18:30 UTC
Message-ID: <20130225183009.GB13912@sigill.intra.peff.net>
In-Reply-To: <7vppzsaqc5.fsf@alter.siamese.dyndns.org>

On Fri, Feb 22, 2013 at 03:20:10PM -0800, Junio C Hamano wrote:

Show 13 quoted lines

> As pp_user_info() is called from very few places, I do not think it
> is unreasonable to add an output parameter (i.e. "unsigned *") to
> let the caller know that we made a best guess given malformed input
> and handle the error in the caller.  The make_cover_letter() caller
> may look like:
> 
> 	pp_user_info(&pp, NULL, &sb, committer, encoding, &errors);
>         if (errors & PP_CORRUPT_DATE)
> 		warning("unparsable datestamp in '%s'", committer);
> 
> although it is unlikely to see this error in practice, given that
> committer is coming from git_committer_info(0) and would have the
> current timestamp.

Sadly that is not quite enough for the object-parsing cases (which are the ones we _really_ want to add context to, because they are buried inside other pp_* calls. Probably adding an object context field (or an error return) to the pretty-print context would make sense. But I don't relish the thought of annotating each pretty-print caller.

I think we're OK to be silent and just react in an appropriate way; having looked over the other callers of split_ident_line, we already do so in some places. See my patch 1 below for details.

Once fsck is taught to note this, then the warning is a lot less important (my patch 3 below).

> The whole "cat-file -p" is a historical wart, aka poor-man's
> "show".  I do not even consider it a part of the plumbing.  It is a
> fair game for Porcelainisque improvement ;-)

Good, that's how I feel, too. See my patch 4. :)

Here are the patches I'd like to do:

  [1/4]: handle malformed dates in ident lines
  [2/4]: skip_prefix: return a non-const pointer
  [3/4]: fsck: check "tagger" lines
  [4/4]: cat-file: print tags raw for "cat-file -p"

The first one is solid, and should probably go to maint and/or the -rc track, as it fixes a segfault on bogus input. It's hopefully a no-brainer, as the existing behavior is obviously unacceptable. We may change our mind later about exactly what to print for such bogus input, but whatever we print in such a case is just trying to be nice to the user, and anybody who depends on our particular handling of malformed objects is crazy.

The rest can wait, as they are about improving output when fed bogus input, or tightening fsck. Moreover, they have some problems which make them not suitable for applying yet. I'll give details in each patch.

-Peff