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

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

[Max Kirillov]

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

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

Signed-off-by: Max Kirillov <max@max630.net>
---
This caused failure of "%(contents:lines=-1)` should fail" case from
t6302-for-each-ref-filter.sh for me in pu. Don't know why nobody has noticed
it. It did not trigger errno, instead wrapping the value. I have libc6 2.13
(debian wheezy)

Still can be fooled with carefully chosen negative input. For i386 it's
between INT_MIN and something like -UINT_MIN

Adding people from the commit which uses the function.
 git-compat-util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index f649e81..1c0229b 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -815,7 +815,7 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
 
 	errno = 0;
 	ul = strtoul(s, &p, base);
-	if (errno || *p || p == s || (unsigned int) ul != ul)
+	if (errno || *p || p == s || ul > (unsigned long) INT_MAX)
 		return -1;
 	*result = ul;
 	return 0;
-- 
2.3.4.2801.g3d0809b

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

[Matthieu Moy]

Max Kirillov <max@max630.net> 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?

> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -815,7 +815,7 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
>  
>  	errno = 0;
>  	ul = strtoul(s, &p, base);
> -	if (errno || *p || p == s || (unsigned int) ul != ul)
> +	if (errno || *p || p == s || ul > (unsigned long) INT_MAX)

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/

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

[Max Kirillov]

On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote:
>> 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

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

[Matthieu Moy]

[ Cc-ing Michael Haggerty who wrote the numparse module ]

Max Kirillov <max@max630.net> writes:

> On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote:
>>> 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.

Not just the return type (which is the error status), but also the type
of the result argument indeed. It's not clear to me whether this is
intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced
it, the commit message doesn't help). I first read strtoul_ui as
"strtoul with a better UI (user interface)", but maybe the name was
meant to say "a fuction that uses strtoul and returns an ui (unsigned
int)".

I think it would be better to just return a long to avoid needless
limitations, but changing the argument to "long" would interfer with
in-flight topics. Not worth the trouble.

One potential issue with your patch is that you're forbidding the
interval [2^31, 2^32[ which was previously allowed, both on 32 and 64
bits. I'm not sure whether we have a use for this in the codebase.

This alternative patch is rather ugly to, but I think it is less
limiting and does not have the "large negative wrapped to positive"
issue:

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

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

On 64 bits architectures, it's not as bad: you need to go really far in
the negatives to wrap to positive values.

> 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

I went through the thread quickly, my understanding is that there were
more work to do, but no objection to merging.

Michael, any plan to resurect the topic?

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

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

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Not just the return type (which is the error status), but also the type
> of the result argument indeed. It's not clear to me whether this is
> intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced
> it, the commit message doesn't help). I first read strtoul_ui as
> "strtoul with a better UI (user interface)", but maybe the name was
> meant to say "a fuction that uses strtoul and returns an ui (unsigned
> int)".

Just for this part.  Yes, ui does not mean user interface but "we
are grabbing an unsigned int and as its internal implementation we
happen to use strtoul" is where the name comes from.

> I went through the thread quickly, my understanding is that there were
> more work to do, but no objection to merging.

Yes, there were some in-flight topics that interfered with it and
the topic quickly went stale without getting rerolled.  There wasn't
any fundamental issue with the topic itself.

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

[Max Kirillov]

On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:
> I think it would be better to just return a long to avoid needless
> limitations, but changing the argument to "long" would interfer with
> in-flight topics. Not worth the trouble.

Sure.

> 
> One potential issue with your patch is that you're forbidding the
> interval [2^31, 2^32[ which was previously allowed, both on 32 and 64
> bits. I'm not sure whether we have a use for this in the codebase.

As far as I could see it was used only for file modes. Which
does not need that big numbers.

> This alternative patch is rather ugly to, but I think it is less
> limiting and does not have the "large negative wrapped to positive"
> issue:
> 
> --- 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. And even for 64-bit
it's too obscure. In form of "(ul & 0xffffffffL) == 0" it
would be more clear. Or just make explicit comparison with
intended limit, like I did.

Well, actually I don't have strong preferences as long as
"make -C t" does not alarm me with things I did not break.
Maybe somebody else will comment more.

-- 
Max

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

[Matthieu Moy]

Max Kirillov <max@max630.net> 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/