patchstrtoul_ui: actually report error in case of negative input

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(-)

Max Kirillov <max@max630.net> writes:

Thanks for noticing and reporting.

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)

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

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

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

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

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:

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

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.

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.

On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:

Sure.

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

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 Kirillov <max@max630.net> writes:

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.

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.

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