Brandon Casey <casey@xxxxxxxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
