Skip to content

Commit

Permalink
Merge bitcoin#21563: net: Restrict period when cs_vNodes mutex is locked
Browse files Browse the repository at this point in the history
8c8237a net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac18 net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d 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 8c8237a
  vasild:
    ACK 8c8237a
  ajtowns:
    utACK 8c8237a - logic seems sound
  MarcoFalke:
    review ACK 8c8237a 👢

Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
  • Loading branch information
MarcoFalke committed Apr 25, 2021
2 parents 66fd3b2 + 8c8237a commit 8f80092
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 27 deletions.
15 changes: 1 addition & 14 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down
25 changes: 14 additions & 11 deletions src/net.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
1 change: 0 additions & 1 deletion src/test/fuzz/process_message.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

Expand Down
1 change: 0 additions & 1 deletion src/test/fuzz/process_messages.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}

0 comments on commit 8f80092

Please sign in to comment.