diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-03-28 10:44:53 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-03-29 11:45:46 -0400 |
commit | fa369651c5523f393e0bfcfa328b8e27006711aa (patch) | |
tree | fb560aac582c7073afa51e95f9c375203cea3973 | |
parent | 210b533a114462473024c92a20a0765001abd45d (diff) | |
download | bitcoin-fa369651c5523f393e0bfcfa328b8e27006711aa.tar.xz |
net: Add missing cs_vNodes lock
-rw-r--r-- | src/init.cpp | 15 | ||||
-rw-r--r-- | src/net.cpp | 15 | ||||
-rw-r--r-- | src/net.h | 17 | ||||
-rw-r--r-- | src/net_processing.h | 1 | ||||
-rw-r--r-- | src/test/denialofservice_tests.cpp | 1 |
5 files changed, 31 insertions, 18 deletions
diff --git a/src/init.cpp b/src/init.cpp index eb5329df66..7fac8b457c 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -197,7 +197,20 @@ 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.peer_logic) UnregisterValidationInterface(node.peer_logic.get()); - if (node.connman) node.connman->Stop(); + // Follow the lock order requirements: + // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraOutboundCount + // 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(); + } StopTorControl(); diff --git a/src/net.cpp b/src/net.cpp index 8352c40b98..dcc613ba88 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -2387,7 +2387,7 @@ void CConnman::Interrupt() } } -void CConnman::Stop() +void CConnman::StopThreads() { if (threadMessageHandler.joinable()) threadMessageHandler.join(); @@ -2399,14 +2399,17 @@ void CConnman::Stop() threadDNSAddressSeed.join(); if (threadSocketHandler.joinable()) threadSocketHandler.join(); +} - if (fAddressesInitialized) - { +void CConnman::StopNodes() +{ + if (fAddressesInitialized) { DumpAddresses(); fAddressesInitialized = false; } // Close sockets + LOCK(cs_vNodes); for (CNode* pnode : vNodes) pnode->CloseSocketDisconnect(); for (ListenSocket& hListenSocket : vhListenSocket) @@ -2415,10 +2418,10 @@ void CConnman::Stop() 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 : vNodes) { DeleteNode(pnode); } - for (CNode *pnode : vNodesDisconnected) { + for (CNode* pnode : vNodesDisconnected) { DeleteNode(pnode); } vNodes.clear(); @@ -2433,7 +2436,7 @@ void CConnman::DeleteNode(CNode* pnode) assert(pnode); bool fUpdateConnectionTime = false; m_msgproc->FinalizeNode(pnode->GetId(), fUpdateConnectionTime); - if(fUpdateConnectionTime) { + if (fUpdateConnectionTime) { addrman.Connected(pnode->addr); } delete pnode; @@ -188,16 +188,13 @@ public: ~CConnman(); bool Start(CScheduler& scheduler, const Options& options); - // TODO: Remove NO_THREAD_SAFETY_ANALYSIS. Lock cs_vNodes before reading the variable vNodes. - // - // When removing NO_THREAD_SAFETY_ANALYSIS be aware of the following lock order requirements: - // * CheckForStaleTipAndEvictPeers locks cs_main before indirectly calling GetExtraOutboundCount - // which locks cs_vNodes. - // * ProcessMessage locks cs_main and g_cs_orphans before indirectly calling ForEachNode which - // locks cs_vNodes. - // - // Thus the implicit locking order requirement is: (1) cs_main, (2) g_cs_orphans, (3) cs_vNodes. - void Stop() NO_THREAD_SAFETY_ANALYSIS; + void StopThreads(); + void StopNodes(); + void Stop() + { + StopThreads(); + StopNodes(); + }; void Interrupt(); bool GetNetworkActive() const { return fNetworkActive; }; diff --git a/src/net_processing.h b/src/net_processing.h index 65e3963c41..3d9bc65a3a 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -14,6 +14,7 @@ class CTxMemPool; extern RecursiveMutex cs_main; +extern RecursiveMutex g_cs_orphans; /** Default for -maxorphantx, maximum number of orphan transactions kept in memory */ static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 7310498eb6..6314c1a42f 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -52,7 +52,6 @@ struct COrphanTx { NodeId fromPeer; int64_t nTimeExpire; }; -extern RecursiveMutex g_cs_orphans; extern std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans); static CService ip(uint32_t i) |