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

Junio C Hamano wrote:

Brandon Casey [off-list ref] writes:

quoted

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

quoted

+	/*
+	 * 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

quoted

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

quoted

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

quoted

+	/* 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?

quoted

+	/* 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