[PATCH] mktag.c: improve verification of tagger field and tests

[PATCH] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Since nearly its birth, git's tags have included a "tagger" field which
describe the name of tagger, email of tagger, and date and time of tagging.
But, this field was only loosely tested by git-mktag. Provide some thorough
testing for this field and also ensure that the tag header is separated
from the tag body by an empty line to reduce the convenience of creating
a flawed tag.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Well, since I looked at this code and used it in filter-branch, I figured I
should fix the verification code for the tagger field (even though it's
probably dieing soon).

I'm thinking this utility should be fairly strict about the format it accepts.

Some assumptions:
   -tagger field has form: A U Thor <author@example.com> SSSSSSSSS [[+]hhmm]
                           where the SSSS's are the time stamp and the time
                           zone is optional
   -author name must not be empty, but can be a single space
   -author email can be empty. i.e. '<>'
   -timestamp can have leading spaces
   -time zone can have leading spaces
   -no trailing spaces e.g. after the timezone field, or after the timestamp
    when timezone is not supplied

 Should leading spaces be accepted? The rest of git seems to handle them, I'm
 not sure if there was ever anything out there that would have created a tag
 with leading spaces. Possibly a fixed width printf of the timestamp?

 Should the timezone be optional? How about the prefixed +|-?

-brandon


 mktag.c          |   70 +++++++++++++++++++++++++----
 t/t3800-mktag.sh |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 185 insertions(+), 14 deletions(-)

diff --git a/mktag.c b/mktag.c
index b05260c..49f4ca1 100644
--- a/mktag.c
+++ b/mktag.c
@@ -8,10 +8,11 @@
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *
- * The first three lines are guaranteed to be at least 63 bytes:
+ * The first four lines are guaranteed to be at least 77 bytes:
  * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, and "tag .\n" at 6 bytes is the
- * shortest single-character-tag line.
+ * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
+ * single-character-tag line, and "tagger . <> 0\n" at 14 bytes is
+ * the shortest possible tagger-line.
  */
 
 /*
@@ -43,9 +44,9 @@ static int verify_tag(char *buffer, unsigned long size)
 	int typelen;
 	char type[20];
 	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line;
+	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
 
-	if (size < 64)
+	if (size < 78)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
 	buffer[size] = 0;
@@ -96,12 +97,63 @@ static int verify_tag(char *buffer, unsigned long size)
 
 	/* Verify the tagger line */
 	tagger_line = tag_line;
+	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+		return error("char" PD_FMT ": could not find \"tagger \"",
+			tagger_line - buffer);
+
+	/*
+	 * Check for correct form for name and email
+	 * i.e. " <" followed by "> " on _this_ line
+	 */
+	tagger_line += 7;
+	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb, "> ")) ||
+		strchr(tagger_line, '\n') < rb)
+		return error("char" PD_FMT ": malformed tagger",
+			tagger_line - buffer);
+
+	/* Check for author name, at least one character, space is acceptable */
+	if (lb == tagger_line)
+		return error("char" PD_FMT ": missing tagger name",
+			tagger_line - buffer);
+
+	/* timestamp */
+	tagger_line = rb + 2;
+	while (*tagger_line == ' ')
+		tagger_line++;
+	if (*tagger_line == '\n')
+		return error("char" PD_FMT ": missing tag timestamp",
+			tagger_line - buffer);
+	for (;;) {
+		unsigned char c = *tagger_line++;
+		if (c == ' ' || c == '\n')
+			break;
+		if (c >= '0' && c <= '9')
+			continue;
+		return error("char" PD_FMT ": malformed tag timestamp",
+			tagger_line - buffer);
+	}
 
-	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
-		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
+	/* optional timezone, 4 digits hhmm with optional leading +|- */
+	if (tagger_line[-1] != '\n') {
+		while (*tagger_line == ' ')
+			tagger_line++;
+		if ((*tagger_line == '+' || *tagger_line == '-') &&
+			tagger_line[1] != '\n')
+			tagger_line++;
+		if (!(tagger_line[0] >= '0' && tagger_line[0] <= '9' &&
+		      tagger_line[1] >= '0' && tagger_line[1] <= '9' &&
+		      tagger_line[2] >= '0' && tagger_line[2] <= '5' &&
+		      tagger_line[3] >= '0' && tagger_line[3] <= '9' &&
+		      tagger_line[4] == '\n'))
+			return error("char" PD_FMT ": malformed tag timezone",
+				tagger_line - buffer);
+		tagger_line += 5;
+	}
 
