diff options
author | MarcoFalke <falke.marco@gmail.com> | 2021-04-25 10:08:52 +0200 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2021-04-25 10:08:57 +0200 |
commit | 8f80092d78f758fdb2e79e2a832a0c7a26fa2da1 (patch) | |
tree | 46ecd55ee42909cec1971658dbaf120ebcb4006a /src | |
parent | 66fd3b28e85c167f3955b5603496daf8d91abcad (diff) | |
parent | 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f (diff) | |
download | bitcoin-8f80092d78f758fdb2e79e2a832a0c7a26fa2da1.tar.xz |
Merge bitcoin/bitcoin#21563: net: Restrict period when cs_vNodes mutex is locked
8c8237a4a10feb2ac9ce46f67b5d14bf879b670f net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d807a1eea5a7c24ae0fe27dc913b1bd net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d1103cd6c25daf07afdf4e65febca6d3f7 net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov)
Pull request description:
This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`.
This change makes the explicit locking of recursive mutexes in the explicit order redundant.
ACKs for top commit:
jnewbery:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
vasild:
ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f
ajtowns:
utACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f - logic seems sound
MarcoFalke:
review ACK 8c8237a4a10feb2ac9ce46f67b5d14bf879b670f 👢
Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
Diffstat (limited to 'src')
-rw-r--r-- | src/init.cpp | 15 | ||||
-rw-r--r-- | src/net.cpp | 25 | ||||
-rw-r--r-- | src/test/fuzz/process_message.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/process_messages.cpp | 1 |
4 files changed, 15 insertions, 27 deletions
diff --git a/src/init.cpp b/src/init.cpp index cb056c85eb..bb5b144802 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -194,20 +194,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 5aa267f0d7..fe1a0dfded 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2635,23 +2635,26 @@ void CConnman::StopNodes() } } - // Close sockets - LOCK(cs_vNodes); - for (CNode* pnode : vNodes) + // Delete peer connections. + std::vector<CNode*> nodes; + WITH_LOCK(cs_vNodes, nodes.swap(vNodes)); + for (CNode* pnode : nodes) { pnode->CloseSocketDisconnect(); - 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 : vNodes) { 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())); + } + } + } + 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 96e1cfa08f..7b99193ad0 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 203c0ef8e1..11b236c9bd 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(); } |