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

Brandon Casey [off-list ref] writes:

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.