From e27e0ac76bf36b76ae741029951f0ef7fdb9c5cc Mon Sep 17 00:00:00 2001
From: Boris Zbarsky <bzbarsky@apple.com>
Date: Wed, 31 Aug 2022 17:37:55 -0400
Subject: [PATCH] Shut down subscription clients when DeviceController shuts
 down.

Other clients get shut down due to their sessions going away, but subscription
clients can be in a state where they are not waiting on a response, and those
should get shut down too.

Also:

1) Fixes ReadClient::Close to release its exchange, so shutting down a
subscription while it's waiting for a response from the server actually shuts it
down, instead of delivering OnDone and then getting a message on the exchange
and possibly sending more notifications after OnDone.

2) Fixes potential use-after-free in the ShutdownSubscriptions functions.

Fixes https://github.com/project-chip/connectedhomeip/issues/22319
---
 src/app/InteractionModelEngine.cpp      | 30 +++++++++++++++++++++++--
 src/app/InteractionModelEngine.h        |  5 +++++
 src/app/ReadClient.cpp                  |  1 +
 src/controller/CHIPDeviceController.cpp |  3 +++
 4 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index 58520b147292b9..abd99824eab361 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -253,24 +253,50 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscription(const ScopedNodeId & aPe
 
 void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId)
 {
-    for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
+    // This is assuming that ReadClient::Close will not affect any other
+    // ReadClients in the list.
+    for (auto * readClient = mpActiveReadClientList; readClient != nullptr;)
     {
+        // Grab the next client now, because we might be about to delete readClient.
+        auto * nextClient = readClient->GetNextClient();
         if (readClient->IsSubscriptionType() && readClient->GetFabricIndex() == aFabricIndex &&
             readClient->GetPeerNodeId() == aPeerNodeId)
         {
             readClient->Close(CHIP_NO_ERROR);
         }
+        readClient = nextClient;
+    }
+}
+
+void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex)
+{
+    // This is assuming that ReadClient::Close will not affect any other
+    // ReadClients in the list.
+    for (auto * readClient = mpActiveReadClientList; readClient != nullptr;)
+    {
+        // Grab the next client now, because we might be about to delete readClient.
+        auto * nextClient = readClient->GetNextClient();
+        if (readClient->IsSubscriptionType() && readClient->GetFabricIndex() == aFabricIndex)
+        {
+            readClient->Close(CHIP_NO_ERROR);
+        }
+        readClient = nextClient;
     }
 }
 
 void InteractionModelEngine::ShutdownAllSubscriptions()
 {
-    for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
+    // This is assuming that ReadClient::Close will not affect any other
+    // ReadClients in the list.
+    for (auto * readClient = mpActiveReadClientList; readClient != nullptr;)
     {
+        // Grab the next client now, because we might be about to delete readClient.
+        auto * nextClient = readClient->GetNextClient();
         if (readClient->IsSubscriptionType())
         {
             readClient->Close(CHIP_NO_ERROR);
         }
+        readClient = nextClient;
     }
 }
 
diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
index 650e5c88246290..f1547341a7ade4 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -139,6 +139,11 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
      */
     void ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId);
 
+    /**
+     * Tears down all active subscriptions for a given fabric.
+     */
+    void ShutdownSubscriptions(FabricIndex aFabricIndex);
+
     /**
      * Tears down all active subscriptions.
      */
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
index 3c3bc5b19e2e76..f2dc0f78543c69 100644
--- a/src/app/ReadClient.cpp
+++ b/src/app/ReadClient.cpp
@@ -194,6 +194,7 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription, bool allowOn
     {
         mpCallback.OnDone(this);
     }
+    mExchange.Release();
 }
 
 const char * ReadClient::GetStateStr() const
diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp
index 0bf9ee911359d9..fbf08b57baba75 100644
--- a/src/controller/CHIPDeviceController.cpp
+++ b/src/controller/CHIPDeviceController.cpp
@@ -329,6 +329,9 @@ void DeviceController::Shutdown()
 
     if (mFabricIndex != kUndefinedFabricIndex)
     {
+        // Shut down any subscription clients for this fabric.
+        app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricIndex);
+
         // Shut down any ongoing CASE session activity we have.  We're going to
         // assume that all sessions for our fabric belong to us here.
         mSystemState->CASESessionMgr()->ReleaseSessionsForFabric(mFabricIndex);