This adds a fuzzing harness dedicated to the version handshake. To avoid determinism issues, the harness creates necessary components each iteration (addrman, peerman, etc). A harness like this would have easily caught https://bitcoincore.org/en/2024/07/03/disclose-timestamp-overflow/.
As a performance optimization, this PR includes a change to PeerManager to lazily initialize various filters (to avoid large unnecessary memory allocations each iteration).
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | marcofleon, brunoerg, glozow, mzumsande |
| Concept ACK | maflcko, kevkevinpal, morehouse, edilmedeiros |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK, but would be good to explain how this is different from the existing approach, introduced in “fuzz: version handshake” #20370, which was a replacement for “fuzz: Version handshake” #20097 .
Now that the timedata bug is fixed, and the module and related globals removed completely, it is a bit hard(er) to re-introduce it for testing, I guess. IIRC I did check it at some point and found that the existing fuzz targets would act as a regression test.
would be good to explain how this is different from the existing approach, introduced in “fuzz: version handshake” #20370, which was a replacement for “fuzz: Version handshake” #20097 .
The main difference is that this harness focuses on the version handshake, where a modification to the existing process_messages harness would further widen the space of reachable code. It could make sense to have both but I would expect the focused harness to be better/faster at finding bugs.
IIRC I did check it at some point and found that the existing fuzz targets would act as a regression test.
Happy to take that out of the op, the primary goal is to have fuzzing coverage for the version handshake (non-existent currently), which would be able to find such bugs.
Happy to take that out of the op, the primary goal is to have fuzzing coverage for the version handshake (non-existent currently), which would be able to find such bugs.
Are you sure it is completely non-existent? I am sure I did fuzz the version handshake at some point in time and the current coverage report (https://maflcko.github.io/b-c-cov/fuzz.coverage/src/net_processing.cpp.gcov.html) seems to agree. Maybe I am missing something.
No objection in any case. I am just trying to understand better what exactly this improves. Maybe you can show a coverage report that shows higher coverage compared to before or how a faked introduced bug is found faster compared to before.
Concept ACK in any case.
Are you sure it is completely non-existent?
The coverage we do have comes from the process_messages harness, but the harness doesn’t fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
I’ll generate a coverage report to show the difference
The coverage we do have comes from the process_messages harness, but the harness doesn’t fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Concept ACK 75fc22f I think it makes sense to fuzz the handshake itself
Maybe I’m just not too familiar with fuzz testing but this seems very similar to process_messages.
Would it make sense to modify process_messages to also test the handshake instead of creating another fuzz target?
Are you sure it is completely non-existent?
The coverage we do have comes from the
process_messagesharness, but the harness doesn’t fuzz the handshake it only makes sure that the handshake completes, so that the other messages can be fuzzed.
Sorry for the wrong comments above. Not sure why I hallucinated that this was already fuzzed. I guess I remembered seeing the timedata warnings printed in the fuzz runner and somehow assumed this part of the code is already fully fuzzed.
However, I guess the timedata fuzz target explicitly avoided the abs64 UB call (https://github.com/bitcoin/bitcoin/blob/b6440f20f2cc4b30fc2e3432a57eaf9ba0f653cc/src/test/fuzz/timedata.cpp#L28). And the process_messages one as well: https://github.com/bitcoin/bitcoin/blob/b6440f20f2cc4b30fc2e3432a57eaf9ba0f653cc/src/test/util/net.cpp#L37
889 /** Block hash of chain tip the last time we reset m_recent_rejects and
890- * m_recent_rejects_reconsiderable. */
891+ * RecentRejectsReconsiderableFilter(). */
892 uint256 hashRecentRejectsChainTip GUARDED_BY(cs_main);
893
894+ CRollingBloomFilter& RecentRejectsFilter() EXCLUSIVE_LOCKS_REQUIRED(::cs_main)
Measuring exec/s without lazy init:
With:
83+ fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(
84+ -std::chrono::seconds{10min}.count(), // Allow mocktime to go backwards slightly
85+ std::chrono::seconds{24h}.count()));
86+
87+ CSerializedNetMsg net_msg;
88+ net_msg.m_type = PickValue(fuzzed_data_provider, ALL_NET_MESSAGE_TYPES);
VERSION and VERACK? Are other messages valid during the handshake?
There are more message types that are allowed after the version message and before the verack (some of them are also allowed post verack), essentially all of the ones processed in this block (wtxidrelay, sendaddrv2, sendtxrcncl, sendheaders, sendcmpct).
I used ALL_NET_MESSAGE_TYPES, so that we can avoid touching this test if we add more handshake related message types (since we’ll add all new messages to that container).
80+ }
81+
82+ SetMockTime(GetTime() +
83+ fuzzed_data_provider.ConsumeIntegralInRange<int64_t>(
84+ -std::chrono::seconds{10min}.count(), // Allow mocktime to go backwards slightly
85+ std::chrono::seconds{24h}.count()));
If time isn’t attacker controlled, how would it go backwards?
My thinking was that this could simulate NTP adjustments that go backwards.
And how realistic is the range [-10m, +24h]? Naively, 24h seems like a long time between messages in the handshake.
I’ve changed the upper bound to TIMEOUT_INTERVAL (20min) which is the default timeout value used for connections.
896@@ -897,7 +897,18 @@ class PeerManagerImpl final : public PeerManager
897 *
898 * Memory used: 1.3 MB
899 */
900- CRollingBloomFilter m_recent_rejects GUARDED_BY(m_tx_download_mutex){120'000, 0.000'001};
901+ std::unique_ptr<CRollingBloomFilter> m_recent_rejects GUARDED_BY(m_tx_download_mutex){nullptr};
ActiveTipChange (#30111) should be updated too (m_recent_rejects and m_recent_rejects_reconsiderable are directly dereferenced there).
Ha, good catch! I missed this because reset() is both a method on unique_ptr and CRollingBloomFilter.
This seems to be harmless and work out the way it should, i.e. unique_ptr::reset() will delete the current filter and on the next call to RecentRejectsFilter() it will be re-allocated (which afaict would have the same effect as resetting the filter with CRollingBloomFilter::reset()). But obviously, I should change it to RecentRejectsFilter().reset() to call reset() on the filter not the unique_ptr.
.reset() instead of ->reset(), the explanation makes sense! Funny (almost) bug!
m_maybe_recent_rejects or m_lazy_recent_rejects? (Same for the other fields)
39+
40+FUZZ_TARGET(p2p_handshake, .init = ::initialize)
41+{
42+ FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
43+
44+ ConnmanTestMsg& connman = *static_cast<ConnmanTestMsg*>(g_setup->m_node.connman.get());
style nit, if it compiles (feel free to ignore):
0 auto& connman = static_cast<ConnmanTestMsg&>(*g_setup->m_node.connman.get());
48+
49+ node::Warnings warnings{};
50+ NetGroupManager netgroupman{{}};
51+ AddrMan addrman{netgroupman, true, 0};
52+ auto peerman = PeerManager::make(connman, addrman,
53+ nullptr, chainman,
46+ SetMockTime(1610000000); // any time to successfully reset ibd
47+ chainman.ResetIbd();
48+
49+ node::Warnings warnings{};
50+ NetGroupManager netgroupman{{}};
51+ AddrMan addrman{netgroupman, true, 0};
4636@@ -4604,15 +4637,15 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4637 std::sort(unique_parents.begin(), unique_parents.end());
4638 unique_parents.erase(std::unique(unique_parents.begin(), unique_parents.end()), unique_parents.end());
4639
4640- // Distinguish between parents in m_recent_rejects and m_recent_rejects_reconsiderable.
4641+ // Distinguish between parents in m_recent_rejects and RecentRejectsReconsiderableFilter().
Shouldn’t it be RecentRejectsFilter() or am I missing anything?
0 // Distinguish between parents in RecentRejectsFilter() and RecentRejectsReconsiderableFilter().
Sorry for not reviewing this earlier.
I think the title seems to imply that this is a test-only change, but you are also lazy-initializing some fields in the peer manager. What about changing the title to p2p: Lazy init some bloom filters; fuzz version handshake.
Left some nits. Feel free to ignore.
-BEGIN VERIFY SCRIPT-
sed -i 's/m_recent_confirmed_transactions/m_lazy_recent_confirmed_transactions/g' $(git grep -l 'm_recent_confirmed_transactions')
sed -i 's/m_recent_rejects_reconsiderable/m_lazy_recent_rejects_reconsiderable/g' $(git grep -l 'm_recent_rejects_reconsiderable')
sed -i 's/m_recent_rejects/m_lazy_recent_rejects/g' $(git grep -l 'm_recent_rejects')
-END VERIFY SCRIPT-
Concept ACK
I’m curious why the filters should be made lazy for this to work.
I’m curious why the filters should be made lazy for this to work.
It’s not required but it makes the harness more performant, i.e. we can run more testcases each second (see #30413 (review)).
net_processing from this harness to the process_message and process_messages harnesses to see the differences. This target hits more specific parts of the version handshake. The stability looks good as well, at about 94%.
I’m new to this kind of test, so just to be sure this is not a specific quirk of my setup (Apple M3 Pro Sonoma 14.5 compiled with clang-18 installed through macports).
I’m getting some runtime errors at the beginning of the test. After that it seems to run smoothly.
Compiled with ./configure --enable-fuzz --with-sanitizers=fuzzer,address,undefined CC=clang-mp-18 CXX=clang++-mp-18 --with-boost=/opt/local/libexec/boost/1.78.
Running with FUZZ=p2p_handshake src/test/fuzz/fuzz
0❯ FUZZ=p2p_handshake src/test/fuzz/fuzz
1fuzz(21234,0x201680c00) malloc: nano zone abandoned due to inability to reserve vm space.
2/opt/local/libexec/llvm-18/bin/../include/c++/v1/variant:495:12: runtime error: call to function decltype(auto) std::__1::__variant_detail::__visitation::__base::__dispatcher<0ul, 0ul>::__dispatch[abi:ne180100]<void std::__1::__variant_detail::__ctor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>>::__generic_construct[abi:ne180100]<std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>>(std::__1::__variant_detail::__ctor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>>&, std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>&&)::'lambda'(std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>&, auto&&)&&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&&>(std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&&) through pointer to incorrect function type 'void (*)((lambda at /opt/local/libexec/llvm-18/bin/../include/c++/v1/variant:814:11) &&, std::__variant_detail::__base<std::__variant_detail::_Trait::_Available, RPCArg::Optional, std::string, UniValue> &, std::__variant_detail::__base<std::__variant_detail::_Trait::_Available, RPCArg::Optional, std::string, UniValue> &&)'
3variant:532: note: decltype(auto) std::__1::__variant_detail::__visitation::__base::__dispatcher<0ul, 0ul>::__dispatch[abi:ne180100]<void std::__1::__variant_detail::__ctor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>>::__generic_construct[abi:ne180100]<std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>>(std::__1::__variant_detail::__ctor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>>&, std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>&&)::'lambda'(std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>&, auto&&)&&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&&>(std::__1::__variant_detail::__move_constructor<std::__1::__variant_detail::__traits<RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>, (std::__1::__variant_detail::_Trait)1>, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&, std::__1::__variant_detail::__base<(std::__1::__variant_detail::_Trait)1, RPCArg::Optional, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>>, UniValue>&&) defined here
4SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /opt/local/libexec/llvm-18/bin/../include/c++/v1/variant:495:12
5rpc/server.h:106:15: runtime error: call to function getblockchaininfo() through pointer to incorrect function type 'RPCHelpMan (*)()'
6blockchain.cpp:1269: note: getblockchaininfo() defined here
7SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior rpc/server.h:106:15
8rpc/server.h:108:15: runtime error: call to function getblockchaininfo() through pointer to incorrect function type 'RPCHelpMan (*)()'
9blockchain.cpp:1269: note: getblockchaininfo() defined here
10SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior rpc/server.h:108:15
11INFO: Running with entropic power schedule (0xFF, 100).
12INFO: Seed: 277630637
13INFO: Loaded 1 modules (1220045 inline 8-bit counters): 1220045 [0x103dc9e10, 0x103ef3bdd),
14INFO: Loaded 1 PC tables (1220045 PCs): 1220045 [0x103ef3be0,0x1051918b0),
15INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
16INFO: A corpus is not provided, starting from an empty corpus
17[#2](/bitcoin-bitcoin/2/) INITED cov: 1784 ft: 1784 corp: 1/1b exec/s: 0 rss: 131Mb
18[#4](/bitcoin-bitcoin/4/) NEW cov: 1787 ft: 1787 corp: 2/3b lim: 4 exec/s: 0 rss: 132Mb L: 2/2 MS: 2 CopyPart-CrossOver-
19[#5](/bitcoin-bitcoin/5/) NEW cov: 1787 ft: 1790 corp: 3/7b lim: 4 exec/s: 0 rss: 133Mb L: 4/4 MS: 1 CopyPart-
20[#8](/bitcoin-bitcoin/8/) NEW cov: 1787 ft: 1793 corp: 4/10b lim: 4 exec/s: 0 rss: 134Mb L: 3/4 MS: 3 CopyPart-ChangeByte-CrossOver-
21[#9](/bitcoin-bitcoin/9/) NEW cov: 1798 ft: 2467 corp: 5/13b lim: 4 exec/s: 0 rss: 134Mb L: 3/4 MS: 1 ChangeByte-
22[#19](/bitcoin-bitcoin/19/) NEW cov: 1798 ft: 3487 corp: 6/16b lim: 4 exec/s: 0 rss: 139Mb L: 3/4 MS: 5 EraseBytes-ChangeByte-ShuffleBytes-EraseBytes-InsertByte-
23[#117](/bitcoin-bitcoin/117/) REDUCE cov: 1798 ft: 3487 corp: 6/14b lim: 4 exec/s: 0 rss: 179Mb L: 1/4 MS: 3 CrossOver-CrossOver-EraseBytes-
24[#127](/bitcoin-bitcoin/127/) REDUCE cov: 1798 ft: 3487 corp: 6/13b lim: 4 exec/s: 0 rss: 184Mb L: 2/4 MS: 5 ChangeBit-CrossOver-EraseBytes-ShuffleBytes-ChangeBit-
25[#339](/bitcoin-bitcoin/339/) NEW cov: 1798 ft: 3490 corp: 7/18b lim: 6 exec/s: 0 rss: 272Mb L: 5/5 MS: 2 CopyPart-InsertByte-
26[#420](/bitcoin-bitcoin/420/) REDUCE cov: 1798 ft: 3490 corp: 7/17b lim: 6 exec/s: 0 rss: 305Mb L: 1/5 MS: 1 EraseBytes-
27[#961](/bitcoin-bitcoin/961/) NEW cov: 1801 ft: 3495 corp: 8/27b lim: 11 exec/s: 0 rss: 453Mb L: 10/10 MS: 1 InsertRepeatedBytes-
28...
ACK afd237bb5d85923273a69f7b45dc6aae6aa1680e
Left it running overnight (slight changes since it), coverage and performance looks great.
0@@ -0,0 +1,107 @@
1+// Copyright (c) 2020-present The Bitcoin Core developers
0// Copyright (c) 2024-present The Bitcoin Core developers
71+ }
72+
73+ LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 100)
74+ {
75+ CNode& connection = *PickValue(fuzzed_data_provider, peers);
76+ if (connection.fDisconnect || connection.fSuccessfullyConnected) {
connection.fSuccessfullyConnected was removed it would be a general p2p message harness similar to process_messages.
process_messages here as well: #30413 (comment).
Code Review ACK afd237bb5d85923273a69f7b45dc6aae6aa1680e
The lazy initialization seem like a good thing in itself because it might also save a little bit of memory during IBD when we don’t need the filters yet - m_lazy_recent_rejects and m_lazy_recent_rejects_reconsiderable should only be initialized after getting out of IBD.
Maybe, as a follow-up, it could make sense to skip early when in IBD in https://github.com/bitcoin/bitcoin/blob/66e82dc90c598c9c42ff980693ef5367a845e1d0/src/net_processing.cpp#L4227-L4233 so that a stray tx-inv received during IBD doesn’t result in the filter being initialized in AlreadyHaveTx (when we wouldn’t call AddTxAnnouncement() for it anyway)
@edilmedeiros I’ve seen people have similar issues on a Mac, I don’t think these are real issues but rather false positives.
You can configure without the address and/or undefined sanitizer and that should get rid of the errors, but this obviously is not a great solution if you want to actually use those sanitizers. A second option could be to try to use our ubsan suppressions (https://github.com/bitcoin/bitcoin/blob/master/test/sanitizer_suppressions/ubsan) and add the errors you are seeing to it. See the fuzz test runner for how to tell ubsan to use the suppressions file: https://github.com/bitcoin/bitcoin/blob/b8755164cf9ce7159837d11d19065a5fd6d7d26d/test/fuzz/test_runner.py#L24
