From 8e90f48b6718d84cf6dde5b4f20427bd4382822f Mon Sep 17 00:00:00 2001 From: "Alan M. Carroll" Date: Thu, 14 Dec 2023 00:32:12 -0600 Subject: [PATCH] TextView: Fix overlow for svtoi. --- code/include/swoc/TextView.h | 6 +++--- code/src/TextView.cc | 35 +++++++++++++++++++++-------------- unit_tests/test_TextView.cc | 30 ++++++++++++++++++++++++------ 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/code/include/swoc/TextView.h b/code/include/swoc/TextView.h index 05b224a..01ddb65 100644 --- 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 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::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 index 4c7710d..fbfb343 100644 --- 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::max(); + static constexpr uintmax_t ABS_MIN = uintmax_t(std::numeric_limits::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(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::max(); + const auto OVERFLOW_LIMIT = MAX / base; + intmax_t v = 0; while (src.size() && (0 <= (v = svtoi_convert[static_cast(*src)])) && v < base) { - auto n = zret * base + v; - if (n < zret) { - zret = std::numeric_limits::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 index dd8aba7..2f13dfb 100644 --- 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::max(); + static constexpr auto UMAX = std::numeric_limits::max(); + static constexpr auto IMAX = std::numeric_limits::max(); + static constexpr auto IMIN = std::numeric_limits::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.