-	/* TODO: check for committer info + blank line? */
-	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
+	/* Verify the blank line separating the header from the body */
+	if (*tagger_line != '\n')
+		return error("char" PD_FMT ": trailing garbage in tag header",
+			tagger_line - buffer);
 
 	/* The actual stuff afterwards we don't care about.. */
 	return 0;
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index bdc6e13..0098389 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -44,6 +44,8 @@ cat >tag.sig <<EOF
 xxxxxx 139e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger . <> 0
+
 EOF
 
 check_verify_failure '"object" line label check' '^error: char0: .*"object "$'
@@ -55,6 +57,8 @@ cat >tag.sig <<EOF
 object zz9e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger . <> 0
+
 EOF
 
 check_verify_failure '"object" line SHA1 check' '^error: char7: .*SHA1 hash$'
@@ -66,6 +70,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 xxxx tag
 tag mytag
+tagger . <> 0
+
 EOF
 
 check_verify_failure '"type" line label check' '^error: char47: .*"\\ntype "$'
@@ -85,6 +91,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tag
 xxx mytag
+tagger . <> 0
+
 EOF
 
 check_verify_failure '"tag" line label check #1' \
@@ -121,6 +129,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tagggg
 tag mytag
+tagger . <> 0
+
 EOF
 
 check_verify_failure 'verify object (SHA1/type) check' \
@@ -133,6 +143,8 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag my	tag
+tagger . <> 0
+
 EOF
 
 check_verify_failure 'verify tag-name check' \
@@ -145,10 +157,12 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
+
+This is filler
 EOF
 
 check_verify_failure '"tagger" line label check #1' \
-	'^error: char70: could not find "tagger"$'
+	'^error: char70: could not find "tagger "$'
 
 ############################################################
 # 12. tagger line label check #2
@@ -158,19 +172,124 @@ object $head
 type commit
 tag mytag
 tagger
+
+This is filler
 EOF
 
 check_verify_failure '"tagger" line label check #2' \
-	'^error: char70: could not find "tagger"$'
+	'^error: char70: could not find "tagger "$'
 
 ############################################################
-# 13. create valid tag
+# 13. detect missing tag author name
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
-tagger another@example.com
+tagger  <> 0
+
+This is filler
+EOF
+
+check_verify_failure 'detect missing tag author name' \
+	'^error: char77: missing tagger name$'
+
+############################################################
+# 14. detect missing tag author name
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <
+ > 0
+
+EOF
+
+check_verify_failure 'detect malformed tagger' \
+	'^error: char77: malformed tagger$'
+
+############################################################
+# 15. allow empty tag email
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <> 0
+
+EOF
+
+test_expect_success \
+    'allow empty tag email' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 16. detect missing tag timestamp
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 
+
+EOF
+
+check_verify_failure 'detect missing tag timestamp' \
+	'^error: char107: missing tag timestamp$'
+
+############################################################
+# 17. detect invalid tag timestamp
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> Tue Mar 25 15:47:44 2008
+
+EOF
+
+check_verify_failure 'detect invalid tag timestamp' \
+	'^error: char108: malformed tag timestamp$'
+
+############################################################
+# 18. detect invalid tag timezone
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 GMT
+
+EOF
+
+check_verify_failure 'detect invalid tag timezone' \
+	'^error: char118: malformed tag timezone$'
+
+############################################################
+# 19. detect invalid header entry
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 -0500
+this line should not be here
+
+EOF
+
+check_verify_failure 'detect invalid header entry' \
+	'^error: char124: trailing garbage in tag header$'
+
+############################################################
+# 20. create valid tag
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 -0500
+
 EOF
 
 test_expect_success \
@@ -178,7 +297,7 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 14. check mytag
+# 21. check mytag
 
 test_expect_success \
     'check mytag' \
-- 
1.5.4.4.481.g5075

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Brandon Casey wrote:

>    -tagger field has form: A U Thor <author@example.com> SSSSSSSSS [[+]hhmm]
>                            where the SSSS's are the time stamp and the time
>                            zone is optional

Just to clarify, I'm not implying fixed width for the timestamp here. It's
1 or more digits.

-brandon

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Carlos Rica]

On Wed, Mar 26, 2008 at 1:40 AM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>  Well, since I looked at this code and used it in filter-branch, I figured I
>  should fix the verification code for the tagger field (even though it's
>  probably dieing soon).
>
>  I'm thinking this utility should be fairly strict about the format it accepts.

Why not using git-tag to make tags in filter-branch?

git-mktag was used in git-tag.sh before convert it into
builtin-tag.c, and I didn't know that anyone was using it.

I agree that, if this program exists and it is used,
we should double-check the accepted format and data,
so this patch is a good addition.

However, I think that we should progressively deprecate its
use to avoid mantaining two different ways for creating tags,
so you must have a very good reason to keep using
this tool in a script...

