Skip to content

Commit 5b6f0f3

Browse files
fanquakefanquake
committed
Merge bitcoin/bitcoin#26036: net: add NetEventsInterface::g_msgproc_mutex
d575a67 net_processing: add thread safety annotation for m_highest_fast_announce (Anthony Towns) 0ae7987 net_processing: add thread safety annotations for PeerManagerImpl members accessed only via the msgproc thread (Anthony Towns) a66a7cc net_processing: add thread safety annotations for Peer members accessed only via the msgproc thread (Anthony Towns) bf12abe net: drop cs_sendProcessing (Anthony Towns) 1e78f56 net: add NetEventsInterface::g_msgproc_mutex (Anthony Towns) Pull request description: There are many cases where we assume message processing is single-threaded in order for how we access node-related memory to be safe. Add an explicit mutex that we can use to document this, which allows the compiler to catch any cases where we try to access that memory from other threads and break that assumption. ACKs for top commit: MarcoFalke: review ACK d575a67 📽 dergoegge: Code review ACK d575a67 w0xlt: ACK bitcoin/bitcoin@d575a67 vasild: ACK d575a67 modulo the missing runtime checks Tree-SHA512: b886d1aa4adf318ae64e32ccaf3d508dbb79d6eed3f1fa9d8b2ed96f3c72a3d38cd0f12e05826c9832a2a1302988adfd2b43ea9691aa844f37d8f5c37ff20e05
2 parents 71ac70d + d575a67 commit 5b6f0f3

File tree

11 files changed

+82
-95
lines changed

11 files changed

+82
-95
lines changed

‎src/net.cpp

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1991,8 +1991,12 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
19911991
}
19921992
}
19931993

1994+
Mutex NetEventsInterface::g_msgproc_mutex;
1995+
19941996
void CConnman::ThreadMessageHandler()
19951997
{
1998+
LOCK(NetEventsInterface::g_msgproc_mutex);
1999+
19962000
SetSyscallSandboxPolicy(SyscallSandboxPolicy::MESSAGE_HANDLER);
19972001
while (!flagInterruptMsgProc)
19982002
{
@@ -2014,10 +2018,7 @@ void CConnman::ThreadMessageHandler()
20142018
if (flagInterruptMsgProc)
20152019
return;
20162020
// Send messages
2017-
{
2018-
LOCK(pnode->cs_sendProcessing);
2019-
m_msgproc->SendMessages(pnode);
2020-
}
2021+
m_msgproc->SendMessages(pnode);
20212022

20222023
if (flagInterruptMsgProc)
20232024
return;

‎src/net.h

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,8 +377,6 @@ class CNode
377377
std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
378378
size_t nProcessQueueSize GUARDED_BY(cs_vProcessMsg){0};
379379

380-
RecursiveMutex cs_sendProcessing;
381-
382380
uint64_t nRecvBytes GUARDED_BY(cs_vRecv){0};
383381

384382
std::atomic<std::chrono::seconds> m_last_send{0s};
@@ -629,6 +627,9 @@ class CNode
629627
class NetEventsInterface
630628
{
631629
public:
630+
/** Mutex for anything that is only accessed via the msg processing thread */
631+
static Mutex g_msgproc_mutex;
632+
632633
/** Initialize a peer (setup state, queue any initial messages) */
633634
virtual void InitializeNode(CNode& node, ServiceFlags our_services) = 0;
634635

@@ -642,15 +643,15 @@ class NetEventsInterface
642643
* @param[in] interrupt Interrupt condition for processing threads
643644
* @return True if there is more work to be done
644645
*/
645-
virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) = 0;
646+
virtual bool ProcessMessages(CNode* pnode, std::atomic<bool>& interrupt) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
646647

647648
/**
648649
* Send queued protocol messages to a given node.
649650
*
650651
* @param[in] pnode The node which we are sending messages to.
651652
* @return True if there is more work to be done
652653
*/
653-
virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_sendProcessing) = 0;
654+
virtual bool SendMessages(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
654655

655656

656657
protected:

‎src/net_processing.cpp

Lines changed: 40 additions & 31 deletions
Large diffs are not rendered by default.

‎src/net_processing.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
8484

8585
/** Process a single message from a peer. Public for fuzz testing */
8686
virtual void ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv,
87-
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) = 0;
87+
const std::chrono::microseconds time_received, const std::atomic<bool>& interruptMsgProc) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex) = 0;
8888

8989
/** This function is used for testing the stale tip eviction logic, see denialofservice_tests.cpp */
9090
virtual void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) = 0;

‎src/test/denialofservice_tests.cpp

Lines changed: 16 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,8 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
4545
// work.
4646
BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
4747
{
48+
LOCK(NetEventsInterface::g_msgproc_mutex);
49+
4850
ConnmanTestMsg& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
4951
// Disable inactivity checks for this test to avoid interference
5052
connman.SetPeerConnectTimeout(99999s);
@@ -80,10 +82,8 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
8082
}
8183

