fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. #21169

practicalswift · 2021-02-13T08:11:04Z

Add RPC interface fuzzing.

This PR increases overall fuzzing line coverage from ~65% to ~70% 🎉

To test this PR:

$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make -C src/ test/fuzz/fuzz
$ FUZZ=rpc src/test/fuzz/fuzz

See doc/fuzzing.md for more information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the Bitcoin Core fuzzing corpus repo.

Happy fuzzing :)

jonatack

Concept ACK, debug build clean and it runs

$ FUZZ=rpc src/test/fuzz/fuzz
INFO: Seed: 974835892
INFO: Loaded 1 modules   (625697 inline 8-bit counters): 625697 [0x5636dc7b9268, 0x5636dc851e89), 
INFO: Loaded 1 PC tables (625697 PCs): 625697 [0x5636dc851e90,0x5636dd1de0a0), 
INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
INFO: A corpus is not provided, starting from an empty corpus
#2	INITED cov: 110 ft: 111 corp: 1/1b exec/s: 0 rss: 191Mb
#4	NEW    cov: 110 ft: 118 corp: 2/3b lim: 4 exec/s: 0 rss: 191Mb L: 2/2 MS: 2 ChangeBit-InsertByte-
#10	NEW    cov: 110 ft: 125 corp: 3/6b lim: 4 exec/s: 0 rss: 191Mb L: 3/3 MS: 1 CopyPart-
#13	NEW    cov: 110 ft: 130 corp: 4/10b lim: 4 exec/s: 0 rss: 191Mb L: 4/4 MS: 3 ChangeBinInt-EraseBytes-CrossOver-
...
#217864	REDUCE cov: 16595 ft: 45707 corp: 992/59Kb lim: 116 exec/s: 1998 rss: 616Mb L: 84/116 MS: 1 EraseBytes-
#218080	REDUCE cov: 16595 ft: 45709 corp: 993/59Kb lim: 116 exec/s: 2000 rss: 616Mb L: 115/116 MS: 1 InsertRepeatedBytes-
#218133	REDUCE cov: 16595 ft: 45709 corp: 993/59Kb lim: 116 exec/s: 2001 rss: 616Mb L: 84/116 MS: 3 ChangeByte-CMP-EraseBytes- DE: "St9exception"-
	NEW_FUNC[1/25]: 0x5636d8897d20 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release() /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/shared_ptr_base.h:152
	NEW_FUNC[2/25]: 0x5636d88db840 in std::__allocated_ptr<std::allocator<std::_Sp_counted_ptr_inplace<CBlock, std::allocator<CBlock>, (__gnu_cxx::_Lock_policy)2> > > std::__allocate_guarded<std::allocator<std::_Sp_counted_ptr_inplace<CBlock, std::allocator<CBlock>, (__gnu_cxx::_Lock_policy)2> > >(std::allocator<std::_Sp_counted_ptr_inplace<CBlock, std::allocator<CBlock>, (__gnu_cxx::_Lock_policy)2> >&) /usr/bin/../lib/gcc/x86_64-linux-gnu/10/../../../../include/c++/10/bits/allocated_ptr.h:96
#218742	REDUCE cov: 16767 ft: 45872 corp: 994/59Kb lim: 122 exec/s: 1988 rss: 616Mb L: 30/116 MS: 4 EraseBytes-ChangeBit-ChangeBinInt-CMP- DE: "submitblock"-

src/test/fuzz/rpc.cpp

maflcko · 2021-02-22T07:49:30Z

I am not sure if this is worth it. This needs a lot of maintenance (adding or removing rpcs, marking them unsafe, ...) and I am sceptical that this will find any actual issues.

src/test/fuzz/rpc.cpp

adamjonas · 2021-04-27T16:35:19Z

@MarcoFalke to clarify, are you a NACK on this?

practicalswift · 2021-04-27T19:42:33Z

@adamjonas

Good question and thanks for asking it explicitly! :)

I should probably have stated that more clearly in the PR description, but it should be noted that this fuzzing harness is by far is the "most covering" harness in the repo.

Including this harness in the existing set of fuzzing harnesses adds coverage to five(!) percent of the code base that is currently uncovered by fuzz testing. The total fuzzing coverage increases from 65% to 70%.

The opportunities to achieve such extreme coverage jumps from adding a single harness are extremely rare at the relatively high levels of fuzzing coverage we've reached. TBH it would feel a bit disappointing if we had to pass on the excellent coverage opportunity that RPC fuzzing brings :)

@MarcoFalke, if we can find a way to reduce/mitigate the potential maintenance work you predict in #21169 (comment) would you be willing to reconsider your position? :)

src/test/fuzz/rpc.cpp

…o 70%.

maflcko · 2021-04-28T07:45:13Z

Concept ACK 545404e

If this is leading to too much (maintenance) hassle, it can be reverted.

maflcko · 2021-04-28T08:10:05Z

src/test/fuzz/rpc.cpp

+    "dumpwallet", // avoid writing to disk
+#endif
+    "echoipc",           // avoid assertion failure (Assertion `"EnsureAnyNodeContext(request.context).init" && check' failed.)
+    "generatetoaddress", // avoid timeout


why is this unsafe, but the other generate* calls are safe?

Thanks! Addressed in #21810:

"generatetoaddress", // avoid prohibitively slow execution (when `num_blocks` is large) "generatetodescriptor", // avoid prohibitively slow execution (when `nblocks` is large)

maflcko · 2021-04-28T08:10:14Z

src/test/fuzz/rpc.cpp