Regards

--
Carlos

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Carlos Rica wrote:
> On Wed, Mar 26, 2008 at 1:40 AM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>>  Well, since I looked at this code and used it in filter-branch, I figured I
>>  should fix the verification code for the tagger field (even though it's
>>  probably dieing soon).
>>
>>  I'm thinking this utility should be fairly strict about the format it accepts.
> 
> Why not using git-tag to make tags in filter-branch?

How?

With cat-file and mktag I don't have to parse the tag, I can ignore
nearly the entire thing and only change the parts I'm interested in.

With git-tag, wouldn't I have to parse the tag, splitting the header
from the body and redirect the body to a tmpfile, parse the tagger field,
and set GIT_COMMITTER_NAME, GIT_COMMITTER_EMAIL, GIT_COMMITTER_DATE
environment variables?

> git-mktag was used in git-tag.sh before convert it into
> builtin-tag.c, and I didn't know that anyone was using it.

I don't think anything other than filter-branch is (and the usage
there is only tentative).

> I agree that, if this program exists and it is used,
> we should double-check the accepted format and data,
> so this patch is a good addition.
> 
> However, I think that we should progressively deprecate its
> use to avoid mantaining two different ways for creating tags,
> so you must have a very good reason to keep using
> this tool in a script...

Eventually filter-branch along with the rest of git will be rewritten
in c and the need for many low-level git tools will vanish.

-brandon

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Brandon Casey wrote:
> Carlos Rica wrote:
>> On Wed, Mar 26, 2008 at 1:40 AM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>>>  Well, since I looked at this code and used it in filter-branch, I figured I
>>>  should fix the verification code for the tagger field (even though it's
>>>  probably dieing soon).
>>>
>>>  I'm thinking this utility should be fairly strict about the format it accepts.
>> Why not using git-tag to make tags in filter-branch?

I replied, and then I thought, maybe you didn't notice that patch I submitted
for filter-branch.

So, to clarify, I am not using mktag at a user-level. i.e. I am not using it in a
script that I supplied _to_ filter-branch. I added a call to it from _within_
filter-branch so that the "object" part of tag objects can be retained when filtering.

-brandon

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> Eventually filter-branch along with the rest of git will be rewritten
> in c and the need for many low-level git tools will vanish.

That holds true only for the tools shipped with git.git itself.  However,
you never should forget that people _script_ around git.  I do not see a
reason to stop supporting mktag in this discussion.

I would suggest:

 * make mktag.c a built-in first;

 * rename verify_tag() in mktag.c to verify_tag_buffer(), and update its
   implementation to tighten the format validation, perhaps along the
   lines you propose in your patch, and move it to tag.c.  By the way, I
   think tagger information should get the same validation as committer
   and author information gets elsewhere in the system;

 * add a call to verify_tag_buffer() you introduce above immediately
   before write_sha1_file() in builtin-tag.c, to make sure both programs
   produce valid tags, with the same definition of validity.

Re: [PATCH] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Junio C Hamano wrote:
> By the way, I
>    think tagger information should get the same validation as committer
>    and author information gets elsewhere in the system;

I agree. Do you have any pointers to suggest? I haven't seen any place in
the code that does thorough validation. Usually, these fields are generated
within git, or converted to the internal form using some dwim procedure.

I keyed on get_ac_line() in builtin-blame.c and force_author in builtin-commit.c

builtin-commit.c searches for the angle brackets '<' and '>' as a validation
for the argument to --author.

get_ac_line() searches from the end of the string and sets tz, timestamp,
and email to the last space-separated entries. timestamp is fed to strtoul
and tz is fed to atoi. These two both skip leading space, but they are
never fed leading space from get_ac_line().

date.c:parse_date() keys on +\- to recognize timezone.

I think I'll make all of the optional components non-optional and make the
format a little stricter.

-brandon

[PATCH v2] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Since nearly its birth, git's tags have included a "tagger" field which
describes the name of tagger, email of tagger, and date and time of tagging.
But, this field was only loosely tested by git-mktag. Provide some thorough
testing for this field and also ensure that the tag header is separated
from the tag body by an empty line to reduce the convenience of creating
a flawed tag.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---


Changes from previous version:

Leading spaces before the timestamp and timezone are no longer accepted.
Timezone is no longer optional and must be within -1400 to 1400
Timezone must be prefixed with + or -

-brandon


 mktag.c          |   61 ++++++++++++++++++++++----
 t/t3800-mktag.sh |  129 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 176 insertions(+), 14 deletions(-)

