Re: [PATCH] strtoul_ui: actually report error in case of negative input
Max Kirillovwrites: > 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, , 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 & 0xL) > == 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: actually report error in case of negative input
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, , 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 & 0xL) == 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: actually report error in case of negative input
Matthieu Moywrites: > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: actually report error in case of negative input
[ Cc-ing Michael Haggerty who wrote the numparse module ] Max Kirillovwrites: > 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, , 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: actually report error in case of negative input
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] strtoul_ui: actually report error in case of negative input
Max Kirillovwrites: > 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, , 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/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] strtoul_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--- 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, , 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html