-
Notifications
You must be signed in to change notification settings - Fork 38.7k
util: Properly handle -noincludeconf on command line (take 2) #22137
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ryanofsky
left a comment
There was a problem hiding this 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
Ah I remember this is why I removed the loop initially. Quite funny that a compiler warning encourages you to introduce a bug. |
ryanofsky
left a comment
There was a problem hiding this 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;|
Thanks for the unit test. Force pushed |
This bug was introduced in commit fad0867. Unit test Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>
ryanofsky
left a comment
There was a problem hiding this 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!
|
cr ACK fa910b4: patch looks correct |
…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
This bug was introduced in commit fad0867. Unit test Co-Authored-By: Russell Yanofsky <russ@yanofsky.org> Github-Pull: bitcoin#22137 Rebased-From: fa910b4
This bug was introduced in commit fad0867. Unit test Co-Authored-By: Russell Yanofsky <russ@yanofsky.org> Github-Pull: bitcoin#22137 Rebased-From: fa910b4
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
…_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
Before:
After:
Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34884