util: raise a deadly signal in system.cpp fuzzer #18908

pull brakmic wants to merge 1 commits into bitcoin:master from brakmic:fuzz-ub-argsmanager changing 1 files +1 −0

brakmic commented at 6:52 PM on May 7, 2020: contributor

This PR deliberately raises a deadly signal in system.cpp fuzzer while testing ArgsManager::ParseParameters. The motivation for this change is based on experiences from this PR #18901

As the fuzzer provoked UB, I implemented a way to catch this error. However, after consultation with elichai and sipa I came to the conclusion that there was no proper solution in user-related code. The problem was to be fixed in the fuzzer itself. So, a simple assert got introduced to provoke a deadly signal when fuzzer walks over it.

For more information, please, check the discussion below.
elichai commented at 7:13 PM on May 7, 2020: contributor

Could you expand on which fuzzer hit that? is it fuzz/system.cpp?

I Believe the problem is actually with the fuzzer, I think ConsumeRandomLengthStringVector can generate an empty string, which then c_str() returns NULL. but the C++ standard[1] says that argv contain only valid non null pointers up to argc-1. So I believe the fuzzer should actually be fixed, not the code here.

[1] http://eel.is/c++draft/basic.start.main#2

brakmic commented at 7:15 PM on May 7, 2020: contributor

Could you expand on which fuzzer hit that? is it fuzz/system.cpp?

It is the system.cpp fuzzer.

Run system with args ['/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/system', '-runs=1', '/home/travis/build/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/system']INFO: Seed: 3656541697
INFO: Loaded 1 modules   (407748 inline 8-bit counters): 407748 [0x563d4c439078, 0x563d4c49c93c), 
INFO: Loaded 1 PC tables (407748 PCs): 407748 [0x563d4c49c940,0x563d4cad5580), 
INFO:      942 files found in /home/travis/build/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/system
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 942 min: 1b max: 4093b total: 407084b rss: 88Mb

brakmic commented at 7:16 PM on May 7, 2020: contributor

@elichai

And now it throws no UB error anymore. Not sure if the fuzzer is the problem.

