diff --git a/changes/ticket30920 b/changes/ticket30920 new file mode 100644 index 0000000000..d2fd8c9dab --- /dev/null +++ b/changes/ticket30920 @@ -0,0 +1,3 @@ + o Minor bugfix (configuration): + - Check for multiplication overflow when parsing memory units inside + configuration. Fixes bug 30920; bugfix on 0.0.9rc1~46. diff --git a/src/lib/confmgt/.may_include b/src/lib/confmgt/.may_include index 2564133917..5ff949f103 100644 --- a/src/lib/confmgt/.may_include +++ b/src/lib/confmgt/.may_include @@ -4,6 +4,7 @@ lib/conf/*.h lib/confmgt/*.h lib/container/*.h lib/encoding/*.h +lib/intmath/*.h lib/log/*.h lib/malloc/*.h lib/string/*.h diff --git a/src/lib/confmgt/unitparse.c b/src/lib/confmgt/unitparse.c index c3ed8285a4..8cbf9903e7 100644 --- a/src/lib/confmgt/unitparse.c +++ b/src/lib/confmgt/unitparse.c @@ -15,6 +15,7 @@ #include "lib/log/util_bug.h" #include "lib/string/parse_int.h" #include "lib/string/util_string.h" +#include "lib/intmath/muldiv.h" #include @@ -109,6 +110,7 @@ const struct unit_table_t time_msec_units[] = { * table u, then multiply the number by the unit multiplier. * On success, set *ok to 1 and return this product. * Otherwise, set *ok to 0. + * Warns user when overflow or a negative value is detected. */ uint64_t config_parse_units(const char *val, const unit_table_t *u, int *ok) @@ -142,10 +144,35 @@ config_parse_units(const char *val, const unit_table_t *u, int *ok) for ( ;u->unit;++u) { if (!strcasecmp(u->unit, cp)) { - if (use_float) - v = (uint64_t)(u->multiplier * d); - else - v *= u->multiplier; + if (use_float) { + d = u->multiplier * d; + + if (d < 0) { + log_warn(LD_CONFIG, "Negative value arised when parsing %s %s", + val, u->unit); + *ok = 0; + goto done; + } + + // Some compilers may warn about casting a double to an unsigned type + // because they don't know if d is >= 0 + if (d >= 0 && (d > (double)INT64_MAX || (uint64_t)d > INT64_MAX)) { + log_warn(LD_CONFIG, "Overflow detected parsing %s %s", val, u->unit); + *ok = 0; + goto done; + } + + v = (uint64_t) d; + } else { + v = tor_mul_u64_nowrap(v, u->multiplier); + + if (v > INT64_MAX) { + log_warn(LD_CONFIG, "Overflow detected parsing %s %s", val, u->unit); + *ok = 0; + goto done; + } + } + *ok = 1; goto done; } diff --git a/src/lib/intmath/muldiv.c b/src/lib/intmath/muldiv.c index 6a292db7ba..3330a4c569 100644 --- a/src/lib/intmath/muldiv.c +++ b/src/lib/intmath/muldiv.c @@ -69,6 +69,20 @@ gcd64(uint64_t a, uint64_t b) return a; } +/** Return the unsigned integer product of a and b, if overflow + * is detected return UINT64_MAX instead. */ +uint64_t +tor_mul_u64_nowrap(uint64_t a, uint64_t b) +{ + if (a == 0 || b == 0) { + return 0; + } else if (PREDICT_UNLIKELY(UINT64_MAX / a < b)) { + return UINT64_MAX; + } else { + return a*b; + } +} + /* Given a fraction *numer / *denom, simplify it. * Requires that the denominator is greater than 0. */ void diff --git a/src/lib/intmath/muldiv.h b/src/lib/intmath/muldiv.h index 64500b6dce..7aa0f9b235 100644 --- a/src/lib/intmath/muldiv.h +++ b/src/lib/intmath/muldiv.h @@ -18,6 +18,8 @@ unsigned round_to_next_multiple_of(unsigned number, unsigned divisor); uint32_t round_uint32_to_next_multiple_of(uint32_t number, uint32_t divisor); uint64_t round_uint64_to_next_multiple_of(uint64_t number, uint64_t divisor); +uint64_t tor_mul_u64_nowrap(uint64_t a, uint64_t b); + void simplify_fraction64(uint64_t *numer, uint64_t *denom); /* Compute the CEIL of a divided by b, for nonnegative a diff --git a/src/test/test_confparse.c b/src/test/test_confparse.c index 3e122a5129..808389de14 100644 --- a/src/test/test_confparse.c +++ b/src/test/test_confparse.c @@ -898,11 +898,22 @@ test_confparse_unitparse(void *args) tt_assert(ok); /* u64 overflow */ - /* XXXX our implementation does not currently detect this. See bug 30920. */ - /* tt_u64_op(config_parse_memunit("20000000 TB", &ok), OP_EQ, 0); tt_assert(!ok); - */ + // This test fails the double check as the float representing 15000000.5 TB + // is greater than (double) INT64_MAX + tt_u64_op(config_parse_memunit("15000000.5 TB", &ok), OP_EQ, 0); + tt_assert(!ok); + // 8388608.1 TB passes double check because it falls in the same float + // value as (double)INT64_MAX (which is 2^63) due to precision. + // But will fail the int check because the unsigned representation of + // the float, which is 2^63, is strictly greater than INT64_MAX (2^63-1) + tt_u64_op(config_parse_memunit("8388608.1 TB", &ok), OP_EQ, 0); + tt_assert(!ok); + + /* negative float */ + tt_u64_op(config_parse_memunit("-1.5 GB", &ok), OP_EQ, 0); + tt_assert(!ok); /* i32 overflow */ tt_int_op(config_parse_interval("1000 months", &ok), OP_EQ, -1); diff --git a/src/test/test_util.c b/src/test/test_util.c index 3e4975fcd8..5f46e4fcff 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -33,6 +33,7 @@ #include "lib/process/env.h" #include "lib/process/pidfile.h" #include "lib/intmath/weakrng.h" +#include "lib/intmath/muldiv.h" #include "lib/thread/numcpus.h" #include "lib/math/fp.h" #include "lib/math/laplace.h" @@ -5975,6 +5976,14 @@ test_util_nowrap_math(void *arg) tt_u64_op(UINT32_MAX, OP_EQ, tor_add_u32_nowrap(2, UINT32_MAX-1)); tt_u64_op(UINT32_MAX, OP_EQ, tor_add_u32_nowrap(UINT32_MAX, UINT32_MAX)); + tt_u64_op(0, OP_EQ, tor_mul_u64_nowrap(0, 0)); + tt_u64_op(1, OP_EQ, tor_mul_u64_nowrap(1, 1)); + tt_u64_op(2, OP_EQ, tor_mul_u64_nowrap(2, 1)); + tt_u64_op(4, OP_EQ, tor_mul_u64_nowrap(2, 2)); + tt_u64_op(UINT64_MAX, OP_EQ, tor_mul_u64_nowrap(UINT64_MAX, 1)); + tt_u64_op(UINT64_MAX, OP_EQ, tor_mul_u64_nowrap(2, UINT64_MAX)); + tt_u64_op(UINT64_MAX, OP_EQ, tor_mul_u64_nowrap(UINT64_MAX, UINT64_MAX)); + done: ; }