8284
// Test starts here
83-
{
84-
LOCK(dummyNode1.cs_sendProcessing);
85-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
86-
}
85+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
86+
8787
{
8888
LOCK(dummyNode1.cs_vSend);
8989
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
@@ -93,20 +93,14 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
9393
int64_t nStartTime = GetTime();
9494
// Wait 21 minutes
9595
SetMockTime(nStartTime+21*60);
96-
{
97-
LOCK(dummyNode1.cs_sendProcessing);
98-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
99-
}
96+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in getheaders
10097
{
10198
LOCK(dummyNode1.cs_vSend);
10299
BOOST_CHECK(dummyNode1.vSendMsg.size() > 0);
103100
}
104101
// Wait 3 more minutes
105102
SetMockTime(nStartTime+24*60);
106-
{
107-
LOCK(dummyNode1.cs_sendProcessing);
108-
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
109-
}
103+
BOOST_CHECK(peerman.SendMessages(&dummyNode1)); // should result in disconnect
110104
BOOST_CHECK(dummyNode1.fDisconnect == true);
111105

112106
peerman.FinalizeNode(dummyNode1);
@@ -274,6 +268,8 @@ BOOST_AUTO_TEST_CASE(block_relay_only_eviction)
274268

275269
BOOST_AUTO_TEST_CASE(peer_discouragement)
276270
{
271+
LOCK(NetEventsInterface::g_msgproc_mutex);
272+
277273
auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
278274
auto connman = std::make_unique<ConnmanTestMsg>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
279275
auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(),
@@ -308,10 +304,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
308304
nodes[0]->fSuccessfullyConnected = true;
309305
connman->AddTestNode(*nodes[0]);
310306
peerLogic->UnitTestMisbehaving(nodes[0]->GetId(), DISCOURAGEMENT_THRESHOLD); // Should be discouraged
311-
{
312-
LOCK(nodes[0]->cs_sendProcessing);
313-
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
314-
}
307+
BOOST_CHECK(peerLogic->SendMessages(nodes[0]));
308+
315309
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
316310
BOOST_CHECK(nodes[0]->fDisconnect);
317311
BOOST_CHECK(!banman->IsDiscouraged(other_addr)); // Different address, not discouraged
@@ -330,21 +324,15 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
330324
nodes[1]->fSuccessfullyConnected = true;
331325
connman->AddTestNode(*nodes[1]);
332326
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), DISCOURAGEMENT_THRESHOLD - 1);
333-
{
334-
LOCK(nodes[1]->cs_sendProcessing);
335-
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
336-
}
327+
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
337328
// [0] is still discouraged/disconnected.
338329
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
339330
BOOST_CHECK(nodes[0]->fDisconnect);
340331
// [1] is not discouraged/disconnected yet.
341332
BOOST_CHECK(!banman->IsDiscouraged(addr[1]));
342333
BOOST_CHECK(!nodes[1]->fDisconnect);
343334
peerLogic->UnitTestMisbehaving(nodes[1]->GetId(), 1); // [1] reaches discouragement threshold
344-
{
345-
LOCK(nodes[1]->cs_sendProcessing);
346-
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
347-
}
335+
BOOST_CHECK(peerLogic->SendMessages(nodes[1]));
348336
// Expect both [0] and [1] to be discouraged/disconnected now.
349337
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
350338
BOOST_CHECK(nodes[0]->fDisconnect);
@@ -367,10 +355,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
367355
nodes[2]->fSuccessfullyConnected = true;
368356
connman->AddTestNode(*nodes[2]);
369357
peerLogic->UnitTestMisbehaving(nodes[2]->GetId(), DISCOURAGEMENT_THRESHOLD);
370-
{
371-
LOCK(nodes[2]->cs_sendProcessing);
372-
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
373-
}
358+
BOOST_CHECK(peerLogic->SendMessages(nodes[2]));
374359
BOOST_CHECK(banman->IsDiscouraged(addr[0]));
375360
BOOST_CHECK(banman->IsDiscouraged(addr[1]));
376361
BOOST_CHECK(banman->IsDiscouraged(addr[2]));
@@ -386,6 +371,8 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
386371

