{"thread":{"id":"40371","subject":"[PATCH] strtoul_ui: reject negative values","startedAt":"2015-09-17T14:37:26Z","lastAt":"2015-09-17T16:28:33Z","messageCount":6,"participants":["Matthieu Moy","Marc Branchaud","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"270214","messageId":"1442500646-15293-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40371","inReplyTo":null,"subject":"[PATCH] strtoul_ui: reject negative values","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-09-17T14:37:26Z","receivedAt":"2015-09-17T14:37:26Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"strtoul_ui uses strtoul to get a long unsigned, then checks that casting\nto unsigned does not lose information and return the casted value.\n\nOn 64 bits architecture, checking that the cast does not change the value\ncatches most errors, but when sizeof(int) == sizeof(long) (e.g. i386),\nthe check does nothing. Unfortunately, strtoul silently accepts negative\nvalues, and as a result strtoul_ui(\"-1\", ...) raised no error.\n\nThis patch catches negative values before it's too late, i.e. before\ncalling strtoul. We still silently accept very large integers that wrap\nto a valid \"unsigned int\".\n\nReported-by: Max Kirillov \nSigned-off-by: Matthieu Moy \n---\nSo, here's a proper patch (I mean, a band-aid patch, but properly\nsend ;-) ).\n\nIt should be merged before Kartik's series (or inserted at the start\nof the series) so that we get the fix before the test breakage.\n\n git-compat-util.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f649e81..1df82fa 100644\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 \tchar *p;\n \n \terrno = 0;\n+\t/* negative values would be accepted by strtoul */\n+\tif (strchr(s, '-'))\n+\t\treturn -1;\n \tul = strtoul(s, &p, base);\n \tif (errno || *p || p == s || (unsigned int) ul != ul)\n \t\treturn -1;\n-- \n2.5.0.402.g8854c44\n"},{"id":"270217","messageId":"55FAD981.5080808@xiplink.com","threadId":"40371","inReplyTo":"1442500646-15293-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] strtoul_ui: reject negative values","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2015-09-17T15:17:21Z","receivedAt":"2015-09-17T15:17:21Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 15-09-17 10:37 AM, Matthieu Moy wrote:\n> strtoul_ui uses strtoul to get a long unsigned, then checks that casting\n> to unsigned does not lose information and return the casted value.\n> \n> On 64 bits architecture, checking that the cast does not change the value\n> catches most errors, but when sizeof(int) == sizeof(long) (e.g. i386),\n> the check does nothing. Unfortunately, strtoul silently accepts negative\n> values, and as a result strtoul_ui(\"-1\", ...) raised no error.\n> \n> This patch catches negative values before it's too late, i.e. before\n> calling strtoul. We still silently accept very large integers that wrap\n> to a valid \"unsigned int\".\n> \n> Reported-by: Max Kirillov \n> Signed-off-by: Matthieu Moy \n> ---\n> So, here's a proper patch (I mean, a band-aid patch, but properly\n> send ;-) ).\n> \n> It should be merged before Kartik's series (or inserted at the start\n> of the series) so that we get the fix before the test breakage.\n> \n> git-compat-util.h | 3 +++\n> 1 file changed, 3 insertions(+)\n> \n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index f649e81..1df82fa 100644\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> \tchar *p;\n> \n> \terrno = 0;\n> +\t/* negative values would be accepted by strtoul */\n> +\tif (strchr(s, '-'))\n> +\t\treturn -1;\n\nI think this is broken, in that it doesn't match strtoul's normal behaviour,\nfor strings like \"1234-5678\", no?\n\nThe test also doesn't work if the string has leading whitespace (\" -5\").\n\n> \tul = strtoul(s, &p, base);\n> \tif (errno || *p || p == s || (unsigned int) ul != ul)\n> \t\treturn -1;\n\nHmm, but we check *p here, so IIUC it's an error if the string has any\ntrailing non-digits. Weird.\n\n\t\tM.\n"},{"id":"270218","messageId":"vpqr3lxoy9t.fsf@anie.imag.fr","threadId":"40371","inReplyTo":"55FAD981.5080808@xiplink.com","subject":"Re: [PATCH] strtoul_ui: reject negative values","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2015-09-17T15:34:22Z","receivedAt":"2015-09-17T15:34:22Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Marc Branchaud writes:\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>> \tchar *p;\n>> \n>> \terrno = 0;\n>> +\t/* negative values would be accepted by strtoul */\n>> +\tif (strchr(s, '-'))\n>> +\t\treturn -1;\n>\n> I think this is broken, in that it doesn't match strtoul's normal behaviour,\n> for strings like \"1234-5678\", no?\n\nThe goal here is just to read a positive integer value. Rejecting\n\"1234-5678\" is indeed a good thing. We already rejected it before my\npatch by checking for p (AKA endptr for strtoul), as you noted below.\n\n> The test also doesn't work if the string has leading whitespace (\"\n> -5\").\n\nWhy? It rejects any string that contain the character '-', regardless of\ntrailing spaces.\n\n>> \tul = strtoul(s, &p, base);\n>> \tif (errno || *p || p == s || (unsigned int) ul != ul)\n>> \t\treturn -1;\n>\n> Hmm, but we check *p here, so IIUC it's an error if the string has any\n> trailing non-digits. Weird.\n\nstrtoul_ui is more defensive than strtoul, by design.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"270223","messageId":"55FAE65B.1060204@xiplink.com","threadId":"40371","inReplyTo":"vpqr3lxoy9t.fsf@anie.imag.fr","subject":"Re: [PATCH] strtoul_ui: reject negative values","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2015-09-17T16:12:11Z","receivedAt":"2015-09-17T16:12:11Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 15-09-17 11:34 AM, Matthieu Moy wrote:\n> Marc Branchaud writes:\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>>> \tchar *p;\n>>> \n>>> \terrno = 0;\n>>> +\t/* negative values would be accepted by strtoul */\n>>> +\tif (strchr(s, '-'))\n>>> +\t\treturn -1;\n>>\n>> I think this is broken, in that it doesn't match strtoul's normal behaviour,\n>> for strings like \"1234-5678\", no?\n> \n> The goal here is just to read a positive integer value. Rejecting\n> \"1234-5678\" is indeed a good thing. We already rejected it before my\n> patch by checking for p (AKA endptr for strtoul), as you noted below.\n> \n>> The test also doesn't work if the string has leading whitespace (\"\n>> -5\").\n> \n> Why? It rejects any string that contain the character '-', regardless of\n> trailing spaces.\n\nRight, sorry.\n\n>>> \tul = strtoul(s, &p, base);\n>>> \tif (errno || *p || p == s || (unsigned int) ul != ul)\n>>> \t\treturn -1;\n>>\n>> Hmm, but we check *p here, so IIUC it's an error if the string has any\n>> trailing non-digits. Weird.\n> \n> strtoul_ui is more defensive than strtoul, by design.\n\nFair enough, just not what I expected from a function with that name.\n\n\t\tM.\n"},{"id":"270225","messageId":"xmqqpp1hj9yu.fsf@gitster.mtv.corp.google.com","threadId":"40371","inReplyTo":"1442500646-15293-1-git-send-email-Matthieu.Moy@imag.fr","subject":"Re: [PATCH] strtoul_ui: reject negative values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-17T16:18:17Z","receivedAt":"2015-09-17T16:18:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthieu Moy writes:\n\n> This patch catches negative values before it's too late, i.e. before\n> calling strtoul. We still silently accept very large integers that wrap\n> to a valid \"unsigned int\".\n\nIs the last statement correct? A very large long uint that wrap to\nuint would not fit in long uint and you would get ERANGE, no?\n\n> So, here's a proper patch (I mean, a band-aid patch, but properly\n> send ;-) ).\n\nYup.\n\n> It should be merged before Kartik's series (or inserted at the start\n> of the series) so that we get the fix before the test breakage.\n\nWhich one of his series?\n\n>\n> git-compat-util.h | 3 +++\n> 1 file changed, 3 insertions(+)\n>\n> diff --git a/git-compat-util.h b/git-compat-util.h\n> index f649e81..1df82fa 100644\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> \tchar *p;\n> \n> \terrno = 0;\n> +\t/* negative values would be accepted by strtoul */\n> +\tif (strchr(s, '-'))\n> +\t\treturn -1;\n> \tul = strtoul(s, &p, base);\n> \tif (errno || *p || p == s || (unsigned int) ul != ul)\n> \t\treturn -1;\n"},{"id":"270227","messageId":"1442507313-13028-1-git-send-email-Matthieu.Moy@imag.fr","threadId":"40371","inReplyTo":"xmqqpp1hj9yu.fsf@gitster.mtv.corp.google.com","subject":"[PATCH v2] strtoul_ui: reject negative values","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@imag.fr","sentAt":"2015-09-17T16:28:33Z","receivedAt":"2015-09-17T16:28:33Z","isPatch":true,"sender":{"key":"git@matthieu-moy.fr","avatar":"https://avatars.githubusercontent.com/u/14709?v=4"},"body":"strtoul_ui uses strtoul to get a long unsigned, then checks that casting\nto unsigned does not lose information and return the casted value.\n\nOn 64 bits architecture, checking that the cast does not change the value\ncatches most errors, but when sizeof(int) == sizeof(long) (e.g. i386),\nthe check does nothing. Unfortunately, strtoul silently accepts negative\nvalues, and as a result strtoul_ui(\"-1\", ...) raised no error.\n\nThis patch catches negative values before it's too late, i.e. before\ncalling strtoul.\n\nReported-by: Max Kirillov \nSigned-off-by: Matthieu Moy \n---\nJunio C Hamano writes:\n\n> Matthieu Moy writes:\n>\n>> This patch catches negative values before it's too late, i.e. before\n>> calling strtoul. We still silently accept very large integers that wrap\n>> to a valid \"unsigned int\".\n>\n> Is the last statement correct? A very large long uint that wrap to\n> uint would not fit in long uint and you would get ERANGE, no?\n\nIndeed. strtoul happily accepts negative values, but not overly large\nones.\n\nI removed the sentence from the message. Actually, I think we are now\naccepting exactly the right interval of values.\n\n>> It should be merged before Kartik's series (or inserted at the start\n>> of the series) so that we get the fix before the test breakage.\n>\n> Which one of his series?\n\nkn/for-each-tag, which uses strtoul_ui for align: and\ncontent:lines=.\n\n git-compat-util.h | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex f649e81..1df82fa 100644\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 \tchar *p;\n \n \terrno = 0;\n+\t/* negative values would be accepted by strtoul */\n+\tif (strchr(s, '-'))\n+\t\treturn -1;\n \tul = strtoul(s, &p, base);\n \tif (errno || *p || p == s || (unsigned int) ul != ul)\n \t\treturn -1;\n-- \n2.5.0.402.g8854c44\n"}]}