{"thread":{"id":"40344","subject":"[PATCH] strtoul_ui: actually report error in case of negative input","startedAt":"2015-09-13T22:00:36Z","lastAt":"2015-09-16T06:08:43Z","messageCount":7,"participants":["Max Kirillov","Matthieu Moy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"269893","messageId":"1442181636-27821-1-git-send-email-max@max630.net","threadId":"40344","inReplyTo":null,"subject":"[PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-09-13T22:00:36Z","receivedAt":"2015-09-13T22:00:36Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"If s == \"-1\" and CPU is i386, then none of the checks is triggered, including\nthe last \"(unsigned int) ul != ul\", because ul == 2**32 - 1, which fits into\n\"unsigned int\".\n\nFix it by changing the last check to trigger earlier, as soon as it\nbecomes bigger than INT_MAX.\n\nSigned-off-by: Max Kirillov <max@max630.net>\n---\nThis caused failure of \"%(contents:lines=-1)` should fail\" case from\nt6302-for-each-ref-filter.sh for me in pu. Don't know why nobody has noticed\nit. It did not trigger errno, instead wrapping the value. I have libc6 2.13\n(debian wheezy)\n\nStill can be fooled with carefully chosen negative input. For i386 it's\nbetween INT_MIN and something like -UINT_MIN\n\nAdding people from the commit which uses the function.\n git-compat-util.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f649e81..1c0229b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -815,7 +815,7 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n \n \terrno = 0;\n \tul = strtoul(s, &p, base);\n-\tif (errno || *p || p == s || (unsigned int) ul != ul)\n+\tif (errno || *p || p == s || ul > (unsigned long) INT_MAX)\n \t\treturn -1;\n \t*result = ul;\n \treturn 0;\n-- \n2.3.4.2801.g3d0809b\n"},{"id":"269906","messageId":"vpqr3m1seap.fsf@anie.imag.fr","threadId":"40344","inReplyTo":"1442181636-27821-1-git-send-email-max@max630.net","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-14T06:30:54Z","receivedAt":"2015-09-14T06:30:54Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Max Kirillov <max@max630.net> writes:\n\n> If s == \"-1\" and CPU is i386, then none of the checks is triggered, including\n> the last \"(unsigned int) ul != ul\", because ul == 2**32 - 1, which fits into\n> \"unsigned int\".\n\nThanks for noticing and reporting.\n\n> Fix it by changing the last check to trigger earlier, as soon as it\n> becomes bigger than INT_MAX.\n\nWhat if the value is actually greater than INT_MAX? The function is\nreturning an unsigned long (64 bits on 64bits architectures), and your\nversion is restricting it to integers smaller than 2^31, right?\n\n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -815,7 +815,7 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n>  \n>  \terrno = 0;\n>  \tul = strtoul(s, &p, base);\n> -\tif (errno || *p || p == s || (unsigned int) ul != ul)\n> +\tif (errno || *p || p == s || ul > (unsigned long) INT_MAX)\n\nI think you at least want to use LONG_MAX and drop the cast here\n(untested, and beware of my advices when given before coffee).\nThat would restrict to values smaller than 2^63, and I guess no one is\ninterested in the interval ]2^63, 2^64].\n\nThe other option would be to look for a leading '-' before calling\nstrtoul.\n\n(Actually, this makes me wonder why strtoul happily returns a big\npositive when fed with the string \"-1\", but we can't change it)\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"269959","messageId":"20150914202647.GA7806@wheezy.local","threadId":"40344","inReplyTo":"vpqr3m1seap.fsf@anie.imag.fr","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-09-14T20:26:47Z","receivedAt":"2015-09-14T20:26:47Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote:\n>> Fix it by changing the last check to trigger earlier, as soon as it\n>> becomes bigger than INT_MAX.\n> \n> What if the value is actually greater than INT_MAX? The function is\n> returning an unsigned long (64 bits on 64bits architectures), and your\n> version is restricting it to integers smaller than 2^31, right?\n\nthe return type of the function is \"int\", so this is not\ngoing to work anyway.\n\nAs I mentioned, some negative values are still accepted\nas coresponding mod 2**32 positive numbers (-3221225472 as\n1073741824), so there really is room for improvement, but it\ncannot be accomplished just by examining strtoul output.\n\nI saw in the list archives an attempt to abandon the\nfunction in favor of more accurate parser [1], but seems\nlike it did not make it into the project.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/265635\n\n-- \nMax\n"},{"id":"269984","messageId":"vpq4miwfa78.fsf@anie.imag.fr","threadId":"40344","inReplyTo":"20150914202647.GA7806@wheezy.local","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-15T06:50:03Z","receivedAt":"2015-09-15T06:50:03Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"[ Cc-ing Michael Haggerty who wrote the numparse module ]\n\nMax Kirillov <max@max630.net> writes:\n\n> On Mon, Sep 14, 2015 at 08:30:54AM +0200, Matthieu Moy wrote:\n>>> Fix it by changing the last check to trigger earlier, as soon as it\n>>> becomes bigger than INT_MAX.\n>> \n>> What if the value is actually greater than INT_MAX? The function is\n>> returning an unsigned long (64 bits on 64bits architectures), and your\n>> version is restricting it to integers smaller than 2^31, right?\n>\n> the return type of the function is \"int\", so this is not\n> going to work anyway.\n\nNot just the return type (which is the error status), but also the type\nof the result argument indeed. It's not clear to me whether this is\nintentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced\nit, the commit message doesn't help). I first read strtoul_ui as\n\"strtoul with a better UI (user interface)\", but maybe the name was\nmeant to say \"a fuction that uses strtoul and returns an ui (unsigned\nint)\".\n\nI think it would be better to just return a long to avoid needless\nlimitations, but changing the argument to \"long\" would interfer with\nin-flight topics. Not worth the trouble.\n\nOne potential issue with your patch is that you're forbidding the\ninterval [2^31, 2^32[ which was previously allowed, both on 32 and 64\nbits. I'm not sure whether we have a use for this in the codebase.\n\nThis alternative patch is rather ugly to, but I think it is less\nlimiting and does not have the \"large negative wrapped to positive\"\nissue:\n\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n        char *p;\n \n        errno = 0;\n+       /* negative values would be accepted by strtoul */\n+       if (strchr(s, '-'))\n+               return -1;\n        ul = strtoul(s, &p, base);\n        if (errno || *p || p == s || (unsigned int) ul != ul)\n                return -1;\n\nWhat do you think?\n\n> As I mentioned, some negative values are still accepted\n> as coresponding mod 2**32 positive numbers (-3221225472 as\n> 1073741824), so there really is room for improvement, but it\n> cannot be accomplished just by examining strtoul output.\n\nOn 64 bits architectures, it's not as bad: you need to go really far in\nthe negatives to wrap to positive values.\n\n> I saw in the list archives an attempt to abandon the\n> function in favor of more accurate parser [1], but seems\n> like it did not make it into the project.\n>\n> [1] http://thread.gmane.org/gmane.comp.version-control.git/265635\n\nI went through the thread quickly, my understanding is that there were\nmore work to do, but no objection to merging.\n\nMichael, any plan to resurect the topic?\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270087","messageId":"xmqqsi6ftb74.fsf@gitster.mtv.corp.google.com","threadId":"40344","inReplyTo":"vpq4miwfa78.fsf@anie.imag.fr","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-16T01:17:03Z","receivedAt":"2015-09-16T01:17:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:\n\n> Not just the return type (which is the error status), but also the type\n> of the result argument indeed. It's not clear to me whether this is\n> intentional (09f2825 (git-grep: don't use sscanf, 2007-03-12) introduced\n> it, the commit message doesn't help). I first read strtoul_ui as\n> \"strtoul with a better UI (user interface)\", but maybe the name was\n> meant to say \"a fuction that uses strtoul and returns an ui (unsigned\n> int)\".\n\nJust for this part.  Yes, ui does not mean user interface but \"we\nare grabbing an unsigned int and as its internal implementation we\nhappen to use strtoul\" is where the name comes from.\n\n> I went through the thread quickly, my understanding is that there were\n> more work to do, but no objection to merging.\n\nYes, there were some in-flight topics that interfered with it and\nthe topic quickly went stale without getting rerolled.  There wasn't\nany fundamental issue with the topic itself.\n"},{"id":"270095","messageId":"20150916042037.GB7806@wheezy.local","threadId":"40344","inReplyTo":"vpq4miwfa78.fsf@anie.imag.fr","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Max Kirillov","fromEmail":"max@max630.net","sentAt":"2015-09-16T04:20:38Z","receivedAt":"2015-09-16T04:20:38Z","isPatch":true,"sender":{"key":"max@max630.net","avatar":"https://avatars.githubusercontent.com/u/381560?v=4"},"body":"On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:\n> I think it would be better to just return a long to avoid needless\n> limitations, but changing the argument to \"long\" would interfer with\n> in-flight topics. Not worth the trouble.\n\nSure.\n\n> \n> One potential issue with your patch is that you're forbidding the\n> interval [2^31, 2^32[ which was previously allowed, both on 32 and 64\n> bits. I'm not sure whether we have a use for this in the codebase.\n\nAs far as I could see it was used only for file modes. Which\ndoes not need that big numbers.\n\n> This alternative patch is rather ugly to, but I think it is less\n> limiting and does not have the \"large negative wrapped to positive\"\n> issue:\n> \n> --- a/git-compat-util.h\n> +++ b/git-compat-util.h\n> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n>         char *p;\n>  \n>         errno = 0;\n> +       /* negative values would be accepted by strtoul */\n> +       if (strchr(s, '-'))\n> +               return -1;\n>         ul = strtoul(s, &p, base);\n>         if (errno || *p || p == s || (unsigned int) ul != ul)\n>                 return -1;\n> \n> What do you think?\n\nExplicit rejection of '-' is of course useful addition.\n\nI still find \"(unsigned int) ul != ul\" bad. As far as I\nunderstand it makes no sense for i386. And even for 64-bit\nit's too obscure. In form of \"(ul & 0xffffffffL) == 0\" it\nwould be more clear. Or just make explicit comparison with\nintended limit, like I did.\n\nWell, actually I don't have strong preferences as long as\n\"make -C t\" does not alarm me with things I did not break.\nMaybe somebody else will comment more.\n\n-- \nMax\n"},{"id":"270099","messageId":"vpq1tdy99qs.fsf@anie.imag.fr","threadId":"40344","inReplyTo":"20150916042037.GB7806@wheezy.local","subject":"Re: [PATCH] strtoul_ui: actually report error in case of negative input","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-16T06:08:43Z","receivedAt":"2015-09-16T06:08:43Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Max Kirillov <max@max630.net> writes:\n\n> On Tue, Sep 15, 2015 at 08:50:03AM +0200, Matthieu Moy wrote:\n>> --- a/git-compat-util.h\n>> +++ b/git-compat-util.h\n>> @@ -814,6 +814,9 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)\n>>         char *p;\n>>  \n>>         errno = 0;\n>> +       /* negative values would be accepted by strtoul */\n>> +       if (strchr(s, '-'))\n>> +               return -1;\n>>         ul = strtoul(s, &p, base);\n>>         if (errno || *p || p == s || (unsigned int) ul != ul)\n>>                 return -1;\n>> \n>> What do you think?\n>\n> Explicit rejection of '-' is of course useful addition.\n>\n> I still find \"(unsigned int) ul != ul\" bad. As far as I\n> understand it makes no sense for i386.\n\nNothing would make sense here for i386: there's no case where you want\nto reject anything on this architecture. Well, you may have expected\nstrtoul to reject big numbers, but it did not and it's too late.\n\n> And even for 64-bit it's too obscure. In form of \"(ul & 0xffffffffL)\n> == 0\" it would be more clear.\n\nI disagree. \"(unsigned int) ul != ul\" reads immediately as \"if casting\nul to unsigned int changes its value\", regardless of sizeof(int). This\nis exactly what the check is doing.\n\n> Or just make explicit comparison with intended limit, like I did.\n\nWhat you really want is to compare with UINT_MAX which would not make\nsense on 32 bits architectures.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"}]}