Crashes while trying to show tag objects with bad timestamps

When messing around with various repositories, I noticed that git 1.8 (currently using 1.8.2.rc0.22.gb3600c3) has problems parsing tag objects that have invalid timestamps.

Times in tag objects appear to be kept as Unix timestamps, but I didn't
realize this at first, and ran something roughly equivalent to:
  git cat-file -p $tagname | git hash-object -w -t tag --stdin
creating a tag object the "tagger" line containing formatted time
instead of a Unix timestamp.

Git doesn't handle the resulting tag objects nicely at all. For example, running `git cat-file -p` on the new object outputs a really odd timestamp "Thu Jun Thu Jan 1 00:16:09 1970 +0016" (I'm guessing it parses the year as Unix time), and `git show` outright crashes (backtrace included below.)

I would have expected both commands to print a "tag object corrupt" message, or maybe even a more specific "bad timestamp in tagger line"...

To reproduce:

printf '%s\n' \
  'object 4b825dc642cb6eb9a060e54bf8d69288fbee4904' 'type tree' \
  'tag test' 'tagger User <user@none> Thu Jun 9 16:44:04 2005 +0000' \
  '' 'Test tag' | git hash-object -w -t tag --stdin | xargs git show

On Sat, Feb 23, 2013 at 12:30:28AM +0200, Mantas Mikulėnas wrote:

Thanks, that makes it easy to replicate. It looks like it is not just tags, but rather the pp_user_info function does not realize that split_ident may return NULL for the date field if it is unparseable. Something like this stops the crash and just gives a bogus date:

Jeff King <peff@peff.net> writes:

Yes for the warning, and no for disappointing. IIRC, in the very early implementations allowed tag object without dates.

I _think_ we can start tightening fsck, though.

On Fri, Feb 22, 2013 at 02:53:48PM -0800, Junio C Hamano wrote:

Unfortunately, a good warning is harder than I had hoped. At the point where we notice the problem, pp_user_info, we have very little context. We can say only something like:

  warning: malformed date in ident 'Jeff King <peff@peff.net> BOGUS'

but we cannot say in which object, or even that it was a "tagger" line (and in some cases we do not even have an object, as in make_cover_letter).

Then I think it would make sense to allow the very specific no-date tag, but not allow arbitrary crud. I wonder if there's an example in the kernel or in git.git.

I also took a look at parsing routine of "cat-file -p". It's totally hand-rolled, separate from what "git show" does, and is not build on the pretty-print code at all. I wonder, though, if it actually makes sense to munge the date there. The commit-object pretty-printer for cat-file just shows the object intact. It seems weirdly inconsistent that we would munge tags just to rewrite the date. If you want a real pretty-printer, you should be using porcelain like "show".

It would be a regression, of course, for people relying on "cat-file -p" to have consistent output. But I am very tempted to call it a bug, and tempted to call "cat-file -p" inside a script a bad thing (you cannot, after all, tell what object type you have; you should figure out the type you expect and then use "cat-file <type> <obj>" to make sure you get the right one).

-Peff

On Sat, Feb 23, 2013 at 1:04 AM, Jeff King <peff@peff.net> wrote:

I couldn't find any such examples. However, I did find several tags with no "tagger" line at all: git.git has "v0.99" and linux.git has many such tags starting with "v2.6.11" ending with "v2.6.13-rc3".

It seems that `git cat-file -p` doesn't like such tags too – if there is no "tagger", it doesn't display *any* header lines. More bugs?

-- Mantas Mikulėnas

On Sat, Feb 23, 2013 at 01:14:40AM +0200, Mantas Mikulėnas wrote:

Yes, I think Junio was mis-remembering the exact condition. It looks like we added tagger lines in c818566 ([PATCH] Update tags to record who made them, 2005-07-14), which pulls the identity straight from "git var GIT_COMMITTER_IDENT". I double-checked to be sure that we included the date stamp at that time, and we did.

When parsing such a tag, we put a "0" in the date field of the "struct tag", and I suspect that is what caused the memory confusion.

So I think we are fine to fsck tagger lines as we do ordinary author/committer ident lines; the only exception is that we should not complain if they do not exist.

