GlobalMutex silences some of the thread safety analysis and was introduced to overcome some limitations on those, is supposed to be used only for mutexes that are defined globally. I agree it is confusing. I don't like it and have a plan to remove GlobalMutex but it is stuck at #25390.
<details>
<summary>[patch] change to Mutex</summary>
diff --git i/src/net.cpp w/src/net.cpp
index c2160e945f..c8ee95a3d0 100644
--- i/src/net.cpp
+++ w/src/net.cpp
@@ -886,12 +886,14 @@ size_t CConnman::SocketSendData(CNode& node) const
* to forge. In order to partition a node the attacker must be
* simultaneously better at all of them than honest peers.
* If we find a candidate perform the eviction.
*/
bool CConnman::AttemptToEvictConnection()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
std::vector<NodeEvictionCandidate> vEvictionCandidates;
{
LOCK(m_nodes_mutex);
for (const CNode* node : m_nodes) {
if (node->fDisconnect)
@@ -937,12 +939,14 @@ bool CConnman::AttemptToEvictConnection()
return true;
}
return false;
}
void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
struct sockaddr_storage sockaddr;
socklen_t len = sizeof(sockaddr);
auto sock = hListenSocket.sock->Accept((struct sockaddr*)&sockaddr, &len);
CAddress addr;
if (!sock) {
@@ -1107,23 +1111,27 @@ bool CConnman::AddConnection(const std::string& address, ConnectionType conn_typ
OpenNetworkConnection(CAddress(), false, &grant, address.c_str(), conn_type);
return true;
}
void CConnman::DeleteDisconnectedNode(CNode* pnode)
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
// Destroy the object only after other threads have stopped using it.
// Prevent double free by setting nRefCount to -1 before delete.
int expectedRefCount = 0;
if (pnode->nRefCount.compare_exchange_strong(expectedRefCount, -1)) {
WITH_LOCK(m_nodes_disconnected_mutex, m_nodes_disconnected.remove(pnode));
DeleteNode(pnode);
}
}
void CConnman::DisconnectAndReleaseNode(CNode* pnode)
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
LOCK(m_nodes_mutex);
if (std::find(m_nodes.begin(), m_nodes.end(), pnode) != m_nodes.end()) {
// remove from m_nodes
m_nodes.erase(remove(m_nodes.begin(), m_nodes.end(), pnode), m_nodes.end());
@@ -1138,12 +1146,14 @@ void CConnman::DisconnectAndReleaseNode(CNode* pnode)
WITH_LOCK(m_nodes_disconnected_mutex, m_nodes_disconnected.push_back(pnode));
}
}
void CConnman::DisconnectNodes()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
{
LOCK(m_nodes_mutex);
if (!fNetworkActive) {
// Disconnect any connected nodes
for (CNode* pnode : m_nodes) {
@@ -1270,12 +1280,13 @@ Sock::EventsPerSock CConnman::GenerateWaitSockets(Span<CNode* const> nodes)
return events_per_sock;
}
void CConnman::SocketHandler()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
AssertLockNotHeld(m_total_bytes_sent_mutex);
Sock::EventsPerSock events_per_sock;
{
const NodesSnapshot snap{*this, /*shuffle=*/false};
@@ -1381,12 +1392,14 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes,
if (InactivityCheck(*pnode)) pnode->fDisconnect = true;
}
}
void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock)
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
for (const ListenSocket& listen_socket : vhListenSocket) {
if (interruptNet) {
return;
}
const auto it = events_per_sock.find(listen_socket.sock);
if (it != events_per_sock.end() && it->second.occurred & Sock::RECV) {
@@ -1394,12 +1407,13 @@ void CConnman::SocketHandlerListening(const Sock::EventsPerSock& events_per_sock
}
}
}
void CConnman::ThreadSocketHandler()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
AssertLockNotHeld(m_total_bytes_sent_mutex);
while (!interruptNet)
{
DisconnectNodes();
NotifyNumConnectionsChanged();
@@ -2093,12 +2107,14 @@ void CConnman::ThreadMessageHandler()
fMsgProcWake = false;
}
}
void CConnman::ThreadI2PAcceptIncoming()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
static constexpr auto err_wait_begin = 1s;
static constexpr auto err_wait_cap = 5min;
auto err_wait = err_wait_begin;
bool advertising_listen_addr = false;
i2p::Connection conn;
@@ -2481,12 +2497,14 @@ void CConnman::StopThreads()
if (threadSocketHandler.joinable())
threadSocketHandler.join();
}
void CConnman::StopNodes()
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
if (fAddressesInitialized) {
DumpAddresses();
fAddressesInitialized = false;
if (m_use_addrman_outgoing) {
// Anchor connections are only dumped during clean shutdown.
diff --git i/src/net.h w/src/net.h
index d5833d7e2d..d45f3c606f 100644
--- i/src/net.h
+++ w/src/net.h
@@ -760,15 +760,17 @@ public:
~CConnman();
bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !m_added_nodes_mutex, !m_addr_fetches_mutex, !mutexMsgProc);
void StopThreads();
- void StopNodes();
- void Stop()
+ void StopNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
+ void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex)
{
+ AssertLockNotHeld(m_nodes_disconnected_mutex);
+
StopThreads();
StopNodes();
};
void Interrupt() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc);
bool GetNetworkActive() const { return fNetworkActive; };
@@ -917,31 +919,31 @@ private:
void ThreadOpenAddedConnections() EXCLUSIVE_LOCKS_REQUIRED(!m_added_nodes_mutex, !m_unused_i2p_sessions_mutex);
void AddAddrFetch(const std::string& strDest) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex);
void ProcessAddrFetch() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_unused_i2p_sessions_mutex);
void ThreadOpenConnections(std::vector<std::string> connect) EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_added_nodes_mutex, !m_nodes_mutex, !m_unused_i2p_sessions_mutex);
void ThreadMessageHandler() EXCLUSIVE_LOCKS_REQUIRED(!mutexMsgProc);
- void ThreadI2PAcceptIncoming();
- void AcceptConnection(const ListenSocket& hListenSocket);
+ void ThreadI2PAcceptIncoming() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
+ void AcceptConnection(const ListenSocket& hListenSocket) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
/**
* Create a `CNode` object from a socket that has just been accepted and add the node to
* the `m_nodes` member.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] sock Connected socket to communicate with the peer.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] permission_flags The peer's permissions.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] addr_bind The address and port at our side of the connection.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] addr The address and port at the peer's side of the connection.
*/
void CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
NetPermissionFlags permission_flags,
const CAddress& addr_bind,
- const CAddress& addr);
+ const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
- void DeleteDisconnectedNode(CNode* pnode);
- void DisconnectAndReleaseNode(CNode* pnode);
- void DisconnectNodes();
+ void DeleteDisconnectedNode(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
+ void DisconnectAndReleaseNode(CNode* pnode) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
+ void DisconnectNodes() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
void NotifyNumConnectionsChanged();
/** Return true if the peer is inactive and should be disconnected. */
bool InactivityCheck(const CNode& node) const;
/**
* Generate a collection of sockets to check for IO readiness.
@@ -950,13 +952,13 @@ private:
*/
Sock::EventsPerSock GenerateWaitSockets(Span<CNode* const> nodes);
/**
* Check connected and listening sockets for IO readiness and process them accordingly.
*/
- void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
+ void SocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc);
/**
* Do the read/write for connected sockets that are ready for IO.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] nodes Nodes to process. The socket of each node is checked against `what`.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] events_per_sock Sockets that are ready for IO.
*/
@@ -965,15 +967,15 @@ private:
EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
/**
* Accept incoming connections, one from each read-ready listening socket.
* [@param](/bitcoin-bitcoin/contributor/param/)[in] events_per_sock Sockets that are ready for IO.
*/
- void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock);
+ void SocketHandlerListening(const Sock::EventsPerSock& events_per_sock) EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
- void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex, !mutexMsgProc);
+ void ThreadSocketHandler() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex, !m_total_bytes_sent_mutex, !mutexMsgProc);
void ThreadDNSAddressSeed() EXCLUSIVE_LOCKS_REQUIRED(!m_addr_fetches_mutex, !m_nodes_mutex);
uint64_t CalculateKeyedNetGroup(const CAddress& ad) const;
CNode* FindNode(const CNetAddr& ip);
CNode* FindNode(const CSubNet& subNet);
@@ -983,13 +985,13 @@ private:
/**
* Determine whether we're already connected to a given address, in order to
* avoid initiating duplicate connections.
*/
bool AlreadyConnectedToAddress(const CAddress& addr);
- bool AttemptToEvictConnection();
+ bool AttemptToEvictConnection() EXCLUSIVE_LOCKS_REQUIRED(!m_nodes_disconnected_mutex);
CNode* ConnectNode(CAddress addrConnect, const char *pszDest, bool fCountFailure, ConnectionType conn_type) EXCLUSIVE_LOCKS_REQUIRED(!m_unused_i2p_sessions_mutex);
void AddWhitelistPermissionFlags(NetPermissionFlags& flags, const CNetAddr &addr) const;
void DeleteNode(CNode* pnode);
NodeId GetNewNodeId();
@@ -1042,13 +1044,13 @@ private:
const NetGroupManager& m_netgroupman;
std::deque<std::string> m_addr_fetches GUARDED_BY(m_addr_fetches_mutex);
Mutex m_addr_fetches_mutex;
std::vector<std::string> m_added_nodes GUARDED_BY(m_added_nodes_mutex);
mutable Mutex m_added_nodes_mutex;
std::vector<CNode*> m_nodes GUARDED_BY(m_nodes_mutex);
- GlobalMutex m_nodes_disconnected_mutex;
+ Mutex m_nodes_disconnected_mutex;
std::list<CNode*> m_nodes_disconnected GUARDED_BY(m_nodes_disconnected_mutex);
mutable RecursiveMutex m_nodes_mutex;
std::atomic<NodeId> nLastNodeId{0};
unsigned int nPrevNodeCount{0};
/**
</details>
