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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Max Kirillov <max@xxxxxxxxxx> writes:

> 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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]