commit d2463c0cfee066111c3a72d188cd897957aa2988 Author: Nick Mathewson nickm@torproject.org Date: Wed Sep 10 23:57:31 2014 -0400
Avoid overflows and underflows in sscanf and friends
(Patch from teor on 13104) --- src/common/util.c | 23 +++++-- src/test/test_util.c | 167 ++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 171 insertions(+), 19 deletions(-)
diff --git a/src/common/util.c b/src/common/util.c index 2d7893b..297ae78 100644 --- a/src/common/util.c +++ b/src/common/util.c @@ -2804,10 +2804,14 @@ scan_unsigned(const char **bufp, unsigned long *out, int width, int base) while (**bufp && (hex?TOR_ISXDIGIT(**bufp):TOR_ISDIGIT(**bufp)) && scanned_so_far < width) { int digit = hex?hex_decode_digit(*(*bufp)++):digit_to_num(*(*bufp)++); - unsigned long new_result = result * base + digit; - if (new_result < result) - return -1; /* over/underflow. */ - result = new_result; + // Check for overflow beforehand, without actually causing any overflow + // This preserves functionality on compilers that don't wrap overflow + // (i.e. that trap or optimise away overflow) + // result * base + digit > ULONG_MAX + // result * base > ULONG_MAX - digit + if (result > (ULONG_MAX - digit)/base) + return -1; /* Processing this digit would overflow */ + result = result * base + digit; ++scanned_so_far; }
@@ -2842,10 +2846,17 @@ scan_signed(const char **bufp, long *out, int width) if (scan_unsigned(bufp, &result, width, 10) < 0) return -1;
- if (neg) { + if (neg && result > 0) { if (result > ((unsigned long)LONG_MAX) + 1) return -1; /* Underflow */ - *out = -(long)result; + // Avoid overflow on the cast to signed long when result is LONG_MIN + // by subtracting 1 from the unsigned long positive value, + // then, after it has been cast to signed and negated, + // subtracting the original 1 (the double-subtraction is intentional). + // Otherwise, the cast to signed could cause a temporary long + // to equal LONG_MAX + 1, which is undefined. + // We avoid underflow on the subtraction by treating -0 as positive. + *out = (-(long)(result - 1)) - 1; } else { if (result > LONG_MAX) return -1; /* Overflow */ diff --git a/src/test/test_util.c b/src/test/test_util.c index 151ec69..16a2927 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -1672,6 +1672,7 @@ static void test_util_sscanf(void) { unsigned u1, u2, u3; + unsigned long ulng; char s1[20], s2[10], s3[10], ch; int r; long lng1,lng2; @@ -1713,11 +1714,6 @@ test_util_sscanf(void) test_eq(0, tor_sscanf("", "%u", &u1)); /* absent number */ test_eq(0, tor_sscanf("A", "%u", &u1)); /* bogus number */ test_eq(0, tor_sscanf("-1", "%u", &u1)); /* negative number */ - test_eq(1, tor_sscanf("4294967295", "%u", &u1)); /* UINT32_MAX should work */ - test_eq(4294967295u, u1); - test_eq(0, tor_sscanf("4294967296", "%u", &u1)); /* But not at 32 bits */ - test_eq(1, tor_sscanf("4294967296", "%9u", &u1)); /* but parsing only 9... */ - test_eq(429496729u, u1);
/* Numbers with size (eg. %2u) */ test_eq(0, tor_sscanf("-1", "%2u", &u1)); @@ -1812,46 +1808,191 @@ test_util_sscanf(void) test_eq(int2, -1);
#if SIZEOF_INT == 4 + /* %u */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("4294967295", "%u", &u1)); + test_eq(4294967295U, u1); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("4294967296", "%u", &u1)); + /* but parsing only 9... */ + test_eq(1, tor_sscanf("4294967296", "%9u", &u1)); + test_eq(429496729U, u1); + + /* %x */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFF", "%x", &u1)); + test_eq(0xFFFFFFFF, u1); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("100000000", "%x", &u1)); + + /* %d */ + /* INT32_MIN and INT32_MAX should work */ r = tor_sscanf("-2147483648. 2147483647.", "%d. %d.", &int1, &int2); test_eq(r,2); - test_eq(int1, -2147483647-1); + test_eq(int1, -2147483647 - 1); test_eq(int2, 2147483647);
- r = tor_sscanf("-2147483679.", "%d.", &int1); + /* But INT32_MIN - 1 and INT32_MAX + 1 shouldn't work */ + r = tor_sscanf("-2147483649.", "%d.", &int1); test_eq(r,0);
- r = tor_sscanf("2147483678.", "%d.", &int1); + r = tor_sscanf("2147483648.", "%d.", &int1); + test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-2147483648. 2147483648.", + "%d. %d.", &int1, &int2); + test_eq(r,1); + + r = tor_sscanf("-2147483649. 2147483647.", + "%d. %d.", &int1, &int2); + test_eq(r,0); + + r = tor_sscanf("2147483648. -2147483649.", + "%d. %d.", &int1, &int2); test_eq(r,0); #elif SIZEOF_INT == 8 + /* %u */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("18446744073709551615", "%u", &u1)); + test_eq(18446744073709551615U, u1); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("18446744073709551616", "%u", &u1)); + /* but parsing only 19... */ + test_eq(1, tor_sscanf("18446744073709551616", "%19u", &u1)); + test_eq(1844674407370955161U, u1); + + /* %x */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFFFFFFFFFF", "%x", &u1)); + test_eq(0xFFFFFFFFFFFFFFFF, u1); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("10000000000000000", "%x", &u1)); + + /* %d */ + /* INT64_MIN and INT64_MAX should work */ r = tor_sscanf("-9223372036854775808. 9223372036854775807.", "%d. %d.", &int1, &int2); test_eq(r,2); - test_eq(int1, -9223372036854775807-1); + test_eq(int1, -9223372036854775807 - 1); test_eq(int2, 9223372036854775807);
+ /* But INT64_MIN - 1 and INT64_MAX + 1 shouldn't work */ r = tor_sscanf("-9223372036854775809.", "%d.", &int1); test_eq(r,0);
r = tor_sscanf("9223372036854775808.", "%d.", &int1); test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-9223372036854775808. 9223372036854775808.", + "%d. %d.", &int1, &int2); + test_eq(r,1); + + r = tor_sscanf("-9223372036854775809. 9223372036854775807.", + "%d. %d.", &int1, &int2); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808. -9223372036854775809.", + "%d. %d.", &int1, &int2); + test_eq(r,0); #endif
#if SIZEOF_LONG == 4 + /* %lu */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("4294967295", "%lu", &ulng)); + test_eq(4294967295UL, ulng); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("4294967296", "%lu", &ulng)); + /* but parsing only 9... */ + test_eq(1, tor_sscanf("4294967296", "%9lu", &ulng)); + test_eq(429496729UL, ulng); + + /* %lx */ + /* UINT32_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFF", "%lx", &ulng)); + test_eq(0xFFFFFFFFUL, ulng); + + /* But UINT32_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("100000000", "%lx", &ulng)); + + /* %ld */ + /* INT32_MIN and INT32_MAX should work */ r = tor_sscanf("-2147483648. 2147483647.", "%ld. %ld.", &lng1, &lng2); test_eq(r,2); - test_eq(lng1, -2147483647 - 1); - test_eq(lng2, 2147483647); + test_eq(lng1, -2147483647L - 1L); + test_eq(lng2, 2147483647L); + + /* But INT32_MIN - 1 and INT32_MAX + 1 shouldn't work */ + r = tor_sscanf("-2147483649.", "%ld.", &lng1); + test_eq(r,0); + + r = tor_sscanf("2147483648.", "%ld.", &lng1); + test_eq(r,0); + + /* and the first failure stops further processing */ + r = tor_sscanf("-2147483648. 2147483648.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,1); + + r = tor_sscanf("-2147483649. 2147483647.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); + + r = tor_sscanf("2147483648. -2147483649.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); #elif SIZEOF_LONG == 8 + /* %lu */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("18446744073709551615", "%lu", &ulng)); + test_eq(18446744073709551615UL, ulng); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("18446744073709551616", "%lu", &ulng)); + /* but parsing only 19... */ + test_eq(1, tor_sscanf("18446744073709551616", "%19lu", &ulng)); + test_eq(1844674407370955161UL, ulng); + + /* %lx */ + /* UINT64_MAX should work */ + test_eq(1, tor_sscanf("FFFFFFFFFFFFFFFF", "%lx", &ulng)); + test_eq(0xFFFFFFFFFFFFFFFFUL, ulng); + + /* But UINT64_MAX + 1 shouldn't work */ + test_eq(0, tor_sscanf("10000000000000000", "%lx", &ulng)); + + /* %ld */ + /* INT64_MIN and INT64_MAX should work */ r = tor_sscanf("-9223372036854775808. 9223372036854775807.", "%ld. %ld.", &lng1, &lng2); test_eq(r,2); - test_eq(lng1, -9223372036854775807L - 1); + test_eq(lng1, -9223372036854775807L - 1L); test_eq(lng2, 9223372036854775807L);
+ /* But INT64_MIN - 1 and INT64_MAX + 1 shouldn't work */ + r = tor_sscanf("-9223372036854775809.", "%ld.", &lng1); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808.", "%ld.", &lng1); + test_eq(r,0); + + /* and the first failure stops further processing */ r = tor_sscanf("-9223372036854775808. 9223372036854775808.", "%ld. %ld.", &lng1, &lng2); test_eq(r,1); - r = tor_sscanf("-9223372036854775809. 9223372036854775808.", + + r = tor_sscanf("-9223372036854775809. 9223372036854775807.", + "%ld. %ld.", &lng1, &lng2); + test_eq(r,0); + + r = tor_sscanf("9223372036854775808. -9223372036854775809.", "%ld. %ld.", &lng1, &lng2); test_eq(r,0); #endif