patchstrtoul_ui: reject negative values

> strtoul_ui uses strtoul to get a long unsigned, then checks that casting
> to unsigned does not lose information and return the casted value.
> 
> On 64 bits architecture, checking that the cast does not change the value
> catches most errors, but when sizeof(int) == sizeof(long) (e.g. i386),
> the check does nothing. Unfortunately, strtoul silently accepts negative
> values, and as a result strtoul_ui("-1", ...) raised no error.
> 
> This patch catches negative values before it's too late, i.e. before
> calling strtoul. We still silently accept very large integers that wrap
> to a valid "unsigned int".
> 
> Reported-by: Max Kirillov <max@max630.net>
> Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
> ---
> So, here's a proper patch (I mean, a band-aid patch, but properly
> send ;-) ).
> 
> It should be merged before Kartik's series (or inserted at the start
> of the series) so that we get the fix before the test breakage.
> 
>  git-compat-util.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..1df82fa 100644
> --- 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;

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

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

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

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

>
>  git-compat-util.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f649e81..1df82fa 100644
> --- 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;
>  	ul = strtoul(s, &p, base);
>  	if (errno || *p || p == s || (unsigned int) ul != ul)
>  		return -1;

> Matthieu Moy <Matthieu.Moy@imag.fr> writes:
>
>> This patch catches negative values before it's too late, i.e. before
>> calling strtoul. We still silently accept very large integers that wrap
>> to a valid "unsigned int".
>
> Is the last statement correct?  A very large long uint that wrap to
> uint would not fit in long uint and you would get ERANGE, no?