TextView: Fix overlow for svtoi.

SolidWallOfCode · Dec 14, 2023 · 8e90f48 · 8e90f48
1 parent e5a9536
commit 8e90f48
Show file tree

Hide file tree

Showing 3 changed files with 48 additions and 23 deletions.
diff --git a/code/include/swoc/TextView.h b/code/include/swoc/TextView.h
@@ -1057,15 +1057,15 @@ uintmax_t svtou(TextView src, TextView *parsed = nullptr, int base = 0);
 template <int RADIX>
 uintmax_t
 svto_radix(TextView &src) {
-  static_assert(0 < RADIX && RADIX <= 36, "Radix must be in the range 1..36");
+  static_assert(1 <= RADIX && RADIX <= 36, "Radix must be in the range 2..36");
   static constexpr auto MAX            = std::numeric_limits<uintmax_t>::max();
   static constexpr auto OVERFLOW_LIMIT = MAX / RADIX;
   uintmax_t zret                       = 0;
-  int8_t v;
+  uintmax_t v;
   while (src.size() && (0 <= (v = swoc::svtoi_convert[uint8_t(*src)])) && v < RADIX) {
     // Tweaked for performance - need to check range after @a RADIX multiply.
     ++src; // Update view iff the character is parsed.
-    if (zret <= OVERFLOW_LIMIT && uintmax_t(v) <= MAX - (zret *= RADIX)) {
+    if (zret <= OVERFLOW_LIMIT && v <= (MAX - (zret *= RADIX)) ) {
       zret += v;
     } else {
       zret = MAX; // clamp to max - once set will always hit this case for subsequent input.

diff --git a/code/src/TextView.cc b/code/src/TextView.cc
@@ -43,6 +43,8 @@ const int8_t svtoi_convert[256] = {
 
 intmax_t
 svtoi(TextView src, TextView *out, int base) {
+  static constexpr uintmax_t ABS_MAX = std::numeric_limits<intmax_t>::max();
+  static constexpr uintmax_t ABS_MIN = uintmax_t(std::numeric_limits<intmax_t>::min());
   intmax_t zret = 0;
 
   if (src.ltrim_if(&isspace)) {
@@ -55,13 +57,15 @@ svtoi(TextView src, TextView *out, int base) {
     } else if ('+' == *src) {
       ++src;
     }
-    zret = intmax_t(svtou(src, &parsed, base));
+    auto n = svtou(src, &parsed, base);
     if (!parsed.empty()) {
       if (out) {
         out->assign(start, parsed.data_end());
       }
       if (neg) {
-        zret = -zret;
+        zret = -intmax_t(std::min<uintmax_t>(n, ABS_MIN));
+      } else {
+        zret = std::min(n, ABS_MAX);
       }
     }
   }
@@ -75,12 +79,9 @@ svtou(TextView src, TextView *out, int base) {
   if (out) {
     out->clear();
   }
-  if (!(0 <= base && base <= 36)) {
-    return 0;
-  }
+
   if (src.ltrim_if(&isspace).size()) {
-    auto origin = src.data();
-    int8_t v    = 0;
+    auto origin = src.data(); // cache to handle prefix skipping.
     // If base is 0, it wasn't specified - check for standard base prefixes
     if (0 == base) {
       base = 10;
@@ -103,6 +104,9 @@ svtou(TextView src, TextView *out, int base) {
         }
       }
     }
+    if (!(1 <= base && base <= 36)) {
+      return 0;
+    }
 
     // For performance in common cases, use the templated conversion.
     switch (base) {
@@ -118,17 +122,20 @@ svtou(TextView src, TextView *out, int base) {
     case 16:
       zret = svto_radix<16>(src);
       break;
-    default:
+    default: {
+      static constexpr auto MAX            = std::numeric_limits<uintmax_t>::max();
+      const auto OVERFLOW_LIMIT = MAX / base;
+      intmax_t v    = 0;
       while (src.size() && (0 <= (v = svtoi_convert[static_cast<unsigned char>(*src)])) && v < base) {
-        auto n = zret * base + v;
-        if (n < zret) {
-          zret = std::numeric_limits<uintmax_t>::max();
-          break; // overflow, stop parsing.
-        }
-        zret = n;
         ++src;
+        if (zret <= OVERFLOW_LIMIT && uintmax_t(v) <= (MAX - (zret *= base))) {
+          zret += v;
+        } else {
+          zret = MAX;
+        }
       }
       break;
+      }
     }
 
     if (out) {

diff --git a/unit_tests/test_TextView.cc b/unit_tests/test_TextView.cc
@@ -500,32 +500,50 @@ TEST_CASE("TextView Conversions", "[libswoc][TextView]") {
   REQUIRE(x.size() == 0);
 
   // Check overflow conditions
-  static constexpr auto MAX = std::numeric_limits<uintmax_t>::max();
+  static constexpr auto UMAX = std::numeric_limits<uintmax_t>::max();
+  static constexpr auto IMAX = std::numeric_limits<intmax_t>::max();
+  static constexpr auto IMIN = std::numeric_limits<intmax_t>::min();
 
   // One less than max.
   x.assign("18446744073709551614");
-  REQUIRE(MAX - 1 == swoc::svto_radix<10>(x));
+  REQUIRE(UMAX - 1 == swoc::svto_radix<10>(x));
   REQUIRE(x.size() == 0);
 
   // Exactly max.
   x.assign("18446744073709551615");
-  REQUIRE(MAX == swoc::svto_radix<10>(x));
+  REQUIRE(UMAX == swoc::svto_radix<10>(x));
   REQUIRE(x.size() == 0);
+  x.assign("18446744073709551615");
+  CHECK(UMAX == svtou(x));
 
   // Should overflow and clamp.
   x.assign("18446744073709551616");
-  REQUIRE(MAX == swoc::svto_radix<10>(x));
+  REQUIRE(UMAX == swoc::svto_radix<10>(x));
   REQUIRE(x.size() == 0);
 
   // Even more digits.
   x.assign("18446744073709551616123456789");
-  REQUIRE(MAX == swoc::svto_radix<10>(x));
+  REQUIRE(UMAX == swoc::svto_radix<10>(x));
   REQUIRE(x.size() == 0);
 
   // This is a special value - where N*10 > N while also overflowing. The final "1" causes this.
   // Be sure overflow is detected.
   x.assign("27381885734412615681");
-  REQUIRE(MAX == swoc::svto_radix<10>(x));
+  REQUIRE(UMAX == swoc::svto_radix<10>(x));
+
+  x.assign("9223372036854775807");
+  CHECK(svtou(x) == IMAX);
+  CHECK(svtoi(x) == IMAX);
+  x.assign("9223372036854775808");
+  CHECK(svtou(x) == uintmax_t(IMAX) + 1);
+  CHECK(svtoi(x) == IMAX);
+
+  x.assign("-9223372036854775807");
+  CHECK(svtoi(x) == IMIN + 1);
+  x.assign("-9223372036854775808");
+  CHECK(svtoi(x) == IMIN);
+  x.assign("-9223372036854775809");
+  CHECK(svtoi(x) == IMIN);
 
   // floating point is never exact, so "good enough" is all that is measureable. This checks the
   // value is within one epsilon (minimum change possible) of the compiler generated value.