+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <base58.h>
+#include <chainparamsbase.h>


Thanks! Addressed in follow-up.

maflcko · 2021-04-28T08:10:41Z

src/test/fuzz/rpc.cpp

+#include <base58.h>
+#include <chainparamsbase.h>
+#include <core_io.h>
+#include <interfaces/chain.h>


Thanks! Addressed in follow-up.

maflcko · 2021-04-28T08:10:48Z

src/test/fuzz/rpc.cpp

+    "importwallet", // avoid reading from disk
+    "loadwallet",   // avoid reading from disk
+#endif
+    "mockscheduler",         // avoid assertion failure (Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed.)


Seems so! Thanks! Re-enabled in follow-up.

maflcko · 2021-04-28T08:19:39Z

src/test/fuzz/rpc.cpp

+    "preciousblock",
+    "pruneblockchain",
+    "reconsiderblock",
+    "savemempool",


This seems dangerous. If the rpc is ever changed to take a file path, how would we know to adjust this line?

Good point. Disabled as a precautionary measure in #21810.

dump*, save*, import* and load* commands should probably not be part of RPC_COMMANDS_SAFE_FOR_FUZZING.

Longer term we could perhaps also find a clever way to annotate/auto-detect RPC commands with file path arguments and have them automatically removed from RPC_COMMANDS_SAFE_FOR_FUZZING if they are incorrectly included there for some reason. Belts and suspenders! :)

…g coverage from 65% to 70%. 545404e fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. (practicalswift) Pull request description: Add RPC interface fuzzing. This PR increases overall fuzzing line coverage from [~65%](https://marcofalke.github.io/btc_cov/fuzz.coverage/) to ~70% 🎉 To test this PR: ``` $ make distclean $ ./autogen.sh $ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined $ make -C src/ test/fuzz/fuzz $ FUZZ=rpc src/test/fuzz/fuzz ``` See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for more information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets). Happy fuzzing :) ACKs for top commit: MarcoFalke: Concept ACK 545404e Tree-SHA512: 35fc1b508af42bf480ee3762326b09ff2eecdb7960a1917ad16345fadd5c0c21d666dafe736176e5a848ff6492483c782e4ea914cd9000faf50190df051950fd

5252f86 fuzz: Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of #ifdef forests (practicalswift) 54549dd fuzz: RPC fuzzer post-merge follow-ups. Remove unused includes. Update list of fuzzed RPC commands. (practicalswift) Pull request description: Various RPC fuzzer follow-ups: * Remove unused includes. * Update list of fuzzed RPC commands. * Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of `#ifdef` forests. Context: bitcoin/bitcoin#21169 (review) ACKs for top commit: MarcoFalke: Concept ACK 5252f86 Tree-SHA512: 286d70798131706ffb157758e1c73f7f00ed96ce120c7d9dc849e672b283f1362df47b206cfec9da44d5debb5869225e721761dcd5c38a7d5d1019dc6c912ab2

5252f86 fuzz: Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of #ifdef forests (practicalswift) 54549dd fuzz: RPC fuzzer post-merge follow-ups. Remove unused includes. Update list of fuzzed RPC commands. (practicalswift) Pull request description: Various RPC fuzzer follow-ups: * Remove unused includes. * Update list of fuzzed RPC commands. * Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of `#ifdef` forests. Context: bitcoin#21169 (review) ACKs for top commit: MarcoFalke: Concept ACK 5252f86 Tree-SHA512: 286d70798131706ffb157758e1c73f7f00ed96ce120c7d9dc849e672b283f1362df47b206cfec9da44d5debb5869225e721761dcd5c38a7d5d1019dc6c912ab2

Adds fuzztests for RPC. FIXME there is a crash; I just disabled the test for now and we can revisit in a future PR.

fanquake added the Tests label Feb 13, 2021

practicalswift force-pushed the fuzzing-rpc branch from 7a864ca to e4e3b56 Compare February 13, 2021 08:48

jonatack reviewed Feb 13, 2021

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

practicalswift mentioned this pull request Apr 9, 2021

fuzz: Extend psbt fuzz target a bit #21606

Merged

maflcko reviewed Apr 9, 2021

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

practicalswift force-pushed the fuzzing-rpc branch from e4e3b56 to 555556e Compare April 27, 2021 21:05

maflcko reviewed Apr 28, 2021

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

maflcko reviewed Apr 28, 2021

View reviewed changes

src/test/fuzz/rpc.cpp Outdated Show resolved Hide resolved

fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% t…

545404e

…o 70%.

practicalswift force-pushed the fuzzing-rpc branch from 555556e to 545404e Compare April 28, 2021 06:38

maflcko merged commit e458631 into bitcoin:master Apr 28, 2021

maflcko reviewed Apr 28, 2021

View reviewed changes

practicalswift mentioned this pull request Apr 29, 2021

fuzz: Various RPC fuzzer follow-ups #21810

Merged

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge e458631 into merged_master (Bitcoin PR bitcoin/bitcoin#21169)

d51f425

Adds fuzztests for RPC. FIXME there is a crash; I just disabled the test for now and we can revisit in a future PR.

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

fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. #21169

fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. #21169

Uh oh!

Conversation

practicalswift commented Feb 13, 2021

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

adamjonas commented Apr 27, 2021

Uh oh!

practicalswift commented Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Apr 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maflcko commented Feb 22, 2021 •

edited

Loading

practicalswift commented Apr 27, 2021 •

edited

Loading

practicalswift Apr 29, 2021 •

edited

Loading