Yeah, I think we should just rid of that parser entirely. It is very inconsistent with the pretty-printer used by "git show", as well as the one used by "git for-each-ref", not to mention parse_tag (ugh, how many tag parsers do we have?).

-Peff

Jeff King <peff@peff.net> writes:

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.

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 ;-)

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

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).

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

The split_ident_line function is used by many code paths to find the name, mail, and date fields of an identity line. It will return failure when the output is completely unparseable, but may return success (along with a NULL date field) if the date is empty or malformed. Callers that care about the date need to handle this situation explicitly, or they will end up segfaulting as they feed the NULL date to strtoul or similar.

There are basically three options:

  1. Reject the ident line entirely (i.e., do the same thing
     we would do if the name or email were missing). We
     already use this strategy in git-commit's
     determine_author_info.

  2. Process the name and email, but refuse to work on any
     date portions. format_person_part does this, and a
     commit with such an ident would yield an empty string
     for "--format=%at".

  3. Proceed with some obviously bogus sentinel value for
     the time (e.g., the start of the epoch).

Only two of the existing callers read the date but do not handle this case at all: pp_user_info and git-blame's get_ac_line. This patch modifies both to use option (3). The hope is that this is friendly (we still produce some output) but the epoch start date will give the user a clue that the date is not valid.

Signed-off-by: Jeff King <peff@peff.net>
---
Having written that, I feel like the case for doing (3) is somewhat
flimsy. I don't know that it really matters that much either way, but
I'd be just as happy to do any of the others.

In fact, I really wonder if split_ident_line shouldn't simply be returning error in such a case. The comment above it claims that reflogs do not have such a timestamp, but they do. I suspect it is a weird interaction of the reflog walker on format_person_part, but I haven't checked. I wonder if we can simply fix the reflog code to pass a string with the date in the usual way (since we _should_ have it at some point). If not, then we can perhaps make things safer for other callers with a wrapper like:

  /* safe default version */
  int split_ident_line(...)
  {
          /* calls less safe but more flexible version; i.e., the
           * existing one */
          if (split_ident_line_date_optional(...) < 0)
                  return -1;
          if (!ident.date_begin)
                  return -1;
          return 0;
  }

 builtin/blame.c | 13 +++++++++----
 pretty.c        | 15 ++++++++++++---
 2 files changed, 21 insertions(+), 7 deletions(-)

The const rules in C are such that one cannot write a function that takes a const or non-const pointer and returns a pointer that matches the input in const-ness. Instead, you must take a const pointer (because you are promising not to modify it), and then either return a const pointer (which is safer, but annoying to callers who originally had a non-const pointer) or a non-const pointer (less safe, as you may accidentally drop constness, but less annoying).

This is a well-known problem, and the standard string functions like strchr take the "less annoying" approach. Let's mimic them. Even though this is technically less safe, skip_prefix tends to be used alongside standard string manipulation functions already, so it is not really introducing a new problem.

Signed-off-by: Jeff King <peff@peff.net>
---
I have mixed feelings on this. It _is_ less safe, and this is a known
bug in the C standard. Still, it seems like the more idiomatic C thing
to do.

My main motivation is to avoid a bunch of casts in the next patch.

 builtin/commit.c  | 2 +-
 git-compat-util.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

The fsck_tag function does not check very much about tags at all; it just makes sure that we were able to load the pointed-to object during the parse_tag phase. This does check some basic things (the "object" line is OK, and the pointed-to object exists with the expected type).

We did not, however, check the "tagger" line at all; if they exist, we should feed them to fsck_ident (and it is OK if they do not, as early versions of git did not include them).

This patch runs through the whole tag object during fsck_tag, similar to what we do in fsck_commit. Some of these checks are technically redundant with just checking that parse_tag filled in the "tag->tagged" field. However:

  1. We have to parse through those lines anyway to get to
     the tagger line, so we need to sanity check our
     parsing.

  2. We can give more specific errors (e.g., report a
     malformed "object" line).

  3. Previously we depended on implementation details of
     parse_tag for our fsck (e.g., that it would never fill
     in "tagged" if the types did not match). Now our
     exhaustive checks are in one place, which makes it
     easier to verify exactly what fsck is checking.

