Crashes while trying to show tag objects with bad timestamps

Crashes while trying to show tag objects with bad timestamps

[Mantas Mikulėnas]

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


> #0  0x00007f42560bb5f3 in ____strtoull_l_internal () from /usr/lib/libc.so.6
> No symbol table info available.
> #1  0x00000000004b4c81 in pp_user_info (pp=pp@entry=0x7fff3c30f1a0, 
>     what=what@entry=0x50c13b "Tagger", sb=sb@entry=0x7fff3c30f140, 
>     line=0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", encoding=0x507e20 "UTF-8") at pretty.c:431
>         name = {alloc = 24, len = 15, buf = 0xc24690 "Jilles Tjoelker"}
>         mail = {alloc = 24, len = 15, buf = 0xc24750 "jilles@stack.nl"}
>         ident = {
>           name_begin = 0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", 
>           name_end = 0xc267d6 " <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", 
>           mail_begin = 0xc267d8 "jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", mail_end = 0xc267e7 "> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n", 
>           date_begin = 0x0, date_end = 0x0, tz_begin = 0x0, tz_end = 0x0}
>         linelen = <optimized out>
>         line_end = <optimized out>
>         date = <optimized out>
>         mailbuf = 0xc267d8 "jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
>         namebuf = 0xc267c7 "Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
>         namelen = 33
>         maillen = 15
>         max_length = 78
>         time = <optimized out>
>         tz = <optimized out>
> #2  0x0000000000439af5 in show_tagger (buf=<optimized out>, len=<optimized out>, 
>     rev=<optimized out>) at builtin/log.c:400
>         pp = {fmt = CMIT_FMT_MEDIUM, abbrev = 0, subject = 0x0, after_subject = 0x0, 
>           preserve_subject = 0, date_mode = DATE_NORMAL, date_mode_explicit = 0, 
>           need_8bit_cte = 0, notes_message = 0x0, reflog_info = 0x0, 
>           output_encoding = 0x0, mailmap = 0x0, color = 0}
>         out = {alloc = 0, len = 0, buf = 0x7a8188 <strbuf_slopbuf> ""}
> #3  show_tag_object (rev=0x7fff3c30f1f0, 
>     sha1=0xc2be44 "\230\211\275\331\365Q\306z\017\071d\331\035\062\247a\347~M8P", <incomplete sequence \303>) at builtin/log.c:427
>         new_offset = 151
>         type = OBJ_TAG
>         buf = 0xc26770 "object ffa28d13e40e03bd367d0219c7eb516be0f180d2\ntype commit\ntag hyperion-1.0rc1\ntagger Jilles Tjoelker <jilles@stack.nl> Thu Jun 9 16:44:04 2005 +0000\n\nTag 1.0rc1.\n\n"
>         size = 165
>         offset = <optimized out>


-- 
Mantas Mikulėnas <grawity@gmail.com>

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

[Jeff King]

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

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

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:

diff --git a/pretty.c b/pretty.c
index eae57ad..9688857 100644
--- a/pretty.c
+++ b/pretty.c
@@ -428,8 +428,16 @@ void pp_user_info(const struct pretty_print_context *pp,
 	strbuf_add(&name, namebuf, namelen);
 
 	namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
-	time = strtoul(ident.date_begin, &date, 10);
-	tz = strtol(date, NULL, 10);
+
+	if (ident.date_begin) {
+		time = strtoul(ident.date_begin, &date, 10);
+		tz = strtol(date, NULL, 10);
+	}
+	else {
+		/* ident line had malformed date */
+		time = 0;
+		tz = 0;
+	}
 
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		strbuf_addstr(sb, "From: ");

I guess we should probably issue a warning, too. Also disappointingly,
git-fsck does not seem to detect this breakage at all.

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

If "cat-file -p" is not using the usual pretty-print routines, it
probably should. I'll take a look.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I guess we should probably issue a warning, too. Also disappointingly,
> git-fsck does not seem to detect this breakage at all.

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.

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

[Jeff King]

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

> > I guess we should probably issue a warning, too. Also disappointingly,
> > git-fsck does not seem to detect this breakage at all.
> 
> Yes for the warning, 

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

> and no for disappointing.  IIRC, in the very early implementations
> allowed tag object without dates.
> 
> I _think_ we can start tightening fsck, though.

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

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

[Mantas Mikulėnas]

On Sat, Feb 23, 2013 at 1:04 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 22, 2013 at 02:53:48PM -0800, Junio C Hamano wrote:
>> and no for disappointing.  IIRC, in the very early implementations
>> allowed tag object without dates.
>>
>> I _think_ we can start tightening fsck, though.
>
> 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 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

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

[Jeff King]

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

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

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.

> 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?

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Feb 22, 2013 at 02:53:48PM -0800, Junio C Hamano wrote:
>
>> > I guess we should probably issue a warning, too. Also disappointingly,
>> > git-fsck does not seem to detect this breakage at all.
>> 
>> Yes for the warning, 
>
> 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).

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.

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

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

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

