Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jun 3, 2021

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member Author

maflcko commented Jun 3, 2021

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.

@maflcko maflcko changed the title util: Properly handle -noincludeconf on command line util: Properly handle -noincludeconf on command line (take 2) Jun 3, 2021
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

@maflcko
Copy link
Member Author

maflcko commented Jun 4, 2021

Thanks for the unit test. Force pushed

This bug was introduced in commit
fad0867.

Unit test
Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK fa910b4. Nice cleanups!

@practicalswift
Copy link
Contributor

cr ACK fa910b4: patch looks correct

@fanquake fanquake merged commit 791f985 into bitcoin:master Jun 7, 2021
@maflcko maflcko deleted the 2106-utilTake2 branch June 7, 2021 07:04
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 9, 2021
…line (take 2)

fa910b4 util: Properly handle -noincludeconf on command line (MarcoFalke)

Pull request description:

  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

ACKs for top commit:
  practicalswift:
    cr ACK fa910b4: patch looks correct
  ryanofsky:
    Code review ACK fa910b4. Nice cleanups!

Tree-SHA512: 5dfad82a78bca7a9a6bcc6aead2d7fbde166a09a5300a82f80dd1aee1de00e070bcb30b7472741a5396073b370898696e78c33038f94849219281d99358248ed
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 29, 2021
This bug was introduced in commit
fad0867.

Unit test
Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>

Github-Pull: bitcoin#22137
Rebased-From: fa910b4
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 30, 2021
This bug was introduced in commit
fad0867.

Unit test
Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>

Github-Pull: bitcoin#22137
Rebased-From: fa910b4
stevenroose added a commit to ElementsProject/elements that referenced this pull request Jul 1, 2021
0896d0a util: Properly handle -noincludeconf on command line (MarcoFalke)
5f18d33 Cleanup -includeconf error message (MarcoFalke)
99b8659 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
c98902b fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)

Pull request description:

  Backport of bitcoin/bitcoin#22279 and bitcoin/bitcoin#22002 and bitcoin/bitcoin#22137

ACKs for top commit:
  jonasnick:
    utACK 0896d0a

Tree-SHA512: 7a9c4a20fc51ac3e66fd0b8d6f28200b9342774fcb003c561e277fab4a68c3ebd2cab4c3081170199a51aabf3956e0f7248fa6c853c8aa971645fb9039adc688
fanquake added a commit that referenced this pull request Jul 8, 2021
…_decode

da81624 util: Properly handle -noincludeconf on command line (MarcoFalke)
513613d Cleanup -includeconf error message (MarcoFalke)
70eac6f Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke)
c5357fa fuzz: add missing ECCVerifyHandle to base_encode_decode (Andrew Poelstra)

Pull request description:

  Backports #22279, #22002 and #22137 to fix fuzzing issues in the 0.21 branch: https://github.com/bitcoin/bitcoin/runs/2864012729.

ACKs for top commit:
  achow101:
    ACK da81624

Tree-SHA512: ab8751387e42e03ff43594ae34be8ed0dba903d7da1aaecb9f19c08366570d8995abe89ba0c9bafe37662940f3e83bef1e9e50f330e86114cd6a773becd1fd21
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants