Merge remote-tracking branch 'tor-github/pr/1338'

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.

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

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 <string.h>
 
@@ -109,6 +110,7 @@ const struct unit_table_t time_msec_units[] = {
  * table <b>u</b>, then multiply the number by the unit multiplier.
  * On success, set *<b>ok</b> to 1 and return this product.
  * Otherwise, set *<b>ok</b> 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;
     }

src/lib/intmath/muldiv.c

@@ -69,6 +69,20 @@ gcd64(uint64_t a, uint64_t b)
   return a;
 }
 
+/** Return the unsigned integer product of <b>a</b> and <b>b</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 *<b>numer</b> / *<b>denom</b>, simplify it.
  * Requires that the denominator is greater than 0. */
 void

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 <b>a</b> divided by <b>b</b>, for nonnegative <b>a</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);

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:
   ;
 }