Re: [PATCH] strtoul_ui: actually report error in case of negative input

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 16, 2015, 06:08 UTC
Message-ID: <vpq1tdy99qs.fsf@anie.imag.fr>
In-Reply-To: <20150916042037.GB7806@wheezy.local>

Max Kirillov <max@max630.net> writes:

Show 20 quoted lines

> On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:
>> --- 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;
>> 
>> What do you think?
>
> Explicit rejection of '-' is of course useful addition.
>
> I still find "(unsigned int) ul != ul" bad. As far as I
> understand it makes no sense for i386.

Nothing would make sense here for i386: there's no case where you want to reject anything on this architecture. Well, you may have expected strtoul to reject big numbers, but it did not and it's too late.

> And even for 64-bit it's too obscure. In form of "(ul & 0xffffffffL)
> == 0" it would be more clear.

I disagree. "(unsigned int) ul != ul" reads immediately as "if casting ul to unsigned int changes its value", regardless of sizeof(int). This is exactly what the check is doing.

> Or just make explicit comparison with intended limit, like I did.

What you really want is to compare with UINT_MAX which would not make sense on 32 bits architectures.

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