diff --git a/mktag.c b/mktag.c
index b05260c..8887080 100644
--- a/mktag.c
+++ b/mktag.c
@@ -8,10 +8,11 @@
  * message and a signature block that git itself doesn't care about,
  * but that can be verified with gpg or similar.
  *
- * The first three lines are guaranteed to be at least 63 bytes:
+ * The first four lines are guaranteed to be at least 83 bytes:
  * "object <sha1>\n" is 48 bytes, "type tag\n" at 9 bytes is the
- * shortest possible type-line, and "tag .\n" at 6 bytes is the
- * shortest single-character-tag line.
+ * shortest possible type-line, "tag .\n" at 6 bytes is the shortest
+ * single-character-tag line, and "tagger . <> 0 +0000\n" at 20 bytes is
+ * the shortest possible tagger-line.
  */
 
 /*
@@ -43,9 +44,9 @@ static int verify_tag(char *buffer, unsigned long size)
 	int typelen;
 	char type[20];
 	unsigned char sha1[20];
-	const char *object, *type_line, *tag_line, *tagger_line;
+	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
 
-	if (size < 64)
+	if (size < 84)
 		return error("wanna fool me ? you obviously got the size wrong !");
 
 	buffer[size] = 0;
@@ -97,11 +98,53 @@ static int verify_tag(char *buffer, unsigned long size)
 	/* Verify the tagger line */
 	tagger_line = tag_line;
 
-	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
-		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
+	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+		return error("char" PD_FMT ": could not find \"tagger \"",
+			tagger_line - buffer);
+
+	/*
+	 * Check for correct form for name and email
+	 * i.e. " <" followed by "> " on _this_ line
+	 */
+	tagger_line += 7;
+	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
+		strchr(tagger_line, '\n') < rb)
+		return error("char" PD_FMT ": malformed tagger",
+			tagger_line - buffer);
+
+	/* Check for author name, at least one character, space is acceptable */
+	if (lb == tagger_line)
+		return error("char" PD_FMT ": missing tagger name",
+			tagger_line - buffer);
+
+	/* timestamp */
+	tagger_line = rb + 2;
+	if (*tagger_line == ' ')
+		return error("char" PD_FMT ": malformed tag timestamp",
+			tagger_line - buffer);
+	for (;;) {
+		unsigned char c = *tagger_line++;
+		if (c == ' ')
+			break;
+		if (isdigit(c))
+			continue;
+		return error("char" PD_FMT ": malformed tag timestamp",
+			tagger_line - buffer);
+	}
 
-	/* TODO: check for committer info + blank line? */
-	/* Also, the minimum length is probably + "tagger .", or 63+8=71 */
+	/* timezone, 5 digits [+-]hhmm, max. 1400 */
+	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
+	      isdigit(tagger_line[1]) && isdigit(tagger_line[2]) &&
+	      isdigit(tagger_line[3]) && isdigit(tagger_line[4]) &&
+	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
+		return error("char" PD_FMT ": malformed tag timezone",
+			tagger_line - buffer);
+	tagger_line += 6;
+
+	/* Verify the blank line separating the header from the body */
+	if (*tagger_line != '\n')
+		return error("char" PD_FMT ": trailing garbage in tag header",
+			tagger_line - buffer);
 
 	/* The actual stuff afterwards we don't care about.. */
 	return 0;
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index bdc6e13..8a27400 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -44,6 +44,8 @@ cat >tag.sig <<EOF
 xxxxxx 139e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure '"object" line label check' '^error: char0: .*"object "$'
@@ -55,6 +57,8 @@ cat >tag.sig <<EOF
 object zz9e9b33986b1c2670fff52c5067603117b3e895
 type tag
 tag mytag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure '"object" line SHA1 check' '^error: char7: .*SHA1 hash$'
@@ -66,6 +70,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 xxxx tag
 tag mytag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure '"type" line label check' '^error: char47: .*"\\ntype "$'
@@ -85,6 +91,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tag
 xxx mytag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure '"tag" line label check #1' \
@@ -121,6 +129,8 @@ cat >tag.sig <<EOF
 object 779e9b33986b1c2670fff52c5067603117b3e895
 type tagggg
 tag mytag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure 'verify object (SHA1/type) check' \
@@ -133,6 +143,8 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag my	tag
+tagger . <> 0 +0000
+
 EOF
 
 check_verify_failure 'verify tag-name check' \
@@ -145,10 +157,12 @@ cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
+
+This is filler
 EOF
 
 check_verify_failure '"tagger" line label check #1' \
-	'^error: char70: could not find "tagger"$'
+	'^error: char70: could not find "tagger "$'
 
 ############################################################
 # 12. tagger line label check #2
@@ -158,19 +172,124 @@ object $head
 type commit
 tag mytag
 tagger
