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

[Brandon Casey <casey@nrlssc.navy.mil>]

Junio C Hamano wrote:
> Brandon Casey <casey@nrlssc.navy.mil> 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