From: Matthieu Moy Date: Mon, 14 Sep 2015 06:30:54 GMT Subject: Re: [PATCH] strtoul_ui: actually report error in case of negative input Message-ID: In-Reply-To: <1442181636-27821-1-git-send-email-max@max630.net> Max Kirillov 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/