Re: [PATCH] strtoul_ui: reject negative values

[Marc Branchaud <marcnarc@xiplink.com>]

On 15-09-17 11:34 AM, Matthieu Moy wrote:
> Marc Branchaud <marcnarc@xiplink.com> writes:
> 
>>> --- 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.

Right, sorry.

>>>  	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.

Fair enough, just not what I expected from a function with that name.

		M.