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();