387372
BOOST_AUTO_TEST_CASE(DoS_bantime)
388373
{
374+
LOCK(NetEventsInterface::g_msgproc_mutex);
375+
389376
auto banman = std::make_unique<BanMan>(m_args.GetDataDirBase() / "banlist", nullptr, DEFAULT_MISBEHAVING_BANTIME);
390377
auto connman = std::make_unique<CConnman>(0x1337, 0x1337, *m_node.addrman, *m_node.netgroupman);
391378
auto peerLogic = PeerManager::make(*connman, *m_node.addrman, banman.get(),
@@ -411,10 +398,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
411398
dummyNode.fSuccessfullyConnected = true;
412399

413400
peerLogic->UnitTestMisbehaving(dummyNode.GetId(), DISCOURAGEMENT_THRESHOLD);
414-
{
415-
LOCK(dummyNode.cs_sendProcessing);
416-
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
417-
}
401+
BOOST_CHECK(peerLogic->SendMessages(&dummyNode));
418402
BOOST_CHECK(banman->IsDiscouraged(addr));
419403

420404
peerLogic->FinalizeNode(dummyNode);

‎src/test/fuzz/process_message.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,8 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
7373
SetMockTime(1610000000); // any time to successfully reset ibd
7474
chainstate.ResetIbd();
7575

76+
LOCK(NetEventsInterface::g_msgproc_mutex);
77+
7678
const std::string random_message_type{fuzzed_data_provider.ConsumeBytesAsString(CMessageHeader::COMMAND_SIZE).c_str()};
7779
if (!LIMIT_TO_MESSAGE_TYPE.empty() && random_message_type != LIMIT_TO_MESSAGE_TYPE) {
7880
return;
@@ -92,10 +94,7 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE
9294
GetTime<std::chrono::microseconds>(), std::atomic<bool>{false});
9395
} catch (const std::ios_base::failure&) {
9496
}
95-
{
96-
LOCK(p2p_node.cs_sendProcessing);
97-
g_setup->m_node.peerman->SendMessages(&p2p_node);
98-
}
97+
g_setup->m_node.peerman->SendMessages(&p2p_node);
9998
SyncWithValidationInterfaceQueue();
10099
g_setup->m_node.connman->StopNodes();
101100
}

‎src/test/fuzz/process_messages.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
4040
SetMockTime(1610000000); // any time to successfully reset ibd
4141
chainstate.ResetIbd();
4242

43+
LOCK(NetEventsInterface::g_msgproc_mutex);
44+
4345
std::vector<CNode*> peers;
4446
const auto num_peers_to_add = fuzzed_data_provider.ConsumeIntegralInRange(1, 3);
4547
for (int i = 0; i < num_peers_to_add; ++i) {
@@ -70,10 +72,7 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
7072
connman.ProcessMessagesOnce(random_node);
7173
} catch (const std::ios_base::failure&) {
7274
}
73-
{
74-
LOCK(random_node.cs_sendProcessing);
75-
g_setup->m_node.peerman->SendMessages(&random_node);
76-
}
75+
g_setup->m_node.peerman->SendMessages(&random_node);
7776
}
7877
SyncWithValidationInterfaceQueue();
7978
g_setup->m_node.connman->StopNodes();

‎src/test/fuzz/util.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -328,7 +328,7 @@ auto ConsumeNode(FuzzedDataProvider& fuzzed_data_provider, const std::optional<N
328328
}
329329
inline std::unique_ptr<CNode> ConsumeNodeAsUniquePtr(FuzzedDataProvider& fdp, const std::optional<NodeId>& node_id_in = std::nullopt) { return ConsumeNode<true>(fdp, node_id_in); }
330330

331-
void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept;
331+
void FillNode(FuzzedDataProvider& fuzzed_data_provider, ConnmanTestMsg& connman, CNode& node) noexcept EXCLUSIVE_LOCKS_REQUIRED(NetEventsInterface::g_msgproc_mutex);
332332

333333
class FuzzedFileProvider
334334
{

‎src/test/net_tests.cpp

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -805,6 +805,8 @@ BOOST_AUTO_TEST_CASE(LocalAddress_BasicLifecycle)
805805

806806
BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
807807
{
808+
LOCK(NetEventsInterface::g_msgproc_mutex);
809+
808810
// Tests the following scenario:
809811
// * -bind=3.4.5.6:20001 is specified
810812
// * we make an outbound connection to a peer
@@ -889,10 +891,7 @@ BOOST_AUTO_TEST_CASE(initial_advertise_from_version_message)
889891
}
890892
};
891893

892-
{
893-
LOCK(peer.cs_sendProcessing);
894-
m_node.peerman->SendMessages(&peer);
895-
}
894+
m_node.peerman->SendMessages(&peer);
896895

897896
BOOST_CHECK(sent);
898897

‎src/test/util/net.cpp

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
4444
(void)connman.ReceiveMsgFrom(node, msg_version);
4545
node.fPauseSend = false;
4646
connman.ProcessMessagesOnce(node);
47-
{
48-
LOCK(node.cs_sendProcessing);
49-
peerman.SendMessages(&node);
50-
}
47+
peerman.SendMessages(&node);
5148
if (node.fDisconnect) return;
5249
assert(node.nVersion == version);
5350
assert(node.GetCommonVersion() == std::min(version, PROTOCOL_VERSION));
@@ -60,10 +57,7 @@ void ConnmanTestMsg::Handshake(CNode& node,
6057
(void)connman.ReceiveMsgFrom(node, msg_verack);
6158
node.fPauseSend = false;
6259
connman.ProcessMessagesOnce(node);
63-
{
64-
LOCK(node.cs_sendProcessing);
65-
peerman.SendMessages(&node);
66-
}
60+
peerman.SendMessages(&node);
6761
assert(node.fSuccessfullyConnected == true);
6862
}
6963
}

0 commit comments

Comments
 (0)