{"thread":{"id":"51852","subject":"[PATCH 1/2] rev-parse: demonstrate overflow of N for \"foo^N\" and \"foo~N\"","startedAt":"2019-09-15T12:08:16Z","lastAt":"2019-09-15T16:12:25Z","messageCount":4,"participants":["René Scharfe","brian m. carlson"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"382389","messageId":"c43610f2-a51f-7333-a200-a76930a0b705@web.de","threadId":"51852","inReplyTo":null,"subject":"[PATCH 1/2] rev-parse: demonstrate overflow of N for \"foo^N\" and \"foo~N\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-15T12:03:25Z","receivedAt":"2019-09-15T12:08:16Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If the number gets too high for an int, weird things may happen, as\nsigned overflows are undefined.  Add a test to show this; rev-parse\n\"sucessfully\" interprets 100000000000000000000000000000000 to be the\nsame as 0, at least on x64 with GCC 9.2.1 and Clang 8.0.1, which is\nobviously bogus.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n t/t1506-rev-parse-diagnosis.sh | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 4ee009da66..5c4df47401 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -215,4 +215,12 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n \ttest_cmp expect actual\n '\n\n+test_expect_failure 'reject Nth parent if N is too high' '\n+\ttest_must_fail git rev-parse HEAD^100000000000000000000000000000000\n+'\n+\n+test_expect_failure 'reject Nth ancestor if N is too high' '\n+\ttest_must_fail git rev-parse HEAD~100000000000000000000000000000000\n+'\n+\n test_done\n--\n2.23.0\n\n"},{"id":"382390","messageId":"2be6e3ee-209e-9cd1-eb43-284f9a8462b3@web.de","threadId":"51852","inReplyTo":"c43610f2-a51f-7333-a200-a76930a0b705@web.de","subject":"[PATCH 2/2] sha1-name: check for overflow of N in \"foo^N\" and \"foo~N\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-15T12:10:28Z","receivedAt":"2019-09-15T12:11:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Reject values that don't fit into an int, as get_parent() and\nget_nth_ancestor() cannot handle them.  That's better than potentially\nreturning a random object.\n\nIf this restriction turns out to be too tight then we can switch to a\nwider data type, but we'd still have to check for overflow.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n sha1-name.c                    | 15 ++++++++++++---\n t/t1506-rev-parse-diagnosis.sh |  4 ++--\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1-name.c b/sha1-name.c\nindex c665e3f96d..7a047e9e2b 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -1160,13 +1160,22 @@ static enum get_oid_result get_oid_1(struct repository *r,\n \t}\n\n \tif (has_suffix) {\n-\t\tint num = 0;\n+\t\tunsigned int num = 0;\n \t\tint len1 = cp - name;\n \t\tcp++;\n-\t\twhile (cp < name + len)\n-\t\t\tnum = num * 10 + *cp++ - '0';\n+\t\twhile (cp < name + len) {\n+\t\t\tunsigned int digit = *cp++ - '0';\n+\t\t\tif (unsigned_mult_overflows(num, 10))\n+\t\t\t\treturn MISSING_OBJECT;\n+\t\t\tnum *= 10;\n+\t\t\tif (unsigned_add_overflows(num, digit))\n+\t\t\t\treturn MISSING_OBJECT;\n+\t\t\tnum += digit;\n+\t\t}\n \t\tif (!num && len1 == len - 1)\n \t\t\tnum = 1;\n+\t\telse if (num > INT_MAX)\n+\t\t\treturn MISSING_OBJECT;\n \t\tif (has_suffix == '^')\n \t\t\treturn get_parent(r, name, len1, oid, num);\n \t\t/* else if (has_suffix == '~') -- goes without saying */\ndiff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh\nindex 5c4df47401..6a938b205b 100755\n--- a/t/t1506-rev-parse-diagnosis.sh\n+++ b/t/t1506-rev-parse-diagnosis.sh\n@@ -215,11 +215,11 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' '\n \ttest_cmp expect actual\n '\n\n-test_expect_failure 'reject Nth parent if N is too high' '\n+test_expect_success 'reject Nth parent if N is too high' '\n \ttest_must_fail git rev-parse HEAD^100000000000000000000000000000000\n '\n\n-test_expect_failure 'reject Nth ancestor if N is too high' '\n+test_expect_success 'reject Nth ancestor if N is too high' '\n \ttest_must_fail git rev-parse HEAD~100000000000000000000000000000000\n '\n\n--\n2.23.0\n"},{"id":"382391","messageId":"20190915151513.GU11334@genre.crustytoothpaste.net","threadId":"51852","inReplyTo":"2be6e3ee-209e-9cd1-eb43-284f9a8462b3@web.de","subject":"Re: [PATCH 2/2] sha1-name: check for overflow of N in \"foo^N\" and \"foo~N\"","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2019-09-15T15:15:13Z","receivedAt":"2019-09-15T15:15:24Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2019-09-15 at 12:10:28, René Scharfe wrote:\n> Reject values that don't fit into an int, as get_parent() and\n> get_nth_ancestor() cannot handle them.  That's better than potentially\n> returning a random object.\n> \n> If this restriction turns out to be too tight then we can switch to a\n> wider data type, but we'd still have to check for overflow.\n\nCertainly we want Git to perform as well as possible on large\nrepositories, but I doubt if it will scale to more than 2 billion\nrevisions, even with significant effort.  I think this restriction\nshould be fine.\n\n> diff --git a/sha1-name.c b/sha1-name.c\n> index c665e3f96d..7a047e9e2b 100644\n> --- a/sha1-name.c\n> +++ b/sha1-name.c\n> @@ -1160,13 +1160,22 @@ static enum get_oid_result get_oid_1(struct repository *r,\n>  \t}\n> \n>  \tif (has_suffix) {\n> -\t\tint num = 0;\n> +\t\tunsigned int num = 0;\n>  \t\tint len1 = cp - name;\n>  \t\tcp++;\n> -\t\twhile (cp < name + len)\n> -\t\t\tnum = num * 10 + *cp++ - '0';\n> +\t\twhile (cp < name + len) {\n> +\t\t\tunsigned int digit = *cp++ - '0';\n> +\t\t\tif (unsigned_mult_overflows(num, 10))\n> +\t\t\t\treturn MISSING_OBJECT;\n> +\t\t\tnum *= 10;\n> +\t\t\tif (unsigned_add_overflows(num, digit))\n> +\t\t\t\treturn MISSING_OBJECT;\n\nI was worried whether these functions only handled size_t or if they\nalso handle unsigned int, but I checked and they seem to be fine for any\nunsigned type.\n\n> +\t\t\tnum += digit;\n> +\t\t}\n>  \t\tif (!num && len1 == len - 1)\n>  \t\t\tnum = 1;\n> +\t\telse if (num > INT_MAX)\n> +\t\t\treturn MISSING_OBJECT;\n>  \t\tif (has_suffix == '^')\n>  \t\t\treturn get_parent(r, name, len1, oid, num);\n>  \t\t/* else if (has_suffix == '~') -- goes without saying */\n\nThis approach seems reasonable.  I must admit some curiosity as to how\nyou discovered this issue, though.  Did you have a cat assisting you in\ntyping revisions?\n-- \nbrian m. carlson: Houston, Texas, US\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"382392","messageId":"651e7bdb-c2e8-e607-d78d-5292b3509a8d@web.de","threadId":"51852","inReplyTo":"20190915151513.GU11334@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] sha1-name: check for overflow of N in \"foo^N\" and \"foo~N\"","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2019-09-15T16:12:13Z","receivedAt":"2019-09-15T16:12:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.09.19 um 17:15 schrieb brian m. carlson:\n> This approach seems reasonable.  I must admit some curiosity as to how\n> you discovered this issue, though.  Did you have a cat assisting you in\n> typing revisions?\n\nFound it by reading the code, but I'm not sure anymore what I was\nactually looking for.\n\nWould a fuzzer (or a cat) be able to catch that?  The function is\nhappily eating extra digits -- it's not crashing for me.\n\nRené\n"}]}