From 5e9f8225982e9711b548c886fe6b477cdefd0fd4 Mon Sep 17 00:00:00 2001 From: Jarryd Beck Date: Tue, 25 Jul 2017 08:07:03 +1000 Subject: [PATCH] Improve integer parsing Fixes #39. Closes #40. This is an overhaul of the way that integer arguments are parsed. Instead of using std::istream, which allows, for example, negative integers for unsigned types, we use our own parser. This allows us to do proper range checking depending on the type, and to correctly check for negative values passed to unsigned types. This also allows the handling of base 16 numbers. --- include/cxxopts.hpp | 163 ++++++++++++++++++++++++++++++++++++++++++-- test/options.cpp | 97 ++++++++++++++++++++++++++ 2 files changed, 254 insertions(+), 6 deletions(-) diff --git a/include/cxxopts.hpp b/include/cxxopts.hpp index f4fa7df..30a7432 100644 --- a/include/cxxopts.hpp +++ b/include/cxxopts.hpp @@ -1,6 +1,6 @@ /* -Copyright (c) 2014, 2015, 2016 Jarryd Beck +Copyright (c) 2014, 2015, 2016, 2017 Jarryd Beck Permission is hereby granted, free of charge, to any person obtaining a copy of this software and associated documentation files (the "Software"), to deal @@ -31,6 +31,7 @@ THE SOFTWARE. #endif #include +#include #include #include #include @@ -411,20 +412,170 @@ namespace cxxopts namespace values { + std::basic_regex integer_pattern + ("(-)?(0x)?([1-9a-zA-Z][0-9a-zA-Z]*)|(0)"); + + namespace detail + { + template + struct SignedCheck; + + template + struct SignedCheck + { + template + void + operator()(bool negative, U u, const std::string& text) + { + if (negative) + { + if (u > U(-std::numeric_limits::min())) + { + throw argument_incorrect_type(text); + } + } + else + { + if (u > std::numeric_limits::max()) + { + throw argument_incorrect_type(text); + } + } + } + }; + + template + struct SignedCheck + { + template + void + operator()(bool, U, const std::string&) {} + }; + + template + void + check_signed_range(bool negative, U value, const std::string& text) + { + SignedCheck::is_signed>()(negative, value, text); + } + } + template void - parse_value(const std::string& text, T& value) + integer_parser(const std::string& text, T& value) { - std::istringstream is(text); - if (!(is >> value)) + std::smatch match; + std::regex_match(text, match, integer_pattern); + + if (match.length() == 0) { throw argument_incorrect_type(text); } - if (is.rdbuf()->in_avail() != 0) + if (match.length(4) > 0) { - throw argument_incorrect_type(text); + value = 0; + return; } + + using US = typename std::make_unsigned::type; + + constexpr auto umax = std::numeric_limits::max(); + constexpr bool is_signed = std::numeric_limits::is_signed; + const bool negative = match.length(1) > 0; + const auto base = match.length(2) > 0 ? 16 : 10; + + auto value_match = match[3]; + + US result = 0; + + for (auto iter = value_match.first; iter != value_match.second; ++iter) + { + int digit = 0; + + if (*iter >= '0' && *iter <= '9') + { + digit = *iter - '0'; + } + else if (*iter >= 'a' && *iter <= 'f') + { + digit = *iter - 'a' + 10; + } + else if (*iter >= 'A' && *iter <= 'F') + { + digit = *iter - 'A' + 10; + } + + if (umax - digit < result * base) + { + throw argument_incorrect_type(text); + } + + result = result * base + digit; + } + + detail::check_signed_range(negative, result, text); + + if (negative) + { + if (!is_signed) + { + throw argument_incorrect_type(text); + } + value = -result; + } + else + { + value = result; + } + } + + void + parse_value(const std::string& text, uint8_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, int8_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, uint16_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, int16_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, uint32_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, int32_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, uint64_t& value) + { + integer_parser(text, value); + } + + void + parse_value(const std::string& text, int64_t& value) + { + integer_parser(text, value); } inline diff --git a/test/options.cpp b/test/options.cpp index 62c21ed..69cb964 100644 --- a/test/options.cpp +++ b/test/options.cpp @@ -239,3 +239,100 @@ TEST_CASE("Empty with implicit value", "[implicit]") REQUIRE(options.count("implicit") == 1); REQUIRE(options["implicit"].as() == ""); } + +TEST_CASE("Integers", "[options]") +{ + cxxopts::Options options("parses_integers", "parses integers correctly"); + options.add_options() + ("positional", "Integers", cxxopts::value>()); + + Argv av({"ints", "--", "5", "6", "-6", "0", "0xab", "0xAf"}); + + char** argv = av.argv(); + auto argc = av.argc(); + + options.parse_positional("positional"); + options.parse(argc, argv); + + REQUIRE(options.count("positional") == 6); + + auto& positional = options["positional"].as>(); + CHECK(positional[0] == 5); + CHECK(positional[1] == 6); + CHECK(positional[2] == -6); + CHECK(positional[3] == 0); + CHECK(positional[4] == 0xab); + CHECK(positional[5] == 0xaf); +} + +TEST_CASE("Unsigned integers", "[options]") +{ + cxxopts::Options options("parses_unsigned", "detects unsigned errors"); + options.add_options() + ("positional", "Integers", cxxopts::value>()); + + Argv av({"ints", "--", "-2"}); + + char** argv = av.argv(); + auto argc = av.argc(); + + options.parse_positional("positional"); + CHECK_THROWS_AS(options.parse(argc, argv), cxxopts::argument_incorrect_type); +} + +TEST_CASE("Integer bounds", "[integer]") +{ + cxxopts::Options options("integer_boundaries", "check min/max integer"); + options.add_options() + ("positional", "Integers", cxxopts::value>()); + + SECTION("No overflow") + { + Argv av({"ints", "--", "127", "-128", "0x7f", "-0x80", "0x7e"}); + + auto argv = av.argv(); + auto argc = av.argc(); + + options.parse_positional("positional"); + options.parse(argc, argv); + + REQUIRE(options.count("positional") == 5); + + auto& positional = options["positional"].as>(); + CHECK(positional[0] == 127); + CHECK(positional[1] == -128); + CHECK(positional[2] == 0x7f); + CHECK(positional[3] == -0x80); + CHECK(positional[4] == 0x7e); + } +} + +TEST_CASE("Overflow on boundary", "[integer]") +{ + using namespace cxxopts::values; + + int8_t si; + uint8_t ui; + + CHECK_THROWS_AS((integer_parser("128", si)), cxxopts::argument_incorrect_type); + CHECK_THROWS_AS((integer_parser("-129", si)), cxxopts::argument_incorrect_type); + CHECK_THROWS_AS((integer_parser("256", ui)), cxxopts::argument_incorrect_type); + CHECK_THROWS_AS((integer_parser("-0x81", si)), cxxopts::argument_incorrect_type); + CHECK_THROWS_AS((integer_parser("0x80", si)), cxxopts::argument_incorrect_type); + CHECK_THROWS_AS((integer_parser("0x100", ui)), cxxopts::argument_incorrect_type); +} + +TEST_CASE("Integer overflow", "[options]") +{ + cxxopts::Options options("reject_overflow", "rejects overflowing integers"); + options.add_options() + ("positional", "Integers", cxxopts::value>()); + + Argv av({"ints", "--", "128"}); + + auto argv = av.argv(); + auto argc = av.argc(); + + options.parse_positional("positional"); + CHECK_THROWS_AS(options.parse(argc, argv), cxxopts::argument_incorrect_type); +}