[Jeff King]

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

> 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

[PATCH 1/4] handle malformed dates in ident lines

[Jeff King]

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

diff --git a/builtin/blame.c b/builtin/blame.c
index 86100e9..2edff25 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1375,10 +1375,15 @@ static void get_ac_line(const char *inbuf, const char *what,
 	maillen = ident.mail_end - ident.mail_begin;
 	mailbuf = ident.mail_begin;
 
-	*time = strtoul(ident.date_begin, NULL, 10);
-
-	len = ident.tz_end - ident.tz_begin;
-	strbuf_add(tz, ident.tz_begin, len);
+	if (ident.date_begin) {
+		*time = strtoul(ident.date_begin, NULL, 10);
+		len = ident.tz_end - ident.tz_begin;
+		strbuf_add(tz, ident.tz_begin, len);
+	}
+	else {
+		*time = 0;
+		strbuf_addstr(tz, "-0000");
+	}
 
 	/*
 	 * Now, convert both name and e-mail using mailmap
diff --git a/pretty.c b/pretty.c
index eae57ad..4b19908 100644
--- a/pretty.c
+++ b/pretty.c
@@ -391,7 +391,7 @@ void pp_user_info(const struct pretty_print_context *pp,
 	struct strbuf mail;
 	struct ident_split ident;
 	int linelen;
-	char *line_end, *date;
+	char *line_end;
 	const char *mailbuf, *namebuf;
 	size_t namelen, maillen;
 	int max_length = 78; /* per rfc2822 */
@@ -428,8 +428,17 @@ void pp_user_info(const struct pretty_print_context *pp,
 	strbuf_add(&name, namebuf, namelen);
 
 	namelen = name.len + mail.len + 3; /* ' ' + '<' + '>' */
-	time = strtoul(ident.date_begin, &date, 10);
-	tz = strtol(date, NULL, 10);
+
+	if (ident.date_begin) {
+		char *date;
+		time = strtoul(ident.date_begin, &date, 10);
+		tz = strtol(date, NULL, 10);
+	}
+	else {
+		/* ident has missing or malformed date */
+		time = 0;
+		tz = 0;
+	}
 
 	if (pp->fmt == CMIT_FMT_EMAIL) {
 		strbuf_addstr(sb, "From: ");
-- 
1.8.1.4.4.g265d2fa

[PATCH/RFC 2/4] skip_prefix: return a non-const pointer

[Jeff King]

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 3348aa1..bb6890b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -895,7 +895,7 @@ static int template_untouched(struct strbuf *sb)
 		return 0;
 
 	stripspace(&tmpl, cleanup_mode == CLEANUP_ALL);
-	start = (char *)skip_prefix(sb->buf, tmpl.buf);
+	start = skip_prefix(sb->buf, tmpl.buf);
 	if (!start)
 		start = sb->buf;
 	strbuf_release(&tmpl);
diff --git a/git-compat-util.h b/git-compat-util.h
index b7eaaa9..56c066b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -320,10 +320,10 @@ static inline const char *skip_prefix(const char *str, const char *prefix)
 extern int prefixcmp(const char *str, const char *prefix);
 extern int suffixcmp(const char *str, const char *suffix);
 
-static inline const char *skip_prefix(const char *str, const char *prefix)
+static inline char *skip_prefix(const char *str, const char *prefix)
 {
 	size_t len = strlen(prefix);
-	return strncmp(str, prefix, len) ? NULL : str + len;
+	return strncmp(str, prefix, len) ? NULL : (char *)(str + len);
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
1.8.1.4.4.g265d2fa

[PATCH 3/4] fsck: check "tagger" lines

[Jeff King]

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

diff --git a/fsck.c b/fsck.c
index 99c0497..20d55c4 100644
--- a/fsck.c
+++ b/fsck.c
@@ -340,15 +340,75 @@ static int fsck_tag(struct tag *tag, fsck_error error_func)
 	return 0;
 }
 
-static int fsck_tag(struct tag *tag, fsck_error error_func)
+static int fsck_tag_buffer(char *buf, struct tag *tag, fsck_error error_func)
 {
 	struct object *tagged = tag->tagged;
+	unsigned char sha1[20];
+	char *eol;
+
+	buf = skip_prefix(buf, "object ");
+	if (!buf)
+		return error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'object' line");
+	if (get_sha1_hex(buf, sha1) || buf[40] != '\n')
+		return error_func(&tag->object, FSCK_ERROR, "invalid 'object' line format - bad sha1");
+	buf += 41;
 
+	/*
+	 * We already called parse_tag, so we don't have to bother looking up
+	 * the sha1 again.
+	 */
 	if (!tagged)
 		return error_func(&tag->object, FSCK_ERROR, "could not load tagged object");
+
+	buf = skip_prefix(buf, "type ");
+	if (!buf)
+		return error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'type' line");
+	eol = strchr(buf, '\n');
+	if (!eol)
+		return error_func(&tag->object, FSCK_ERROR, "invalid format - truncation at 'type' line");
+	*eol = '\0';
+	if (type_from_string(buf) != tagged->type)
+		return error_func(&tag->object, FSCK_ERROR, "'type' line does not match type of tagged object");
+	*eol = '\n';
+
+	buf = skip_prefix(eol + 1, "tag ");
+	if (!buf)
+		return error_func(&tag->object, FSCK_ERROR, "invalid format - expected 'tag' line");
+	eol = strchr(buf, '\n');
+	if (!eol)
+		return error_func(&tag->object, FSCK_ERROR, "invalid format - truncation at 'tag' line");
+
+	/*
+	 * A missing tagger is OK, as very old versions of git did not produce
+	 * such a line. But if we do have it, we should verify its contents.
+	 */
+	buf = skip_prefix(eol + 1, "tagger ");
+	if (buf) {
+		int err = fsck_ident(&buf, &tag->object, error_func);
+		if (err)
+			return err;
+	}
+
 	return 0;
 }
 
+static int fsck_tag(struct tag *tag, fsck_error error_func)
+{
+	char *buf;
+	unsigned long size;
+	enum object_type type;
+	int err;
+
+	buf = read_sha1_file(tag->object.sha1, &type, &size);
+	if (!buf)
+		return error_func(&tag->object, FSCK_ERROR, "could not read tag object");
+
+	err = fsck_tag_buffer(buf, tag, error_func);
+
+	free(buf);
+	return err;
+}
+
 int fsck_object(struct object *obj, int strict, fsck_error error_func)
 {
 	if (!obj)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index d730734..3a3bce6 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -180,6 +180,49 @@ test_expect_success 'tag pointing to something else than its type' '
 	test_must_fail git fsck --tags
 '
 
+test_expect_success 'tag with missing tagger is OK' '
+	sha=$(echo blob | git hash-object -w --stdin) &&
+	test_when_finished "remove_object $sha" &&
+	cat >good-tag <<-EOF &&
+	object $sha
+	type blob
+	tag missing-tagger-ok
+
+	This is totally fine.
+	EOF
+
+	tag=$(git hash-object -t tag -w --stdin <good-tag) &&
+	test_when_finished "remove_object $tag" &&
+	git update-ref refs/tags/good $tag &&
+	test_when_finished "git update-ref -d refs/tags/good" &&
+	git fsck >out 2>&1 &&
+	>expect &&
+	test_cmp expect out
+'
+
+test_expect_success 'tag with bogus tagger is not OK' '
+	sha=$(echo blob | git hash-object -w --stdin) &&
+	test_when_finished "remove_object $sha" &&
+	cat >wrong-tag <<-EOF &&
+	object $sha
+	type blob
+	tag bogus-tagger
+	tagger T A Gger <tagger@example.com> Mon Feb 25 12:32:51 2013 -0500
+
+	That date is bogus (it should be an epoch + timezone). Any bogus ident
+	line should trigger, but this was chosen to match a breakage seen in
+	the wild.
+	EOF
+
+	tag=$(git hash-object -t tag -w --stdin <wrong-tag) &&
+	test_when_finished "remove_object $tag" &&
+	git update-ref refs/tags/wrong $tag &&
+	test_when_finished "git update-ref -d refs/tags/wrong" &&
+	git fsck --tags >out 2>&1 &&
+	cat out &&
+	grep "error in tag $tag.* - bad date" out
+'
+
 test_expect_success 'cleaned up' '
 	git fsck >actual 2>&1 &&
 	test_cmp empty actual
-- 
1.8.1.4.4.g265d2fa

[PATCH 4/4] cat-file: print tags raw for "cat-file -p"

[Jeff King]

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

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 00528dd..b195edf 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -16,73 +16,6 @@
 #define BATCH 1
 #define BATCH_CHECK 2
 
-static void pprint_tag(const unsigned char *sha1, const char *buf, unsigned long size)
-{
-	/* the parser in tag.c is useless here. */
-	const char *endp = buf + size;
-	const char *cp = buf;
-
-	while (cp < endp) {
-		char c = *cp++;
-		if (c != '\n')
-			continue;
-		if (7 <= endp - cp && !memcmp("tagger ", cp, 7)) {
-			const char *tagger = cp;
-
-			/* Found the tagger line.  Copy out the contents
-			 * of the buffer so far.
-			 */
-			write_or_die(1, buf, cp - buf);
-
-			/*
-			 * Do something intelligent, like pretty-printing
-			 * the date.
-			 */
-			while (cp < endp) {
-				if (*cp++ == '\n') {
-					/* tagger to cp is a line
-					 * that has ident and time.
-					 */
-					const char *sp = tagger;
-					char *ep;
-					unsigned long date;
-					long tz;
-					while (sp < cp && *sp != '>')
-						sp++;
-					if (sp == cp) {
-						/* give up */
-						write_or_die(1, tagger,
-							     cp - tagger);
-						break;
-					}
-					while (sp < cp &&
-					       !('0' <= *sp && *sp <= '9'))
-						sp++;
-					write_or_die(1, tagger, sp - tagger);
-					date = strtoul(sp, &ep, 10);
-					tz = strtol(ep, NULL, 10);
-					sp = show_date(date, tz, 0);
-					write_or_die(1, sp, strlen(sp));
-					xwrite(1, "\n", 1);
-					break;
-				}
-			}
-			break;
-		}
-		if (cp < endp && *cp == '\n')
-			/* end of header */
-			break;
-	}
-	/* At this point, we have copied out the header up to the end of
-	 * the tagger line and cp points at one past \n.  It could be the
-	 * next header line after the tagger line, or it could be another
-	 * \n that marks the end of the headers.  We need to copy out the
-	 * remainder as is.
-	 */
-	if (cp < endp)
-		write_or_die(1, cp, endp - cp);
-}
-
 static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 {
 	unsigned char sha1[20];
@@ -133,10 +66,6 @@ static int cat_one_file(int opt, const char *exp_type, const char *obj_name)
 		buf = read_sha1_file(sha1, &type, &size);
 		if (!buf)
 			die("Cannot read object %s", obj_name);
-		if (type == OBJ_TAG) {
-			pprint_tag(sha1, buf, size);
-			return 0;
-		}
 
 		/* otherwise just spit out the data */
 		break;
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index d8b7f2f..da4ffbb 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -135,14 +135,11 @@ tag_size=$(strlen "$tag_content")
 tag_content="$tag_header_without_timestamp 0000000000 +0000
 
 $tag_description"
-tag_pretty_content="$tag_header_without_timestamp Thu Jan 1 00:00:00 1970 +0000
-
-$tag_description"
 
 tag_sha1=$(echo_without_newline "$tag_content" | git mktag)
 tag_size=$(strlen "$tag_content")
 
-run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_pretty_content" 1
+run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content" 1
 
 test_expect_success \
     "Reach a blob from a tag pointing to it" \
-- 
1.8.1.4.4.g265d2fa

Re: [PATCH 4/4] cat-file: print tags raw for "cat-file -p"

[Mantas Mikulėnas]

On Mon, Feb 25, 2013 at 8:50 PM, Jeff King <peff@peff.net> wrote:
> 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.

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

-- 
Mantas Mikulėnas <grawity@gmail.com>

[RFC/PATCH] hash-object doc: "git hash-object -w" can write invalid objects

[Jonathan Nieder]

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:

> 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.
[...]
> 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),

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.

>                                and `git show` outright crashes
> (backtrace included below.)

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(+)

diff --git a/Documentation/git-hash-object.txt b/Documentation/git-hash-object.txt
index 02c1f12..8ed8c6e 100644
--- a/Documentation/git-hash-object.txt
+++ b/Documentation/git-hash-object.txt
@@ -30,6 +30,8 @@ OPTIONS
 
 -w::
 	Actually write the object into the object database.
+	This does not check that the resulting object is valid;
+	for that, see linkgit:git-fsck[1].
 
 --stdin::
 	Read the object from standard input instead of from a file.
@@ -53,6 +55,14 @@ OPTIONS
 	conversion. If the file is read from standard input then this
 	is always implied, unless the --path option is given.
 
+SEE ALSO
+--------
+linkgit:git-mktree[1],
+linkgit:git-commit-tree[1],
+linkgit:git-tag[1],
+linkgit:git-filter-branch[1],
+sha1sum(1)
+
 GIT
 ---
 Part of the linkgit:git[1] suite
-- 
1.8.1.4

Re: [RFC/PATCH] hash-object doc: "git hash-object -w" can write invalid objects

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

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

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.

Re: [RFC/PATCH] hash-object doc: "git hash-object -w" can write invalid objects

[Jeff King]

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

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

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

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

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

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

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