Re: [PATCH] strtoul_ui: reject negative values

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 17, 2015, 15:34 UTC
Message-ID: <vpqr3lxoy9t.fsf@anie.imag.fr>
In-Reply-To: <55FAD981.5080808@xiplink.com>

Marc Branchaud <marcnarc@xiplink.com> writes:

Show 12 quoted lines

>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
>>  	char *p;
>>  
>>  	errno = 0;
>> +	/* negative values would be accepted by strtoul */
>> +	if (strchr(s, '-'))
>> +		return -1;
>
> I think this is broken, in that it doesn't match strtoul's normal behaviour,
> for strings like "1234-5678", no?

The goal here is just to read a positive integer value. Rejecting "1234-5678" is indeed a good thing. We already rejected it before my patch by checking for p (AKA endptr for strtoul), as you noted below.

> The test also doesn't work if the string has leading whitespace ("
> -5").

Why? It rejects any string that contain the character '-', regardless of trailing spaces.

Show 6 quoted lines

>>  	ul = strtoul(s, &p, base);
>>  	if (errno || *p || p == s || (unsigned int) ul != ul)
>>  		return -1;
>
> Hmm, but we check *p here, so IIUC it's an error if the string has any
> trailing non-digits.  Weird.

strtoul_ui is more defensive than strtoul, by design.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/