add a round trip test to the fuzzer (#1060)

This is the next phase of the fuzzer. It runs a round trip and makes
sure that the config files generated by the app will load into the same
results, to test full round trip on the config files.

Issues fixed
- fix a bug in the string escape code caught by initial round trip tests
- resolve inconsistencies in handling of {} for empty vector indication
between config and cli parsing

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

README.md

@@ -287,7 +287,9 @@ string, with the dash or dashes. An option or flag can have as many names as you
 want, and afterward, using `count`, you can use any of the names, with dashes as
 needed, to count the options. One of the names is allowed to be given without
 proceeding dash(es); if present the option is a positional option, and that name
-will be used on the help line for its positional form.
+will be used on the help line for its positional form. The string `++` is also
+not allowed as option name due to its use as an array separator and marker on
+config files.
 
 The `add_option_function<type>(...` function will typically require the template
 parameter be given unless a `std::function` object with an exact match is

fuzz/CMakeLists.txt

@@ -31,7 +31,7 @@ if(CMAKE_CXX_STANDARD GREATER 16)
       COMMAND
         cli11_app_fuzzer corp -max_total_time=${CLI11_FUZZ_TIME_APP} -max_len=2148
         -dict=${CMAKE_CURRENT_SOURCE_DIR}/fuzz_dictionary1.txt
-        -exact_artifact_path=${CLI11_FUZZ_ARTIFACT_PATH}/cli11_app_fail_artifact.txt)
+        -exact_artifact_path=${CLI11_FUZZ_ARTIFACT_PATH}/cli11_app_roundtrip_fail_artifact.txt)
 
     add_custom_target(
       QUICK_CLI11_FILE_FUZZ

fuzz/cli11_app_fuzz.cpp

@@ -26,13 +26,17 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
         parseString.erase(0, 25);
     }
     CLI::FuzzApp fuzzdata;
+    CLI::FuzzApp fuzzdata2;
     auto app = fuzzdata.generateApp();
+    auto app2 = fuzzdata2.generateApp();
     try {
         if(!optionString.empty()) {
             app->add_option(optionString, fuzzdata.buffer);
+            app2->add_option(optionString, fuzzdata2.buffer);
         }
         if(!flagString.empty()) {
             app->add_flag(flagString, fuzzdata.intbuffer);
+            app2->add_flag(flagString, fuzzdata2.intbuffer);
         }
     } catch(const CLI::ConstructionError &e) {
         return 0;  // Non-zero return values are reserved for future use.
@@ -50,6 +54,10 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t *Data, size_t Size) {
     std::string configOut = app->config_to_str();
     app->clear();
     std::stringstream out(configOut);
-    app->parse_from_stream(out);
+    app2->parse_from_stream(out);
+    auto result = fuzzdata2.compare(fuzzdata);
+    if(!result) {
+        throw CLI::ValidationError("fuzzer", "file input results don't match parse results");
+    }
     return 0;
 }

fuzz/fuzzApp.cpp

@@ -5,6 +5,7 @@
 // SPDX-License-Identifier: BSD-3-Clause
 
 #include "fuzzApp.hpp"
