tests: Add fuzzing harness for CAddrMan

practicalswift commented at 6:42 pm on May 24, 2020: contributor

Add fuzzing harness for CAddrMan.

~~Fill some fuzzing coverage gaps for functions in addrdb.h, merkleblock.h and outputtype.h.~~

See doc/fuzzing.md for 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 :)

DrahtBot added the label Build system on May 24, 2020

DrahtBot added the label Tests on May 24, 2020

fanquake removed the label Build system on May 25, 2020

practicalswift force-pushed on May 25, 2020

in src/test/fuzz/kitchen_sink.cpp:23 in 403c5ad9ad outdated

18 #include <vector>
19 
20+void initialize()
21+{
22+    SelectParams(CBaseChainParams::REGTEST);
23+    StartMutedLogging();

MarcoFalke commented at 12:02 pm on May 26, 2020:

Why can’t this be a testing setup with logging disabled, like in the other tests?

practicalswift commented at 12:44 pm on May 26, 2020:

Unfortunately TestingSetup introduces non-determinism which makes coverage vary between runs also when using the same set of inputs, see #19067 (review) :)

MarcoFalke commented at 12:50 pm on May 26, 2020:

What about BasicTestgingSetup then?

practicalswift commented at 1:05 pm on May 26, 2020:

I think it is the same problem there: I get more setup than I need and a subset of that setup work introduces coverage non-determinism :)

I prefer opt-ing in to exactly the setup I need: it avoids the non-determinism problem and I also think that makes it easier to reason about the dependencies of each fuzzing harness.

MarcoFalke commented at 1:17 pm on May 26, 2020:

If non-determinism is an issue for fuzz tests, it is also an issue for unit tests etc. Instead of solving it fresh in every single test case, I’d rather fix it once and for all.

Using hacks that don’t clean up after themselves, depend on global state and leave global state dirty is asking for mishaps. Currently the fuzz datadir is set to the default location of the datadir. I don’t think I need to repeat why this is bad.

If the basic testing setup is still too bloated, then please add one that is stripped down, so that unit and bench tests can use it as well.

practicalswift commented at 5:32 pm on May 26, 2020:

I was wrong about BasicTestingSetup: it seems like only TestingSetup introduces coverage non-determinism.

