net: change FindNode() to not return a node and rename it
What the developer wrote
Authored by Vasil Dimov
net: change FindNode() to not return a node and rename it
All callers of `CConnman::FindNode()` use its return value `CNode*` only
as a boolean null/notnull. So change that method to return `bool`.
This removes the dangerous pattern of handling a `CNode` object (the
return value of `FindNode()`) without holding `CConnman::m_nodes_mutex`
and without having that object's reference count incremented for the
duration of the usage.
Also rename the method to better describe what it does.
What changed, and why it matters
This commit is a preventive safety change in Bitcoin Core's networking code. It renames and rewrites an internal lookup function so it only answers 'yes/no' about whether a connection already exists, instead of handing back a live peer object that callers might accidentally use without proper locking or reference counting. The commit itself does not claim to fix an active bug or vulnerability, but it removes a risky pattern that could lead to crashes or memory-safety issues in the future.
Treat as a positive hardening commit. Reviewers should verify that no remaining callers retain CNode* from FindNode and that the new bool helpers are used consistently. No emergency action is warranted absent additional evidence of an active vulnerability.
Security signals we found
Removes use-after-scope / use-after-unlock pattern by not exposing CNode* outside the locked region
Improves locking discipline by keeping m_nodes_mutex internal to the lookup helper
Refactors boolean-only callers to use a bool API, reducing future misuse surface
No CVE, advisory, or exploit mentioned in commit or supplied references
Evidence from the diff
CConnman::FindNode() previously returned a CNode* pointer while holding m_nodes_mutex, but callers only used the pointer as a boolean. That created a hazardous pattern: a caller could dereference or retain the CNode after the mutex was released and without incrementing the reference count. The patch changes the two overloads to bool-returning methods (AlreadyConnectedToHost and AlreadyConnectedToAddressPort), keeps the lock entirely inside the helper, and updates all call sites to use the boolean result. This is a hardening/refactoring change rather than a patch for a reported exploitable vulnerability.
Changed components
src/net.cppsrc/net.hCConnman::FindNode / AlreadyConnectedToHost / AlreadyConnectedToAddressPortCConnman::ConnectNodeCConnman::OpenNetworkConnectionInspect captured patch +26 / −24
diff --git a/src/net.cpp b/src/net.cpp
index 6e95d6a3..e0b5107f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -331,26 +331,16 @@ bool IsLocal(const CService& addr)
return mapLocalHost.count(addr) > 0;
}
-CNode* CConnman::FindNode(const std::string& addrName)
+bool CConnman::AlreadyConnectedToHost(const std::string& host) const
{
LOCK(m_nodes_mutex);
- for (CNode* pnode : m_nodes) {
- if (pnode->m_addr_name == addrName) {
- return pnode;
- }
- }
- return nullptr;
+ return std::ranges::any_of(m_nodes, [&host](CNode* node) { return node->m_addr_name == host; });
}
-CNode* CConnman::FindNode(const CService& addr)
+bool CConnman::AlreadyConnectedToAddressPort(const CService& addr_port) const
{
LOCK(m_nodes_mutex);
- for (CNode* pnode : m_nodes) {
- if (static_cast<CService>(pnode->addr) == addr) {
- return pnode;
- }
- }
- return nullptr;
+ return std::ranges::any_of(m_nodes, [&addr_port](CNode* node) { return node->addr == addr_port; });
}
bool CConnman::AlreadyConnectedToAddress(const CNetAddr& addr) const
@@ -393,10 +383,8 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
return nullptr;
// Look for an existing connection
- CNode* pnode = FindNode(static_cast<CService>(addrConnect));
- if (pnode)
- {
- LogPrintf("Failed to open new connection, already connected\n");
+ if (AlreadyConnectedToAddressPort(addrConnect)) {
+ LogInfo("Failed to open new connection to %s, already connected", addrConnect.ToStringAddrPort());
return nullptr;
}
}
@@ -426,9 +414,7 @@ CNode* CConnman::ConnectNode(CAddress addrConnect, const char *pszDest, bool fCo
}
// It is possible that we already have a connection to the IP/port pszDest resolved to.
// In that case, drop the connection that was just created.
- LOCK(m_nodes_mutex);
- CNode* pnode = FindNode(static_cast<CService>(addrConnect));
- if (pnode) {
+ if (AlreadyConnectedToAddressPort(addrConnect)) {
LogPrintf("Not opening a connection to %s, already connected to %s\n", pszDest, addrConnect.ToStringAddrPort());
return nullptr;
}
@@ -2996,8 +2982,9 @@ void CConnman::OpenNetworkConnection(const CAddress& addrConnect, bool fCountFai
if (IsLocal(addrConnect) || banned_or_discouraged || AlreadyConnectedToAddress(addrConnect)) {
return;
}
- } else if (FindNode(std::string(pszDest)))
+ } else if (AlreadyConnectedToHost(pszDest)) {
return;
+ }
CNode* pnode = ConnectNode(addrConnect, pszDest, fCountFailure, conn_type, use_v2transport);
diff --git a/src/net.h b/src/net.h
index 52044fe1..44730419 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1365,8 +1365,23 @@ private:
uint64_t CalculateKeyedNetGroup(const CNetAddr& ad) const;
- CNode* FindNode(const std::string& addrName);
- CNode* FindNode(const CService& addr);
+ /**
+ * Determine whether we're already connected to a given "host:port".
+ * Note that for inbound connections, the peer is likely using a random outbound
+ * port on their side, so this will likely not match any inbound connections.
+ * @param[in] host String of the form "host[:port]", e.g. "localhost" or "localhost:8333" or "1.2.3.4:8333".
+ * @return true if connected to `host`.
+ */
+ bool AlreadyConnectedToHost(const std::string& host) const;
+
+ /**
+ * Determine whether we're already connected to a given address:port.
+ * Note that for inbound connections, the peer is likely using a random outbound
+ * port on their side, so this will likely not match any inbound connections.
+ * @param[in] addr_port Address and port to check.
+ * @return true if connected to addr_port.
+ */
+ bool AlreadyConnectedToAddressPort(const CService& addr_port) const;
/**
* Determine whether we're already connected to a given address.
Why this scored 51/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.