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

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 15, 2015, 06:50 UTC
Message-ID: <vpq4miwfa78.fsf@anie.imag.fr>
In-Reply-To: <20150914202647.GA7806@wheezy.local>

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

Max Kirillov <max@max630.net> writes:

Show 10 quoted lines

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