Fix crash when parsing command line with -noincludeconf=0 #22002

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2105-parseCommandlineCrash changing 2 files +12 −7

MarcoFalke commented at 11:23 AM on May 20, 2021: member

The error message has several issues:

It may crash instead of cleanly shutting down, when -noincludeconf=0 is passed
It doesn't quote the value
It includes an erroneous trailing \n
It is redundantly mentioning "-includeconf cannot be used from commandline;" several times, when once should be more than sufficient

Fix all issues by:

Replacing get_str() with write() to fix the crash and quoting issue
Remove the \n and only print the first value to fix the other issues

Before:

$ ./src/bitcoind -noincludeconf=0
terminate called after throwing an instance of 'std::runtime_error'
  what():  JSON value is not a string as expected
Aborted (core dumped)

$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=a b
-includeconf cannot be used from commandline; -includeconf=c

After:

$ ./src/bitcoind -noincludeconf=0
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf=true

$ ./src/bitcoind -includeconf='a b' -includeconf=c
Error: Error parsing command line arguments: -includeconf cannot be used from commandline; -includeconf="a b"

Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34493

Testcase: https://github.com/bitcoin/bitcoin/files/6515429/clusterfuzz-testcase-minimized-system-6328535926046720.log

FUZZ=system ./src/test/fuzz/fuzz ./clusterfuzz-testcase-minimized-system-6328535926046720.log

See https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md

MarcoFalke added the label Utils/log/libs on May 20, 2021
MarcoFalke requested review from ryanofsky on May 20, 2021
amadeuszpawlik commented at 2:01 PM on May 20, 2021: contributor

Tested and verified; shuts down cleanly instead of runtime_error crash that I get running the same test on master.
adamjonas commented at 12:54 AM on May 21, 2021: member

Fuzzed fa175b7 and verified the test case no longer causes a crash
Fix crash when parsing command line with -noincludeconf=0 fa9f711c37

Cleanup -includeconf error message

Remove the erroneous trailing newline '\n'. Also, print only the first
value to remove needless redundancy in the error message.

fad0867d6a

MarcoFalke force-pushed on May 21, 2021
MarcoFalke commented at 9:26 AM on May 21, 2021: member

Split into two commits to simplify review
mjdietzx approved
mjdietzx commented at 4:47 PM on May 21, 2021: contributor

crACK fa9f711c3746ca3962f15224285a453744cd45b3
sipa commented at 5:42 PM on May 21, 2021: member

utACK fad0867d6ab9430070aa7d60bf7617a6508e0586
fanquake commented at 3:01 AM on May 24, 2021: member

@mjdietzx note that you've ACK'd the wrong commit hash here.
fanquake merged this on May 24, 2021
fanquake closed this on May 24, 2021
sidhujag referenced this in commit 2613f8a1ea on May 24, 2021
MarcoFalke commented at 5:06 AM on May 24, 2021: member

Might be good to hear an "Approach ACK" or similar from the settings person @ryanofsky
MarcoFalke deleted the branch on May 24, 2021
ryanofsky commented at 11:20 PM on May 27, 2021: member

Might be good to hear an "Approach ACK" or similar from the settings person @ryanofsky

Post-merge code review ACK fad0867d6ab9430070aa7d60bf7617a6508e0586. Good fixes. Fuzzer framework has been pretty annoying in some of my PRs recently, but I can't really complain if it's finding bugs in code I probably introduced :pouting_cat:

in src/util/system.cpp:371 in fad0867d6a

 365 | @@ -366,14 +366,12 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
 366 |      }
 367 |  
 368 |      // we do not allow -includeconf from command line
 369 | -    bool success = true;
 370 |      if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) {
 371 | -        for (const auto& include : util::SettingsSpan(*includes)) {

MarcoFalke commented at 9:53 AM on June 3, 2021:

fad0867d6ab9430070aa7d60bf7617a6508e0586: This loop can't be removed, because it implicitly checks whether the span is empty

MarcoFalke commented at 9:55 AM on June 3, 2021: member

Obviously, I was just testing the review process to see if a bug can be introduced without anyone noticing :sweat_smile:

Fixed in #22137
luke-jr referenced this in commit 21fa0cf39b on Jun 29, 2021
luke-jr referenced this in commit a272484dd2 on Jun 29, 2021
fanquake referenced this in commit 70eac6fcd0 on Jun 30, 2021
fanquake referenced this in commit 513613d8a8 on Jun 30, 2021
stevenroose referenced this in commit bd2e2d5c64 on Jul 1, 2021
fanquake referenced this in commit bd2f4164c6 on Jul 8, 2021
gwillen referenced this in commit 2dc03d35a0 on Jun 1, 2022
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#22366 [0.21] fuzz: add missing ECCVerifyHandle to base_encode_decode