+
+This is filler
 EOF
 
 check_verify_failure '"tagger" line label check #2' \
-	'^error: char70: could not find "tagger"$'
+	'^error: char70: could not find "tagger "$'
 
 ############################################################
-# 13. create valid tag
+# 13. detect missing tag author name
 
 cat >tag.sig <<EOF
 object $head
 type commit
 tag mytag
-tagger another@example.com
+tagger  <> 0 +0000
+
+This is filler
+EOF
+
+check_verify_failure 'detect missing tag author name' \
+	'^error: char77: missing tagger name$'
+
+############################################################
+# 14. detect missing tag author name
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <
+ > 0 +0000
+
+EOF
+
+check_verify_failure 'detect malformed tagger' \
+	'^error: char77: malformed tagger$'
+
+############################################################
+# 15. allow empty tag email
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <> 0 +0000
+
+EOF
+
+test_expect_success \
+    'allow empty tag email' \
+    'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
+
+############################################################
+# 16. detect missing tag timestamp
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com>  
+
+EOF
+
+check_verify_failure 'detect missing tag timestamp' \
+	'^error: char107: malformed tag timestamp$'
+
+############################################################
+# 17. detect invalid tag timestamp
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> Tue Mar 25 15:47:44 2008
+
+EOF
+
+check_verify_failure 'detect invalid tag timestamp' \
+	'^error: char108: malformed tag timestamp$'
+
+############################################################
+# 18. detect invalid tag timezone
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 GMT
+
+EOF
+
+check_verify_failure 'detect invalid tag timezone' \
+	'^error: char118: malformed tag timezone$'
+
+############################################################
+# 19. detect invalid header entry
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 -0500
+this line should not be here
+
+EOF
+
+check_verify_failure 'detect invalid header entry' \
+	'^error: char124: trailing garbage in tag header$'
+
+############################################################
+# 20. create valid tag
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 -0500
+
 EOF
 
 test_expect_success \
@@ -178,7 +297,7 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 14. check mytag
+# 21. check mytag
 
 test_expect_success \
     'check mytag' \
-- 
1.5.4.4.481.g5075

Re: [PATCH v2] mktag.c: improve verification of tagger field and tests

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> @@ -97,11 +98,53 @@ static int verify_tag(char *buffer, unsigned long size)
>  	/* Verify the tagger line */
>  	tagger_line = tag_line;
>  
> -	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
> -		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
> +	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
> +		return error("char" PD_FMT ": could not find \"tagger \"",
> +			tagger_line - buffer);

You increment tagger_line by 7 after this step, so it might be a good idea
to make sure [7] != '\0', but does it make sense to compare it with '\n'
here?  I can see the original compared [6] with '\n', but I do not think
it makes sense to inherit it when you are "improving" the validation.

> +	/*
> +	 * Check for correct form for name and email
> +	 * i.e. " <" followed by "> " on _this_ line
> +	 */
> +	tagger_line += 7;
> +	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
> +		strchr(tagger_line, '\n') < rb)
> +		return error("char" PD_FMT ": malformed tagger",
> +			tagger_line - buffer);

The intention is 'on the line there must be " <" followed by something
followed by "> " before the end of line.'.  That's fine, but can the last
strchr() ever return NULL?

> +	if (lb == tagger_line)
> +		return error("char" PD_FMT ": missing tagger name",
> +			tagger_line - buffer);
> +
> +	/* timestamp */
> +	tagger_line = rb + 2;
> +	if (*tagger_line == ' ')
> +		return error("char" PD_FMT ": malformed tag timestamp",
> +			tagger_line - buffer);

'After "> ", there has to be the timestamp'.

> +	for (;;) {
> +		unsigned char c = *tagger_line++;
> +		if (c == ' ')
> +			break;
> +		if (isdigit(c))
> +			continue;
> +		return error("char" PD_FMT ": malformed tag timestamp",
> +			tagger_line - buffer);
> +	}

If the char immediately after "> " is ' ', it definitely is bogus, and you
want to make sure one or more digits, so the validation is correct but
feels a bit roundabout.

> +	/* timezone, 5 digits [+-]hhmm, max. 1400 */
> +	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
> +	      isdigit(tagger_line[1]) && isdigit(tagger_line[2]) &&
> +	      isdigit(tagger_line[3]) && isdigit(tagger_line[4]) &&
> +	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
> +		return error("char" PD_FMT ": malformed tag timezone",
> +			tagger_line - buffer);
> +	tagger_line += 6;

The open-coded strtoul() bothers me a bit, but it is not much longer nor
less readable than:

        (*tagger_line == '+' || *tagger_line == '-')
        && strtoul(tagger_line + 1, &ep, 10) <= 1400
        && ep - (tagger_line + 1) == 4
        && *ep == '\n'

