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

Max Kirillov [off-list ref] writes:

If s == "-1" and CPU is i386, then none of the checks is triggered, including
the last "(unsigned int) ul != ul", because ul == 2**32 - 1, which fits into
"unsigned int".

Thanks for noticing and reporting.

Fix it by changing the last check to trigger earlier, as soon as it
becomes bigger than INT_MAX.

What if the value is actually greater than INT_MAX? The function is
returning an unsigned long (64 bits on 64bits architectures), and your
version is restricting it to integers smaller than 2^31, right?

I think you at least want to use LONG_MAX and drop the cast here
(untested, and beware of my advices when given before coffee).
That would restrict to values smaller than 2^63, and I guess no one is
interested in the interval ]2^63, 2^64].

The other option would be to look for a leading '-' before calling
strtoul.

(Actually, this makes me wonder why strtoul happily returns a big
positive when fed with the string "-1", but we can't change it)

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

On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote:

quoted

Fix it by changing the last check to trigger earlier, as soon as it
becomes bigger than INT_MAX.

What if the value is actually greater than INT_MAX? The function is
returning an unsigned long (64 bits on 64bits architectures), and your
version is restricting it to integers smaller than 2^31, right?

the return type of the function is "int", so this is not
going to work anyway.

As I mentioned, some negative values are still accepted
as coresponding mod 2**32 positive numbers (-3221225472 as
1073741824), so there really is room for improvement, but it
cannot be accomplished just by examining strtoul output.

I saw in the list archives an attempt to abandon the
function in favor of more accurate parser [1], but seems
like it did not make it into the project.

[1] http://thread.gmane.org/gmane.comp.version-control.git/265635

-- 
Max