From bc4564e34be48e935191d838fa4c30c41acad5b1 Mon Sep 17 00:00:00 2001 From: metsw24-max Date: Mon, 18 May 2026 22:54:52 +0530 Subject: [PATCH 1/2] Replace unsafe numeric parsing with std::from_chars + hardening --- args.hxx | 218 ++++++++++++++++------------------- fuzz/fuzz_numeric_parser.cxx | 51 ++++++++ 2 files changed, 153 insertions(+), 116 deletions(-) create mode 100644 fuzz/fuzz_numeric_parser.cxx diff --git a/args.hxx b/args.hxx index 00d6893..1f7c0b6 100644 --- a/args.hxx +++ b/args.hxx @@ -266,67 +266,6 @@ namespace args return true; } - /** Parse an untrusted decimal size_t without locale-sensitive conversions. - * Leading and trailing whitespace are accepted, but signs and embedded - * whitespace are rejected. Returns false on overflow. - */ - inline bool ParseSizeT(const std::string &value, size_t &out) - { - auto it = std::find_if_not(value.begin(), value.end(), [](char c) - { - return std::isspace(static_cast(c)) != 0; - }); - - if (it == value.end() || *it == '+' || *it == '-') - { - return false; - } - - size_t parsed = 0; - bool sawDigit = false; - for (; it != value.end(); ++it) - { - const unsigned char ch = static_cast(*it); - if (std::isspace(ch) != 0) - { - break; - } - if (!std::isdigit(ch)) - { - return false; - } - - size_t multiplied = 0; - if (!SafeMultiply(parsed, static_cast(10), multiplied)) - { - return false; - } - - const size_t digit = static_cast(ch - static_cast('0')); - if (!SafeAdd(multiplied, digit, parsed)) - { - return false; - } - sawDigit = true; - } - - if (!sawDigit) - { - return false; - } - - for (; it != value.end(); ++it) - { - if (std::isspace(static_cast(*it)) == 0) - { - return false; - } - } - - out = parsed; - return true; - } - /** (INTERNAL) Wrap a vector of words into a vector of lines * * Empty words are skipped. Word "\n" forces wrapping. @@ -1545,9 +1484,42 @@ namespace args { syntax = value_.at(0); const std::string &raw = value_.at(1); + bool failed = false; + + const auto firstNonSpace = std::find_if_not(raw.begin(), raw.end(), [](char c) + { + return std::isspace(static_cast(c)) != 0; + }); + + if (firstNonSpace != raw.end() && *firstNonSpace == '-') + { + failed = true; + } size_t parsed = 0; - if (!ParseSizeT(raw, parsed)) + if (!failed) + { + std::istringstream ss(raw); + ss >> parsed; + if (ss.fail()) + { + failed = true; + } + else + { + char extra; + if (ss >> extra) + { + failed = true; + } + else if (!ss.eof()) + { + failed = true; + } + } + } + + if (failed) { #ifdef ARGS_NOEXCEPT error = Error::Parse; @@ -3765,68 +3737,80 @@ namespace args { ++begin; } - const bool negative = begin != end_pos && *begin == '-'; - if (negative || (begin != end_pos && *begin == '+')) - { - ++begin; - } - int base = 10; - if (end_pos - begin > 1 && *begin == '0') + if (std::is_unsigned::value && begin != end_pos && *begin == '-') { - if (*(begin + 1) == 'x' || *(begin + 1) == 'X') - { - begin += 2; - base = 16; - } - else - { - base = 8; - } + return false; } - typedef typename std::make_unsigned::type UnsignedT; - UnsignedT parsed = 0; - auto result = std::from_chars(begin, end_pos, parsed, base); - - // Find end of valid parse - end_pos = result.ptr; - - // Skip trailing whitespace following std::from_chars parse - while (end_pos != value.c_str() + value.length() && - std::isspace(static_cast(*end_pos))) - { - ++end_pos; - } - - if (end_pos != value.c_str() + value.length() || - result.ec == std::errc::invalid_argument || - result.ec == std::errc::result_out_of_range) + if (begin == end_pos) { return false; } - - if (negative) - { - const UnsignedT minMagnitude = - static_cast(std::numeric_limits::max()) + static_cast(1); - if (parsed > minMagnitude) + + const bool success = [&]() { + // Use automatic base detection to preserve 0 and 0x prefixes + if (std::is_unsigned::value) { - return false; + typedef typename std::make_unsigned::type UnsignedT; + UnsignedT parsed = 0; + auto result = std::from_chars(begin, end_pos, parsed, 0); + + end_pos = result.ptr; + while (end_pos != value.c_str() + value.length() && + std::isspace(static_cast(*end_pos))) + { + ++end_pos; + } + + if (end_pos != value.c_str() + value.length() || + result.ec == std::errc::invalid_argument || + result.ec == std::errc::result_out_of_range) + { + return false; + } + + // Defensive check: Ensure parsed value won't overflow when cast + if (parsed > static_cast(std::numeric_limits::max())) + { + return false; + } + + destination = static_cast(parsed); + return true; } - destination = parsed == minMagnitude ? - std::numeric_limits::min() : - static_cast(-static_cast(parsed)); - } - else - { - if (parsed > static_cast(std::numeric_limits::max())) + else { - return false; + T parsed = 0; + auto result = std::from_chars(begin, end_pos, parsed, 0); + + end_pos = result.ptr; + while (end_pos != value.c_str() + value.length() && + std::isspace(static_cast(*end_pos))) + { + ++end_pos; + } + + if (end_pos != value.c_str() + value.length() || + result.ec == std::errc::invalid_argument || + result.ec == std::errc::result_out_of_range) + { + return false; + } + + // Defensive check: Ensure parsed value is within bounds + if (parsed > std::numeric_limits::max() || + parsed < std::numeric_limits::min()) + { + return false; + } + + destination = parsed; + return true; } - destination = static_cast(parsed); - } - return true; + }(); + + return success; #else // C++11/14 fallback using strtoll/strtoull @@ -3841,9 +3825,10 @@ namespace args const unsigned long long parsed = std::strtoull(begin, &end, 0); if (end == begin) { + errno = saved_errno; return false; } - while (end != nullptr && *end != '\0' && std::isspace(static_cast(*end))) + while (*end != '\0' && std::isspace(static_cast(*end))) { ++end; } @@ -3860,9 +3845,10 @@ namespace args const long long parsed = std::strtoll(begin, &end, 0); if (end == begin) { + errno = saved_errno; return false; } - while (end != nullptr && *end != '\0' && std::isspace(static_cast(*end))) + while (*end != '\0' && std::isspace(static_cast(*end))) { ++end; } @@ -5175,4 +5161,4 @@ namespace args #pragma pop_macro("min") #pragma pop_macro("max") -#endif +#endif \ No newline at end of file diff --git a/fuzz/fuzz_numeric_parser.cxx b/fuzz/fuzz_numeric_parser.cxx new file mode 100644 index 0000000..f72cd43 --- /dev/null +++ b/fuzz/fuzz_numeric_parser.cxx @@ -0,0 +1,51 @@ +#include +#include +#include +#include +#include "../args.hxx" + +// Expose the ValueReader class for fuzzing +extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { + if (size == 0) return 0; + + // Create a null-terminated string from fuzzer input + std::string input(reinterpret_cast(data), size); + + // Test numeric parsing with various types + args::ValueReader reader; + + // Test signed integer types + { + int dest_int = 0; + reader("fuzz_test", input, dest_int); // Should not crash + + short dest_short = 0; + reader("fuzz_test", input, dest_short); // Should not crash + + long long dest_ll = 0; + reader("fuzz_test", input, dest_ll); // Should not crash + } + + // Test unsigned integer types + { + unsigned int dest_uint = 0; + reader("fuzz_test", input, dest_uint); // Should not crash + + unsigned short dest_ushort = 0; + reader("fuzz_test", input, dest_ushort); // Should not crash + + unsigned long long dest_ull = 0; + reader("fuzz_test", input, dest_ull); // Should not crash + } + + // Test floating point types + { + float dest_float = 0.0f; + reader("fuzz_test", input, dest_float); // Should not crash + + double dest_double = 0.0; + reader("fuzz_test", input, dest_double); // Should not crash + } + + return 0; +} \ No newline at end of file From b4ba0b3ec238d1ff1053ce35ae6196d25016c9ee Mon Sep 17 00:00:00 2001 From: metsw24-max Date: Wed, 20 May 2026 16:21:03 +0530 Subject: [PATCH 2/2] updated --- args.hxx | 114 ++++++--------------------------------- test/safe_arithmetic.cxx | 27 ---------- 2 files changed, 16 insertions(+), 125 deletions(-) diff --git a/args.hxx b/args.hxx index 1f7c0b6..088210d 100644 --- a/args.hxx +++ b/args.hxx @@ -59,10 +59,6 @@ #include #include #include -#if __cplusplus >= 201703L -#include -#include -#endif #if defined(_MSC_VER) && _MSC_VER <= 1800 #define noexcept @@ -1491,7 +1487,10 @@ namespace args return std::isspace(static_cast(c)) != 0; }); - if (firstNonSpace != raw.end() && *firstNonSpace == '-') + // Reject explicit signs: cword must be a plain non-negative + // decimal index. istringstream would otherwise silently + // accept "+1". + if (firstNonSpace != raw.end() && (*firstNonSpace == '-' || *firstNonSpace == '+')) { failed = true; } @@ -3729,95 +3728,14 @@ namespace args } const char *begin = value.c_str(); - - // Preserve existing parsing semantics with std::from_chars -#if __cplusplus >= 201703L - const char *end_pos = value.c_str() + value.length(); - while (begin != end_pos && std::isspace(static_cast(*begin))) - { - ++begin; - } - - if (std::is_unsigned::value && begin != end_pos && *begin == '-') - { - return false; - } - - if (begin == end_pos) - { - return false; - } - - const bool success = [&]() { - // Use automatic base detection to preserve 0 and 0x prefixes - if (std::is_unsigned::value) - { - typedef typename std::make_unsigned::type UnsignedT; - UnsignedT parsed = 0; - auto result = std::from_chars(begin, end_pos, parsed, 0); - end_pos = result.ptr; - while (end_pos != value.c_str() + value.length() && - std::isspace(static_cast(*end_pos))) - { - ++end_pos; - } - - if (end_pos != value.c_str() + value.length() || - result.ec == std::errc::invalid_argument || - result.ec == std::errc::result_out_of_range) - { - return false; - } - - // Defensive check: Ensure parsed value won't overflow when cast - if (parsed > static_cast(std::numeric_limits::max())) - { - return false; - } - - destination = static_cast(parsed); - return true; - } - else - { - T parsed = 0; - auto result = std::from_chars(begin, end_pos, parsed, 0); - - end_pos = result.ptr; - while (end_pos != value.c_str() + value.length() && - std::isspace(static_cast(*end_pos))) - { - ++end_pos; - } - - if (end_pos != value.c_str() + value.length() || - result.ec == std::errc::invalid_argument || - result.ec == std::errc::result_out_of_range) - { - return false; - } - - // Defensive check: Ensure parsed value is within bounds - if (parsed > std::numeric_limits::max() || - parsed < std::numeric_limits::min()) - { - return false; - } - - destination = parsed; - return true; - } - }(); - - return success; -#else - // C++11/14 fallback using strtoll/strtoull - - // Save errno to detect if strtoull/strtoll sets it + // C++11-compatible: use strtoull/strtoll. Hardening retained from + // the original from_chars draft (errno save/restore, ERANGE check, + // narrowing range check, trailing-whitespace tolerance). No + // unconditional dependency on / C++17. const int saved_errno = errno; errno = 0; - + char *end = nullptr; if (std::is_unsigned::value) @@ -3832,9 +3750,10 @@ namespace args { ++end; } - if (*end != '\0' || errno == ERANGE || parsed > static_cast(std::numeric_limits::max())) + if (*end != '\0' || errno == ERANGE || + parsed > static_cast(std::numeric_limits::max())) { - errno = saved_errno; // Restore errno on error + errno = saved_errno; return false; } @@ -3856,16 +3775,15 @@ namespace args parsed < static_cast(std::numeric_limits::min()) || parsed > static_cast(std::numeric_limits::max())) { - errno = saved_errno; // Restore errno on error + errno = saved_errno; return false; } destination = static_cast(parsed); } - - errno = saved_errno; // Restore errno on success + + errno = saved_errno; return true; -#endif } template @@ -5161,4 +5079,4 @@ namespace args #pragma pop_macro("min") #pragma pop_macro("max") -#endif \ No newline at end of file +#endif diff --git a/test/safe_arithmetic.cxx b/test/safe_arithmetic.cxx index 753b1b1..c0381b1 100644 --- a/test/safe_arithmetic.cxx +++ b/test/safe_arithmetic.cxx @@ -261,30 +261,6 @@ void TestWrapLargeStringInput() std::cout << "PASS: Wrap long string with width 1" << std::endl; } -// Test bounded decimal parsing used for shell-provided completion indices -void TestParseSizeT() -{ - size_t parsed = 0; - - test::require(args::ParseSizeT("42", parsed)); - test::require(parsed == 42); - std::cout << "PASS: ParseSizeT parses decimal input" << std::endl; - - test::require(args::ParseSizeT(" \t42\n", parsed)); - test::require(parsed == 42); - std::cout << "PASS: ParseSizeT permits surrounding whitespace" << std::endl; - - test::require_false(args::ParseSizeT("+42", parsed)); - test::require_false(args::ParseSizeT("-1", parsed)); - test::require_false(args::ParseSizeT("4 2", parsed)); - std::cout << "PASS: ParseSizeT rejects signed and embedded-whitespace input" << std::endl; - - std::ostringstream overflow; - overflow << std::numeric_limits::max() << "0"; - test::require_false(args::ParseSizeT(overflow.str(), parsed)); - std::cout << "PASS: ParseSizeT detects overflow" << std::endl; -} - // Main test runner int main() { @@ -310,9 +286,6 @@ int main() TestWrapBoundaryConditions(); TestWrapLargeStringInput(); - std::cout << "\n=== ParseSizeT Tests ===" << std::endl; - TestParseSizeT(); - std::cout << "\n=== All Tests Passed ===" << std::endl; return 0; }