From a678df1bf928caeeef642ef07f73484a580fea57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 15 Sep 2019 14:03:25 +0200 Subject: [PATCH 1/2] rev-parse: demonstrate overflow of N for "foo^N" and "foo~N" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the number gets too high for an int, weird things may happen, as signed overflows are undefined. Add a test to show this; rev-parse "sucessfully" interprets 100000000000000000000000000000000 to be the same as 0, at least on x64 with GCC 9.2.1 and Clang 8.0.1, which is obviously bogus. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- t/t1506-rev-parse-diagnosis.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh index 4ee009da66..5c4df47401 100755 --- a/t/t1506-rev-parse-diagnosis.sh +++ b/t/t1506-rev-parse-diagnosis.sh @@ -215,4 +215,12 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' ' test_cmp expect actual ' +test_expect_failure 'reject Nth parent if N is too high' ' + test_must_fail git rev-parse HEAD^100000000000000000000000000000000 +' + +test_expect_failure 'reject Nth ancestor if N is too high' ' + test_must_fail git rev-parse HEAD~100000000000000000000000000000000 +' + test_done From 59fa5f5a25d9ccc57558ac44cce83d37ac1cec58 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Sun, 15 Sep 2019 14:10:28 +0200 Subject: [PATCH 2/2] sha1-name: check for overflow of N in "foo^N" and "foo~N" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reject values that don't fit into an int, as get_parent() and get_nth_ancestor() cannot handle them. That's better than potentially returning a random object. If this restriction turns out to be too tight then we can switch to a wider data type, but we'd still have to check for overflow. Signed-off-by: René Scharfe Signed-off-by: Junio C Hamano --- sha1-name.c | 15 ++++++++++++--- t/t1506-rev-parse-diagnosis.sh | 4 ++-- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/sha1-name.c b/sha1-name.c index 728e6f1f61..ea0bf6970e 100644 --- a/sha1-name.c +++ b/sha1-name.c @@ -1163,13 +1163,22 @@ static enum get_oid_result get_oid_1(struct repository *r, } if (has_suffix) { - int num = 0; + unsigned int num = 0; int len1 = cp - name; cp++; - while (cp < name + len) - num = num * 10 + *cp++ - '0'; + while (cp < name + len) { + unsigned int digit = *cp++ - '0'; + if (unsigned_mult_overflows(num, 10)) + return MISSING_OBJECT; + num *= 10; + if (unsigned_add_overflows(num, digit)) + return MISSING_OBJECT; + num += digit; + } if (!num && len1 == len - 1) num = 1; + else if (num > INT_MAX) + return MISSING_OBJECT; if (has_suffix == '^') return get_parent(r, name, len1, oid, num); /* else if (has_suffix == '~') -- goes without saying */ diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh index 5c4df47401..6a938b205b 100755 --- a/t/t1506-rev-parse-diagnosis.sh +++ b/t/t1506-rev-parse-diagnosis.sh @@ -215,11 +215,11 @@ test_expect_success 'arg before dashdash must be a revision (ambiguous)' ' test_cmp expect actual ' -test_expect_failure 'reject Nth parent if N is too high' ' +test_expect_success 'reject Nth parent if N is too high' ' test_must_fail git rev-parse HEAD^100000000000000000000000000000000 ' -test_expect_failure 'reject Nth ancestor if N is too high' ' +test_expect_success 'reject Nth ancestor if N is too high' ' test_must_fail git rev-parse HEAD~100000000000000000000000000000000 '