+#include <algorithm>
 
 namespace CLI {
 /*
@@ -148,4 +149,147 @@ std::shared_ptr<CLI::App> FuzzApp::generateApp() {
     return fApp;
 }
 
+bool FuzzApp::compare(const FuzzApp &other) const {
+    if(val32 != other.val32) {
+        return false;
+    }
+    if(val16 != other.val16) {
+        return false;
+    }
+    if(val8 != other.val8) {
+        return false;
+    }
+    if(val64 != other.val64) {
+        return false;
+    }
+
+    if(uval32 != other.uval32) {
+        return false;
+    }
+    if(uval16 != other.uval16) {
+        return false;
+    }
+    if(uval8 != other.uval8) {
+        return false;
+    }
+    if(uval64 != other.uval64) {
+        return false;
+    }
+
+    if(atomicval64 != other.atomicval64) {
+        return false;
+    }
+    if(atomicuval64 != other.atomicuval64) {
+        return false;
+    }
+
+    if(v1 != other.v1) {
+        return false;
+    }
+    if(v2 != other.v2) {
+        return false;
+    }
+
+    if(vv1 != other.vv1) {
+        return false;
+    }
+    if(vstr != other.vstr) {
+        return false;
+    }
+
+    if(vecvecd != other.vecvecd) {
+        return false;
+    }
+    if(vvs != other.vvs) {
+        return false;
+    }
+    if(od1 != other.od1) {
+        return false;
+    }
+    if(ods != other.ods) {
+        return false;
+    }
+    if(p1 != other.p1) {
+        return false;
+    }
+    if(p2 != other.p2) {
+        return false;
+    }
+    if(t1 != other.t1) {
+        return false;
+    }
+    if(tcomplex != other.tcomplex) {
+        return false;
+    }
+    if(tcomplex2 != other.tcomplex2) {
+        return false;
+    }
+    if(vectup != other.vectup) {
+        return false;
+    }
+    if(vstrv != other.vstrv) {
+        return false;
+    }
+
+    if(flag1 != other.flag1) {
+        return false;
+    }
+    if(flagCnt != other.flagCnt) {
+        return false;
+    }
+    if(flagAtomic != other.flagAtomic) {
+        return false;
+    }
+
+    if(iwrap.value() != other.iwrap.value()) {
+        return false;
+    }
+    if(dwrap.value() != other.dwrap.value()) {
+        return false;
+    }
+    if(swrap.value() != other.swrap.value()) {
+        return false;
+    }
+    if(buffer != other.buffer) {
+        return false;
+    }
+    if(intbuffer != other.intbuffer) {
+        return false;
+    }
+    if(doubleAtomic != other.doubleAtomic) {
+        return false;
+    }
+
+    // for testing restrictions and reduction methods
+    if(vstrA != other.vstrA) {
+        return false;
+    }
+    if(vstrB != other.vstrB) {
+        return false;
+    }
+    if(vstrC != other.vstrC) {
+        return false;
+    }
+    if(vstrD != other.vstrD) {
+        // the return result if reversed so it can alternate
+        std::vector<std::string> res = vstrD;
+        std::reverse(res.begin(), res.end());
+        if(res != other.vstrD) {
+            return false;
+        }
+    }
+    if(vstrE != other.vstrE) {
+        return false;
+    }
+    if(vstrF != other.vstrF) {
+        return false;
+    }
+    if(mergeBuffer != other.mergeBuffer) {
+        return false;
+    }
+    if(validator_strings != other.validator_strings) {
+        return false;
+    }
+    return true;
+}
 }  // namespace CLI

fuzz/fuzzApp.hpp

@@ -54,8 +54,10 @@ class stringWrapper {
 class FuzzApp {
   public:
     FuzzApp() = default;
-
+    /** generate a fuzzing application with a bunch of different interfaces*/
     std::shared_ptr<CLI::App> generateApp();
+    /** compare two fuzz apps for equality*/
+    CLI11_NODISCARD bool compare(const FuzzApp &other) const;
 
     int32_t val32{0};
     int16_t val16{0};

include/CLI/Error.hpp

@@ -132,8 +132,8 @@ class BadNameString : public ConstructionError {
     static BadNameString BadPositionalName(std::string name) {
         return BadNameString("Invalid positional Name: " + name);
     }
-    static BadNameString DashesOnly(std::string name) {
-        return BadNameString("Must have a name, not just dashes: " + name);
+    static BadNameString ReservedName(std::string name) {
+        return BadNameString("Names '-','--','++' are reserved and not allowed as option names " + name);
     }
     static BadNameString MultiPositionalNames(std::string name) {
         return BadNameString("Only one positional name allowed, remove: " + name);

include/CLI/StringTools.hpp

@@ -59,7 +59,12 @@ template <typename T> std::string join(const T &v, std::string delim = ",") {
     while(beg != end) {
         s << delim << *beg++;
     }
-    return s.str();
+    auto rval = s.str();
+    if(!rval.empty() && delim.size() == 1 && rval.back() == delim[0]) {
+        // remove trailing delimiter if the last entry was empty
+        rval.pop_back();
+    }
+    return rval;
 }
 
 /// Simple function to join a string from processed elements

include/CLI/impl/App_inl.hpp

@@ -1547,7 +1547,9 @@ CLI11_INLINE bool App::_parse_single_config(const ConfigItem &item, std::size_t
 
                 if(!converted) {
                     errno = 0;
-                    res = op->get_flag_value(item.name, res);
+                    if(res != "{}" || op->get_expected_max() <= 1) {
+                        res = op->get_flag_value(item.name, res);
+                    }
                 }
 
                 op->add_result(res);
@@ -1896,7 +1898,7 @@ App::_parse_arg(std::vector<std::string> &args, detail::Classifier current_type,
             // using dot notation is equivalent to single argument subcommand
             auto *sub = _find_subcommand(arg_name.substr(0, dotloc), true, false);
             if(sub != nullptr) {
-                auto v = args.back();
+                std::string v = args.back();
                 args.pop_back();
                 arg_name = arg_name.substr(dotloc + 1);
                 if(arg_name.size() > 1) {

include/CLI/impl/Config_inl.hpp

@@ -321,6 +321,19 @@ inline std::vector<ConfigItem> ConfigBase::from_config(std::istream &input) cons
                 if(!item.empty() && item.back() == '\\') {
                     item.pop_back();
                     lineExtension = true;
+                } else if(detail::hasMLString(item, keyChar)) {
+                    // deal with the first line closing the multiline literal
+                    item.pop_back();
+                    item.pop_back();
+                    item.pop_back();
+                    if(keyChar == '\"') {
+                        try {
+                            item = detail::remove_escaped_characters(item);
+                        } catch(const std::invalid_argument &iarg) {
+                            throw CLI::ParseError(iarg.what(), CLI::ExitCodes::InvalidError);
+                        }
+                    }
+                    inMLineValue = false;
                 }
                 while(inMLineValue) {
                     std::string l2;

include/CLI/impl/Option_inl.hpp

@@ -284,7 +284,9 @@ CLI11_NODISCARD CLI11_INLINE std::string Option::get_name(bool positional, bool
 }
 
 CLI11_INLINE void Option::run_callback() {
+    bool used_default_str = false;
     if(force_callback_ && results_.empty()) {
+        used_default_str = true;
         add_result(default_str_);
     }
     if(current_option_state_ == option_state::parsing) {
@@ -294,16 +296,18 @@ CLI11_INLINE void Option::run_callback() {
 
     if(current_option_state_ < option_state::reduced) {
         _reduce_results(proc_results_, results_);
-        current_option_state_ = option_state::reduced;
     }
-    if(current_option_state_ >= option_state::reduced) {
-        current_option_state_ = option_state::callback_run;
-        if(!(callback_)) {
-            return;
-        }
+
+    current_option_state_ = option_state::callback_run;
+    if(callback_) {
         const results_t &send_results = proc_results_.empty() ? results_ : proc_results_;
         bool local_result = callback_(send_results);
-
+        if(used_default_str) {
+            // we only clear the results if the callback was actually used
+            // otherwise the callback is the storage of the default
+            results_.clear();
+            proc_results_.clear();
+        }
         if(!local_result)
             throw ConversionError(get_name(), results_);
     }

include/CLI/impl/Split_inl.hpp

@@ -125,8 +125,8 @@ get_names(const std::vector<std::string> &input) {
                 long_names.push_back(name);
             else
                 throw BadNameString::BadLongName(name);
-        } else if(name == "-" || name == "--") {
-            throw BadNameString::DashesOnly(name);
+        } else if(name == "-" || name == "--" || name == "++") {
+            throw BadNameString::ReservedName(name);
         } else {
             if(!pos_name.empty())
                 throw BadNameString::MultiPositionalNames(name);

include/CLI/impl/StringTools_inl.hpp

@@ -459,7 +459,8 @@ CLI11_INLINE std::string binary_escape_string(const std::string &string_to_escap
     if(escaped_string != string_to_escape) {
         auto sqLoc = escaped_string.find('\'');
         while(sqLoc != std::string::npos) {
-            escaped_string.replace(sqLoc, sqLoc + 1, "\\x27");
+            escaped_string[sqLoc] = '\\';
+            escaped_string.insert(sqLoc + 1, "x27");
             sqLoc = escaped_string.find('\'');
         }
         escaped_string.insert(0, "'B\"(");

tests/AppTest.cpp

@@ -2072,6 +2072,18 @@ TEST_CASE_METHOD(TApp, "EnvOnly", "[app]") {
     CHECK_THROWS_AS(run(), CLI::RequiredError);
 }
 
+// reported bug #1013 on github
+TEST_CASE_METHOD(TApp, "groupEnvRequired", "[app]") {
+    std::string str;
+    auto *group1 = app.add_option_group("group1");
+    put_env("CLI11_TEST_GROUP_REQUIRED", "string_abc");
+    group1->add_option("-f", str, "f")->envname("CLI11_TEST_GROUP_REQUIRED")->required();
+
+    run();
+    CHECK(str == "string_abc");
+    unset_env("CLI11_TEST_GROUP_REQUIRED");
+}
+
 TEST_CASE_METHOD(TApp, "RangeInt", "[app]") {
     int x{0};
     app.add_option("--one", x)->check(CLI::Range(3, 6));