diff options
author | Vasil Dimov <vd@FreeBSD.org> | 2021-04-26 16:22:07 +0200 |
---|---|---|
committer | Vasil Dimov <vd@FreeBSD.org> | 2021-11-18 13:29:23 +0100 |
commit | 75e8bf55f5a014faada7712a9640dc35e8c86f15 (patch) | |
tree | e008c898e12afe42bf673a15a48de49028c0803d | |
parent | b9cf505bdfade074f3d2b0473d56d2cbc2711cb3 (diff) | |
download | bitcoin-75e8bf55f5a014faada7712a9640dc35e8c86f15.tar.xz |
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
```
-rw-r--r-- | src/net.cpp | 72 | ||||
-rw-r--r-- | src/net.h | 37 |
2 files changed, 60 insertions, 49 deletions
diff --git a/src/net.cpp b/src/net.cpp index ad558dd598..165a28c04e 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1513,18 +1513,12 @@ void CConnman::SocketHandler() } } + const NodesSnapshot snap{*this, /*shuffle=*/false}; + // // Service each socket // - std::vector<CNode*> vNodesCopy; - { - LOCK(cs_vNodes); - vNodesCopy = vNodes; - for (CNode* pnode : vNodesCopy) - pnode->AddRef(); - } - for (CNode* pnode : vNodesCopy) - { + for (CNode* pnode : snap.Nodes()) { if (interruptNet) return; @@ -1606,11 +1600,6 @@ void CConnman::SocketHandler() if (InactivityCheck(*pnode)) pnode->fDisconnect = true; } - { - LOCK(cs_vNodes); - for (CNode* pnode : vNodesCopy) - pnode->Release(); - } } void CConnman::ThreadSocketHandler() @@ -2224,49 +2213,34 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai void CConnman::ThreadMessageHandler() { SetSyscallSandboxPolicy(SyscallSandboxPolicy::MESSAGE_HANDLER); - FastRandomContext rng; while (!flagInterruptMsgProc) { - std::vector<CNode*> vNodesCopy; - { - LOCK(cs_vNodes); - vNodesCopy = vNodes; - for (CNode* pnode : vNodesCopy) { - pnode->AddRef(); - } - } - bool fMoreWork = false; - // Randomize the order in which we process messages from/to our peers. - // This prevents attacks in which an attacker exploits having multiple - // consecutive connections in the vNodes list. - Shuffle(vNodesCopy.begin(), vNodesCopy.end(), rng); - - for (CNode* pnode : vNodesCopy) { - if (pnode->fDisconnect) - continue; + // Randomize the order in which we process messages from/to our peers. + // This prevents attacks in which an attacker exploits having multiple + // consecutive connections in the vNodes list. + const NodesSnapshot snap{*this, /*shuffle=*/true}; - // Receive messages - bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc); - fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend); - if (flagInterruptMsgProc) - return; - // Send messages - { - LOCK(pnode->cs_sendProcessing); - m_msgproc->SendMessages(pnode); - } + for (CNode* pnode : snap.Nodes()) { + if (pnode->fDisconnect) + continue; - if (flagInterruptMsgProc) - return; - } + // Receive messages + bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc); + fMoreWork |= (fMoreNodeWork && !pnode->fPauseSend); + if (flagInterruptMsgProc) + return; + // Send messages + { + LOCK(pnode->cs_sendProcessing); + m_msgproc->SendMessages(pnode); + } - { - LOCK(cs_vNodes); - for (CNode* pnode : vNodesCopy) - pnode->Release(); + if (flagInterruptMsgProc) + return; + } } WAIT_LOCK(mutexMsgProc, lock); @@ -1177,6 +1177,43 @@ private: */ std::vector<CService> m_onion_binds; + /** + * RAII helper to atomically create a copy of `vNodes` and add a reference + * to each of the nodes. The nodes are released when this object is destroyed. + */ + class NodesSnapshot + { + public: + explicit NodesSnapshot(const CConnman& connman, bool shuffle) + { + { + LOCK(connman.cs_vNodes); + m_nodes_copy = connman.vNodes; + for (auto& node : m_nodes_copy) { + node->AddRef(); + } + } + if (shuffle) { + Shuffle(m_nodes_copy.begin(), m_nodes_copy.end(), FastRandomContext{}); + } + } + + ~NodesSnapshot() + { + for (auto& node : m_nodes_copy) { + node->Release(); + } + } + + const std::vector<CNode*>& Nodes() const + { + return m_nodes_copy; + } + + private: + std::vector<CNode*> m_nodes_copy; + }; + friend struct CConnmanTest; friend struct ConnmanTestMsg; }; |