Run system with args ['/home/travis/build/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/system', '-runs=1', '/home/travis/build/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/system']INFO: Seed: 2965575158
INFO: Loaded 1 modules   (406999 inline 8-bit counters): 406999 [0x55beda5d8e28, 0x55beda63c3ff), 
INFO: Loaded 1 PC tables (406999 PCs): 406999 [0x55beda63c400,0x55bedac72170), 
INFO:      942 files found in /home/travis/build/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_seed_corpus/system
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: seed corpus: files: 942 min: 1b max: 4093b total: 407084b rss: 88Mb
[#512](/bitcoin-bitcoin/512/)	pulse  cov: 4127 ft: 16451 corp: 483/12995b exec/s: 256 rss: 112Mb
[#1009](/bitcoin-bitcoin/1009/)	INITED cov: 4128 ft: 26595 corp: 905/382Kb exec/s: 100 rss: 161Mb
[#1009](/bitcoin-bitcoin/1009/)	DONE   cov: 4128 ft: 26595 corp: 905/382Kb lim: 4093 exec/s: 100 rss: 161Mb
Done 1009 runs in 10 second(s)

https://travis-ci.org/github/bitcoin/bitcoin/jobs/684399768

--EDIT: also Undefined Behavior in C/C++ is actually a "feature", so compiler vendors can produce less complex compilers. But in any case, we should always test for nullptrs, regardless of current compiler implementation, fuzzer problems etc., imo.

--EDIT 2: have changed this PR to ready to review. However, if there is a reason not to fix this piece of code, but instead the fuzzer itself, I can close it anytime.

brakmic marked this as ready for review on May 7, 2020
DrahtBot added the label Docs on May 7, 2020
DrahtBot added the label Utils/log/libs on May 7, 2020
in doc/release-notes-18908.md:1 in d0fa28b296 outdated
```
   0 | @@ -0,0 +1 @@
   1 | +- Fixed undefined behavior error in ArgsManager::ParseParameters that appeared while running system.cpp fuzzer.
```
sipa commented at 11:35 PM on May 7, 2020:

This does not require release notes. It's not a user-visible change.

brakmic commented at 8:38 AM on May 8, 2020:

Have removed the document.
in src/util/system.cpp:288 in d0fa28b296 outdated
```
 284 | @@ -285,6 +285,10 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin
 285 |      m_settings.command_line_options.clear();
 286 |  
 287 |      for (int i = 1; i < argc; i++) {
 288 | +        if (argv[i] == nullptr) {
```
sipa commented at 11:43 PM on May 7, 2020:

I think this is the wrong place to fix things. In production this condition cannot occur, so this should be treated as a code error (which warrants documenting assumptions, or perhaps an assertion), not a user error (which warrants a runtime error message like this).
brakmic commented at 8:40 AM on May 8, 2020:

An assert leaves the fuzzer with "deadly signal" which also breaks the build. Am not sure how to make it:

pass the fuzzer

but without introducing any user-error checking logic
sipa commented at 5:41 PM on May 8, 2020:

And the fuzzer should get a deadly signal, because something in the code is wrong. It is correctly reporting an issue.

brakmic commented at 5:44 PM on May 8, 2020:

Ok, then all is fine.

However, the problem happened in an unrelated PR or mine (and will presumably happen in other people's PR's too). This was my actual motivation: how to get a clean build on all available travis platforms? :)

But, if it's acceptable to have a deadly signal, it's OK for me too.

sipa commented at 5:49 PM on May 8, 2020:

Well, if the fuzzer finds a bug, the bug should be fixed!

brakmic commented at 5:51 PM on May 8, 2020:

Yes, that's true. Have updated the title of this PR and its description.

Thanks to you and @elichai
sipa commented at 11:47 PM on May 7, 2020: member

The actual bug is in the fuzzer: in case 7, the argv array is resized, and then arguments are pushed back onto it. This means that the argv array will always consist of N nullptrs, followed by N real arguments.

Probably whoever wrote this code intended to use argv.reserve rather than argv.resize.
brakmic renamed this:
~~util: fix UB error in ArgsManager::ParseParameters~~
util: raise a deadly signal in system.cpp fuzzer because of problems in ArgsManager::ParseParameters
on May 8, 2020
brakmic renamed this:
~~util: raise a deadly signal in system.cpp fuzzer because of problems in ArgsManager::ParseParameters~~
util: raise a deadly signal in system.cpp fuzzer
on May 8, 2020

util: raise a deadly signal in system.cpp fuzzer

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Elichai Turkel <elichai.turkel@gmail.com>

2ac68becc8

MarcoFalke commented at 5:58 PM on May 8, 2020: member

Can you please fix the fuzz code as pointed out in #18908#pullrequestreview-407905781

Interesting that the issue is only hit when compiling with C++17

Also cc @practicalswift
brakmic commented at 6:06 PM on May 8, 2020: contributor

Can you please fix the fuzz code as pointed out in #18908 (review)

Sure, will look into it an open a new PR regarding system.cpp fuzzer.
brakmic commented at 6:26 PM on May 8, 2020: contributor

@MarcoFalke

Done, https://github.com/bitcoin/bitcoin/pull/18917
practicalswift commented at 7:59 PM on May 8, 2020: contributor

Thanks for bringing this issue to my attention @brakmic.

With the fix in #18917 I think this PR can be closed?
brakmic commented at 8:07 PM on May 8, 2020: contributor

Thanks for bringing this issue to attention @brakmic.

With the fix in #18917 I think this PR can be closed?

Yes, I think we can close it now. :)
brakmic closed this on May 8, 2020
MarcoFalke referenced this in commit b55866969e on May 8, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#18901 fuzz: use std::optional for sep_pos_opt variable #18917 fuzz: fix vector size problem in system fuzzer