From dfb4b221dddf4a001df5ffee1d175ba02a703063 Mon Sep 17 00:00:00 2001 From: Sami Ahmed Date: Fri, 27 Mar 2026 21:57:34 -0700 Subject: [PATCH] Fix shutdown race conditions causing bad_weak_ptr crash (v5.4.1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes multiple concurrency bugs exposed during shutdown when Tor proxy connections are failing: - Reorder shutdown: stop network threads before destroying Tor V3 services - Make RPC listener responsive to fShutdown (poll_one+sleep vs blocking run_one) - Wrap StopRequests() in try/catch and drain io_service on exit - Fix leaked CNode AddRef in ThreadSocketHandler2 and ThreadMessageHandler2 (return→break so Release loop executes) - Guard vNodes.size() read with cs_vNodes lock (data race) - Guard Qt UI signal callbacks with fShutdown check (use-after-free) - Add cs_vNodes lock in CNetCleanup global destructor - Force-disconnect remaining nodes in StopNode() after threads stop - Make Tor maintenance thread sleep in 500ms intervals for prompt shutdown Co-Authored-By: Claude Opus 4.6 --- src/clientversion.h | 2 +- src/init.cpp | 8 ++++++-- src/net.cpp | 43 ++++++++++++++++++++++++++++++++---------- src/qt/clientmodel.cpp | 5 ++++- src/tor/onion_v3.cpp | 7 +++++-- src/trianglesrpc.cpp | 19 +++++++++++++++++-- 6 files changed, 66 insertions(+), 18 deletions(-) diff --git a/src/clientversion.h b/src/clientversion.h index ad8b95a..eb742cb 100644 --- a/src/clientversion.h +++ b/src/clientversion.h @@ -8,7 +8,7 @@ // These need to be macros, as version.cpp's and triangles-qt.rc's voodoo requires it #define CLIENT_VERSION_MAJOR 5 #define CLIENT_VERSION_MINOR 4 -#define CLIENT_VERSION_REVISION 0 +#define CLIENT_VERSION_REVISION 1 #define CLIENT_VERSION_BUILD 0 // Converts the parameter X to a string after macro replacement on X has been performed. diff --git a/src/init.cpp b/src/init.cpp index be90cf2..ddd8a47 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -176,6 +176,12 @@ void Shutdown(void* parg) } SecureMsgShutdown(); + + // Stop network threads FIRST so nothing references Tor objects + nTransactionsUpdated++; + StopNode(); + + // NOW safe to destroy Tor state - all threads have stopped ShutdownTorV3(); StopEmbeddedTor(); @@ -194,10 +200,8 @@ void Shutdown(void* parg) pNotificationQueue = NULL; } - nTransactionsUpdated++; // CTxDB().Close(); bitdb.Flush(false); - StopNode(); bitdb.Flush(true); fs::remove(GetPidFile()); UnregisterWallet(pwalletMain); diff --git a/src/net.cpp b/src/net.cpp index f7628b7..32d6feb 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -878,10 +878,19 @@ void ThreadSocketHandler2(void* parg) } } } - if (vNodes.size() != nPrevNodeCount) { - nPrevNodeCount = vNodes.size(); - uiInterface.NotifyNumConnectionsChanged(vNodes.size()); + // Read vNodes.size() under the lock to avoid data race + unsigned int nNodeCount; + { + LOCK(cs_vNodes); + nNodeCount = vNodes.size(); + } + if (nNodeCount != nPrevNodeCount) + { + nPrevNodeCount = nNodeCount; + if (!fShutdown) + uiInterface.NotifyNumConnectionsChanged(nNodeCount); + } } @@ -1013,7 +1022,7 @@ void ThreadSocketHandler2(void* parg) for (CNode* pnode : vNodesCopy) { if (fShutdown) - return; + break; // // Receive @@ -1039,7 +1048,7 @@ void ThreadSocketHandler2(void* parg) if (!pnode->ReceiveMsgBytes(pchBuf, nBytes)) pnode->CloseSocketDisconnect(); pnode->nLastRecv = GetTime(); - pnode->nRecvBytes += nBytes; + pnode->nRecvBytes += nBytes; } else if (nBytes == 0) { @@ -1104,6 +1113,8 @@ void ThreadSocketHandler2(void* parg) pnode->Release(); } + if (fShutdown) + return; MilliSleep(10); } } @@ -1832,6 +1843,9 @@ void ThreadMessageHandler2(void* parg) pnodeTrickle = vNodesCopy[GetRand(vNodesCopy.size())]; for (CNode* pnode : vNodesCopy) { + if (fShutdown) + break; + // Receive messages { TRY_LOCK(pnode->cs_vRecvMsg, lockRecv); @@ -1839,8 +1853,6 @@ void ThreadMessageHandler2(void* parg) if (!ProcessMessages(pnode)) pnode->CloseSocketDisconnect(); } - if (fShutdown) - return; // Send messages { @@ -1848,8 +1860,6 @@ void ThreadMessageHandler2(void* parg) if (lockSend) SendMessages(pnode, pnode == pnodeTrickle); } - if (fShutdown) - return; } { @@ -2180,6 +2190,18 @@ bool StopNode() } MilliSleep(50); DumpAddresses(); + + // Force-disconnect and clean up all remaining nodes now that threads have stopped. + // Close sockets first so any lingering I/O fails immediately. + { + LOCK(cs_vNodes); + for (CNode* pnode : vNodes) + { + pnode->CloseSocketDisconnect(); + pnode->Cleanup(); + } + } + return true; } @@ -2191,7 +2213,8 @@ public: } ~CNetCleanup() { - // Close sockets + // Close sockets - acquire lock in case other threads are still winding down + LOCK(cs_vNodes); for (CNode* pnode : vNodes) if (pnode->hSocket != INVALID_SOCKET) closesocket(pnode->hSocket); diff --git a/src/qt/clientmodel.cpp b/src/qt/clientmodel.cpp index 8a8ffe8..04caf3f 100644 --- a/src/qt/clientmodel.cpp +++ b/src/qt/clientmodel.cpp @@ -193,13 +193,16 @@ static void NotifyBlocksChanged(ClientModel *clientmodel) static void NotifyNumConnectionsChanged(ClientModel *clientmodel, int newNumConnections) { - // Too noisy: OutputDebugStringF("NotifyNumConnectionsChanged %i\n", newNumConnections); + // Don't queue UI updates during shutdown - ClientModel may be destroyed + // before Qt processes the queued invocation, causing use-after-free. + if (fShutdown) return; QMetaObject::invokeMethod(clientmodel, "updateNumConnections", Qt::QueuedConnection, Q_ARG(int, newNumConnections)); } static void NotifyAlertChanged(ClientModel *clientmodel, const uint256 &hash, ChangeType status) { + if (fShutdown) return; OutputDebugStringF("NotifyAlertChanged %s status=%i\n", hash.GetHex().c_str(), status); QMetaObject::invokeMethod(clientmodel, "updateAlert", Qt::QueuedConnection, Q_ARG(QString, QString::fromStdString(hash.GetHex())), diff --git a/src/tor/onion_v3.cpp b/src/tor/onion_v3.cpp index 392dcc3..0826ba4 100644 --- a/src/tor/onion_v3.cpp +++ b/src/tor/onion_v3.cpp @@ -2206,7 +2206,9 @@ void ThreadTorMaintenance(void* parg) while (!fShutdown) { - MilliSleep(30000); // check every 30 seconds + // Sleep in short intervals so the thread exits promptly on shutdown + for (int i = 0; i < 60 && !fShutdown; i++) + MilliSleep(500); if (fShutdown) break; // --- Tor health check & auto-restart --- @@ -2243,7 +2245,8 @@ void ThreadTorMaintenance(void* parg) else { printf("WARNING: Tor restart failed, retrying in %d seconds\n", restartBackoffSec); - MilliSleep(restartBackoffSec * 1000); + for (int i = 0; i < restartBackoffSec * 2 && !fShutdown; i++) + MilliSleep(500); if (restartBackoffSec < 300) restartBackoffSec *= 2; } diff --git a/src/trianglesrpc.cpp b/src/trianglesrpc.cpp index c78cc23..2f5d5e2 100644 --- a/src/trianglesrpc.cpp +++ b/src/trianglesrpc.cpp @@ -946,9 +946,24 @@ void ThreadRPCServer2(void* parg) vnThreadsRunning[THREAD_RPCLISTENER]--; while (!fShutdown) - io_service.run_one(); + { + // Use poll_one + sleep instead of blocking run_one so the thread + // remains responsive to fShutdown and can exit promptly. + if (!io_service.poll_one()) + { + io_service.restart(); + MilliSleep(50); + } + } vnThreadsRunning[THREAD_RPCLISTENER]++; - StopRequests(); + + // Safely shut down: close acceptors, then drain any remaining handlers + try { + StopRequests(); + } catch (...) { + // Absorb bad_weak_ptr or other exceptions from stale tracked slots + } + io_service.poll(); // process cancellation callbacks so shared_ptrs are released } class JSONRequest