Junio C Hamano wrote:
> 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.
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
--
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
