Skip to content

Commit

Permalink
fix: out of range string detected by the fuzzer (#905)
Browse files Browse the repository at this point in the history
about an out of range string conversion not being caught properly. This
commit changes the logic from an exception to using errno and a
non-throwing alternative.

Issue detected in

https://github.com/CLIUtils/CLI11/actions/runs/5500247554/jobs/10023032108

The problem string was set up as a test.

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
  • Loading branch information
phlptp and pre-commit-ci[bot] authored Jul 11, 2023
1 parent ccf141c commit 792d892
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 34 deletions.
40 changes: 22 additions & 18 deletions include/CLI/TypeTools.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -861,7 +861,7 @@ bool integral_conversion(const std::string &input, T &output) noexcept {
if(input.empty() || input.front() == '-') {
return false;
}
char *val = nullptr;
char *val{nullptr};
errno = 0;
std::uint64_t output_ll = std::strtoull(input.c_str(), &val, 0);
if(errno == ERANGE) {
Expand Down Expand Up @@ -904,8 +904,8 @@ bool integral_conversion(const std::string &input, T &output) noexcept {
return false;
}

/// Convert a flag into an integer value typically binary flags
inline std::int64_t to_flag_value(std::string val) {
/// Convert a flag into an integer value typically binary flags sets errno to nonzero if conversion failed
inline std::int64_t to_flag_value(std::string val) noexcept {
static const std::string trueString("true");
static const std::string falseString("false");
if(val == trueString) {
Expand Down Expand Up @@ -933,7 +933,8 @@ inline std::int64_t to_flag_value(std::string val) {
ret = 1;
break;
default:
throw std::invalid_argument("unrecognized character");
errno = EINVAL;
return -1;
}
return ret;
}
Expand All @@ -942,7 +943,11 @@ inline std::int64_t to_flag_value(std::string val) {
} else if(val == falseString || val == "off" || val == "no" || val == "disable") {
ret = -1;
} else {
ret = std::stoll(val);
char *loc_ptr{nullptr};
ret = std::strtoll(val.c_str(), &loc_ptr, 0);
if(loc_ptr != (val.c_str() + val.size()) && errno == 0) {
errno = EINVAL;
}
}
return ret;
}
Expand Down Expand Up @@ -971,18 +976,16 @@ bool lexical_cast(const std::string &input, T &output) {
template <typename T,
enable_if_t<classify_object<T>::value == object_category::boolean_value, detail::enabler> = detail::dummy>
bool lexical_cast(const std::string &input, T &output) {
try {
auto out = to_flag_value(input);
errno = 0;
auto out = to_flag_value(input);
if(errno == 0) {
output = (out > 0);
return true;
} catch(const std::invalid_argument &) {
return false;
} catch(const std::out_of_range &) {
// if the number is out of the range of a 64 bit value then it is still a number and for this purpose is still
// valid all we care about the sign
} else if(errno == ERANGE) {
output = (input[0] != '-');
return true;
} else {
return false;
}
return true;
}

/// Floats
Expand Down Expand Up @@ -1638,12 +1641,13 @@ inline std::string sum_string_vector(const std::vector<std::string> &values) {
double tv{0.0};
auto comp = lexical_cast(arg, tv);
if(!comp) {
try {
tv = static_cast<double>(detail::to_flag_value(arg));
} catch(const std::exception &) {
fail = true;
errno = 0;
auto fv = detail::to_flag_value(arg);
fail = (errno != 0);
if(fail) {
break;
}
tv = static_cast<double>(fv);
}
val += tv;
}
Expand Down
13 changes: 5 additions & 8 deletions include/CLI/impl/App_inl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1431,18 +1431,15 @@ CLI11_INLINE bool App::_parse_single_config(const ConfigItem &item, std::size_t
auto res = config_formatter_->to_flag(item);
bool converted{false};
if(op->get_disable_flag_override()) {

try {
auto val = detail::to_flag_value(res);
if(val == 1) {
res = op->get_flag_value(item.name, "{}");
converted = true;
}
} catch(...) {
auto val = detail::to_flag_value(res);
if(val == 1) {
res = op->get_flag_value(item.name, "{}");
converted = true;
}
}

if(!converted) {
errno = 0;
res = op->get_flag_value(item.name, res);
}

Expand Down
12 changes: 6 additions & 6 deletions include/CLI/impl/Option_inl.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -389,15 +389,15 @@ CLI11_NODISCARD CLI11_INLINE std::string Option::get_flag_value(const std::strin
return input_value;
}
if(default_flag_values_[static_cast<std::size_t>(ind)].second == falseString) {
try {
auto val = detail::to_flag_value(input_value);
return (val == 1) ? falseString : (val == (-1) ? trueString : std::to_string(-val));
} catch(const std::invalid_argument &) {
errno = 0;
auto val = detail::to_flag_value(input_value);
if(errno != 0) {
errno = 0;
return input_value;
}
} else {
return input_value;
return (val == 1) ? falseString : (val == (-1) ? trueString : std::to_string(-val));
}
return input_value;
}

CLI11_INLINE Option *Option::add_result(std::string s) {
Expand Down
13 changes: 13 additions & 0 deletions tests/FuzzFailTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,16 @@ TEST_CASE("app_fail") {
} catch(const CLI::ConstructionError & /*e*/) {
}
}

TEST_CASE("file_fail") {
CLI::FuzzApp fuzzdata;
auto app = fuzzdata.generateApp();

int index = GENERATE(range(1, 2));
auto parseData = loadFailureFile("fuzz_file_fail", index);
std::stringstream out(parseData);
try {
app->parse_from_stream(out);
} catch(const CLI::ParseError & /*e*/) {
}
}
15 changes: 13 additions & 2 deletions tests/HelpersTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -201,15 +201,26 @@ TEST_CASE("StringTools: Modify3", "[helpers]") {
}

TEST_CASE("StringTools: flagValues", "[helpers]") {
errno = 0;
CHECK(-1 == CLI::detail::to_flag_value("0"));
CHECK(errno == 0);
CHECK(1 == CLI::detail::to_flag_value("t"));
CHECK(1 == CLI::detail::to_flag_value("1"));
CHECK(6 == CLI::detail::to_flag_value("6"));
CHECK(-6 == CLI::detail::to_flag_value("-6"));
CHECK(-1 == CLI::detail::to_flag_value("false"));
CHECK(1 == CLI::detail::to_flag_value("YES"));
CHECK_THROWS_AS(CLI::detail::to_flag_value("frog"), std::invalid_argument);
CHECK_THROWS_AS(CLI::detail::to_flag_value("q"), std::invalid_argument);
errno = 0;
CLI::detail::to_flag_value("frog");
CHECK(errno == EINVAL);
errno = 0;
CLI::detail::to_flag_value("q");
CHECK(errno == EINVAL);
errno = 0;
CLI::detail::to_flag_value(
"77777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777777");
CHECK(errno == ERANGE);
errno = 0;
CHECK(-1 == CLI::detail::to_flag_value("NO"));
CHECK(475555233 == CLI::detail::to_flag_value("475555233"));
}
Expand Down
1 change: 1 addition & 0 deletions tests/fuzzFail/fuzz_file_fail1
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
nflag2=555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555555"="

0 comments on commit 792d892

Please sign in to comment.