Fix shutdown ordering issue in Server.cpp. (project-chip#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 project-chip#20880

Author: bzbarsky-apple

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

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.
 

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().
      */

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<Transport::SecureSession::Type> & type)
 {
     mSecureSessions.ForEachSession([&node, &type](auto session) {

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