From a3d090d1103cd6c25daf07afdf4e65febca6d3f7 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 12 Apr 2021 14:54:58 +0300 Subject: [PATCH 1/3] net: Restrict period when cs_vNodes mutex is locked --- src/init.cpp | 15 +-------------- src/net.cpp | 8 ++++---- src/test/fuzz/process_message.cpp | 1 - src/test/fuzz/process_messages.cpp | 1 - 4 files changed, 5 insertions(+), 20 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 701a7529afa..280a0cbda77 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -200,20 +200,7 @@ void Shutdown(NodeContext& node) // Because these depend on each-other, we make sure that neither can be // using the other before destroying them. if (node.peerman) UnregisterValidationInterface(node.peerman.get()); - // Follow the lock order requirements: - // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraFullOutboundCount - // which locks cs_vNodes. - // * ProcessMessage locks cs_main and g_cs_orphans before indirectly calling ForEachNode which - // locks cs_vNodes. - // * CConnman::Stop calls DeleteNode, which calls FinalizeNode, which locks cs_main and calls - // EraseOrphansFor, which locks g_cs_orphans. - // - // Thus the implicit locking order requirement is: (1) cs_main, (2) g_cs_orphans, (3) cs_vNodes. - if (node.connman) { - node.connman->StopThreads(); - LOCK2(::cs_main, ::g_cs_orphans); - node.connman->StopNodes(); - } + if (node.connman) node.connman->Stop(); StopTorControl(); diff --git a/src/net.cpp b/src/net.cpp index ae38acdc3cb..65f8b8baecf 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2636,8 +2636,9 @@ void CConnman::StopNodes() } // Close sockets - LOCK(cs_vNodes); - for (CNode* pnode : vNodes) + std::vector nodes; + WITH_LOCK(cs_vNodes, nodes.swap(vNodes)); + for (CNode* pnode : nodes) pnode->CloseSocketDisconnect(); for (ListenSocket& hListenSocket : vhListenSocket) if (hListenSocket.socket != INVALID_SOCKET) @@ -2645,13 +2646,12 @@ void CConnman::StopNodes() LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAGetLastError())); // clean up some globals (to help leak detection) - for (CNode* pnode : vNodes) { + for (CNode* pnode : nodes) { DeleteNode(pnode); } for (CNode* pnode : vNodesDisconnected) { DeleteNode(pnode); } - vNodes.clear(); vNodesDisconnected.clear(); vhListenSocket.clear(); semOutbound.reset(); diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 96e1cfa08fb..7b99193ad04 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -100,7 +100,6 @@ void fuzz_target(FuzzBufferType buffer, const std::string& LIMIT_TO_MESSAGE_TYPE g_setup->m_node.peerman->SendMessages(&p2p_node); } SyncWithValidationInterfaceQueue(); - LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement g_setup->m_node.connman->StopNodes(); } diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 203c0ef8e15..11b236c9bdd 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -80,6 +80,5 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages) } } SyncWithValidationInterfaceQueue(); - LOCK2(::cs_main, g_cs_orphans); // See init.cpp for rationale for implicit locking order requirement g_setup->m_node.connman->StopNodes(); } From 229ac1892d807a1eea5a7c24ae0fe27dc913b1bd Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 12 Apr 2021 14:54:58 +0300 Subject: [PATCH 2/3] net: Combine two loops into one, and update comments --- src/net.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 65f8b8baecf..2cb95516a48 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2635,20 +2635,20 @@ void CConnman::StopNodes() } } - // Close sockets + // Delete peer connections. std::vector nodes; WITH_LOCK(cs_vNodes, nodes.swap(vNodes)); - for (CNode* pnode : nodes) + for (CNode* pnode : nodes) { pnode->CloseSocketDisconnect(); + DeleteNode(pnode); + } + + // Close listening sockets. for (ListenSocket& hListenSocket : vhListenSocket) if (hListenSocket.socket != INVALID_SOCKET) if (!CloseSocket(hListenSocket.socket)) LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAGetLastError())); - // clean up some globals (to help leak detection) - for (CNode* pnode : nodes) { - DeleteNode(pnode); - } for (CNode* pnode : vNodesDisconnected) { DeleteNode(pnode); } From 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 22 Apr 2021 17:30:35 +0300 Subject: [PATCH 3/3] net, refactor: Fix style in CConnman::StopNodes --- src/net.cpp | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/net.cpp b/src/net.cpp index 2cb95516a48..a8e07a567de 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2644,10 +2644,13 @@ void CConnman::StopNodes() } // Close listening sockets. - for (ListenSocket& hListenSocket : vhListenSocket) - if (hListenSocket.socket != INVALID_SOCKET) - if (!CloseSocket(hListenSocket.socket)) + for (ListenSocket& hListenSocket : vhListenSocket) { + if (hListenSocket.socket != INVALID_SOCKET) { + if (!CloseSocket(hListenSocket.socket)) { LogPrintf("CloseSocket(hListenSocket) failed with error %s\n", NetworkErrorString(WSAGetLastError())); + } + } + } for (CNode* pnode : vNodesDisconnected) { DeleteNode(pnode);