so it probably is fine.

> +	/* Verify the blank line separating the header from the body */
> +	if (*tagger_line != '\n')
> +		return error("char" PD_FMT ": trailing garbage in tag header",
> +			tagger_line - buffer);

Having said all that, I'll queue this in 'next'; perhaps we can fix it up
real quick and merge it in 1.5.5.

Re: [PATCH v2] mktag.c: improve verification of tagger field and tests

[Brandon Casey]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> writes:
> 
>> @@ -97,11 +98,53 @@ static int verify_tag(char *buffer, unsigned long size)
>>  	/* Verify the tagger line */
>>  	tagger_line = tag_line;
>>  
>> -	if (memcmp(tagger_line, "tagger", 6) || (tagger_line[6] == '\n'))
>> -		return error("char" PD_FMT ": could not find \"tagger\"", tagger_line - buffer);
>> +	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
>> +		return error("char" PD_FMT ": could not find \"tagger \"",
>> +			tagger_line - buffer);
> 
> You increment tagger_line by 7 after this step, so it might be a good idea
> to make sure [7] != '\0', but does it make sense to compare it with '\n'
> here?  I can see the original compared [6] with '\n', but I do not think
> it makes sense to inherit it when you are "improving" the validation.

The test for '\n' was just carried on from the original version and is
unnecessary like you said.

I think a test against '\0' is also unnecessary. If tagger_line+7 equals '\0',
then it is still a valid string, and the first strstr() below, searching for " <",
will fail and give an appropriate error message.

>> +	/*
>> +	 * Check for correct form for name and email
>> +	 * i.e. " <" followed by "> " on _this_ line
>> +	 */
>> +	tagger_line += 7;
>> +	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
>> +		strchr(tagger_line, '\n') < rb)
>> +		return error("char" PD_FMT ": malformed tagger",
>> +			tagger_line - buffer);
> 
> The intention is 'on the line there must be " <" followed by something
> followed by "> " before the end of line.'.  That's fine, but can the last
> strchr() ever return NULL?

Yes, I thought of that. Wouldn't NULL always compare less than non-null?
Oh I see, maybe I should add a comment, I took advantage of the fact that a
newline is mandatory in order to simplify the testing. So, I expected NULL
to always cause failure here.

I expect that strchr() to disappear when your outline is followed for making
mktag a builtin and verify_tag() a function also used by builtin-tag. I think
this section testing the tagger field would make a nice generic function for
validating committer, author, and tagger fields and should probably go into
ident.c

>> +	if (lb == tagger_line)
>> +		return error("char" PD_FMT ": missing tagger name",
>> +			tagger_line - buffer);
>> +
>> +	/* timestamp */
>> +	tagger_line = rb + 2;
>> +	if (*tagger_line == ' ')
>> +		return error("char" PD_FMT ": malformed tag timestamp",
>> +			tagger_line - buffer);
> 
> 'After "> ", there has to be the timestamp'.
> 
>> +	for (;;) {
>> +		unsigned char c = *tagger_line++;
>> +		if (c == ' ')
>> +			break;
>> +		if (isdigit(c))
>> +			continue;
>> +		return error("char" PD_FMT ": malformed tag timestamp",
>> +			tagger_line - buffer);
>> +	}
> 
> If the char immediately after "> " is ' ', it definitely is bogus, and you
> want to make sure one or more digits, so the validation is correct but
> feels a bit roundabout.

I reused the form just above which was testing for control characters. Yeah,
I probably wouldn't have done it this way if I wasn't looking at the control
character code. Maybe using strspn() would be more straight forward.

>> +	/* timezone, 5 digits [+-]hhmm, max. 1400 */
>> +	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
>> +	      isdigit(tagger_line[1]) && isdigit(tagger_line[2]) &&
>> +	      isdigit(tagger_line[3]) && isdigit(tagger_line[4]) &&
>> +	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
>> +		return error("char" PD_FMT ": malformed tag timezone",
>> +			tagger_line - buffer);
>> +	tagger_line += 6;
> 
> The open-coded strtoul() bothers me a bit, but it is not much longer nor
> less readable than:
> 
>         (*tagger_line == '+' || *tagger_line == '-')
>         && strtoul(tagger_line + 1, &ep, 10) <= 1400
>         && ep - (tagger_line + 1) == 4
>         && *ep == '\n'
> 
> so it probably is fine.

But strtoul() allows leading spaces, so '+ 300' would be parsed as valid.

I was a little reluctant to use isdigit() and atoi() for fear of some
locale danger?

