From 1e510620558045881036ca7d2d6fe88720a8e9fd Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 21 Jun 2024 15:01:11 -0400 Subject: [PATCH] Fix shutdown ordering issue in Server.cpp. (#34035) We could end up shutting down the exchange manager while we still had live sessions/exchanges, at which point shutting down those would crash as they tried to get information from the exchange manager. The fix is to document the required shutdown ordering and enforce it correctly in CHIPDeviceControllerFactory and Server. Fixes https://github.com/project-chip/connectedhomeip/issues/20880 --- src/app/server/Server.cpp | 6 ++++ .../CHIPDeviceControllerFactory.cpp | 9 ++++++ src/messaging/ExchangeMgr.h | 6 +++- src/transport/SessionManager.cpp | 21 +++++++++++--- src/transport/SessionManager.h | 28 +++++++++++++++++++ 5 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 7a91decb43da84..a71d797651b418 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -596,6 +596,12 @@ void Server::Shutdown() #if CHIP_CONFIG_ENABLE_ICD_SERVER app::InteractionModelEngine::GetInstance()->SetICDManager(nullptr); #endif // CHIP_CONFIG_ENABLE_ICD_SERVER + // Shut down any remaining sessions (and hence exchanges) before we do any + // futher teardown. CASE handshakes have been shut down already via + // shutting down mCASESessionManager and mCASEServer above; shutting + // down mCommissioningWindowManager will shut down any PASE handshakes we + // have going on. + mSessions.ExpireAllSecureSessions(); mCommissioningWindowManager.Shutdown(); mMessageCounterManager.Shutdown(); mExchangeMgr.Shutdown(); diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index eb52e389a6b9d6..24f60ed9d8a50a 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -476,6 +476,15 @@ void DeviceControllerSystemState::Shutdown() mCASESessionManager = nullptr; } + // The above took care of CASE handshakes, and shutting down all the + // controllers should have taken care of the PASE handshakes. Clean up any + // outstanding secure sessions (shouldn't really be any, since controllers + // should have handled that, but just in case). + if (mSessionMgr != nullptr) + { + mSessionMgr->ExpireAllSecureSessions(); + } + // mCASEClientPool and mSessionSetupPool must be deallocated // after mCASESessionManager, which uses them. diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index b6e416cdbf74e7..88346060093ddc 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -80,9 +80,13 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate * Shutdown the ExchangeManager. This terminates this instance * of the object and releases all held resources. * + * Please see documentation for SessionManager::Shutdown() for ordering + * dependecies between that and this Shutdown() method. + * * @note * The protocol should only call this function after ensuring that - * there are no active ExchangeContext objects. Furthermore, it is the + * there are no active ExchangeContext objects (again, see + * SessionManager::Shutdown() documentation). Furthermore, it is the * onus of the application to de-allocate the ExchangeManager * object after calling ExchangeManager::Shutdown(). */ diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 00acc485d47d23..ffb3633b289e21 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -166,10 +166,15 @@ void SessionManager::Shutdown() // Ensure that we don't create new sessions as we iterate our session table. mState = State::kNotReady; - mSecureSessions.ForEachSession([&](auto session) { - session->MarkForEviction(); - return Loop::Continue; - }); + // Just in case some consumer forgot to do it, expire all our secure + // sessions. Note that this stands a good chance of crashing with a + // null-deref if there are in fact any secure sessions left, since they will + // try to notify their exchanges, which will then try to operate on + // partially-shut-down objects. + ExpireAllSecureSessions(); + + // We don't have a safe way to check or affect the state of our + // mUnauthenticatedSessions. We can only hope they got shut down properly. mMessageCounterManager = nullptr; @@ -542,6 +547,14 @@ void SessionManager::ExpireAllPASESessions() }); } +void SessionManager::ExpireAllSecureSessions() +{ + mSecureSessions.ForEachSession([&](auto session) { + session->MarkForEviction(); + return Loop::Continue; + }); +} + void SessionManager::MarkSessionsAsDefunct(const ScopedNodeId & node, const Optional & type) { mSecureSessions.ForEachSession([&node, &type](auto session) { diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index b7a1630b3d2851..7d0c5645365f97 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -345,7 +345,20 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate, public FabricTabl return CHIP_NO_ERROR; } + /** + * Expire all sessions for a given peer, as identified by a specific fabric + * index and node ID. + */ void ExpireAllSessions(const ScopedNodeId & node); + + /** + * Expire all sessions associated with the given fabric index. + * + * *NOTE* This is generally all sessions for a given fabric _EXCEPT_ if there are multiple + * FabricInfo instances in the FabricTable that collide on the same logical fabric (i.e + * root public key + fabric ID tuple). This can ONLY happen if multiple controller + * instances on the same fabric is permitted and each is assigned a unique fabric index. + */ void ExpireAllSessionsForFabric(FabricIndex fabricIndex); /** @@ -376,6 +389,12 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate, public FabricTabl void ExpireAllPASESessions(); + /** + * Expire all secure sessions. See documentation for Shutdown on when it's + * appropriate to use this. + */ + void ExpireAllSecureSessions(); + /** * @brief * Marks all active sessions that match provided arguments as defunct. @@ -422,6 +441,15 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate, public FabricTabl * @brief * Shutdown the Secure Session Manager. This terminates this instance * of the object and reset it's state. + * + * The proper order of shutdown for SessionManager is as follows: + * + * 1) Call ExpireAllSecureSessions() on the SessionManager, and ensure that any unauthenticated + * sessions (e.g. CASEServer and CASESessionManager instances, or anything that does PASE + * handshakes) are released. + * 2) Shut down the exchange manager, so that it's no longer referencing + * the to-be-shut-down SessionManager. + * 3) Shut down the SessionManager. */ void Shutdown();