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

maflcko · 2021-05-20T11:23:50Z

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

amadeuszpawlik · 2021-05-20T14:01:57Z

Tested and verified; shuts down cleanly instead of runtime_error crash that I get running the same test on master.

adamjonas · 2021-05-21T00:54:58Z

Fuzzed fa175b7 and verified the test case no longer causes a crash

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

maflcko · 2021-05-21T09:26:52Z

Split into two commits to simplify review

mjdietzx

crACK fa9f711

sipa · 2021-05-21T17:42:04Z

utACK fad0867

fanquake · 2021-05-24T03:01:57Z

@mjdietzx note that you've ACK'd the wrong commit hash here.

…udeconf=0 fad0867 Cleanup -includeconf error message (MarcoFalke) fa9f711 Fix crash when parsing command line with -noincludeconf=0 (MarcoFalke) Pull request description: 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 ACKs for top commit: sipa: utACK fad0867 Tree-SHA512: b44af93be6bf71b43669058c1449c4c6999f03b5b01b429851b149b12d77733408cb207e9a3edc6f0bffd6030c4c52165e8e23a1c2718ff5082a6ba254cc94a4

maflcko · 2021-05-24T05:06:04Z

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

ryanofsky

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

Post-merge code review ACK fad0867. 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 😾

maflcko

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

Fixed in #22137

maflcko · 2021-06-03T09:53:24Z

src/util/system.cpp

    // we do not allow -includeconf from command line
-    bool success = true;
    if (auto* includes = util::FindKey(m_settings.command_line_options, "includeconf")) {
-        for (const auto& include : util::SettingsSpan(*includes)) {


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

Github-Pull: bitcoin#22002 Rebased-From: fa9f711

Remove the erroneous trailing newline '\n'. Also, print only the first value to remove needless redundancy in the error message. Github-Pull: bitcoin#22002 Rebased-From: fad0867

Github-Pull: bitcoin#22002 Rebased-From: fa9f711

Remove the erroneous trailing newline '\n'. Also, print only the first value to remove needless redundancy in the error message. Github-Pull: bitcoin#22002 Rebased-From: fad0867

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

Keepng our side of this conflict because there are a couple PRs that affec this block, and we already backported both of them in an Elements PR.

maflcko added the Utils/log/libs label May 20, 2021

maflcko requested a review from ryanofsky May 20, 2021 11:23

MarcoFalke added 2 commits May 21, 2021 10:53

Fix crash when parsing command line with -noincludeconf=0

fa9f711

Cleanup -includeconf error message

fad0867

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

maflcko force-pushed the 2105-parseCommandlineCrash branch from fa175b7 to fad0867 Compare May 21, 2021 08:54

mjdietzx approved these changes May 21, 2021

View reviewed changes

fanquake merged commit 3f3c4d2 into bitcoin:master May 24, 2021

maflcko deleted the 2105-parseCommandlineCrash branch May 24, 2021 06:43

ryanofsky reviewed May 27, 2021

View reviewed changes

maflcko commented Jun 3, 2021

View reviewed changes

apoelstra mentioned this pull request Jun 19, 2021

fuzz: backport fixes to fuzzer errors ElementsProject/elements#1012

Merged

fanquake mentioned this pull request Jun 29, 2021

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

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 29, 2021

Fix crash when parsing command line with -noincludeconf=0

21fa0cf

Github-Pull: bitcoin#22002 Rebased-From: fa9f711

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 30, 2021

Fix crash when parsing command line with -noincludeconf=0

70eac6f

Github-Pull: bitcoin#22002 Rebased-From: fa9f711

barton2526 mentioned this pull request Mar 22, 2022

util: Fix crash when parsing command line with -noincludeconf=0, Properly handle -noincludeconf on command line gridcoin-community/Gridcoin-Research#2473

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

maflcko commented May 20, 2021

Uh oh!

amadeuszpawlik commented May 20, 2021 •

edited

Loading

Uh oh!

adamjonas commented May 21, 2021

Uh oh!

maflcko commented May 21, 2021

Uh oh!

mjdietzx left a comment

Uh oh!

sipa commented May 21, 2021

Uh oh!

fanquake commented May 24, 2021

Uh oh!

maflcko commented May 24, 2021

Uh oh!

ryanofsky left a comment

Uh oh!

maflcko left a comment

Uh oh!

maflcko Jun 3, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

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

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

Uh oh!

Conversation

maflcko commented May 20, 2021

Uh oh!

amadeuszpawlik commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjonas commented May 21, 2021

Uh oh!

maflcko commented May 21, 2021

Uh oh!

mjdietzx left a comment

Choose a reason for hiding this comment

Uh oh!

sipa commented May 21, 2021

Uh oh!

fanquake commented May 24, 2021

Uh oh!

maflcko commented May 24, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 3, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

amadeuszpawlik commented May 20, 2021 •

edited

Loading