Signed-off-by: Jeff King <peff@peff.net>
---
Unfortunately, this causes t1050 to fail in an interesting way. It runs
"index-pack --strict" while setting GIT_DIR=nonexistent. As a result,
when we try to read the tag object from disk, we can't find it. We
don't run into the same problem verifying commits and trees, because
those objects leave the raw object data in their "buffer" field.

I'm tempted to call what that test is doing insane, but I wonder if there is another corner case with running "index-pack --strict" as part of an incoming push or fetch. I haven't investigated that yet.

 fsck.c          | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 t/t1450-fsck.sh | 43 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 1 deletion(-)

When "cat-file -p" prints commits, it shows them in their raw format, since git's format is already human-readable. For tags, however, we print the whole thing raw except for one thing: we convert the timestamp on the tagger line into a human-readable date.

This dates all the way back to a0f15fa (Pretty-print tagger dates, 2006-03-01). At that time there was no way to pretty-print a tag. These days "git show" does this already, and is the normal tool for showing a pretty-printed output ("cat-file tag $tag" remains the preferred method for showing porcelain output).

Let's drop this. It makes us more consistent with cat-file's commit pretty-printer, and it means we can drop a whole bunch of hand-rolled tag parsing code (which happened to behave inconsistently with the tag pretty-printing code elsewhere).

Note that "git verify-tag" and "git tag -v" depend on "cat-file -p" to show the tag. This means they will start showing the raw timestamp. We may want to adjust them to use the pretty-printing code from "git show".

Signed-off-by: Jeff King <peff@peff.net>
---
I don't use "git tag -v" much, so I'm not sure what is sane there. But
this seems like it would be a regression for people who want to check
the human-readable date given by GPG against the date in the tag object.

I still think dropping this hand-rolled parsing is a good thing. The most sane thing to me would be to move the parsing from "git show" into the pretty-print code, then have both it and "verify-tag" use it. Probably "for-each-ref" could stand to use it as well, as it has its own home-grown parser.

 builtin/cat-file.c  | 71 -----------------------------------------------------
 t/t1006-cat-file.sh |  5 +---
 2 files changed, 1 insertion(+), 75 deletions(-)

On Mon, Feb 25, 2013 at 8:50 PM, Jeff King <peff@peff.net> wrote:

Personally, I've found it quite confusing that commits (incl. merged tags) can be verified with `git show --show-signature`, but for tags I must use `git tag -v`... took me a while to find the latter.

(`git show --verify` might be even better, but that's just me.)

When using "hash-object -w" to create non-blob objects, it is generally a good policy to run "git fsck" afterward to make sure the resulting object is valid. Add a warning to the manpage.

While it at, gently nudge the user of "hash-object -w" toward higher-level interfaces for creating or modifying trees, commits, and tags.

Reported-by: Mantas Mikulėnas <grawity@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi Mantas,

Mantas Mikulėnas wrote:

[...]

The usual rule is that with invalid objects (e.g. as detected by "git fsck"), any non-crash result is acceptable. Garbage in, garbage out.

Probably worth fixing.

I notice that git-hash-object(1) doesn't contain any reference to git-fsck(1). How about something like this, to start?

Perhaps by default hash-object should automatically fsck the objects it is asked to create.

Thanks, Jonathan

 Documentation/git-hash-object.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

Jonathan Nieder <jrnieder@gmail.com> writes:

Yes, and let the experimentors to override when they are trying to invent a new object type, finished a reader but not a writer (that is why they are exprimenting with hash-object) nor updated fsck, with an explicit command line option to "hash-objects".

Then we do not have to say "-w by default can create an invalid object" in its documentation. In a sense, allowing to create any garbage (by the definition of then-current fsck and the rest of the Git) is the raison d'etre of the command.

Thanks.

On Fri, Feb 22, 2013 at 03:01:32PM -0800, Jonathan Nieder wrote:

Agreed, though I think a more consistent garbage would be good (e.g., time=0, tz=0).

I think it's a good change. Though note that this problem is not discovered by fsck (which I think we should also change).

Not unreasonable. In this case, we also have git-mktag. It would be nice if we could simply run the input through a type-specific sanity checker (optional, I hope; I use hash-object often to craft test cases like this :) ). The same need came up a month or two ago in a discussion of how to use "git replace" safely. But I guess fsck after-the-fact is just another form of the same solution.

-Peff