Skip to content

Commit 7f5ebef

Browse files
seditedsedited
committed
Merge bitcoin#34302: fuzz: Restore SendMessages coverage in process_message(s) fuzz targets
fabf8d1 fuzz: Restore SendMessages coverage in process_message(s) fuzz targets (MarcoFalke) fac7fed refactor: Use std::reference_wrapper<AddrMan> in Connman (MarcoFalke) Pull request description: *Found and reported by Crypt-iQ (thanks!)* Currently the process_message(s) fuzz targets do not have any meaningful `SendMessages` code coverage. This is not ideal. Fix the problem by adding back the coverage, and by hardening the code here, so that the problem hopefully does not happen again in the future. ### Historic context for this regression The regression was introduced in commit fa11eea, which built a new deterministic peerman object. However, the patch was incomplete, because it was missing one hunk to replace `g_setup->m_node.peerman->SendMessages(&p2p_node);` with `peerman->SendMessages(&p2p_node);`. This means the stale and empty peerman from the node context and not the freshly created and deterministic peerman was used. A simple fix would be to just submit the missing patch hunk. However, this still leaves the risk that the issue is re-introduced at any time in the future. So instead, I think the stale and empty peerman should be de-constructed, so that any call to it will lead to a hard sanitizer error and fuzz failure. Doing that also uncovered another issue: The connman was holding on to a reference to a stale and empty addrman. So fix all issues by: * Allowing the addrman reference in connman to be re-seatable * Clearing all stale objects, before creating new objects, and then using references to the new objects in all code ACKs for top commit: Crypt-iQ: crACK fabf8d1 frankomosh: ACK fabf8d1 marcofleon: code review ACK fabf8d1 sedited: ACK fabf8d1 Tree-SHA512: 2e478102b3e928dc7505f00c08d4b9e4f8368407b100bc88f3eb3b82aa6fea5a45bae736c211f5af1551ca0de1a5ffd4a5d196d9473d4c3b87cfed57c9a0b69d
2 parents a6e8cd3 + fabf8d1 commit 7f5ebef

File tree

5 files changed

+55
-43
lines changed

5 files changed

+55
-43
lines changed

