From c1389c77b2f84a973b54aae3ff05d87a0328d002 Mon Sep 17 00:00:00 2001 From: frosch Date: Fri, 16 May 2025 12:07:12 +0200 Subject: [PATCH] Codechange: ParseInteger optionally clamps out-of-range values, instead of treating them as invalid. --- src/core/string_consumer.hpp | 53 +++++++++++++++++++++++------------ src/tests/string_consumer.cpp | 26 ++++++++++++++++- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/src/core/string_consumer.hpp b/src/core/string_consumer.hpp index af35877028..74d88ce586 100644 --- a/src/core/string_consumer.hpp +++ b/src/core/string_consumer.hpp @@ -797,12 +797,12 @@ public: private: template - [[nodiscard]] static std::pair ParseIntegerBase(std::string_view src, int base, bool log_errors) + [[nodiscard]] static std::pair ParseIntegerBase(std::string_view src, int base, bool clamp, bool log_errors) { if (base == 0) { /* Try positive hex */ if (src.starts_with("0x") || src.starts_with("0X")) { - auto [len, value] = ParseIntegerBase(src.substr(2), 16, log_errors); + auto [len, value] = ParseIntegerBase(src.substr(2), 16, clamp, log_errors); if (len == 0) return {}; return {len + 2, value}; } @@ -810,18 +810,21 @@ private: /* Try negative hex */ if (std::is_signed_v && (src.starts_with("-0x") || src.starts_with("-0X"))) { using Unsigned = std::make_unsigned_t; - auto [len, uvalue] = ParseIntegerBase(src.substr(3), 16, log_errors); + auto [len, uvalue] = ParseIntegerBase(src.substr(3), 16, clamp, log_errors); if (len == 0) return {}; T value = static_cast(0 - uvalue); if (value > 0) { - if (log_errors) LogError(fmt::format("Integer out of range: '{}'", src.substr(0, len + 3))); - return {}; + if (!clamp) { + if (log_errors) LogError(fmt::format("Integer out of range: '{}'", src.substr(0, len + 3))); + return {}; + } + value = std::numeric_limits::lowest(); } return {len + 3, value}; } /* Try decimal */ - return ParseIntegerBase(src, 10, log_errors); + return ParseIntegerBase(src, 10, clamp, log_errors); } T value{}; @@ -829,10 +832,16 @@ private: auto result = std::from_chars(src.data(), src.data() + src.size(), value, base); auto len = result.ptr - src.data(); if (result.ec == std::errc::result_out_of_range) { - if (log_errors) LogError(fmt::format("Integer out of range: '{}'+'{}'", src.substr(0, len), src.substr(len, 4))); - return {}; - } - if (result.ec != std::errc{}) { + if (!clamp) { + if (log_errors) LogError(fmt::format("Integer out of range: '{}'+'{}'", src.substr(0, len), src.substr(len, 4))); + return {}; + } + if (src.starts_with("-")) { + value = std::numeric_limits::lowest(); + } else { + value = std::numeric_limits::max(); + } + } else if (result.ec != std::errc{}) { if (log_errors) LogError(fmt::format("Cannot parse integer: '{}'+'{}'", src.substr(0, len), src.substr(len, 4))); return {}; } @@ -843,24 +852,28 @@ public: /** * Peek and parse an integer in number 'base'. * If 'base == 0', then a prefix '0x' decides between base 16 or base 10. + * @param clamp If the value is a valid number, but out of range for T, return the maximum representable value. + * Negative values for unsigned results are still treated as invalid. * @return Length of string match, and parsed value. * @note The parser rejects leading whitespace and unary plus. */ template - [[nodiscard]] std::pair PeekIntegerBase(int base) const + [[nodiscard]] std::pair PeekIntegerBase(int base, bool clamp = false) const { - return ParseIntegerBase(this->src.substr(this->position), base, false); + return ParseIntegerBase(this->src.substr(this->position), base, clamp, false); } /** * Try to read and parse an integer in number 'base', and then advance the reader. * If 'base == 0', then a prefix '0x' decides between base 16 or base 10. + * @param clamp If the value is a valid number, but out of range for T, return the maximum representable value. + * Negative values for unsigned results are still treated as invalid. * @return Parsed value, if valid. * @note The parser rejects leading whitespace and unary plus. */ template - [[nodiscard]] std::optional TryReadIntegerBase(int base) + [[nodiscard]] std::optional TryReadIntegerBase(int base, bool clamp = false) { - auto [len, value] = this->PeekIntegerBase(base); + auto [len, value] = this->PeekIntegerBase(base, clamp); if (len == 0) return std::nullopt; this->SkipIntegerBase(base); return value; @@ -868,14 +881,16 @@ public: /** * Read and parse an integer in number 'base', and advance the reader. * If 'base == 0', then a prefix '0x' decides between base 16 or base 10. + * @param clamp If the value is a valid number, but out of range for T, return the maximum representable value. + * Negative values for unsigned results are still treated as invalid. * @return Parsed value, or 'def' if invalid. * @note The reader is advanced, even if no valid data was present. * @note The parser rejects leading whitespace and unary plus. */ template - [[nodiscard]] T ReadIntegerBase(int base, T def = 0) + [[nodiscard]] T ReadIntegerBase(int base, T def = 0, bool clamp = false) { - auto [len, value] = ParseIntegerBase(this->src.substr(this->position), base, true); + auto [len, value] = ParseIntegerBase(this->src.substr(this->position), base, clamp, true); this->SkipIntegerBase(base); // always advance return len > 0 ? value : def; } @@ -894,14 +909,16 @@ public: * Accepts leading and trailing whitespace. Trailing junk is an error. * @param arg The string to be converted. * @param base The base for parsing the number, defaults to only decimal numbers. Use 0 to also allow hexadecimal. + * @param clamp If the value is a valid number, but out of range for T, return the maximum representable value. + * Negative values for unsigned results are still treated as invalid. * @return The number, or std::nullopt if it could not be parsed. */ template -static inline std::optional ParseInteger(std::string_view arg, int base = 10) +static inline std::optional ParseInteger(std::string_view arg, int base = 10, bool clamp = false) { StringConsumer consumer{arg}; consumer.SkipUntilCharNotIn(StringConsumer::WHITESPACE_NO_NEWLINE); - auto result = consumer.TryReadIntegerBase(base); + auto result = consumer.TryReadIntegerBase(base, clamp); consumer.SkipUntilCharNotIn(StringConsumer::WHITESPACE_NO_NEWLINE); if (consumer.AnyBytesLeft()) return std::nullopt; return result; diff --git a/src/tests/string_consumer.cpp b/src/tests/string_consumer.cpp index d07e67c996..85a523902f 100644 --- a/src/tests/string_consumer.cpp +++ b/src/tests/string_consumer.cpp @@ -321,6 +321,8 @@ TEST_CASE("StringConsumer - parse int") CHECK(consumer.ReadUtf8() == ' '); CHECK(consumer.PeekIntegerBase(16) == std::pair(8, 0xffffffff)); CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(8, 0xffffffff)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(8, 0x7fffffff)); CHECK(consumer.TryReadIntegerBase(16) == std::nullopt); CHECK(consumer.ReadIntegerBase(16) == 0); CHECK(consumer.ReadUtf8() == ' '); @@ -328,6 +330,8 @@ TEST_CASE("StringConsumer - parse int") CHECK(consumer.ReadUtf8() == ' '); CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); CHECK(consumer.PeekIntegerBase(16) == std::pair(9, -0x1aaaaaaa)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(9, -0x1aaaaaaa)); CHECK(consumer.TryReadIntegerBase(16) == std::nullopt); CHECK(consumer.ReadIntegerBase(16) == 0); CHECK(consumer.ReadUtf8() == ' '); @@ -350,6 +354,10 @@ TEST_CASE("StringConsumer - parse int") CHECK(consumer.PeekIntegerBase(10) == std::pair(0, 0)); CHECK(consumer.PeekIntegerBase(10) == std::pair(13, 1234567890123)); CHECK(consumer.PeekIntegerBase(10) == std::pair(13, 1234567890123)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(13, 0xffffffff)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(13, 0x7fffffff)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(13, 1234567890123)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(13, 1234567890123)); CHECK(consumer.TryReadIntegerBase(10) == std::nullopt); CHECK(consumer.ReadIntegerBase(10) == 0); CHECK(consumer.ReadUtf8() == ' '); @@ -370,6 +378,10 @@ TEST_CASE("StringConsumer - parse int") CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); CHECK(consumer.PeekIntegerBase(16) == std::pair(16, 0xffffffff'fffffffe)); CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(16, 0xffffffff)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(16, 0x7fffffff)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(16, 0xffffffff'fffffffe)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(16, 0x7fffffff'ffffffff)); CHECK(consumer.ReadIntegerBase(16) == 0); CHECK(consumer.ReadUtf8() == ' '); CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); @@ -500,9 +512,11 @@ TEST_CASE("StringConsumer - invalid int") TEST_CASE("StringConsumer - most negative") { - StringConsumer consumer("-80000000 -0x80000000 -2147483648"sv); + StringConsumer consumer("-80000000 -0x80000000 -2147483648 -9223372036854775808"sv); CHECK(consumer.PeekIntegerBase(16) == std::pair(0, 0)); CHECK(consumer.PeekIntegerBase(16) == std::pair(9, 0x80000000)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(16, true) == std::pair(9, 0x80000000)); consumer.SkipIntegerBase(16); CHECK(consumer.ReadUtf8() == ' '); CHECK(consumer.PeekIntegerBase(0) == std::pair(0, 0)); @@ -511,4 +525,14 @@ TEST_CASE("StringConsumer - most negative") CHECK(consumer.ReadUtf8() == ' '); CHECK(consumer.PeekIntegerBase(10) == std::pair(0, 0)); CHECK(consumer.PeekIntegerBase(10) == std::pair(11, 0x80000000)); + consumer.SkipIntegerBase(0); + CHECK(consumer.ReadUtf8() == ' '); + CHECK(consumer.PeekIntegerBase(10) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(10) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(10) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(10) == std::pair(20, 0x80000000'00000000)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(20, 0x80000000)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(0, 0)); + CHECK(consumer.PeekIntegerBase(10, true) == std::pair(20, 0x80000000'00000000)); }