>> +	/* Verify the blank line separating the header from the body */
>> +	if (*tagger_line != '\n')
>> +		return error("char" PD_FMT ": trailing garbage in tag header",
>> +			tagger_line - buffer);
> 
> Having said all that, I'll queue this in 'next'; perhaps we can fix it up
> real quick and merge it in 1.5.5.

Sounds good.

-brandon

[PATCH] mktag.c: tweak validation of tagger field and adjust test script

[Brandon Casey]

Updates the verify_tag function to remove an unnecessary test, and add
additional testing for angle brackets in the name and email field, and
spaces in the email field. The timestamp and timezone sections are made
more straight forward by using strspn().

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 mktag.c          |   30 +++++++++---------
 t/t3800-mktag.sh |   88 ++++++++++++++++++++++++++++++++++++++++++++----------
 2 files changed, 87 insertions(+), 31 deletions(-)

diff --git a/mktag.c b/mktag.c
index 8887080..4d7583f 100644
--- a/mktag.c
+++ b/mktag.c
@@ -45,6 +45,7 @@ static int verify_tag(char *buffer, unsigned long size)
 	char type[20];
 	unsigned char sha1[20];
 	const char *object, *type_line, *tag_line, *tagger_line, *lb, *rb;
+	size_t len;
 
 	if (size < 84)
 		return error("wanna fool me ? you obviously got the size wrong !");
@@ -98,18 +99,21 @@ static int verify_tag(char *buffer, unsigned long size)
 	/* Verify the tagger line */
 	tagger_line = tag_line;
 
-	if (memcmp(tagger_line, "tagger ", 7) || (tagger_line[7] == '\n'))
+	if (memcmp(tagger_line, "tagger ", 7))
 		return error("char" PD_FMT ": could not find \"tagger \"",
 			tagger_line - buffer);
 
 	/*
 	 * Check for correct form for name and email
 	 * i.e. " <" followed by "> " on _this_ line
+	 * No angle brackets within the name or email address fields.
+	 * No spaces within the email address field.
 	 */
 	tagger_line += 7;
 	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
-		strchr(tagger_line, '\n') < rb)
-		return error("char" PD_FMT ": malformed tagger",
+		strpbrk(tagger_line, "<>\n") != lb+1 ||
+		strpbrk(lb+2, "><\n ") != rb)
+		return error("char" PD_FMT ": malformed tagger field",
 			tagger_line - buffer);
 
 	/* Check for author name, at least one character, space is acceptable */
@@ -117,25 +121,20 @@ static int verify_tag(char *buffer, unsigned long size)
 		return error("char" PD_FMT ": missing tagger name",
 			tagger_line - buffer);
 
-	/* timestamp */
+	/* timestamp, 1 or more digits followed by space*/
 	tagger_line = rb + 2;
