{"thread":{"id":"58668","subject":"[PATCH 1/3] git_parse_unsigned: reject negative values","startedAt":"2022-10-21T13:45:29Z","lastAt":"2022-11-10T02:35:15Z","messageCount":27,"participants":["Phillip Wood via GitGitGadget","Junio C Hamano","Jeff King","René Scharfe","Phillip Wood","Ævar Arnfjörð Bjarmason","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"465458","messageId":"9c8440e5e82777311c6217cb4a9ddcd5cb8ce689.1666359915.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.git.1666359915.gitgitgadget@gmail.com","subject":"[PATCH 1/3] git_parse_unsigned: reject negative values","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T13:45:12Z","receivedAt":"2022-10-21T13:45:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ngit_parse_unsigned() relies on strtoumax() which unfortunately parses\nnegative values as large positive integers. Fix this by rejecting any\nstring that contains '-' as we do in strtoul_ui(). I've chosen to treat\nnegative numbers as invalid input and set errno to EINVAL rather than\nERANGE one the basis that they are never acceptable if we're looking for\na unsigned integer.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..d5069d4f01d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\tuintmax_t val;\n \t\tuintmax_t factor;\n \n+\t\t/* negative values would be accepted by strtoumax */\n+\t\tif (strchr(value, '-')) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\terrno = 0;\n \t\tval = strtoumax(value, &end, 0);\n \t\tif (errno == ERANGE)\n-- \ngitgitgadget\n\n"},{"id":"465459","messageId":"pull.1389.git.1666359915.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":null,"subject":"[PATCH 0/3] a few config integer parsing fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T13:45:11Z","receivedAt":"2022-10-21T13:45:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This series fixes some issues I noticed when reading the integer parsing\ncode in config.c\n\n * git_parse_unsigned() does not reject negative values\n * git_parse_[un]signed() accept a units specifier without any digits\n * git_parse_signed() has in integer overflow when parsing MAXINT_MIN\n\nIdeally we'd have a test tool to unit test functions like this, I haven't\nfound time to write that yet. cc'ing René for patch 3 as he was the last\nperson to touch that code.\n\nPhillip Wood (3):\n  git_parse_unsigned: reject negative values\n  config: require at least one digit when parsing numbers\n  git_parse_signed(): avoid integer overflow\n\n config.c | 24 +++++++++++++++++++-----\n 1 file changed, 19 insertions(+), 5 deletions(-)\n\n\nbase-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1389%2Fphillipwood%2Fconfig-integer-parsing-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1389/phillipwood/config-integer-parsing-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1389\n-- \ngitgitgadget\n"},{"id":"465460","messageId":"cd753602e48a2faa0d59edca2f6fab0fe753f0f6.1666359915.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.git.1666359915.gitgitgadget@gmail.com","subject":"[PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T13:45:13Z","receivedAt":"2022-10-21T13:45:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the input to strtoimax() or strtoumax() does not contain any digits\nthen they return zero and set `end` to point to the start of the input\nstring.  git_parse_[un]signed() do not check `end` and so fail to return\nan error and instead return a value of zero if the input string is a\nvalid units factor without any digits (e.g \"k\").\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex d5069d4f01d..b7fb68026d8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\tval = strtoimax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n+\t\tif (end == value) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\n@@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\tval = strtoumax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n+\t\tif (end == value) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\n-- \ngitgitgadget\n\n"},{"id":"465461","messageId":"f058f391c3821b341a15fda9ae9fd20dda6a0494.1666359915.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.git.1666359915.gitgitgadget@gmail.com","subject":"[PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-10-21T13:45:14Z","receivedAt":"2022-10-21T13:45:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ngit_parse_signed() checks that the absolute value of the parsed string\nis less than or equal to a caller supplied maximum value. When\ncalculating the absolute value there is a integer overflow if `val ==\nINTMAX_MIN`. To fix this avoid negating `val` when it is negative by\nhaving separate overflow checks for positive and negative values.\n\nAn alternative would be to special case INTMAX_MIN before negating `val`\nas it is always out of range. That would enable us to keep the existing\ncode but I'm not sure that the current two-stage check is any clearer\nthan the new version.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b7fb68026d8..aad3e00341d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \tif (value && *value) {\n \t\tchar *end;\n \t\tintmax_t val;\n-\t\tuintmax_t uval;\n-\t\tuintmax_t factor;\n+\t\tintmax_t factor;\n+\n+\t\tif (max < 0)\n+\t\t\tBUG(\"max must be a positive integer\");\n \n \t\terrno = 0;\n \t\tval = strtoimax(value, &end, 0);\n@@ -1176,9 +1178,8 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\t\terrno = EINVAL;\n \t\t\treturn 0;\n \t\t}\n-\t\tuval = val < 0 ? -val : val;\n-\t\tif (unsigned_mult_overflows(factor, uval) ||\n-\t\t    factor * uval > max) {\n+\t\tif ((val < 0 && -max / factor > val) ||\n+\t\t    (val > 0 && max / factor < val)) {\n \t\t\terrno = ERANGE;\n \t\t\treturn 0;\n \t\t}\n-- \ngitgitgadget\n"},{"id":"465500","messageId":"xmqqbkq5kq0m.fsf@gitster.g","threadId":"58668","inReplyTo":"9c8440e5e82777311c6217cb4a9ddcd5cb8ce689.1666359915.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] git_parse_unsigned: reject negative values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T18:09:13Z","receivedAt":"2022-10-21T18:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> git_parse_unsigned() relies on strtoumax() which unfortunately parses\n> negative values as large positive integers. Fix this by rejecting any\n> string that contains '-' as we do in strtoul_ui(). I've chosen to treat\n> negative numbers as invalid input and set errno to EINVAL rather than\n> ERANGE one the basis that they are never acceptable if we're looking for\n> a unsigned integer.\n\nAnd the code now would reject something like \"43-21\" because it does\nnot insist that \"-\" must be used for a sign, so it makes EINVAL\ndoubly appropriate, I would think.  In the original code, \"43-21\"\nwould have been parsed as \"43\" (by strtoumax) followed by \"-\" (which\nis rejected by get_unit_factor() and yielded EINVAL, so this change\nwould not change the behaviour for such an input, if we receive\nEINVAL when we see a \"-\".\n\nA devil's advocate would consider if we ever want to have a unit\nfactor that has \"-\" in it (e.g. \"10000e-3\" == 10).  I can buy it if\nwe want to declare that it is not worth supporting.\n\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  config.c | 5 +++++\n>  1 file changed, 5 insertions(+)\n>\n> diff --git a/config.c b/config.c\n> index cbb5a3bab74..d5069d4f01d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n>  \t\tuintmax_t val;\n>  \t\tuintmax_t factor;\n>  \n> +\t\t/* negative values would be accepted by strtoumax */\n> +\t\tif (strchr(value, '-')) {\n> +\t\t\terrno = EINVAL;\n> +\t\t\treturn 0;\n> +\t\t}\n>  \t\terrno = 0;\n>  \t\tval = strtoumax(value, &end, 0);\n>  \t\tif (errno == ERANGE)\n"},{"id":"465501","messageId":"xmqqwn8tjazb.fsf@gitster.g","threadId":"58668","inReplyTo":"cd753602e48a2faa0d59edca2f6fab0fe753f0f6.1666359915.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T18:19:20Z","receivedAt":"2022-10-21T18:19:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> If the input to strtoimax() or strtoumax() does not contain any digits\n> then they return zero and set `end` to point to the start of the input\n> string.  git_parse_[un]signed() do not check `end` and so fail to return\n> an error and instead return a value of zero if the input string is a\n> valid units factor without any digits (e.g \"k\").\n>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  config.c | 8 ++++++++\n>  1 file changed, 8 insertions(+)\n>\n> diff --git a/config.c b/config.c\n> index d5069d4f01d..b7fb68026d8 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>  \t\tval = strtoimax(value, &end, 0);\n>  \t\tif (errno == ERANGE)\n>  \t\t\treturn 0;\n> +\t\tif (end == value) {\n> +\t\t\terrno = EINVAL;\n> +\t\t\treturn 0;\n> +\t\t}\n\nThis means well, but doesn't strto*() family of functions silently\nignore leading blanks, e.g.\n\n    l = strtol(\"  432k\", &end, 0);\n\t... l == 432, *end = k ...\n\nIf you really want to reject a string with no number before the\noptional unit, end at this point may not match value.  With \" k\" as\ninput, value would point at the space at the beginning, and end\nwould point at 'k'.\n\nIt does not look _too_ bad if we just let such an empty string\nthrough and interpreted it as zero.  Is that a problem?  Who are we\ntrying to help?\n\n>  \t\tfactor = get_unit_factor(end);\n>  \t\tif (!factor) {\n>  \t\t\terrno = EINVAL;\n> @@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n>  \t\tval = strtoumax(value, &end, 0);\n>  \t\tif (errno == ERANGE)\n>  \t\t\treturn 0;\n> +\t\tif (end == value) {\n> +\t\t\terrno = EINVAL;\n> +\t\t\treturn 0;\n> +\t\t}\n>  \t\tfactor = get_unit_factor(end);\n>  \t\tif (!factor) {\n>  \t\t\terrno = EINVAL;\n"},{"id":"465503","messageId":"xmqqpmeljae9.fsf@gitster.g","threadId":"58668","inReplyTo":"f058f391c3821b341a15fda9ae9fd20dda6a0494.1666359915.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-21T18:31:58Z","receivedAt":"2022-10-21T18:32:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> git_parse_signed() checks that the absolute value of the parsed string\n> is less than or equal to a caller supplied maximum value. When\n> calculating the absolute value there is a integer overflow if `val ==\n> INTMAX_MIN`.\n\nIt is a problem that is worth looking into.\n\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  config.c | 11 ++++++-----\n>  1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index b7fb68026d8..aad3e00341d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>  \tif (value && *value) {\n>  \t\tchar *end;\n>  \t\tintmax_t val;\n> -\t\tuintmax_t uval;\n> -\t\tuintmax_t factor;\n> +\t\tintmax_t factor;\n> +\n> +\t\tif (max < 0)\n> +\t\t\tBUG(\"max must be a positive integer\");\n\nIn parse_signed(), would we expect to accept end-user input that is\na negative integer?  We must.  Otherwise we would not be calling a\n\"signed\" parser.  Now, are there cases where the valid value range\nis bounded by a negative integer at the top?  No current callers may\npass such a value, but is it reasonable to add such a new constraints\nto an existing API function?\n\n"},{"id":"465507","messageId":"Y1L9fF6yd9EIIp9e@coredump.intra.peff.net","threadId":"58668","inReplyTo":"9c8440e5e82777311c6217cb4a9ddcd5cb8ce689.1666359915.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/3] git_parse_unsigned: reject negative values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T20:13:48Z","receivedAt":"2022-10-21T20:14:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 01:45:12PM +0000, Phillip Wood via GitGitGadget wrote:\n\n> git_parse_unsigned() relies on strtoumax() which unfortunately parses\n> negative values as large positive integers. Fix this by rejecting any\n> string that contains '-' as we do in strtoul_ui(). I've chosen to treat\n> negative numbers as invalid input and set errno to EINVAL rather than\n> ERANGE one the basis that they are never acceptable if we're looking for\n> a unsigned integer.\n\nCertainly this seems like the right thing to do for a function parsing\nan unsigned value. It would be nice if we could demonstrate the visible\neffect with a test, though (and of course catch any later regressions).\n\nSadly \"git config\" doesn't let you ask to parse an unsigned type. But we\ncan find things in the core.* region that are parsed by default (and not\nlikely to change, as they represent file/memory sizes). Like:\n\n  git -c core.bigFileThreshold=-1 rev-parse\n\nwhich quietly passes before your patch, but fails after.\n\nIt does make me wonder if anybody uses a negative value like this in the\nwild for \"no limit\", as it does what you might imagine currently (I get\n2^64-1).\n\n-Peff\n"},{"id":"465508","messageId":"Y1L+Qv+cs1bjqjK9@coredump.intra.peff.net","threadId":"58668","inReplyTo":"cd753602e48a2faa0d59edca2f6fab0fe753f0f6.1666359915.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-21T20:17:06Z","receivedAt":"2022-10-21T20:17:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n> \n> If the input to strtoimax() or strtoumax() does not contain any digits\n> then they return zero and set `end` to point to the start of the input\n> string.  git_parse_[un]signed() do not check `end` and so fail to return\n> an error and instead return a value of zero if the input string is a\n> valid units factor without any digits (e.g \"k\").\n\nThis one is easier to test than the last. Just:\n\n  git config --int --default='m' some.key\n\nworks. And even playing devil's advocate, I can't think of a case where\nanybody would rely on the current behavior.\n\n-Peff\n"},{"id":"465563","messageId":"80fe7c62-2c59-a715-3a8b-2dc2331d8e37@web.de","threadId":"58668","inReplyTo":"xmqqpmeljae9.fsf@gitster.g","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-22T08:09:26Z","receivedAt":"2022-10-22T11:06:23Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 21.10.22 um 20:31 schrieb Junio C Hamano:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> git_parse_signed() checks that the absolute value of the parsed string\n>> is less than or equal to a caller supplied maximum value. When\n>> calculating the absolute value there is a integer overflow if `val ==\n>> INTMAX_MIN`.\n>\n> It is a problem that is worth looking into.\n>\n>> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> ---\n>>  config.c | 11 ++++++-----\n>>  1 file changed, 6 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/config.c b/config.c\n>> index b7fb68026d8..aad3e00341d 100644\n>> --- a/config.c\n>> +++ b/config.c\n>> @@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>>  \tif (value && *value) {\n>>  \t\tchar *end;\n>>  \t\tintmax_t val;\n>> -\t\tuintmax_t uval;\n>> -\t\tuintmax_t factor;\n>> +\t\tintmax_t factor;\n>> +\n>> +\t\tif (max < 0)\n>> +\t\t\tBUG(\"max must be a positive integer\");\n>\n> In parse_signed(), would we expect to accept end-user input that is\n> a negative integer?  We must.  Otherwise we would not be calling a\n> \"signed\" parser.  Now, are there cases where the valid value range\n> is bounded by a negative integer at the top?  No current callers may\n> pass such a value, but is it reasonable to add such a new constraints\n> to an existing API function?\n\nHmm, if minimum and maximum are not symmetric, then we need to supply\nboth, don't we?\n\n--- >8 ---\nSubject: [PATCH] config: let git_parse_signed() check minimum\n\ngit_parse_signed() checks that the absolute value of the parsed string\nis less than or equal to a caller supplied maximum value.  When\ncalculating the absolute value there is a integer overflow if `val ==\nINTMAX_MIN`.\n\nAvoid overflow during sign inversion by supplying the minimum value to\nthe function as well.  Make `factor` signed to avoid promoting the\ndivision results in the limit check line to unsigned, but check whether\nit's positive.\n\nAdd a new macro, minimum_signed_value_of_type, and use it in the callers\nof git_parse_signed() to provide the newly required minimum value.  It\ncalculates the minimum for a given type using division instead of right\nshift because the latter is implementation-defined for signed types,\nand we need an arithmetic right shift here, not a logical one.\n\nOriginal-patch-by: Phillip Wood <phillip.wood@dunelm.org.uk>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\ngit_config_get_expiry_in_days() could use git_parse_int(), but that's\na different topic.\n\n config.c          | 28 +++++++++++++++++-----------\n git-compat-util.h |  2 ++\n 2 files changed, 19 insertions(+), 11 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab7..7cf196dc84 100644\n--- a/config.c\n+++ b/config.c\n@@ -1155,26 +1155,24 @@ static uintmax_t get_unit_factor(const char *end)\n \treturn 0;\n }\n\n-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n+static int git_parse_signed(const char *value, intmax_t *ret,\n+\t\t\t    intmax_t min, intmax_t max)\n {\n \tif (value && *value) {\n \t\tchar *end;\n \t\tintmax_t val;\n-\t\tuintmax_t uval;\n-\t\tuintmax_t factor;\n+\t\tintmax_t factor;\n\n \t\terrno = 0;\n \t\tval = strtoimax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n \t\tfactor = get_unit_factor(end);\n-\t\tif (!factor) {\n+\t\tif (factor < 1) {\n \t\t\terrno = EINVAL;\n \t\t\treturn 0;\n \t\t}\n-\t\tuval = val < 0 ? -val : val;\n-\t\tif (unsigned_mult_overflows(factor, uval) ||\n-\t\t    factor * uval > max) {\n+\t\tif (val < min / factor || val > max / factor) {\n \t\t\terrno = ERANGE;\n \t\t\treturn 0;\n \t\t}\n@@ -1218,7 +1216,9 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n static int git_parse_int(const char *value, int *ret)\n {\n \tintmax_t tmp;\n-\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))\n+\tif (!git_parse_signed(value, &tmp,\n+\t\t\t      minimum_signed_value_of_type(int),\n+\t\t\t      maximum_signed_value_of_type(int)))\n \t\treturn 0;\n \t*ret = tmp;\n \treturn 1;\n@@ -1227,7 +1227,9 @@ static int git_parse_int(const char *value, int *ret)\n static int git_parse_int64(const char *value, int64_t *ret)\n {\n \tintmax_t tmp;\n-\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))\n+\tif (!git_parse_signed(value, &tmp,\n+\t\t\t      minimum_signed_value_of_type(int64_t),\n+\t\t\t      maximum_signed_value_of_type(int64_t)))\n \t\treturn 0;\n \t*ret = tmp;\n \treturn 1;\n@@ -1245,7 +1247,9 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n int git_parse_ssize_t(const char *value, ssize_t *ret)\n {\n \tintmax_t tmp;\n-\tif (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))\n+\tif (!git_parse_signed(value, &tmp,\n+\t\t\t      minimum_signed_value_of_type(ssize_t),\n+\t\t\t      maximum_signed_value_of_type(ssize_t)))\n \t\treturn 0;\n \t*ret = tmp;\n \treturn 1;\n@@ -2751,7 +2755,9 @@ int git_config_get_expiry_in_days(const char *key, timestamp_t *expiry, timestam\n \tif (git_config_get_string_tmp(key, &expiry_string))\n \t\treturn 1; /* no such thing */\n\n-\tif (git_parse_signed(expiry_string, &days, maximum_signed_value_of_type(int))) {\n+\tif (git_parse_signed(expiry_string, &days,\n+\t\t\t     minimum_signed_value_of_type(int),\n+\t\t\t     maximum_signed_value_of_type(int))) {\n \t\tconst int scale = 86400;\n \t\t*expiry = now - days * scale;\n \t\treturn 0;\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ea53ea4a78..35425c373b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -112,6 +112,8 @@ struct strbuf;\n\n #define bitsizeof(x)  (CHAR_BIT * sizeof(x))\n\n+#define minimum_signed_value_of_type(a) \\\n+    (INTMAX_MIN / ((intmax_t)1 << (bitsizeof(intmax_t) - bitsizeof(a))))\n #define maximum_signed_value_of_type(a) \\\n     (INTMAX_MAX >> (bitsizeof(intmax_t) - bitsizeof(a)))\n\n--\n2.38.1\n"},{"id":"465566","messageId":"xmqqv8obhkeb.fsf@gitster.g","threadId":"58668","inReplyTo":"80fe7c62-2c59-a715-3a8b-2dc2331d8e37@web.de","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T16:51:08Z","receivedAt":"2022-10-22T16:51:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n>>> +\t\tif (max < 0)\n>>> +\t\t\tBUG(\"max must be a positive integer\");\n>>\n>> In parse_signed(), would we expect to accept end-user input that is\n>> a negative integer?  We must.  Otherwise we would not be calling a\n>> \"signed\" parser.  Now, are there cases where the valid value range\n>> is bounded by a negative integer at the top?  No current callers may\n>> pass such a value, but is it reasonable to add such a new constraints\n>> to an existing API function?\n>\n> Hmm, if minimum and maximum are not symmetric, then we need to supply\n> both, don't we?\n\nAh, thanks for injecting doze of sanity---I totally missed that the\nbound was about the absolute value, so we can say \"this is signed,\nand the allowed values are (-3, -2, -1, 0, 1, 2, 3).  If so, then the\n\"reject negative max\" in the posted patch is not a problem as I said\nabove.  I somehow thought that giving -1 as \"max\" would allow callers\nto say \"non-negative numbers are not allowed\".  But that is not what\nis going on.\n\nAllowing callers to specify both lower and uppoer bounds so that\nthey can say \"the allowed values are (-1, 0, 1, 2, 3)\", while it\nmight make it more useful, is a separate new feature development and\noutside the scope of \"let's tighten the parsing of end user input\"\nPhillip has here.\n\nSorry about the thinko, and thanks for a new and interesting\ntangent.\n"},{"id":"465570","messageId":"xmqqilkbhhlw.fsf@gitster.g","threadId":"58668","inReplyTo":"Y1L+Qv+cs1bjqjK9@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T17:51:23Z","receivedAt":"2022-10-22T17:51:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:\n>\n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>> \n>> If the input to strtoimax() or strtoumax() does not contain any digits\n>> then they return zero and set `end` to point to the start of the input\n>> string.  git_parse_[un]signed() do not check `end` and so fail to return\n>> an error and instead return a value of zero if the input string is a\n>> valid units factor without any digits (e.g \"k\").\n>\n> This one is easier to test than the last. Just:\n>\n>   git config --int --default='m' some.key\n>\n> works. And even playing devil's advocate, I can't think of a case where\n> anybody would rely on the current behavior.\n\nHmph, but --default=\" m\" would not be caught with the patch with the\nsame error, but is still a valid way to say zero mega unit.\n\nPersonally I do not see if this one is worth worrying about at all,\neven though the fixes in 1/3 and 3/3 may be more worthwhile.\n\nTHanks.\n"},{"id":"465571","messageId":"xmqqeduzhhgw.fsf@gitster.g","threadId":"58668","inReplyTo":"Y1L9fF6yd9EIIp9e@coredump.intra.peff.net","subject":"Re: [PATCH 1/3] git_parse_unsigned: reject negative values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T17:54:23Z","receivedAt":"2022-10-22T17:54:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> It does make me wonder if anybody uses a negative value like this in the\n> wild for \"no limit\", as it does what you might imagine currently (I get\n> 2^64-1).\n\nI vaguely recall complaints on the command line argument that used\nto take -1 to mean \"practically unlimited\" when its parsing got\ntightened.  I wouldn't be surprised if we are making a new issue in\nthe same category in the configuration file with this change.\n\nBut we can switch to the signed variant when it becomes an issue, I\nguess.\n\n"},{"id":"465575","messageId":"Y1RRrK4/2vr/YPfz@coredump.intra.peff.net","threadId":"58668","inReplyTo":"xmqqilkbhhlw.fsf@gitster.g","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-22T20:25:16Z","receivedAt":"2022-10-22T20:38:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 22, 2022 at 10:51:23AM -0700, Junio C Hamano wrote:\n\n> > This one is easier to test than the last. Just:\n> >\n> >   git config --int --default='m' some.key\n> >\n> > works. And even playing devil's advocate, I can't think of a case where\n> > anybody would rely on the current behavior.\n> \n> Hmph, but --default=\" m\" would not be caught with the patch with the\n> same error, but is still a valid way to say zero mega unit.\n\nI assumed that --default=\" m\" was supposed to be an error. It is already\nin the current code (because strtoimax doesn't skip the whitespace).\n\nI admit I don't care much either way, though.\n\n-Peff\n"},{"id":"465578","messageId":"xmqq7d0rh8u8.fsf@gitster.g","threadId":"58668","inReplyTo":"Y1RRrK4/2vr/YPfz@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-22T21:00:47Z","receivedAt":"2022-10-22T21:00:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I admit I don't care much either way, though.\n\nMe neither.  If the patch were to make --default=m mean \"one mega unit\"\ninstead of \"zero mega unit\", then I may care a bit more, though.\n"},{"id":"465595","messageId":"c24c3ac9-0de6-62f6-607f-2d8f69ca9fa8@web.de","threadId":"58668","inReplyTo":"xmqqv8obhkeb.fsf@gitster.g","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-23T05:57:00Z","receivedAt":"2022-10-23T05:57:17Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 22.10.22 um 18:51 schrieb Junio C Hamano:\n> René Scharfe <l.s.r@web.de> writes:\n>\n>>>> +\t\tif (max < 0)\n>>>> +\t\t\tBUG(\"max must be a positive integer\");\n>>>\n>>> In parse_signed(), would we expect to accept end-user input that is\n>>> a negative integer?  We must.  Otherwise we would not be calling a\n>>> \"signed\" parser.  Now, are there cases where the valid value range\n>>> is bounded by a negative integer at the top?  No current callers may\n>>> pass such a value, but is it reasonable to add such a new constraints\n>>> to an existing API function?\n>>\n>> Hmm, if minimum and maximum are not symmetric, then we need to supply\n>> both, don't we?\n>\n> Ah, thanks for injecting doze of sanity---I totally missed that the\n> bound was about the absolute value, so we can say \"this is signed,\n> and the allowed values are (-3, -2, -1, 0, 1, 2, 3).  If so, then the\n> \"reject negative max\" in the posted patch is not a problem as I said\n> above.  I somehow thought that giving -1 as \"max\" would allow callers\n> to say \"non-negative numbers are not allowed\".  But that is not what\n> is going on.\n\nRight, currently the value of `max` is used to check the absolute value,\ni.e. it imposes a limit in both the positive and negative direction.\n\n> Allowing callers to specify both lower and uppoer bounds so that\n> they can say \"the allowed values are (-1, 0, 1, 2, 3)\", while it\n> might make it more useful, is a separate new feature development and\n> outside the scope of \"let's tighten the parsing of end user input\"\n> Phillip has here.\n\nAllowing arbitrary limits in both directions might be a new feature, but\nit's required if we want to support the full range of values.  E.g. on\nmy system INT_MAX is 2147483647 and INT_MIN is -2147483647-1.  Currently\ngit_parse_int() rejects INT_MIN as out of range.\n\nIn my eyes the assumption that a single limit can be used to check both\ndirections of the signed number line is flawed and caused the undefined\nbehavior.  Dropping it avoids tricky calculations that try to infer the\nlower limit somehow and opens the full range of values to us.\n\nThat said, I'm not sure how useful the values INT_MIN, INT64_MIN and\nSSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually\nare. But doing the checks properly requires separate min and max values.\n\nRené\n\n\n[*] Perhaps git_parse_ssize_t() should reject values less than -1; only\nthat single negative number is needed to represent errors or unlimited.\n"},{"id":"465691","messageId":"cb034574-8753-d641-c166-ae2c34b1c589@dunelm.org.uk","threadId":"58668","inReplyTo":"xmqqwn8tjazb.fsf@gitster.g","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-25T09:54:24Z","receivedAt":"2022-10-25T10:01:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 21/10/2022 19:19, Junio C Hamano wrote:\n> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>>   \t\tval = strtoimax(value, &end, 0);\n>>   \t\tif (errno == ERANGE)\n>>   \t\t\treturn 0;\n>> +\t\tif (end == value) {\n>> +\t\t\terrno = EINVAL;\n>> +\t\t\treturn 0;\n>> +\t\t}\n> \n> This means well, but doesn't strto*() family of functions silently\n> ignore leading blanks, e.g.\n> \n>      l = strtol(\"  432k\", &end, 0);\n> \t... l == 432, *end = k ...\n> \n> If you really want to reject a string with no number before the\n> optional unit, end at this point may not match value.  With \" k\" as\n> input, value would point at the space at the beginning, and end\n> would point at 'k'.\n\nIt only skips the space if it sees a digit, if it does not find anything \nto convert it sets *end = start. Using peff's trick for testing this \npatch we can see there is no change in behavior if there is leading \nwhitespace\n\n$ bin-wrappers/git config --int --default \" m\" some.key\nfatal: bad numeric config value ' m' for 'some.key': invalid unit\n\n$ git config --int --default \" m\" some.key\nfatal: bad numeric config value ' m' for 'some.key': invalid unit\n\n> It does not look _too_ bad if we just let such an empty string\n> through and interpreted it as zero.  Is that a problem?  Who are we\n> trying to help?\n\nMy reasoning was that a single units factor is likely to be the result \nof some kind of bad edit and defaulting to zero when the user thought \nthey set a large value is not likely to be helpful. Having said that I'm \nnot that wedded to this patch if you feel it would be better to drop it.\n\nBest Wishes\n\nPhillip\n"},{"id":"465692","messageId":"69b91e29-af50-81db-8173-8b42a76ae1fb@dunelm.org.uk","threadId":"58668","inReplyTo":"Y1L+Qv+cs1bjqjK9@coredump.intra.peff.net","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-25T09:55:38Z","receivedAt":"2022-10-25T10:03:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Peff\n\nOn 21/10/2022 21:17, Jeff King wrote:\n> On Fri, Oct 21, 2022 at 01:45:13PM +0000, Phillip Wood via GitGitGadget wrote:\n> \n>> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>>\n>> If the input to strtoimax() or strtoumax() does not contain any digits\n>> then they return zero and set `end` to point to the start of the input\n>> string.  git_parse_[un]signed() do not check `end` and so fail to return\n>> an error and instead return a value of zero if the input string is a\n>> valid units factor without any digits (e.g \"k\").\n> \n> This one is easier to test than the last. Just:\n> \n>    git config --int --default='m' some.key\n\nThanks for posting that, I'd forgotten about the --int flag for git config.\n\nBest Wishes\n\nPhillip\n\n> works. And even playing devil's advocate, I can't think of a case where\n> anybody would rely on the current behavior.\n> \n> -Peff\n"},{"id":"465693","messageId":"190de302-0898-50a1-1a9d-ab1ccc5c6bb8@dunelm.org.uk","threadId":"58668","inReplyTo":"c24c3ac9-0de6-62f6-607f-2d8f69ca9fa8@web.de","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-10-25T10:00:02Z","receivedAt":"2022-10-25T10:06:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 23/10/2022 06:57, René Scharfe wrote:\n> Am 22.10.22 um 18:51 schrieb Junio C Hamano:\n>> René Scharfe <l.s.r@web.de> writes:\n>>\n>>>>> +\t\tif (max < 0)\n>>>>> +\t\t\tBUG(\"max must be a positive integer\");\n>>>>\n>>>> In parse_signed(), would we expect to accept end-user input that is\n>>>> a negative integer?  We must.  Otherwise we would not be calling a\n>>>> \"signed\" parser.  Now, are there cases where the valid value range\n>>>> is bounded by a negative integer at the top?  No current callers may\n>>>> pass such a value, but is it reasonable to add such a new constraints\n>>>> to an existing API function?\n>>>\n>>> Hmm, if minimum and maximum are not symmetric, then we need to supply\n>>> both, don't we?\n>>\n>> Ah, thanks for injecting doze of sanity---I totally missed that the\n>> bound was about the absolute value, so we can say \"this is signed,\n>> and the allowed values are (-3, -2, -1, 0, 1, 2, 3).  If so, then the\n>> \"reject negative max\" in the posted patch is not a problem as I said\n>> above.  I somehow thought that giving -1 as \"max\" would allow callers\n>> to say \"non-negative numbers are not allowed\".  But that is not what\n>> is going on.\n> \n> Right, currently the value of `max` is used to check the absolute value,\n> i.e. it imposes a limit in both the positive and negative direction.\n> \n>> Allowing callers to specify both lower and uppoer bounds so that\n>> they can say \"the allowed values are (-1, 0, 1, 2, 3)\", while it\n>> might make it more useful, is a separate new feature development and\n>> outside the scope of \"let's tighten the parsing of end user input\"\n>> Phillip has here.\n> \n> Allowing arbitrary limits in both directions might be a new feature, but\n> it's required if we want to support the full range of values.  E.g. on\n> my system INT_MAX is 2147483647 and INT_MIN is -2147483647-1.  Currently\n> git_parse_int() rejects INT_MIN as out of range.\n> \n> In my eyes the assumption that a single limit can be used to check both\n> directions of the signed number line is flawed and caused the undefined\n> behavior.  Dropping it avoids tricky calculations that try to infer the\n> lower limit somehow and opens the full range of values to us.\n> \n> That said, I'm not sure how useful the values INT_MIN, INT64_MIN and\n> SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*]) actually\n> are. But doing the checks properly requires separate min and max values.\n\nI'm happy to go either way, while I agree passing separate limits to \nallow INT_MIN is technically correct I'm not sure anyone has complained \nthat the current code is too restrictive.\n\nBest Wishes\n\nPhillip\n\n> René\n> \n> \n> [*] Perhaps git_parse_ssize_t() should reject values less than -1; only\n> that single negative number is needed to represent errors or unlimited.\n"},{"id":"465714","messageId":"xmqqh6zropgo.fsf@gitster.g","threadId":"58668","inReplyTo":"cb034574-8753-d641-c166-ae2c34b1c589@dunelm.org.uk","subject":"Re: [PATCH 2/3] config: require at least one digit when parsing numbers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-25T16:08:55Z","receivedAt":"2022-10-25T16:09:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n> On 21/10/2022 19:19, Junio C Hamano wrote:\n>> \"Phillip Wood via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>>> @@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n>>>   \t\tval = strtoimax(value, &end, 0);\n>>>   \t\tif (errno == ERANGE)\n>>>   \t\t\treturn 0;\n>>> +\t\tif (end == value) {\n>>> +\t\t\terrno = EINVAL;\n>>> +\t\t\treturn 0;\n>>> +\t\t}\n>> This means well, but doesn't strto*() family of functions silently\n>> ignore leading blanks, e.g.\n>>      l = strtol(\"  432k\", &end, 0);\n>> \t... l == 432, *end = k ...\n>> If you really want to reject a string with no number before the\n>> optional unit, end at this point may not match value.  With \" k\" as\n>> input, value would point at the space at the beginning, and end\n>> would point at 'k'.\n>\n> It only skips the space if it sees a digit, if it does not find\n> anything to convert it sets *end = start.\n\nYeah, thanks.  Yes, my earlier observation was based on a faulty\nexperiments, and the code posted as-is would be OK.\n\n"},{"id":"465761","messageId":"20ba163c-b8a2-7f99-d2c2-11c7c38ab32a@web.de","threadId":"58668","inReplyTo":"190de302-0898-50a1-1a9d-ab1ccc5c6bb8@dunelm.org.uk","subject":"Re: [PATCH 3/3] git_parse_signed(): avoid integer overflow","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-26T11:01:09Z","receivedAt":"2022-10-26T11:01:28Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 25.10.22 um 12:00 schrieb Phillip Wood:\n> On 23/10/2022 06:57, René Scharfe wrote:\n>>\n>> That said, I'm not sure how useful the values INT_MIN, INT64_MIN\n>> and SSIZE_MIN (which unlike SSIZE_MAX is not defined by POSIX [*])\n>> actually are. But doing the checks properly requires separate min\n>> and max values.\n>\n> I'm happy to go either way, while I agree passing separate limits to\n> allow INT_MIN is technically correct I'm not sure anyone has\n> complained that the current code is too restrictive.\nRight.  Having separate patches for the different aspects (your fix,\nsimplification due to adding a min parameter, range extension made\npossible by that) is probably best to discuss the merits of each.\n\nRené\n"},{"id":"466951","messageId":"pull.1389.v2.git.1668003388.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.git.1666359915.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] a few config integer parsing fixes","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:16:25Z","receivedAt":"2022-11-09T14:16:37Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"This series fixes some issues I noticed when reading the integer parsing\ncode in config.c\n\n * git_parse_unsigned() does not reject negative values\n * git_parse_[un]signed() accept a units specifier without any digits\n * git_parse_signed() has in integer overflow when parsing MAXINT_MIN\n\nThanks to everyone who commented on V1. I've updated patches 1 & 2 to\ninclude the tests suggested by peff and added tests for OPT_MAGNITUDE() as\nthat uses the same code path.\n\nCover Letter for V1:\n\nIdeally we'd have a test tool to unit test functions like this, I haven't\nfound time to write that yet. cc'ing René for patch 3 as he was the last\nperson to touch that code.\n\nPhillip Wood (3):\n  git_parse_unsigned: reject negative values\n  config: require at least one digit when parsing numbers\n  git_parse_signed(): avoid integer overflow\n\n config.c                 | 24 +++++++++++++++++++-----\n t/t0040-parse-options.sh | 12 ++++++++++++\n t/t1050-large.sh         |  6 ++++++\n t/t1300-config.sh        |  6 ++++++\n 4 files changed, 43 insertions(+), 5 deletions(-)\n\n\nbase-commit: e85701b4af5b7c2a9f3a1b07858703318dce365d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1389%2Fphillipwood%2Fconfig-integer-parsing-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1389/phillipwood/config-integer-parsing-fixes-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1389\n\nRange-diff vs v1:\n\n 1:  9c8440e5e82 ! 1:  d1ac79909b9 git_parse_unsigned: reject negative values\n     @@ Commit message\n          string that contains '-' as we do in strtoul_ui(). I've chosen to treat\n          negative numbers as invalid input and set errno to EINVAL rather than\n          ERANGE one the basis that they are never acceptable if we're looking for\n     -    a unsigned integer.\n     +    a unsigned integer. This is also consistent with the existing behavior\n     +    of rejecting \"1–2\" with EINVAL.\n      \n     +    As we do not have unit tests for this function it is tested indirectly\n     +    by checking that negative values of reject for core.bigFileThreshold are\n     +    rejected. As this function is also used by OPT_MAGNITUDE() a test is\n     +    added to check that rejects negative values too.\n     +\n     +    Helped-by: Jeff King <peff@peff.net>\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## config.c ##\n     @@ config.c: static int git_parse_unsigned(const char *value, uintmax_t *ret, uintm\n       \t\terrno = 0;\n       \t\tval = strtoumax(value, &end, 0);\n       \t\tif (errno == ERANGE)\n     +\n     + ## t/t0040-parse-options.sh ##\n     +@@ t/t0040-parse-options.sh: test_expect_success 'subcommands are incompatible with KEEP_DASHDASH unless in c\n     + \tgrep ^BUG err\n     + '\n     + \n     ++test_expect_success 'negative magnitude' '\n     ++\ttest_must_fail test-tool parse-options --magnitude -1 >out 2>err &&\n     ++\tgrep \"non-negative integer\" err &&\n     ++\ttest_must_be_empty out\n     ++'\n     + test_done\n     +\n     + ## t/t1050-large.sh ##\n     +@@ t/t1050-large.sh: test_description='adding and checking out large blobs'\n     + \n     + . ./test-lib.sh\n     + \n     ++test_expect_success 'core.bigFileThreshold must be non-negative' '\n     ++\ttest_must_fail git -c core.bigFileThreshold=-1 rev-parse >out 2>err &&\n     ++\tgrep \"bad numeric config value\" err &&\n     ++\ttest_must_be_empty out\n     ++'\n     ++\n     + test_expect_success setup '\n     + \t# clone does not allow us to pass core.bigfilethreshold to\n     + \t# new repos, so set core.bigfilethreshold globally\n 2:  cd753602e48 ! 2:  54f2ebefa0d config: require at least one digit when parsing numbers\n     @@ Commit message\n          an error and instead return a value of zero if the input string is a\n          valid units factor without any digits (e.g \"k\").\n      \n     +    Tests are added to check that 'git config --int' and OPT_MAGNITUDE()\n     +    reject a units specifier without a leading digit.\n     +\n     +    Helped-by: Jeff King <peff@peff.net>\n          Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n      \n       ## config.c ##\n     @@ config.c: static int git_parse_unsigned(const char *value, uintmax_t *ret, uintm\n       \t\tfactor = get_unit_factor(end);\n       \t\tif (!factor) {\n       \t\t\terrno = EINVAL;\n     +\n     + ## t/t0040-parse-options.sh ##\n     +@@ t/t0040-parse-options.sh: test_expect_success 'negative magnitude' '\n     + \tgrep \"non-negative integer\" err &&\n     + \ttest_must_be_empty out\n     + '\n     ++\n     ++test_expect_success 'magnitude with units but no numbers' '\n     ++\ttest_must_fail test-tool parse-options --magnitude m >out 2>err &&\n     ++\tgrep \"non-negative integer\" err &&\n     ++\ttest_must_be_empty out\n     ++'\n     ++\n     + test_done\n     +\n     + ## t/t1300-config.sh ##\n     +@@ t/t1300-config.sh: test_expect_success '--type rejects unknown specifiers' '\n     + \ttest_i18ngrep \"unrecognized --type argument\" error\n     + '\n     + \n     ++test_expect_success '--type=int requires at least one digit' '\n     ++\ttest_must_fail git config --type int --default m some.key >out 2>error &&\n     ++\tgrep \"bad numeric config value\" error &&\n     ++\ttest_must_be_empty out\n     ++'\n     ++\n     + test_expect_success '--replace-all does not invent newlines' '\n     + \tq_to_tab >.git/config <<-\\EOF &&\n     + \t[abc]key\n 3:  f058f391c38 = 3:  673e6f1ab93 git_parse_signed(): avoid integer overflow\n\n-- \ngitgitgadget\n"},{"id":"466952","messageId":"d1ac79909b9e777cae40a6a301e5cfd988c5f9d7.1668003388.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.v2.git.1668003388.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] git_parse_unsigned: reject negative values","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:16:26Z","receivedAt":"2022-11-09T14:16:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ngit_parse_unsigned() relies on strtoumax() which unfortunately parses\nnegative values as large positive integers. Fix this by rejecting any\nstring that contains '-' as we do in strtoul_ui(). I've chosen to treat\nnegative numbers as invalid input and set errno to EINVAL rather than\nERANGE one the basis that they are never acceptable if we're looking for\na unsigned integer. This is also consistent with the existing behavior\nof rejecting \"1–2\" with EINVAL.\n\nAs we do not have unit tests for this function it is tested indirectly\nby checking that negative values of reject for core.bigFileThreshold are\nrejected. As this function is also used by OPT_MAGNITUDE() a test is\nadded to check that rejects negative values too.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c                 | 5 +++++\n t/t0040-parse-options.sh | 5 +++++\n t/t1050-large.sh         | 6 ++++++\n 3 files changed, 16 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex cbb5a3bab74..d5069d4f01d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\tuintmax_t val;\n \t\tuintmax_t factor;\n \n+\t\t/* negative values would be accepted by strtoumax */\n+\t\tif (strchr(value, '-')) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\terrno = 0;\n \t\tval = strtoumax(value, &end, 0);\n \t\tif (errno == ERANGE)\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 5cc62306e39..64d2327b744 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -709,4 +709,9 @@ test_expect_success 'subcommands are incompatible with KEEP_DASHDASH unless in c\n \tgrep ^BUG err\n '\n \n+test_expect_success 'negative magnitude' '\n+\ttest_must_fail test-tool parse-options --magnitude -1 >out 2>err &&\n+\tgrep \"non-negative integer\" err &&\n+\ttest_must_be_empty out\n+'\n test_done\ndiff --git a/t/t1050-large.sh b/t/t1050-large.sh\nindex 4f3aa17c994..c71932b0242 100755\n--- a/t/t1050-large.sh\n+++ b/t/t1050-large.sh\n@@ -5,6 +5,12 @@ test_description='adding and checking out large blobs'\n \n . ./test-lib.sh\n \n+test_expect_success 'core.bigFileThreshold must be non-negative' '\n+\ttest_must_fail git -c core.bigFileThreshold=-1 rev-parse >out 2>err &&\n+\tgrep \"bad numeric config value\" err &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success setup '\n \t# clone does not allow us to pass core.bigfilethreshold to\n \t# new repos, so set core.bigfilethreshold globally\n-- \ngitgitgadget\n\n"},{"id":"466953","messageId":"54f2ebefa0d8308dc00f6dd035c1baae4a8e1fc7.1668003388.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.v2.git.1668003388.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] config: require at least one digit when parsing numbers","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:16:27Z","receivedAt":"2022-11-09T14:16:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\nIf the input to strtoimax() or strtoumax() does not contain any digits\nthen they return zero and set `end` to point to the start of the input\nstring.  git_parse_[un]signed() do not check `end` and so fail to return\nan error and instead return a value of zero if the input string is a\nvalid units factor without any digits (e.g \"k\").\n\nTests are added to check that 'git config --int' and OPT_MAGNITUDE()\nreject a units specifier without a leading digit.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c                 | 8 ++++++++\n t/t0040-parse-options.sh | 7 +++++++\n t/t1300-config.sh        | 6 ++++++\n 3 files changed, 21 insertions(+)\n\ndiff --git a/config.c b/config.c\nindex d5069d4f01d..b7fb68026d8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1167,6 +1167,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\tval = strtoimax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n+\t\tif (end == value) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\n@@ -1202,6 +1206,10 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n \t\tval = strtoumax(value, &end, 0);\n \t\tif (errno == ERANGE)\n \t\t\treturn 0;\n+\t\tif (end == value) {\n+\t\t\terrno = EINVAL;\n+\t\t\treturn 0;\n+\t\t}\n \t\tfactor = get_unit_factor(end);\n \t\tif (!factor) {\n \t\t\terrno = EINVAL;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex 64d2327b744..7d7ecfd5716 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -714,4 +714,11 @@ test_expect_success 'negative magnitude' '\n \tgrep \"non-negative integer\" err &&\n \ttest_must_be_empty out\n '\n+\n+test_expect_success 'magnitude with units but no numbers' '\n+\ttest_must_fail test-tool parse-options --magnitude m >out 2>err &&\n+\tgrep \"non-negative integer\" err &&\n+\ttest_must_be_empty out\n+'\n+\n test_done\ndiff --git a/t/t1300-config.sh b/t/t1300-config.sh\nindex c6661e61af5..2575279ab84 100755\n--- a/t/t1300-config.sh\n+++ b/t/t1300-config.sh\n@@ -2228,6 +2228,12 @@ test_expect_success '--type rejects unknown specifiers' '\n \ttest_i18ngrep \"unrecognized --type argument\" error\n '\n \n+test_expect_success '--type=int requires at least one digit' '\n+\ttest_must_fail git config --type int --default m some.key >out 2>error &&\n+\tgrep \"bad numeric config value\" error &&\n+\ttest_must_be_empty out\n+'\n+\n test_expect_success '--replace-all does not invent newlines' '\n \tq_to_tab >.git/config <<-\\EOF &&\n \t[abc]key\n-- \ngitgitgadget\n\n"},{"id":"466954","messageId":"673e6f1ab93200ab3b6b9ca2bded5db5a3274329.1668003388.git.gitgitgadget@gmail.com","threadId":"58668","inReplyTo":"pull.1389.v2.git.1668003388.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] git_parse_signed(): avoid integer overflow","fromName":"Phillip Wood via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-11-09T14:16:28Z","receivedAt":"2022-11-09T14:16:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood <phillip.wood@dunelm.org.uk>\n\ngit_parse_signed() checks that the absolute value of the parsed string\nis less than or equal to a caller supplied maximum value. When\ncalculating the absolute value there is a integer overflow if `val ==\nINTMAX_MIN`. To fix this avoid negating `val` when it is negative by\nhaving separate overflow checks for positive and negative values.\n\nAn alternative would be to special case INTMAX_MIN before negating `val`\nas it is always out of range. That would enable us to keep the existing\ncode but I'm not sure that the current two-stage check is any clearer\nthan the new version.\n\nSigned-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n---\n config.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex b7fb68026d8..aad3e00341d 100644\n--- a/config.c\n+++ b/config.c\n@@ -1160,8 +1160,10 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \tif (value && *value) {\n \t\tchar *end;\n \t\tintmax_t val;\n-\t\tuintmax_t uval;\n-\t\tuintmax_t factor;\n+\t\tintmax_t factor;\n+\n+\t\tif (max < 0)\n+\t\t\tBUG(\"max must be a positive integer\");\n \n \t\terrno = 0;\n \t\tval = strtoimax(value, &end, 0);\n@@ -1176,9 +1178,8 @@ static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)\n \t\t\terrno = EINVAL;\n \t\t\treturn 0;\n \t\t}\n-\t\tuval = val < 0 ? -val : val;\n-\t\tif (unsigned_mult_overflows(factor, uval) ||\n-\t\t    factor * uval > max) {\n+\t\tif ((val < 0 && -max / factor > val) ||\n+\t\t    (val > 0 && max / factor < val)) {\n \t\t\terrno = ERANGE;\n \t\t\treturn 0;\n \t\t}\n-- \ngitgitgadget\n"},{"id":"466964","messageId":"221109.86pmdwp19z.gmgdl@evledraar.gmail.com","threadId":"58668","inReplyTo":"d1ac79909b9e777cae40a6a301e5cfd988c5f9d7.1668003388.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/3] git_parse_unsigned: reject negative values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-11-09T15:57:23Z","receivedAt":"2022-11-09T15:59:11Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 09 2022, Phillip Wood via GitGitGadget wrote:\n\n> From: Phillip Wood <phillip.wood@dunelm.org.uk>\n>\n> git_parse_unsigned() relies on strtoumax() which unfortunately parses\n> negative values as large positive integers. Fix this by rejecting any\n> string that contains '-' as we do in strtoul_ui(). I've chosen to treat\n> negative numbers as invalid input and set errno to EINVAL rather than\n> ERANGE one the basis that they are never acceptable if we're looking for\n> a unsigned integer. This is also consistent with the existing behavior\n> of rejecting \"1–2\" with EINVAL.\n>\n> As we do not have unit tests for this function it is tested indirectly\n> by checking that negative values of reject for core.bigFileThreshold are\n> rejected. As this function is also used by OPT_MAGNITUDE() a test is\n> added to check that rejects negative values too.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>\n> ---\n>  config.c                 | 5 +++++\n>  t/t0040-parse-options.sh | 5 +++++\n>  t/t1050-large.sh         | 6 ++++++\n>  3 files changed, 16 insertions(+)\n>\n> diff --git a/config.c b/config.c\n> index cbb5a3bab74..d5069d4f01d 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -1193,6 +1193,11 @@ static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)\n>  \t\tuintmax_t val;\n>  \t\tuintmax_t factor;\n>  \n> +\t\t/* negative values would be accepted by strtoumax */\n> +\t\tif (strchr(value, '-')) {\n> +\t\t\terrno = EINVAL;\n> +\t\t\treturn 0;\n> +\t\t}\n>  \t\terrno = 0;\n>  \t\tval = strtoumax(value, &end, 0);\n>  \t\tif (errno == ERANGE)\n\nThere's nothing wrong with this, but since the topic here is \"some\nissues I noticed\" here's another one: We don't actually care if you set\n\"errno = EINVAL\" here in particular, just as long as it's not \"ERANGE\",\nanything else will do.\n\nSo, not worth a re-roll in itself, but maybe a prep patch (or follow-up)\nto do this would be nice? to make sure this errno handling is\n\"reachable\"?\n\ndiff --git a/config.c b/config.c\nindex ff4ea29784b..33d05fde0ea 100644\n--- a/config.c\n+++ b/config.c\n@@ -1260,9 +1260,12 @@ NORETURN\n static void die_bad_number(const char *name, const char *value)\n {\n \tconst char *error_type = (errno == ERANGE) ?\n-\t\tN_(\"out of range\") : N_(\"invalid unit\");\n+\t\tN_(\"out of range\") : errno == EINVAL ? N_(\"invalid unit\") : NULL;\n \tconst char *bad_numeric = N_(\"bad numeric config value '%s' for '%s': %s\");\n \n+\tif (!error_type)\n+\t\tBUG(\"unhandled errno %d: %s\", errno, strerror(errno));\n+\n \tif (!value)\n \t\tvalue = \"\";\n \n"},{"id":"467025","messageId":"Y2xjXSAGmHILluuy@nand.local","threadId":"58668","inReplyTo":"pull.1389.v2.git.1668003388.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] a few config integer parsing fixes","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-10T02:35:09Z","receivedAt":"2022-11-10T02:35:15Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 09, 2022 at 02:16:25PM +0000, Phillip Wood via GitGitGadget wrote:\n> This series fixes some issues I noticed when reading the integer parsing\n> code in config.c\n>\n>  * git_parse_unsigned() does not reject negative values\n>  * git_parse_[un]signed() accept a units specifier without any digits\n>  * git_parse_signed() has in integer overflow when parsing MAXINT_MIN\n>\n> Thanks to everyone who commented on V1. I've updated patches 1 & 2 to\n> include the tests suggested by peff and added tests for OPT_MAGNITUDE() as\n> that uses the same code path.\n\nHaving read this round and the discussion on the original version, I\nthink that this is ready to start merging down.\n\nThat said, it would be nice to hear from some other reviewer(s) who\ntook a look or are interested in this area.\n\nThanks,\nTaylor\n"}]}