Skip to content

Commit 75e8bf5

Browse files
committed
net: dedup and RAII-fy the creation of a copy of CConnman::vNodes
The following pattern was duplicated in CConnman: ```cpp lock create a copy of vNodes, add a reference to each one unlock ... use the copy ... lock release each node from the copy unlock ``` Put that code in a RAII helper that reduces it to: ```cpp create snapshot "snap" ... use the copy ... // release happens when "snap" goes out of scope ```
1 parent b9cf505 commit 75e8bf5

File tree

2 files changed

+60
-49
lines changed

2 files changed

+60
-49
lines changed

src/net.cpp

Lines changed: 23 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1513,18 +1513,12 @@ void CConnman::SocketHandler()
15131513
}
15141514
}
15151515

1516+
const NodesSnapshot snap{*this, /*shuffle=*/false};
1517+
15161518
//
15171519
// Service each socket
15181520
//
1519-
std::vector<CNode*> vNodesCopy;
1520-
{
1521-
LOCK(cs_vNodes);
1522-
vNodesCopy = vNodes;
1523-
for (CNode* pnode : vNodesCopy)
1524-
pnode->AddRef();
1525-
}
1526-
for (CNode* pnode : vNodesCopy)
1527-
{
1521+
for (CNode* pnode : snap.Nodes()) {
15281522
if (interruptNet)
15291523
return;
15301524

@@ -1606,11 +1600,6 @@ void CConnman::SocketHandler()
16061600

16071601
if (InactivityCheck(*pnode)) pnode->fDisconnect = true;
16081602
}
1609-
{
1610-
LOCK(cs_vNodes);
1611-
for (CNode* pnode : vNodesCopy)
1612-
pnode->Release();
1613-
}
16141603
}
16151604

16161605
void CConnman::ThreadSocketHandler()
@@ -2224,49 +2213,34 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
22242213
void CConnman::ThreadMessageHandler()
22252214
{
22262215
SetSyscallSandboxPolicy(SyscallSandboxPolicy::MESSAGE_HANDLER);
2227-
FastRandomContext rng;
22282216
while (!flagInterruptMsgProc)
22292217
{
2230-
std::vector<CNode*> vNodesCopy;
2231-
{
2232-
LOCK(cs_vNodes);
2233-
vNodesCopy = vNodes;
2234-
for (CNode* pnode : vNodesCopy) {
2235-
pnode->AddRef();
2236-
}
2237-
}
2238-
22392218
bool fMoreWork = false;
22402219

2241-
// Randomize the order in which we process messages from/to our peers.
2242-
// This prevents attacks in which an attacker exploits having multiple
2243-
// consecutive connections in the vNodes list.
2244-
Shuffle(vNodesCopy.begin(), vNodesCopy.end(), rng);
2245-
2246-
for (CNode* pnode : vNodesCopy)
22472220
{
2248-
if (pnode->fDisconnect)
2249-
continue;
2221+
// Randomize the order in which we process messages from/to our peers.
2222+
// This prevents attacks in which an attacker exploits having multiple
2223+
// consecutive connections in the vNodes list.
2224+
const NodesSnapshot snap{*this, /*shuffle=*/true};
22502225

2251-
// Receive messages
2252-
bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
2253-
fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
2254-
if (flagInterruptMsgProc)
2255-
return;
2256-
// Send messages
2257-
{
2258-
LOCK(pnode->cs_sendProcessing);
2259-
m_msgproc->SendMessages(pnode);
2260-
}
2226+
for (CNode* pnode : snap.Nodes()) {
2227+
if (pnode->fDisconnect)
2228+
continue;
22612229

2262-
if (flagInterruptMsgProc)
2263-
return;
2264-
}
2230+
// Receive messages
2231+
bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc);
2232+
fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend);
2233+
if (flagInterruptMsgProc)
2234+
return;
2235+
// Send messages
2236+
{
2237+
LOCK(pnode->cs_sendProcessing);
2238+
m_msgproc->SendMessages(pnode);
2239+
}
22652240

2266-
{
2267-
LOCK(cs_vNodes);
2268-
for (CNode* pnode : vNodesCopy)
2269-
pnode->Release();
2241+
if (flagInterruptMsgProc)
2242+
return;
2243+
}
22702244
}
22712245

22722246
WAIT_LOCK(mutexMsgProc, lock);

src/net.h

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1177,6 +1177,43 @@ class CConnman
11771177
*/
11781178
std::vector<CService> m_onion_binds;
11791179

1180+
/**
1181+
* RAII helper to atomically create a copy of `vNodes` and add a reference
1182+
* to each of the nodes. The nodes are released when this object is destroyed.
1183+
*/
1184+
class NodesSnapshot
1185+
{
1186+
public:
1187+
explicit NodesSnapshot(const CConnman& connman, bool shuffle)
1188+
{
1189+
{
1190+
LOCK(connman.cs_vNodes);
1191+
m_nodes_copy = connman.vNodes;
1192+
for (auto& node : m_nodes_copy) {
1193+
node->AddRef();
1194+
}
1195+
}
1196+
if (shuffle) {
1197+
Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{});
1198+
}
1199+
}
1200+
1201+
~NodesSnapshot()
1202+
{
1203+
for (auto& node : m_nodes_copy) {
1204+
node->Release();
1205+
}
1206+
}
1207+
1208+
const std::vector<CNode*>& Nodes() const
1209+
{
1210+
return m_nodes_copy;
1211+
}
1212+
1213+
private:
1214+
std::vector<CNode*> m_nodes_copy;
1215+
};
1216+
11801217
friend struct CConnmanTest;
11811218
friend struct ConnmanTestMsg;
11821219
};

0 commit comments

Comments
 (0)