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 :)
42 | +struct RPCFuzzTestingSetup : public TestingSetup { 43 | + RPCFuzzTestingSetup(const std::string& chain_name, const std::vector<const char*>& extra_args) : TestingSetup{chain_name, extra_args} 44 | + { 45 | + } 46 | + 47 | + UniValue CallRPC(const std::string rpc_method, const std::vector<std::string>& arguments)
UniValue CallRPC(const std::string& rpc_method, const std::vector<std::string>& arguments)
First code review LGTM otherwise.
Addressed!
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](/bitcoin-bitcoin/2/) INITED cov: 110 ft: 111 corp: 1/1b exec/s: 0 rss: 191Mb
[#4](/bitcoin-bitcoin/4/) NEW cov: 110 ft: 118 corp: 2/3b lim: 4 exec/s: 0 rss: 191Mb L: 2/2 MS: 2 ChangeBit-InsertByte-
[#10](/bitcoin-bitcoin/10/) NEW cov: 110 ft: 125 corp: 3/6b lim: 4 exec/s: 0 rss: 191Mb L: 3/3 MS: 1 CopyPart-
[#13](/bitcoin-bitcoin/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](/bitcoin-bitcoin/217864/) REDUCE cov: 16595 ft: 45707 corp: 992/59Kb lim: 116 exec/s: 1998 rss: 616Mb L: 84/116 MS: 1 EraseBytes-
[#218080](/bitcoin-bitcoin/218080/) REDUCE cov: 16595 ft: 45709 corp: 993/59Kb lim: 116 exec/s: 2000 rss: 616Mb L: 115/116 MS: 1 InsertRepeatedBytes-
[#218133](/bitcoin-bitcoin/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](/bitcoin-bitcoin/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"-
313 | + return fuzzed_data_provider.ConsumeBool() ? ConsumeScalarRPCArgument(fuzzed_data_provider) : ConsumeArrayRPCArgument(fuzzed_data_provider); 314 | +} 315 | + 316 | +RPCFuzzTestingSetup* InitializeRPCFuzzTestingSetup() 317 | +{ 318 | + static std::unique_ptr<RPCFuzzTestingSetup> setup = std::make_unique<RPCFuzzTestingSetup>(CBaseChainParams::REGTEST, std::vector<const char*>{"-nodebuglogfile"});
Should use MakeNoLogFileContext?
Good point! Done!
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.
@MarcoFalke to clarify, are you a NACK on this?
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? :)
67 | +const std::vector<std::string> RPC_COMMANDS_NOT_SAFE_FOR_FUZZING{ 68 | + "addconnection", // avoid DNS lookups 69 | + "addnode", // avoid DNS lookups 70 | + "addpeeraddress", // avoid DNS lookups 71 | + "analyzepsbt", // avoid signed integer overflow in CFeeRate::GetFee(unsigned long) (https://github.com/bitcoin/bitcoin/issues/20607) 72 | + "decoderawtransaction", // avoid signed integer overflow in ValueFromAmount(long const&) (https://github.com/bitcoin/bitcoin/pull/20406)
fixed?
Fixed! Thanks!
348 | + if (!supported_rpc_command) { 349 | + std::cerr << "Error: Unknown RPC command \"" << rpc_command << "\" found in RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update " << __FILE__ << ".\n"; 350 | + std::terminate(); 351 | + } 352 | + } 353 | + const char* limit_to_rpc_command_env = getenv("LIMIT_TO_RPC_COMMAND");
const char* limit_to_rpc_command_env = std::getenv("LIMIT_TO_RPC_COMMAND");
Addressed. Thanks!
57 | + return tableRPC.listCommands(); 58 | + } 59 | +}; 60 | + 61 | +RPCFuzzTestingSetup* rpc_testing_setup = nullptr; 62 | +std::string limit_to_rpc_command;
std::string g_limit_to_rpc_command;
Addressed. Thanks!
78 | + "generatetoaddress", // avoid timeout 79 | + "gettxoutproof", // avoid slow execution 80 | +#ifdef ENABLE_WALLET 81 | + "importwallet", // avoid reading from disk 82 | +#endif 83 | + "invalidateblock", // avoid nullptr dereference in CBlockIndexWorkComparator::operator() (https://github.com/bitcoin/bitcoin/issues/20914)
fixed
Fixed! Thanks!
Concept ACK 545404e7e1c72985557ccffe865cea269143e5dd
If this is leading to too much (maintenance) hassle, it can be reverted.
72 | + "dumptxoutset", // avoid writing to disk 73 | +#ifdef ENABLE_WALLET 74 | + "dumpwallet", // avoid writing to disk 75 | +#endif 76 | + "echoipc", // avoid assertion failure (Assertion `"EnsureAnyNodeContext(request.context).init" && check' failed.) 77 | + "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)
0 | @@ -0,0 +1,378 @@ 1 | +// Copyright (c) 2021 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | + 5 | +#include <base58.h> 6 | +#include <chainparamsbase.h>
unused?
Thanks! Addressed in follow-up.
0 | @@ -0,0 +1,378 @@ 1 | +// Copyright (c) 2021 The Bitcoin Core developers 2 | +// Distributed under the MIT software license, see the accompanying 3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php. 4 | + 5 | +#include <base58.h> 6 | +#include <chainparamsbase.h> 7 | +#include <core_io.h> 8 | +#include <interfaces/chain.h>
unused?
Thanks! Addressed in follow-up.
78 | + "gettxoutproof", // avoid slow execution 79 | +#ifdef ENABLE_WALLET 80 | + "importwallet", // avoid reading from disk 81 | + "loadwallet", // avoid reading from disk 82 | +#endif 83 | + "mockscheduler", // avoid assertion failure (Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed.)
fixed?
Seems so! Thanks! Re-enabled in follow-up.
147 | + "logging", 148 | + "ping", 149 | + "preciousblock", 150 | + "pruneblockchain", 151 | + "reconsiderblock", 152 | + "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! :)