This shows the issue with TestingSetup:

 0# With TestingSetup:
 1$ src/test/fuzz/kitchen_sink corpus/
 2…
 3[#20425](/bitcoin-bitcoin/20425/)  NEW    cov: 3867 ft: 12141 corp: 248/49Kb lim: 607 exec/s: 464 rss: 357Mb L: 605/607 MS: 5 InsertRepeatedBytes-CopyPart-InsertByte-ChangeBinInt-CopyPart-
 4        NEW_FUNC[1/68]: 0x56533ed75c60 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::c_str() const /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/basic_string.h:2291
 5        NEW_FUNC[2/68]: 0x56533f03dfc0 in std::function<void ()>::function(std::function<void ()> const&) /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/std_function.h:655
 6[#20441](/bitcoin-bitcoin/20441/)  NEW    cov: 4384 ft: 12690 corp: 249/49Kb lim: 607 exec/s: 454 rss: 358Mb L: 326/607 MS: 1 CopyPart-
 7…
 8[#28628](/bitcoin-bitcoin/28628/)        NEW    cov: 4384 ft: 12731 corp: 258/54Kb lim: 652 exec/s: 454 rss: 452Mb L: 652/652 MS: 1 CrossOver-
 9==6668== libFuzzer: run interrupted; exiting
10
11$ src/test/fuzz/kitchen_sink corpus/
12[#423](/bitcoin-bitcoin/423/)    INITED cov: 3867 ft: 12182 corp: 245/48Kb exec/s: 423 rss: 148Mb

Notice the non-deterministic coverage increase from 3867 to 4384 which drops back to 3867 when the fuzzer is restarted.

I’ll go with BasicTestingSetup :)

practicalswift commented at 5:54 pm on May 26, 2020:

Coverage stability with BasicTestingSetup vs TestingSetup:

Fuzzer with an empty test_one_input function, and an initialize function containing static const BasicTestingSetup basic_testing_setup:

0INFO: A corpus is not provided, starting from an empty corpus
1[#2](/bitcoin-bitcoin/2/)      INITED cov: 58 ft: 59 corp: 1/1b exec/s: 0 rss: 125Mb
2[#65536](/bitcoin-bitcoin/65536/)  pulse  cov: 58 ft: 59 corp: 1/1b lim: 652 exec/s: 21845 rss: 145Mb
3[#131072](/bitcoin-bitcoin/131072/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 1300 exec/s: 21845 rss: 188Mb
4[#262144](/bitcoin-bitcoin/262144/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 2611 exec/s: 21845 rss: 339Mb
5[#524288](/bitcoin-bitcoin/524288/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 4096 exec/s: 20164 rss: 715Mb
6[#1048576](/bitcoin-bitcoin/1048576/)        pulse  cov: 58 ft: 59 corp: 1/1b lim: 4096 exec/s: 20560 rss: 716Mb
7[#2097152](/bitcoin-bitcoin/2097152/)        pulse  cov: 58 ft: 59 corp: 1/1b lim: 4096 exec/s: 20560 rss: 716Mb

Fuzzer with an empty test_one_input function, and an initialize function containing static const TestingSetup testing_setup:

 0INFO: A corpus is not provided, starting from an empty corpus
 1[#2](/bitcoin-bitcoin/2/)      INITED cov: 58 ft: 59 corp: 1/1b exec/s: 0 rss: 134Mb
 2[#65536](/bitcoin-bitcoin/65536/)  pulse  cov: 58 ft: 59 corp: 1/1b lim: 652 exec/s: 21845 rss: 155Mb
 3[#131072](/bitcoin-bitcoin/131072/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 1300 exec/s: 21845 rss: 198Mb
 4[#262144](/bitcoin-bitcoin/262144/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 2611 exec/s: 21845 rss: 348Mb
 5[#524288](/bitcoin-bitcoin/524288/) pulse  cov: 58 ft: 59 corp: 1/1b lim: 4096 exec/s: 20164 rss: 715Mb
 6        NEW_FUNC[1/85]: 0x55e526ad96c0 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::basic_string<std::allocator<char> >(char const*, std::allocator<char> const&) /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/basic_string.h:516
 7        NEW_FUNC[2/85]: 0x55e526adc5a0 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_local_data() /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/basic_string.h:180
 8[#910854](/bitcoin-bitcoin/910854/) NEW    cov: 663 ft: 143 corp: 2/3b lim: 4096 exec/s: 20241 rss: 717Mb L: 2/2 MS: 2 InsertByte-CopyPart-
 9[#1048576](/bitcoin-bitcoin/1048576/)        pulse  cov: 663 ft: 143 corp: 2/3b lim: 4096 exec/s: 20560 rss: 717Mb
10        NEW_FUNC[1/10]: 0x55e526ad95c0 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::c_str() const /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/basic_string.h:2291
11        NEW_FUNC[2/10]: 0x55e526adc140 in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_dispose() /usr/lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/bits/basic_string.h:219
12[#1830962](/bitcoin-bitcoin/1830962/)        NEW    cov: 711 ft: 335 corp: 3/4099b lim: 4096 exec/s: 20572 rss: 717Mb L: 4096/4096 MS: 3 ChangeBinInt-CrossOver-ChangeASCIIInt-
13[#2097152](/bitcoin-bitcoin/2097152/)        pulse  cov: 711 ft: 335 corp: 3/4099b lim: 4096 exec/s: 20360 rss: 717Mb

DrahtBot added the label Needs rebase on May 26, 2020

practicalswift force-pushed on May 26, 2020

DrahtBot removed the label Needs rebase on May 26, 2020

practicalswift force-pushed on May 26, 2020

practicalswift commented at 7:20 pm on June 28, 2020: contributor

Anything left to do here? :) The changes are limited to src/test/fuzz/ and should hopefully be trivial to review.

Crypt-iQ commented at 5:47 pm on July 13, 2020: contributor

I cannot build this branch with either clang/clang++ or afl-gcc/afl-g++:

clang version: 6.0.0-1ubuntu2 (tags/RELEASE_600/final) afl-gcc/afl-g++ built from afl master

Make output:

 0Making all in src
 1  2 make[1]: Entering directory '/root/bitcoin/src'
 2  3 make[2]: Entering directory '/root/bitcoin/src'
 3  4   CXX      test/fuzz/test_fuzz_addition_overflow-addition_overflow.o
 4  5 In file included from test/fuzz/addition_overflow.cpp:7:
 5  6 ./test/fuzz/util.h:62:18: error: no template named 'optional' in namespace 'std'; did you mean 'Optional'?
 6  7 NODISCARD inline std::optional<T> ConsumeDeserializable(FuzzedDataProvider& fuzzed_data_provider, const si    ze_t max_length = 4096) noexcept
 7  8                  ^~~~~~~~~~~~~
 8  9                  Optional
 9 10 ./optional.h:14:1: note: 'Optional' declared here
10 11 using Optional = boost::optional<T>;
11 12 ^
12 13 In file included from test/fuzz/addition_overflow.cpp:7:
13 14 ./test/fuzz/util.h:70:16: error: no member named 'nullopt' in namespace 'std'; did you mean simply 'nullop    t'?
14 15         return std::nullopt;
15 16                ^~~~~~~~~~~~
16 17                nullopt
17 18 ./optional.h:24:14: note: 'nullopt' declared here
18 19 static auto& nullopt = boost::none;
19 20              ^
20 21 2 errors generated.
21 22 Makefile:14081: recipe for target 'test/fuzz/test_fuzz_addition_overflow-addition_overflow.o' failed
22 23 make[2]: *** [test/fuzz/test_fuzz_addition_overflow-addition_overflow.o] Error 1
23 24 make[2]: Leaving directory '/root/bitcoin/src'
24 25 Makefile:17268: recipe for target 'all-recursive' failed
25 26 make[1]: *** [all-recursive] Error 1
26 27 make[1]: Leaving directory '/root/bitcoin/src'
27 28 Makefile:785: recipe for target 'all-recursive' failed
28 29 make: *** [all-recursive] Error 1

Am I doing something wrong? I have C++17 features and can build on master.

MarcoFalke commented at 5:54 pm on July 13, 2020: member

@Crypt-iQ So to clarify, you can build current master, but not this pull request merged into current master?

Crypt-iQ commented at 5:57 pm on July 13, 2020: contributor

@MarcoFalke I can build the current master branch, but not this branch as it is. I checkout this branch via:

0git fetch origin pull/19065/head:addrmanfuzz

practicalswift commented at 10:37 pm on July 13, 2020: contributor

@Crypt-iQ Oh, that’s weird! Thanks for reporting. I’ve now rebased this PR on current master. Would you mind testing again? :)

Crypt-iQ commented at 1:20 am on July 14, 2020: contributor

@practicalswift I think you may have forgotten to push it up :) And will test once it’s up

practicalswift force-pushed on Jul 14, 2020

practicalswift commented at 7:44 am on July 14, 2020: contributor

@Crypt-iQ Oh, thanks for letting me know! Now pushed for real :)

practicalswift force-pushed on Jul 14, 2020

DrahtBot commented at 0:08 am on July 15, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20138 (net: Assume that SetCommonVersion is called at most once per peer by MarcoFalke)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Crypt-iQ commented at 2:58 pm on July 15, 2020: contributor

@practicalswift Able to build this branch with afl and with libfuzzer (clang-10).

Coverage from 24 hours of AFL fuzzing here: https://crypt-iq.github.io/addrman_fuzz_outs/src/index.html With more time it should hit the collision code but it was very slow ~4-5 execs/s.

in src/test/fuzz/addrman.cpp:110 in 80ee923d73 outdated

105+
106+        if (fuzzed_data_provider.ConsumeBool()) {
107+            (void)addr_man.size();
108+
109+            CDataStream data_stream(SER_NETWORK, PROTOCOL_VERSION);
110+            data_stream << addr_man;

Crypt-iQ commented at 1:25 am on July 19, 2020:

Is the other way >> skipped because it’s covered in deserialize.cpp? Also since m_asmap isn’t specified there is a single line that the fuzzer misses:

https://github.com/bitcoin/bitcoin/blob/090d87716074434bdc6c7656ec44d049197a793a/src/addrman.h#L370-L376__

practicalswift commented at 8:33 pm on August 18, 2020:

Is the other way >> skipped because it’s covered in deserialize.cpp?

Yes :)

Also since m_asmap isn’t specified there is a single line that the fuzzer misses:

Thanks! Now fixed.

in src/test/fuzz/merkleblock.cpp:35 in c8055503f0 outdated

38+        const std::optional<CBlock> opt_block = ConsumeDeserializable<CBlock>(fuzzed_data_provider);
39+        CBloomFilter bloom_filter;
40+        std::set<uint256> txids;
41+        if (opt_block && !opt_block->vtx.empty()) {
42+            if (fuzzed_data_provider.ConsumeBool()) {
43+                merkle_block = CMerkleBlock{*opt_block, bloom_filter};

Crypt-iQ commented at 2:24 am on July 19, 2020:

Could a case be made to not only have the default, match-all filter?

practicalswift commented at 8:36 pm on August 18, 2020:

Fixed! I’m now covering all three:

CMerkleBlock{*opt_block, bloom_filter}
CMerkleBlock{*opt_block, txids}
CMerkleBlock{} (added)

Crypt-iQ commented at 2:37 am on July 19, 2020: contributor

Still fuzzing addrman to hit the collision code, will report back. Fuzzed merkleblock & kitchensink for ~32 hours each with libfuzzer and --with-sanitizers=address,fuzzer,undefined: https://crypt-iq.github.io/merkleblock_and_kitchensink_cov/src/index.html

Only two minor comments.

practicalswift force-pushed on Aug 18, 2020

practicalswift commented at 8:37 pm on August 18, 2020: contributor

@Crypt-iQ Thanks a lot for reviewing! Feedback addressed. Would you mind re-reviewing? :)

practicalswift force-pushed on Aug 19, 2020

practicalswift commented at 5:17 am on August 28, 2020: contributor

@MarcoFalke Would you mind reviewing? FWIW this PR touches only src/test/fuzz/ :)

practicalswift force-pushed on Sep 1, 2020

fanquake added this to the "Blockers" column in a project

Crypt-iQ commented at 5:14 am on September 4, 2020: contributor

The CAddrMan harness looks good to me. Ubuntu 18:

./configure --enable-fuzz --with-sanitizers=address,undefined,integer,fuzzer reports implicit-signed-integer-truncation
valgrind reports no errors

macOS v10.15.4:

./configure --enable-fuzz --with-sanitizers=address,fuzzer --disable-asm reports no errors
./configure --enable-fuzz --with-sanitizers=undefined,integer,fuzzer reports implicit-signed-integer-truncation

 0addrman.cpp:524:22: runtime error: implicit conversion from type 'int64_t' (aka 'long long') of value 60031562745 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 4196987897 (32-bit, unsigned)
 1    [#0](/bitcoin-bitcoin/0/) 0x103319c69 in CAddrMan::Connected_(CService const&, long long) addrman.cpp:524
 2    [#1](/bitcoin-bitcoin/1/) 0x1032a0a1b in CAddrMan::Connected(CService const&, long long) addrman.h:652
 3    [#2](/bitcoin-bitcoin/2/) 0x10329c621 in test_one_input(std::__1::vector<unsigned char, std::__1::allocator<unsigned char> > const&) addrman.cpp:89
 4    [#3](/bitcoin-bitcoin/3/) 0x1042c8ff8 in LLVMFuzzerTestOneInput fuzz.cpp:45
 5    [#4](/bitcoin-bitcoin/4/) 0x1044cb970 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) FuzzerLoop.cpp:556
 6    [#5](/bitcoin-bitcoin/5/) 0x1044cb0b5 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool*) FuzzerLoop.cpp:470
 7    [#6](/bitcoin-bitcoin/6/) 0x1044cd757 in fuzzer::Fuzzer::ReadAndExecuteSeedCorpora(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:765
 8    [#7](/bitcoin-bitcoin/7/) 0x1044cdab9 in fuzzer::Fuzzer::Loop(std::__1::vector<fuzzer::SizedFile, fuzzer::fuzzer_allocator<fuzzer::SizedFile> >&) FuzzerLoop.cpp:792
 9    [#8](/bitcoin-bitcoin/8/) 0x1044baf7d in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) FuzzerDriver.cpp:829
10    [#9](/bitcoin-bitcoin/9/) 0x1044e6c52 in main FuzzerMain.cpp:19
11    [#10](/bitcoin-bitcoin/10/) 0x7fff73778cc8 in start+0x0 (libdyld.dylib:x86_64+0x1acc8)
12
13SUMMARY: UndefinedBehaviorSanitizer: implicit-signed-integer-truncation addrman.cpp:524:22 in

Happens here because info.nTime is uint32_t: https://github.com/bitcoin/bitcoin/blob/a0a422c34cfd6514d0cc445bd784d3ee1a2d1749/src/addrman.cpp#L524

Still couldn’t hit the collision code, so may need to run for longer. Will review and run the changes to the other harnesses.

in src/test/fuzz/addrman.cpp:63 in 688660d3c4 outdated

48+        }
49+        case 5: {
50+            const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider);
51+            const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider);
52+            if (opt_address && opt_net_addr) {
53+                addr_man.Add(*opt_address, *opt_net_addr, fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(0, 100000000));

Crypt-iQ commented at 5:29 am on September 4, 2020:

How was 100000000 chosen?

practicalswift commented at 8:57 pm on October 5, 2020:

100 000 000 was chosen to avoid this signed integer overflow:

0addrman.cpp:306:67: runtime error: signed integer overflow: 100000000 - -9223372036854775808 cannot be represented in type 'long'
1    [#0](/bitcoin-bitcoin/0/) 0x5594ad96b160 in CAddrMan::Add_(CAddress const&, CNetAddr const&, long) src/addrman.cpp:306:67
2    [#1](/bitcoin-bitcoin/1/) 0x5594ad90d6ce in CAddrMan::Add(CAddress const&, CNetAddr const&, long) src/./addrman.h:556:17
3    [#2](/bitcoin-bitcoin/2/) 0x5594ad909041 in test_one_input(std::vector<unsigned char, std::allocator<unsigned char> > const&) src/test/fuzz/addrman.cpp:53:26

Crypt-iQ commented at 2:39 am on September 11, 2020: contributor

The merkleblock and kitchensink harnesses look good to me. Fuzzers didn’t report any errors with sanitizers nor with valgrind.

So the reason the addrman fuzzer wasn’t able to hit the collision code consistently is because of insecure_rand which changes across runs. Another source of test randomness is the GetAdjustedTime function.

Can also see it when running make cov_fuzz twice as the cov and ft numbers differ with libfuzzer output. First run:

 0Run addrman with args ['/Users/nsa/bitcoin_cov/src/test/fuzz/addrman', '-runs=1', 'qa-assets/fuzz_seed_corpus/addrman']INFO: Seed: 1286454130
 1INFO: Loaded 1 modules   (264114 inline 8-bit counters): 264114 [0x1114a3ee8, 0x1114e469a), 
 2INFO: Loaded 1 PC tables (264114 PCs): 264114 [0x1114e46a0,0x1118ec1c0), 
 3INFO:     5557 files found in qa-assets/fuzz_seed_corpus/addrman
 4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4151 bytes
 5INFO: seed corpus: files: 5557 min: 1b max: 4151b total: 3492077b rss: 44Mb
 6[#1024](/bitcoin-bitcoin/1024/)	pulse  cov: 3008 ft: 8297 corp: 326/14217b exec/s: 341 rss: 45Mb
 7[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 3127 ft: 10801 corp: 530/40Kb exec/s: 89 rss: 45Mb
 8[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 3160 ft: 13578 corp: 830/143Kb exec/s: 42 rss: 45Mb
 9[#5558](/bitcoin-bitcoin/5558/)	INITED cov: 3172 ft: 15459 corp: 1160/728Kb exec/s: 29 rss: 46Mb
10[#5558](/bitcoin-bitcoin/5558/)	DONE   cov: 3172 ft: 15459 corp: 1160/728Kb lim: 4147 exec/s: 29 rss: 46Mb
11Done 5558 runs in 191 second(s)

Second run:

 0Run addrman with args ['/Users/nsa/bitcoin_cov/src/test/fuzz/addrman', '-runs=1', 'qa-assets/fuzz_seed_corpus/addrman']INFO: Seed: 1987669807
 1INFO: Loaded 1 modules   (264114 inline 8-bit counters): 264114 [0x104dd7ee8, 0x104e1869a), 
 2INFO: Loaded 1 PC tables (264114 PCs): 264114 [0x104e186a0,0x1052201c0), 
 3INFO:     5557 files found in qa-assets/fuzz_seed_corpus/addrman
 4INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4151 bytes
 5INFO: seed corpus: files: 5557 min: 1b max: 4151b total: 3492077b rss: 44Mb
 6[#1024](/bitcoin-bitcoin/1024/)	pulse  cov: 3008 ft: 8316 corp: 330/14641b exec/s: 204 rss: 45Mb
 7[#2048](/bitcoin-bitcoin/2048/)	pulse  cov: 3117 ft: 10818 corp: 522/39Kb exec/s: 49 rss: 45Mb
 8[#4096](/bitcoin-bitcoin/4096/)	pulse  cov: 3159 ft: 13573 corp: 838/148Kb exec/s: 27 rss: 45Mb
 9Slowest unit: 12 s:
10artifact_prefix='./'; Test unit written to ./slow-unit-8450b39fa9bc5a9408007860514c53b0b6bf2723
11[#5558](/bitcoin-bitcoin/5558/)	INITED cov: 3181 ft: 15428 corp: 1159/714Kb exec/s: 20 rss: 46Mb
12[#5558](/bitcoin-bitcoin/5558/)	DONE   cov: 3181 ft: 15428 corp: 1159/714Kb lim: 4147 exec/s: 20 rss: 46Mb
13Done 5558 runs in 267 second(s)

practicalswift force-pushed on Sep 17, 2020

practicalswift commented at 9:03 pm on October 5, 2020: contributor

@Crypt-iQ I’m afraid we’ll have to live with some degree of non-determinism. With that said I’m open to all suggestions on how to reduce the non-determinism in testing, but I guess that is out of scope for this PR :)

in src/test/fuzz/addrman.cpp:109 in 4bd95f42e2 outdated

104+        }
105+    }
106+    (void)addr_man.size();
107+    if (fuzzed_data_provider.ConsumeBool()) {
108+        while (fuzzed_data_provider.ConsumeBool()) {
109+            addr_man.m_asmap.push_back(fuzzed_data_provider.ConsumeBool());

Crypt-iQ commented at 10:23 am on October 8, 2020:

Should this be at the beginning so that addrman GetGroup calls use a populated asmap?

practicalswift commented at 6:21 pm on November 11, 2020:

Thanks! Fixed :)

MarcoFalke commented at 11:39 am on October 8, 2020: member

So the reason the addrman fuzzer wasn’t able to hit the collision code consistently is because of insecure_rand which changes across runs. Another source of test randomness is the GetAdjustedTime function.

I presume GetAdjustedTime is non-deterministic because it asks the system for the time. With SetMockTime you can make it return the time you (or the fuzz engine) set.

insecure_rand could be made deterministic by accepting a bool parameter to the addrman constructor and passing it on to insecure_rand.

Emzy commented at 3:48 pm on October 8, 2020: contributor

tACK Tested and worked (Debian GNU/Linux 10 (buster)): src/test/fuzz/addrman src/test/fuzz/kitchen_sink src/test/fuzz/merkleblock

fjahr commented at 8:33 pm on October 18, 2020: member

tested ACK 4bd95f42e2d18bdf86c05f4b9b5e31fcf0e3ba1d

Reviewed code and ran the affected fuzz tests for a while.

practicalswift commented at 0:00 am on November 5, 2020: contributor

Ready for merge? :)

PR touches code only in src/test/fuzz/ and has two Tested ACK:s.

Edit: Noticed that I had unaddressed feedback from @Crypt-iQ in #19065 (review). Will address. Sorry.

Crypt-iQ commented at 9:44 am on November 8, 2020: contributor

@practicalswift Just had that one comment remaining about GetGroup. I tried with @MarcoFalke suggested patch of insecure_rand being deterministic and after about a week the fuzzer still did not hit the collision code according to lcov. Granted, I am not throwing very many cores at it (2), and some input could probably be engineered to trigger it - just haven’t had the time. As a newcomer to the codebase, I don’t know what is considered out of scope PR-wise, but test determinism is always nice-to-have.

MarcoFalke added the label Waiting for author on Nov 8, 2020

practicalswift force-pushed on Nov 8, 2020

practicalswift commented at 10:09 pm on November 8, 2020: contributor

@Crypt-iQ Thanks for clarifying! I’ve now moved the AS-map code to allow for use via GetGroup. I’m now also mocking the time, but I skipped the suggested insecure_rand change since I don’t want to change any code outside of src/test/fuzz/ in this PR. That can be taken in a follow-up if deemed necessary.

I think this PR should be ready for final review now. Many thanks for your patience: may I ask you to review for the n:th time? :)

MarcoFalke removed the label Waiting for author on Nov 9, 2020

MarcoFalke commented at 10:21 am on November 10, 2020: member

Concept ACK. Will review

in src/test/fuzz/addrman.cpp:34 in c44009bb9f outdated

28+    SetMockTime(ConsumeTime(fuzzed_data_provider));
29+    CAddrMan addr_man;
30+    if (fuzzed_data_provider.ConsumeBool()) {
31+        while (fuzzed_data_provider.ConsumeBool()) {
32+            addr_man.m_asmap.push_back(fuzzed_data_provider.ConsumeBool());
33+        }

MarcoFalke commented at 5:20 pm on November 11, 2020:

Encoding one bit as two bytes in the seed sounds really inefficient

practicalswift commented at 6:10 pm on November 11, 2020:

Now optimized by using BytesToBits.

in src/test/fuzz/merkleblock.cpp:30 in c44009bb9f outdated

33+        }
34+    }
35+
36+    {
37+        CMerkleBlock merkle_block;
38+        const std::optional<CBlock> opt_block = ConsumeDeserializable<CBlock>(fuzzed_data_provider);

MarcoFalke commented at 5:29 pm on November 11, 2020:

It doesn’t look like there is any state shared between the two fuzz targets in this file. Often the fuzz engine can only generate random noise and I write initial seeds myself. With a format that has several unrelated seeds concatenated together, this becomes harder.

I guess the options are:

Split into two fuzz scripts (requires one more binary)
Use a switch-case to select a fuzz target within the script (requires one more byte at the end of each fuzz input)

practicalswift commented at 6:12 pm on November 11, 2020:

Thanks for reviewing. Now skipping all changes that are unrelated to CAddrMan to keep this PR focused.

in src/test/fuzz/kitchen_sink.cpp:49 in c44009bb9f outdated

44+    assert(parsed);
45+    assert(output_type == output_type_parsed);
46+    (void)ParseOutputType(fuzzed_data_provider.ConsumeRandomLengthString(64), output_type_parsed);
47+
48+    CAddrMan addr_man;
49+    CDataStream data_stream_peers = ConsumeDataStream(fuzzed_data_provider);

MarcoFalke commented at 5:31 pm on November 11, 2020:

Same here. Creating an addrman stream seed by hand requires a random length string first.

practicalswift commented at 6:12 pm on November 11, 2020:

Thanks for reviewing. Now skipping all changes that are unrelated to CAddrMan to keep this PR focused.

practicalswift force-pushed on Nov 11, 2020

practicalswift renamed this:
~~tests: Add fuzzing harness for CAddrMan. Fill some fuzzing coverage gaps.~~
tests: Add fuzzing harness for CAddrMan
on Nov 11, 2020

practicalswift commented at 6:12 pm on November 11, 2020: contributor

@MarcoFalke Thanks for reviewing. Addressed your optimization request and dropped all changes that are unrelated to CAddrMan. Should hopefully be ready for merge now :)

practicalswift force-pushed on Nov 11, 2020

in src/test/fuzz/addrman.cpp:34 in 090dd83f7b outdated

29+    SetMockTime(ConsumeTime(fuzzed_data_provider));
30+    CAddrMan addr_man;
31+    if (fuzzed_data_provider.ConsumeBool()) {
32+        while (fuzzed_data_provider.ConsumeBool()) {
33+            addr_man.m_asmap = BytesToBits(ConsumeRandomLengthByteVector(fuzzed_data_provider));
34+        }

MarcoFalke commented at 7:14 am on November 12, 2020:

any reason to re-assign this in a loop?

MarcoFalke commented at 9:09 am on November 12, 2020:

Also to make seeds re-usable it should probably use the same encoding like the other places. git grep 'asmap = ConsumeRan'

practicalswift commented at 2:26 pm on November 12, 2020:

Oh, the loop was a bug. Sorry about that! Thanks for catching.

Now using the newly introduced ConsumeRandomLengthBitVector(...) also in src/test/fuzz/net.

practicalswift force-pushed on Nov 12, 2020

tests: Add fuzzing harness for CAddrMan e6bb9fde85

practicalswift force-pushed on Nov 12, 2020

practicalswift commented at 2:26 pm on November 12, 2020: contributor

@MarcoFalke Thanks for re-reviewing! Addressed all feedback. Should hopefully be ready for final review now :)

fuzz: Use ConsumeRandomLengthBitVector(...) in src/test/fuzz/connman and src/test/fuzz/net d04a17a790

in src/test/fuzz/net.cpp:66 in e535f63cbb outdated

62@@ -63,7 +63,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
63             break;
64         }
65         case 3: {
66-            const std::vector<bool> asmap = ConsumeRandomLengthIntegralVector<bool>(fuzzed_data_provider, 128);
67+            const std::vector<bool> asmap = ConsumeRandomLengthBitVector(fuzzed_data_provider);

MarcoFalke commented at 2:53 pm on November 12, 2020:

please also adjust connman with this patch

practicalswift commented at 3:35 pm on November 12, 2020:

Good point. Now using ConsumeRandomLengthBitVector also in src/test/fuzz/connman.

practicalswift force-pushed on Nov 12, 2020

practicalswift commented at 3:35 pm on November 12, 2020: contributor

@MarcoFalke Thanks for re-re-reviewing! Addressed all feedback. Should hopefully be ready for final final review now :)

MarcoFalke commented at 6:24 am on November 13, 2020: member

review ACK d04a17a7907c57f7b570e1b9743fd63489bdad68

MarcoFalke merged this on Nov 13, 2020

MarcoFalke closed this on Nov 13, 2020

fanquake removed this from the "Blockers" column in a project

sidhujag referenced this in commit 3709b77a61 on Nov 13, 2020

practicalswift deleted the branch on Apr 10, 2021

kittywhiskers referenced this in commit 1f71a34c61 on Aug 11, 2022

DrahtBot locked this on Aug 18, 2022

tests: Add fuzzing harness for CAddrMan #19065

Conflicts