‎src/net.cpp‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -506,7 +506,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect,
506506
if (!proxyConnectionFailed) {
507507
// If a connection to the node was attempted, and failure (if any) is not caused by a problem connecting to
508508
// the proxy, mark this as an attempt.
509-
addrman.Attempt(target_addr, fCountFailure);
509+
addrman.get().Attempt(target_addr, fCountFailure);
510510
}
511511
} else if (pszDest && GetNameProxy(proxy)) {
512512
std::string host;
@@ -2301,7 +2301,7 @@ void CConnman::ThreadDNSAddressSeed()
23012301
if (gArgs.GetBoolArg("-forcednsseed", DEFAULT_FORCEDNSSEED)) {
23022302
// When -forcednsseed is provided, query all.
23032303
seeds_right_now = seeds.size();
2304-
} else if (addrman.Size() == 0) {
2304+
} else if (addrman.get().Size() == 0) {
23052305
// If we have no known peers, query all.
23062306
// This will occur on the first run, or if peers.dat has been
23072307
// deleted.
@@ -2323,13 +2323,13 @@ void CConnman::ThreadDNSAddressSeed()
23232323
// DNS seeds, and if that fails too, also try the fixed seeds.
23242324
// (done in ThreadOpenConnections)
23252325
int found = 0;
2326-
const std::chrono::seconds seeds_wait_time = (addrman.Size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
2326+
const std::chrono::seconds seeds_wait_time = (addrman.get().Size() >= DNSSEEDS_DELAY_PEER_THRESHOLD ? DNSSEEDS_DELAY_MANY_PEERS : DNSSEEDS_DELAY_FEW_PEERS);
23272327

23282328
for (const std::string& seed : seeds) {
23292329
if (seeds_right_now == 0) {
23302330
seeds_right_now += DNSSEEDS_TO_QUERY_AT_ONCE;
23312331

2332-
if (addrman.Size() > 0) {
2332+
if (addrman.get().Size() > 0) {
23332333
LogInfo("Waiting %d seconds before querying DNS seeds.\n", seeds_wait_time.count());
23342334
std::chrono::seconds to_wait = seeds_wait_time;
23352335
while (to_wait.count() > 0) {
@@ -2389,7 +2389,7 @@ void CConnman::ThreadDNSAddressSeed()
23892389
vAdd.push_back(addr);
23902390
found++;
23912391
}
2392-
addrman.Add(vAdd, resolveSource);
2392+
addrman.get().Add(vAdd, resolveSource);
23932393
} else {
23942394
// If the seed does not support a subdomain with our desired service bits,
23952395
// we make an ADDR_FETCH connection to the DNS resolved peer address for the
@@ -2412,7 +2412,7 @@ void CConnman::DumpAddresses()
24122412
DumpPeerAddresses(::gArgs, addrman);
24132413

24142414
LogDebug(BCLog::NET, "Flushed %d addresses to peers.dat %dms\n",
2415-
addrman.Size(), Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
2415+
addrman.get().Size(), Ticks<std::chrono::milliseconds>(SteadyClock::now() - start));
24162416
}
24172417

24182418
void CConnman::ProcessAddrFetch()
@@ -2506,7 +2506,7 @@ std::unordered_set<Network> CConnman::GetReachableEmptyNetworks() const
25062506
for (int n = 0; n < NET_MAX; n++) {
25072507
enum Network net = (enum Network)n;
25082508
if (net == NET_UNROUTABLE || net == NET_INTERNAL) continue;
2509-
if (g_reachable_nets.Contains(net) && addrman.Size(net, std::nullopt) == 0) {
2509+
if (g_reachable_nets.Contains(net) && addrman.get().Size(net, std::nullopt) == 0) {
25102510
networks.insert(net);
25112511
}
25122512
}
@@ -2526,7 +2526,7 @@ bool CConnman::MaybePickPreferredNetwork(std::optional<Network>& network)
25262526

25272527
LOCK(m_nodes_mutex);
25282528
for (const auto net : nets) {
2529-
if (g_reachable_nets.Contains(net) && m_network_conn_counts[net] == 0 && addrman.Size(net) != 0) {
2529+
if (g_reachable_nets.Contains(net) && m_network_conn_counts[net] == 0 && addrman.get().Size(net) != 0) {
25302530
network = net;
25312531
return true;
25322532
}
@@ -2578,7 +2578,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
25782578
const bool use_seednodes{!gArgs.GetArgs("-seednode").empty()};
25792579

25802580
auto seed_node_timer = NodeClock::now();
2581-
bool add_addr_fetch{addrman.Size() == 0 && !seed_nodes.empty()};
2581+
bool add_addr_fetch{addrman.get().Size() == 0 && !seed_nodes.empty()};
25822582
constexpr std::chrono::seconds ADD_NEXT_SEEDNODE = 10s;
25832583

25842584
if (!add_fixed_seeds) {
@@ -2591,7 +2591,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
25912591
const auto& seed{SpanPopBack(seed_nodes)};
25922592
AddAddrFetch(seed);
25932593

2594-
if (addrman.Size() == 0) {
2594+
if (addrman.get().Size() == 0) {
25952595
LogInfo("Empty addrman, adding seednode (%s) to addrfetch\n", seed);
25962596
} else {
25972597
LogInfo("Couldn't connect to peers from addrman after %d seconds. Adding seednode (%s) to addrfetch\n", ADD_NEXT_SEEDNODE.count(), seed);
@@ -2646,7 +2646,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
26462646
seed_addrs.end());
26472647
CNetAddr local;
26482648
local.SetInternal("fixedseeds");
2649-
addrman.Add(seed_addrs, local);
2649+
addrman.get().Add(seed_addrs, local);
26502650
add_fixed_seeds = false;
26512651
LogInfo("Added %d fixed seeds from reachable networks.\n", seed_addrs.size());
26522652
}
@@ -2778,7 +2778,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
27782778
continue;
27792779
}
27802780

2781-
addrman.ResolveCollisions();
2781+
addrman.get().ResolveCollisions();
27822782

27832783
const auto current_time{NodeClock::now()};
27842784
int nTries = 0;
@@ -2809,30 +2809,30 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
28092809
if (fFeeler) {
28102810
// First, try to get a tried table collision address. This returns
28112811
// an empty (invalid) address if there are no collisions to try.
2812-
std::tie(addr, addr_last_try) = addrman.SelectTriedCollision();
2812+
std::tie(addr, addr_last_try) = addrman.get().SelectTriedCollision();
28132813

28142814
if (!addr.IsValid()) {
28152815
// No tried table collisions. Select a new table address
28162816
// for our feeler.
2817-
std::tie(addr, addr_last_try) = addrman.Select(true, reachable_nets);
2817+
std::tie(addr, addr_last_try) = addrman.get().Select(true, reachable_nets);
28182818
} else if (AlreadyConnectedToAddress(addr)) {
28192819
// If test-before-evict logic would have us connect to a
28202820
// peer that we're already connected to, just mark that
28212821
// address as Good(). We won't be able to initiate the
28222822
// connection anyway, so this avoids inadvertently evicting
28232823
// a currently-connected peer.
2824-
addrman.Good(addr);
2824+
addrman.get().Good(addr);
28252825
// Select a new table address for our feeler instead.
2826-
std::tie(addr, addr_last_try) = addrman.Select(true, reachable_nets);
2826+
std::tie(addr, addr_last_try) = addrman.get().Select(true, reachable_nets);
28272827
}
28282828
} else {
28292829
// Not a feeler
28302830
// If preferred_net has a value set, pick an extra outbound
28312831
// peer from that network. The eviction logic in net_processing
28322832
// ensures that a peer from another network will be evicted.
28332833
std::tie(addr, addr_last_try) = preferred_net.has_value()
2834-
? addrman.Select(false, {*preferred_net})
2835-
: addrman.Select(false, reachable_nets);
2834+
? addrman.get().Select(false, {*preferred_net})
2835+
: addrman.get().Select(false, reachable_nets);
28362836
}
28372837

28382838
// Require outbound IPv4/IPv6 connections, other than feelers, to be to distinct network groups
@@ -3244,7 +3244,7 @@ void CConnman::ThreadPrivateBroadcast()
32443244
continue;
32453245
}
32463246

3247-
const auto [addr, _] = addrman.Select(/*new_only=*/false, {net.value()});
3247+
const auto [addr, _] = addrman.get().Select(/*new_only=*/false, {net.value()});
32483248

32493249
if (!addr.IsValid() || IsLocal(addr)) {
32503250
++addrman_num_bad_addresses;
@@ -3695,7 +3695,7 @@ CConnman::~CConnman()
36953695

36963696
std::vector<CAddress> CConnman::GetAddressesUnsafe(size_t max_addresses, size_t max_pct, std::optional<Network> network, const bool filtered) const
36973697
{
3698-
std::vector<CAddress> addresses = addrman.GetAddr(max_addresses, max_pct, network, filtered);
3698+
std::vector<CAddress> addresses = addrman.get().GetAddr(max_addresses, max_pct, network, filtered);
36993699
if (m_banman) {
37003700
addresses.erase(std::remove_if(addresses.begin(), addresses.end(),
37013701
[this](const CAddress& addr){return m_banman->IsDiscouraged(addr) || m_banman->IsBanned(addr);}),

‎src/net.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1592,7 +1592,7 @@ class CConnman
15921592
std::vector<ListenSocket> vhListenSocket;
15931593
std::atomic<bool> fNetworkActive{true};
15941594
bool fAddressesInitialized{false};
1595-
AddrMan& addrman;
1595+
std::reference_wrapper<AddrMan> addrman;
15961596
const NetGroupManager& m_netgroupman;
15971597
std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
15981598
Mutex m_addr_fetches_mutex;

‎src/test/fuzz/process_message.cpp‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Distributed under the MIT software license, see the accompanying
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

5+
#include <banman.h>
56
#include <consensus/consensus.h>
67
#include <net.h>
78
#include <net_processing.h>
@@ -67,27 +68,31 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
6768
SeedRandomStateForTest(SeedRand::ZEROS);
6869
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
6970

70-
auto& connman = static_cast<ConnmanTestMsg&>(*g_setup->m_node.connman);
71+
auto& node{g_setup->m_node};
72+
auto& connman{static_cast<ConnmanTestMsg&>(*node.connman)};
7173
connman.ResetAddrCache();
7274
connman.ResetMaxOutboundCycle();
73-
auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
75+
auto& chainman{static_cast<TestChainstateManager&>(*node.chainman)};
7476
const auto block_index_size{WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())};
7577
SetMockTime(1610000000); // any time to successfully reset ibd
7678
chainman.ResetIbd();
7779
chainman.DisableNextWrite();
7880

79-
node::Warnings warnings{};
80-
NetGroupManager netgroupman{{}};
81-
AddrMan addrman{netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0};
82-
auto peerman = PeerManager::make(connman, addrman,
81+
// Reset, so that dangling pointers can be detected by sanitizers.
82+
node.banman.reset();
83+
node.addrman.reset();
84+
node.peerman.reset();
85+
node.addrman = std::make_unique<AddrMan>(*node.netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0);
86+
node.peerman = PeerManager::make(connman, *node.addrman,
8387
/*banman=*/nullptr, chainman,
84-
*g_setup->m_node.mempool, warnings,
88+
*node.mempool, *node.warnings,
8589
PeerManager::Options{
8690
.reconcile_txs = true,
8791
.deterministic_rng = true,
8892
});
8993

90-
connman.SetMsgProc(peerman.get());
94+
connman.SetMsgProc(node.peerman.get());
95+
connman.SetAddrman(*node.addrman);
9196
LOCK(NetEventsInterface::g_msgproc_mutex);
9297

9398
const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::MESSAGE_TYPE_SIZE).c_str()};
@@ -116,10 +121,10 @@ FUZZ_TARGET(process_message, .init = initialize_process_message)
116121
more_work = connman.ProcessMessagesOnce(p2p_node);
117122
} catch (const std::ios_base::failure&) {
118123
}
119-
g_setup->m_node.peerman->SendMessages(&p2p_node);
124+
node.peerman->SendMessages(&p2p_node);
120125
}
121-
g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
122-
g_setup->m_node.connman->StopNodes();
126+
node.validation_signals->SyncWithValidationInterfaceQueue();
127+
node.connman->StopNodes();
123128
if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())) {
124129
// Reuse the global chainman, but reset it when it is dirty
125130
ResetChainman(*g_setup);

‎src/test/fuzz/process_messages.cpp‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// Distributed under the MIT software license, see the accompanying
33
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
44

5+
#include <banman.h>
56
#include <consensus/consensus.h>
67
#include <net.h>
78
#include <net_processing.h>
@@ -57,26 +58,30 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
5758
SeedRandomStateForTest(SeedRand::ZEROS);
5859
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
5960

60-
auto& connman = static_cast<ConnmanTestMsg&>(*g_setup->m_node.connman);
61+
auto& node{g_setup->m_node};
62+
auto& connman{static_cast<ConnmanTestMsg&>(*node.connman)};
6163
connman.ResetAddrCache();
6264
connman.ResetMaxOutboundCycle();
63-
auto& chainman = static_cast<TestChainstateManager&>(*g_setup->m_node.chainman);
65+
auto& chainman{static_cast<TestChainstateManager&>(*node.chainman)};
6466
const auto block_index_size{WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())};
6567
SetMockTime(1610000000); // any time to successfully reset ibd
6668
chainman.ResetIbd();
6769
chainman.DisableNextWrite();
6870

69-
node::Warnings warnings{};
70-
NetGroupManager netgroupman{{}};
71-
AddrMan addrman{netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0};
72-
auto peerman = PeerManager::make(connman, addrman,
71+
// Reset, so that dangling pointers can be detected by sanitizers.
72+
node.banman.reset();
73+
node.addrman.reset();
74+
node.peerman.reset();
75+
node.addrman = std::make_unique<AddrMan>(*node.netgroupman, /*deterministic=*/true, /*consistency_check_ratio=*/0);
76+
node.peerman = PeerManager::make(connman, *node.addrman,
7377
/*banman=*/nullptr, chainman,
74-
*g_setup->m_node.mempool, warnings,
78+
*node.mempool, *node.warnings,
7579
PeerManager::Options{
7680
.reconcile_txs = true,
7781
.deterministic_rng = true,
7882
});
79-
connman.SetMsgProc(peerman.get());
83+
connman.SetMsgProc(node.peerman.get());
84+
connman.SetAddrman(*node.addrman);
8085

8186
LOCK(NetEventsInterface::g_msgproc_mutex);
8287

@@ -115,11 +120,11 @@ FUZZ_TARGET(process_messages, .init = initialize_process_messages)
115120
more_work = connman.ProcessMessagesOnce(random_node);
116121
} catch (const std::ios_base::failure&) {
117122
}
118-
g_setup->m_node.peerman->SendMessages(&random_node);
123+
node.peerman->SendMessages(&random_node);
119124
}
120125
}
121-
g_setup->m_node.validation_signals->SyncWithValidationInterfaceQueue();
122-
g_setup->m_node.connman->StopNodes();
126+
node.validation_signals->SyncWithValidationInterfaceQueue();
127+
node.connman->StopNodes();
123128
if (block_index_size != WITH_LOCK(chainman.GetMutex(), return chainman.BlockIndex().size())) {
124129
// Reuse the global chainman, but reset it when it is dirty
125130
ResetChainman(*g_setup);

‎src/test/util/net.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ struct ConnmanTestMsg : public CConnman {
4040
m_msgproc = msgproc;
4141
}
4242

43+
void SetAddrman(AddrMan& in) { addrman = in; }
44+
4345
void SetPeerConnectTimeout(std::chrono::seconds timeout)
4446
{
4547
m_peer_connect_timeout = timeout;

0 commit comments

Comments
 (0)