-	if (*tagger_line == ' ')
-		return error("char" PD_FMT ": malformed tag timestamp",
+	if (!(len = strspn(tagger_line, "0123456789")))
+		return error("char" PD_FMT ": missing tag timestamp",
 			tagger_line - buffer);
-	for (;;) {
-		unsigned char c = *tagger_line++;
-		if (c == ' ')
-			break;
-		if (isdigit(c))
-			continue;
+	tagger_line += len;
+	if (*tagger_line != ' ')
 		return error("char" PD_FMT ": malformed tag timestamp",
 			tagger_line - buffer);
-	}
+	tagger_line++;
 
 	/* timezone, 5 digits [+-]hhmm, max. 1400 */
 	if (!((tagger_line[0] == '+' || tagger_line[0] == '-') &&
-	      isdigit(tagger_line[1]) && isdigit(tagger_line[2]) &&
-	      isdigit(tagger_line[3]) && isdigit(tagger_line[4]) &&
+	      strspn(tagger_line+1, "0123456789") == 4 &&
 	      tagger_line[5] == '\n' && atoi(tagger_line+1) <= 1400))
 		return error("char" PD_FMT ": malformed tag timezone",
 			tagger_line - buffer);
diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
index 8a27400..df1fd6f 100755
--- a/t/t3800-mktag.sh
+++ b/t/t3800-mktag.sh
@@ -180,7 +180,7 @@ check_verify_failure '"tagger" line label check #2' \
 	'^error: char70: could not find "tagger "$'
 
 ############################################################
-# 13. detect missing tag author name
+# 13. disallow missing tag author name
 
 cat >tag.sig <<EOF
 object $head
@@ -191,11 +191,11 @@ tagger  <> 0 +0000
 This is filler
 EOF
 
-check_verify_failure 'detect missing tag author name' \
+check_verify_failure 'disallow missing tag author name' \
 	'^error: char77: missing tagger name$'
 
 ############################################################
-# 14. detect missing tag author name
+# 14. disallow missing tag author name
 
 cat >tag.sig <<EOF
 object $head
@@ -206,8 +206,8 @@ tagger T A Gger <
 
 EOF
 
-check_verify_failure 'detect malformed tagger' \
-	'^error: char77: malformed tagger$'
+check_verify_failure 'disallow malformed tagger' \
+	'^error: char77: malformed tagger field$'
 
 ############################################################
 # 15. allow empty tag email
@@ -225,7 +225,21 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 16. detect missing tag timestamp
+# 16. disallow spaces in tag email
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tag ger@example.com> 0 +0000
+
+EOF
+
+check_verify_failure 'disallow spaces in tag email' \
+	'^error: char77: malformed tagger field$'
+
+############################################################
+# 17. disallow missing tag timestamp
 
 cat >tag.sig <<EOF
 object $head
@@ -235,11 +249,11 @@ tagger T A Gger <tagger@example.com>
 
 EOF
 
-check_verify_failure 'detect missing tag timestamp' \
-	'^error: char107: malformed tag timestamp$'
+check_verify_failure 'disallow missing tag timestamp' \
+	'^error: char107: missing tag timestamp$'
 
 ############################################################
-# 17. detect invalid tag timestamp
+# 18. detect invalid tag timestamp1
 
 cat >tag.sig <<EOF
 object $head
@@ -249,11 +263,25 @@ tagger T A Gger <tagger@example.com> Tue Mar 25 15:47:44 2008
 
 EOF
 
-check_verify_failure 'detect invalid tag timestamp' \
-	'^error: char108: malformed tag timestamp$'
+check_verify_failure 'detect invalid tag timestamp1' \
+	'^error: char107: missing tag timestamp$'
 
 ############################################################
-# 18. detect invalid tag timezone
+# 19. detect invalid tag timestamp2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 2008-03-31T12:20:15-0500
+
+EOF
+
+check_verify_failure 'detect invalid tag timestamp2' \
+	'^error: char111: malformed tag timestamp$'
+
+############################################################
+# 20. detect invalid tag timezone1
 
 cat >tag.sig <<EOF
 object $head
@@ -263,11 +291,39 @@ tagger T A Gger <tagger@example.com> 1206478233 GMT
 
 EOF
 
-check_verify_failure 'detect invalid tag timezone' \
+check_verify_failure 'detect invalid tag timezone1' \
+	'^error: char118: malformed tag timezone$'
+
+############################################################
+# 21. detect invalid tag timezone2
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 +  30
+
+EOF
+
+check_verify_failure 'detect invalid tag timezone2' \
+	'^error: char118: malformed tag timezone$'
+
+############################################################
+# 22. detect invalid tag timezone3
+
+cat >tag.sig <<EOF
+object $head
+type commit
+tag mytag
+tagger T A Gger <tagger@example.com> 1206478233 -1430
+
+EOF
+
+check_verify_failure 'detect invalid tag timezone3' \
 	'^error: char118: malformed tag timezone$'
 
 ############################################################
-# 19. detect invalid header entry
+# 23. detect invalid header entry
 
 cat >tag.sig <<EOF
 object $head
@@ -282,7 +338,7 @@ check_verify_failure 'detect invalid header entry' \
 	'^error: char124: trailing garbage in tag header$'
 
 ############################################################
-# 20. create valid tag
+# 24. create valid tag
 
 cat >tag.sig <<EOF
 object $head
@@ -297,7 +353,7 @@ test_expect_success \
     'git-mktag <tag.sig >.git/refs/tags/mytag 2>message'
 
 ############################################################
-# 21. check mytag
+# 25. check mytag
 
 test_expect_success \
     'check mytag' \
-- 
1.5.5.rc1.12.g660b9

Re: [PATCH] mktag.c: tweak validation of tagger field and adjust test script

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

>  	/*
>  	 * Check for correct form for name and email
>  	 * i.e. " <" followed by "> " on _this_ line
> +	 * No angle brackets within the name or email address fields.
> +	 * No spaces within the email address field.
>  	 */
>  	tagger_line += 7;
>  	if (!(lb = strstr(tagger_line, " <")) || !(rb = strstr(lb+2, "> ")) ||
> +		strpbrk(tagger_line, "<>\n") != lb+1 ||
> +		strpbrk(lb+2, "><\n ") != rb)
> +		return error("char" PD_FMT ": malformed tagger field",
>  			tagger_line - buffer);

This is the first use of strpbrk() in git.git codebase.  Are BSD folks Ok
with it (I understand this came from SVID 1, just like strspn we already
use elsewhere)?