From 75e8bf55f5a014faada7712a9640dc35e8c86f15 Mon Sep 17 00:00:00 2001
From: Vasil Dimov <vd@FreeBSD.org>
Date: Mon, 26 Apr 2021 16:22:07 +0200
Subject: 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
```
---
 src/net.cpp | 72 ++++++++++++++++++++-----------------------------------------
 src/net.h   | 37 +++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 49 deletions(-)

(limited to 'src')

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);
diff --git a/src/net.h b/src/net.h
index 48dfb3043f..7b97b98ae5 100644
--- a/src/net.h
+++ b/src/net.h
@@ -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;
 };
-- 
cgit v1.2.3