util: Properly handle -noincludeconf on command line (take 2) #22137

Before:

$ ./src/qt/bitcoin-qt -noincludeconf
(memory violation, can be observed with valgrind or similar)

After:

$ ./src/qt/bitcoin-qt -noincludeconf
(passes startup)

Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34884

Seems to cause -Werror,-Wunreachable-code-loop-increment error https://cirrus-ci.com/task/5561194028204032?logs=ci#L2596

Seems to cause -Werror,-Wunreachable-code-loop-increment error https://cirrus-ci.com/task/5561194028204032?logs=ci#L2596

Ah I remember this is why I removed the loop initially. Quite funny that a compiler warning encourages you to introduce a bug.

Code review ACK fa2fc70adefda51913b714993931e281a67de5e0. Fix looks good. I also worked on adding a test for this:

diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index d463bcdd8e3..2df5c808fa2 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -329,6 +329,27 @@ BOOST_FIXTURE_TEST_CASE(util_CheckValue, CheckValueTest)
     CheckValue(M::ALLOW_ANY, "-value=abc", Expect{"abc"}.String("abc").Int(0).Bool(false).List({"abc"}));
 }
 
+class NoIncludeConfTest : public TestChain100Setup
+{
+public:
+    std::string Parse(const char* arg)
+    {
+        TestArgsManager test;
+        test.SetupArgs({{"-includeconf", ArgsManager::ALLOW_ANY}});
+        const char* argv[] = {"ignored", arg};
+        std::string error;
+        bool success = test.ParseParameters(2, (char**)argv, error);
+        return success ? "" : error;
+    }
+};
+
+BOOST_FIXTURE_TEST_CASE(util_NoIncludeConf, NoIncludeConfTest)
+{
+    BOOST_CHECK_EQUAL(Parse("-noincludeconf"), "");
+    BOOST_CHECK_EQUAL(Parse("-includeconf"), "-includeconf cannot be used from commandline; -includeconf=\"\"");
+    BOOST_CHECK_EQUAL(Parse("-includeconf=file"), "-includeconf cannot be used from commandline; -includeconf=\"file\"");
+}
+
 BOOST_AUTO_TEST_CASE(util_ParseParameters)
 {
     TestArgsManager testArgs;
diff --git a/src/util/system.cpp b/src/util/system.cpp
index 27174a6028f..f9fc446e887 100644
--- a/src/util/system.cpp
+++ b/src/util/system.cpp
@@ -367,9 +367,11 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
 
     // we do not allow -includeconf from command line
     if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) {
-        for (const auto& include : util::SettingsSpan(*includes)) {
-            error = "-includeconf cannot be used from commandline; -includeconf=" + include.write();
-            return false; // pick first value as example
+        util::SettingsSpan values{*includes};
+        if (!values.empty()) {
+            // pick first value as example
+            error = "-includeconf cannot be used from commandline; -includeconf=" + values.begin()->write();
+            return false;
         }
     }
     return true;

Thanks for the unit test. Force pushed

Code review ACK fa910b47656d0e69cccb1f31804f2b11aa45d053. Nice cleanups!

cr ACK fa910b47656d0e69cccb1f31804f2b11aa45d053: patch looks correct