Re: [PATCH v2] mktag.c: improve verification of tagger field and tests
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 31, 2008, 08:40 UTC
- Message-ID
- <7vr6drl215.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <47EBC844.3020602@nrlssc.navy.mil>
Brandon Casey <casey@nrlssc.navy.mil> writes:
Show 9 quoted lines
> @@ -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.
Show 9 quoted lines
> + /*
> + * 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?
Show 9 quoted lines
> + 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'.
Show 9 quoted lines
> + 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.
Show 8 quoted lines
> + /* 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.