net: fix use-after-free with v2->v1 reconnection logic
What the developer wrote
Authored by Eugene Siegel
net: fix use-after-free with v2->v1 reconnection logic
CConnman::Stop() resets semOutbound, yet m_reconnections is not
cleared in Stop. Each ReconnectionInfo contains a grant member
that points to the memory that semOutbound pointed to and ~CConnman
will attempt to access the grant field (memory that was already
freed) when destroying m_reconnections. Fix this by calling
m_reconnections.clear() in CConnman::Stop() and add appropriate
annotations.
What changed, and why it matters
This commit fixes a memory-handling bug that occurs when Bitcoin Core shuts down its network connections. A list of pending reconnections held references to a memory resource (semaphore) that was freed during shutdown. When the program later destroyed that list, it could read already-freed memory, which can cause crashes or unpredictable behavior. The fix clears the reconnection list before freeing the underlying resource and adds code annotations to prevent the wrong lock from being held during shutdown.
Apply the patch to ensure m_reconnections is cleared before semOutbound is reset, and adopt the lock annotations to prevent future regressions. No immediate emergency response is indicated because the bug is in the shutdown path and not remotely triggerable.
Security signals we found
use-after-free in shutdown path
destructor accesses freed semaphore memory
missing cleanup of m_reconnections before semaphore destruction
added thread-safety annotations for shutdown locks
Evidence from the diff
CConnman::StopNodes() calls semOutbound.reset(), which destroys the underlying CSemaphore. m_reconnections was not cleared at that point, and each ReconnectionInfo contains a CSemaphoreGrant member that references the destroyed semaphore. When CConnman’s destructor later destroys m_reconnections, the CSemaphoreGrant destructor accesses the freed semaphore, causing a use-after-free. The patch clears m_reconnections under m_reconnections_mutex before semOutbound.reset() and adds lock annotations (AssertLockNotHeld, EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex)) to StopNodes() and Stop().
Changed components
src/net.cppsrc/net.hCConnman::StopNodes()CConnman::Stop()m_reconnectionssemOutboundInspect captured patch +6 / −2
diff --git a/src/net.cpp b/src/net.cpp
index d335f2dc..ef1c6304 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -3483,6 +3483,8 @@ void CConnman::StopThreads()
void CConnman::StopNodes()
{
+ AssertLockNotHeld(m_reconnections_mutex);
+
if (fAddressesInitialized) {
DumpAddresses();
fAddressesInitialized = false;
@@ -3510,6 +3512,7 @@ void CConnman::StopNodes()
DeleteNode(pnode);
}
m_nodes_disconnected.clear();
+ WITH_LOCK(m_reconnections_mutex, m_reconnections.clear());
vhListenSocket.clear();
semOutbound.reset();
semAddnode.reset();
diff --git a/src/net.h b/src/net.h
index 25cb8236..c822afe0 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1138,9 +1138,10 @@ public:
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_reconnections_mutex);
+ void Stop() EXCLUSIVE_LOCKS_REQUIRED(!m_reconnections_mutex)
{
+ AssertLockNotHeld(m_reconnections_mutex);
StopThreads();
StopNodes();
};
Why this scored 49/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.