Fix leaked readClient in onFabricRemoved

When ReadClient::Close is called from onFabricRemove in InteractionModel Engine, readClient is destoryed and becomes not valid so that readClient->GetNextClient() will be use-after-free.

src/app/InteractionModelEngine.cpp

@@ -1967,13 +1967,20 @@ void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, Fa
     });
 
 #if CHIP_CONFIG_ENABLE_READ_CLIENT
-    for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
+    for (auto * readClient = mpActiveReadClientList; readClient != nullptr;)
     {
         if (readClient->GetFabricIndex() == fabricIndex)
         {
             ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex);
+            auto * nextReadClient = readClient->GetNextClient();
             readClient->Close(CHIP_ERROR_IM_FABRIC_DELETED, false);
+            readClient = nextReadClient;
+        }
+        else
+        {
+            readClient = readClient->GetNextClient();
         }
+
     }
 #endif // CHIP_CONFIG_ENABLE_READ_CLIENT
 

src/app/InteractionModelEngine.h

@@ -324,12 +324,19 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
     /**
      * @brief Function decrements the number of subscriptions to resume counter - mNumOfSubscriptionsToResume.
      *        This should be called after we have completed a re-subscribe attempt on a persisted subscription wether the attempt
-     *        was succesful or not.
+     *        was successful or not.
      */
     void DecrementNumSubscriptionsToResume();
 #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
 
 #if CONFIG_BUILD_FOR_HOST_UNIT_TEST
+
+    //
+    // Get fabric table
+    //
+    FabricTable * GetFabricTable() { return mpFabricTable; }
+
+
     //
     // Get direct access to the underlying read handler pool
     //
@@ -714,7 +721,7 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
 #endif // CHIP_CONFIG_SUBSCRIPTION_TIMEOUT_RESUMPTION
 #endif // CHIP_CONFIG_PERSIST_SUBSCRIPTIONS
 
-    FabricTable * mpFabricTable;
+    FabricTable * mpFabricTable = nullptr;
 
     CASESessionManager * mpCASESessionMgr = nullptr;
 

src/app/ReadClient.cpp

@@ -113,6 +113,9 @@ ReadClient::~ReadClient()
             mpImEngine->RemoveReadClient(this);
         }
     }
+    mpExchangeMgr = nullptr;
+    mpNext        = nullptr;
+    mpImEngine    = nullptr;
 }
 
 uint32_t ReadClient::ComputeTimeTillNextSubscription()

src/controller/tests/data_model/TestRead.cpp

@@ -4510,6 +4510,64 @@ TEST_F(TestRead, TestReadHandler_TwoParallelReadsSecondTooManyPaths)
     engine->SetForceHandlerQuota(false);
 }
 
+TEST_F(TestRead, TestReadHandler_MultipleSubscriptions_OnFabricRemoved)
+{
+    auto sessionHandle                       = GetSessionBobToAlice();
+    uint32_t numSuccessCalls                 = 0;
+    uint32_t numSubscriptionEstablishedCalls = 0;
+
+    ScopedChange directive(gReadResponseDirective, ReadResponseDirective::kSendDataResponse);
+
+    // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's
+    // not safe to do so.
+    auto onSuccessCb = [&numSuccessCalls](const app::ConcreteDataAttributePath & attributePath, const auto & dataResponse) {
+        numSuccessCalls++;
+    };
+
+    // Passing of stack variables by reference is only safe because of synchronous completion of the interaction. Otherwise, it's
+    // not safe to do so.
+    auto onFailureCb = [](const app::ConcreteDataAttributePath * attributePath, CHIP_ERROR aError) {
+    };
+
+    auto onSubscriptionEstablishedCb = [&numSubscriptionEstablishedCalls](const app::ReadClient & readClient,
+                                                                          chip::SubscriptionId aSubscriptionId) {
+        numSubscriptionEstablishedCalls++;
+    };
+
+    //
+    // Try to issue parallel subscriptions that will exceed the value for app::InteractionModelEngine::kReadHandlerPoolSize.
+    // If heap allocation is correctly setup, this should result in it successfully servicing more than the number
+    // present in that define.
+    //
+    for (size_t i = 0; i < (app::InteractionModelEngine::kReadHandlerPoolSize + 1); i++)
+    {
+        EXPECT_EQ(Controller::SubscribeAttribute<Clusters::UnitTesting::Attributes::ListStructOctetString::TypeInfo>(
+                      &GetExchangeManager(), sessionHandle, kTestEndpointId, onSuccessCb, onFailureCb, 0, 20,
+                      onSubscriptionEstablishedCb, nullptr, false, true),
+                  CHIP_NO_ERROR);
+    }
+
+    // There are too many messages and the test (gcc_debug, which includes many sanity checks) will be quite slow. Note: report
+    // engine is using ScheduleWork which cannot be handled by DrainAndServiceIO correctly.
+    GetIOContext().DriveIOUntil(System::Clock::Seconds16(60), [&]() {
+        return numSuccessCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1) &&
+            numSubscriptionEstablishedCalls == (app::InteractionModelEngine::kReadHandlerPoolSize + 1);
+    });
+
+    EXPECT_EQ(numSuccessCalls, (app::InteractionModelEngine::kReadHandlerPoolSize + 1));
+    EXPECT_EQ(numSubscriptionEstablishedCalls, (app::InteractionModelEngine::kReadHandlerPoolSize + 1));
+    EXPECT_EQ(mNumActiveSubscriptions, static_cast<int32_t>(app::InteractionModelEngine::kReadHandlerPoolSize + 1));
+
+    app::InteractionModelEngine::GetInstance()->GetFabricTable()->DeleteAllFabrics();
+
+    EXPECT_EQ(mNumActiveSubscriptions, 0);
+    size_t numActiveReadClients = app::InteractionModelEngine::GetInstance()->GetNumActiveReadClients();
+    EXPECT_EQ(numActiveReadClients, 0u);
+    EXPECT_EQ(GetExchangeManager().GetNumActiveExchanges(), 0u);
+
+    SetMRPMode(chip::Test::MessagingContext::MRPMode::kDefault);
+}
+
 TEST_F(TestRead, TestReadAttribute_ManyDataValues)
 {
     auto sessionHandle  = GetSessionBobToAlice();