diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 745d99293a23e0..86439c683e045b 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -56,6 +56,7 @@ CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeM mpFabricTable = apFabricTable; mpCASESessionMgr = apCASESessionMgr; + ReturnErrorOnFailure(mpFabricTable->AddFabricDelegate(this)); ReturnErrorOnFailure(mpExchangeMgr->RegisterUnsolicitedMessageHandlerForProtocol(Protocols::InteractionModel::Id, this)); mReportingEngine.Init(); @@ -76,9 +77,9 @@ void InteractionModelEngine::Shutdown() // while (handlerIter) { - CommandHandlerInterface * next = handlerIter->GetNext(); + CommandHandlerInterface * pNext = handlerIter->GetNext(); handlerIter->SetNext(nullptr); - handlerIter = next; + handlerIter = pNext; } mCommandHandlerList = nullptr; @@ -1217,9 +1218,9 @@ void InteractionModelEngine::ReleasePool(ObjectList *& aObjectList, ObjectPoo ObjectList * current = aObjectList; while (current != nullptr) { - ObjectList * next = current->mpNext; + ObjectList * pNext = current->mpNext; aObjectPool.ReleaseObject(current); - current = next; + current = pNext; } aObjectList = nullptr; @@ -1434,5 +1435,41 @@ size_t InteractionModelEngine::GetNumDirtySubscriptions() const return numDirtySubscriptions; } +void InteractionModelEngine::OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) +{ + mReadHandlers.ForEachActiveObject([fabricIndex](ReadHandler * handler) { + if (handler->GetAccessingFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read handler with FabricIndex: %u", fabricIndex); + handler->Close(); + } + + return Loop::Continue; + }); + + for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient()) + { + if (readClient->GetFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete read client with FabricIndex: %u", fabricIndex); + readClient->Close(CHIP_NO_ERROR, false); + RemoveReadClient(readClient); + } + } + + for (auto & handler : mWriteHandlers) + { + if (!(handler.IsFree()) && handler.GetAccessingFabricIndex() == fabricIndex) + { + ChipLogProgress(InteractionModel, "Fabric removed, deleting obsolete write handler with FabricIndex: %u", fabricIndex); + handler.Close(); + } + } + + // Applications may hold references to CommandHandler instances for async command processing. + // Therefore we can't forcible destroy CommandHandlers here. Their exchanges will get closed by + // the fabric removal, though, so they will fail when they try to actually send their command response + // and will close at that point. +} } // namespace app } // namespace chip diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index 6dffea8e9fdcc6..8da9281f326ac2 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -74,7 +74,8 @@ namespace app { class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, public Messaging::ExchangeDelegate, public CommandHandler::Callback, - public ReadHandler::ManagementCallback + public ReadHandler::ManagementCallback, + public FabricTable::Delegate { public: /** @@ -289,6 +290,8 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ uint16_t GetMinGuaranteedSubscriptionsPerFabric() const; + void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override; + #if CONFIG_BUILD_FOR_HOST_UNIT_TEST // // Get direct access to the underlying read handler pool diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 19e1ced153334c..bdce6443171feb 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -86,7 +86,7 @@ ReadClient::~ReadClient() // This won't be the case if the engine shut down before this destructor was called (in which case, mpImEngine // will point to null) // - if (mpImEngine) + if (mpImEngine && mpImEngine->InActiveReadClientList(this)) { mpImEngine->RemoveReadClient(this); } diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 2b161a7b62e6f5..436e6a5bc714e5 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -272,14 +272,6 @@ class ReadClient : public Messaging::ExchangeDelegate */ ~ReadClient() override; - /* - * This forcibly closes the exchange context if a valid one is pointed to. Such a situation does - * not arise during normal message processing flows that all normally call Close() above. This can only - * arise due to application-initiated destruction of the object when this object is handling receiving/sending - * message payloads. Abort() should be called first before the object is destroyed. - */ - void Abort(); - /** * Send a request. There can be one request outstanding on a given ReadClient. * If SendRequest returns success, no more SendRequest calls can happen on this ReadClient diff --git a/src/app/WriteHandler.h b/src/app/WriteHandler.h index 7a27266d4722d3..a1b8831165437a 100644 --- a/src/app/WriteHandler.h +++ b/src/app/WriteHandler.h @@ -76,6 +76,11 @@ class WriteHandler : public Messaging::ExchangeDelegate */ void Abort(); + /** + * Clean up state when we are done sending the write response. + */ + void Close(); + bool IsFree() const { return mState == State::Uninitialized; } ~WriteHandler() override = default; @@ -133,10 +138,6 @@ class WriteHandler : public Messaging::ExchangeDelegate void MoveToState(const State aTargetState); void ClearState(); const char * GetStateStr() const; - /** - * Clean up state when we are done sending the write response. - */ - void Close(); void DeliverListWriteBegin(const ConcreteAttributePath & aPath); void DeliverListWriteEnd(const ConcreteAttributePath & aPath, bool writeWasSuccessful); diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 2c53a69c557c32..ca8aedcbcdfc37 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp @@ -282,7 +282,6 @@ CHIP_ERROR DeleteFabricFromTable(FabricIndex fabricIndex) void CleanupSessionsForFabric(SessionManager & sessionMgr, FabricIndex fabricIndex) { - InteractionModelEngine::GetInstance()->CloseTransactionsFromFabricIndex(fabricIndex); sessionMgr.ExpireAllSessionsForFabric(fabricIndex); } @@ -379,11 +378,16 @@ class OpCredsFabricTableDelegate : public chip::FabricTable::Delegate ChipLogProgress(Zcl, "OpCreds: Fabric index 0x%x was removed", static_cast(fabricIndex)); EventManagement::GetInstance().FabricRemoved(fabricIndex); + NotifyFabricTableChanged(); } // Gets called when a fabric is added/updated, but not necessarily committed to storage - void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override { NotifyFabricTableChanged(); } + void OnFabricUpdated(const FabricTable & fabricTable, FabricIndex fabricIndex) override + { + ChipLogProgress(InteractionModel, "OnFabricUpdated debugging from opertonal-credential!!!!!!!"); + NotifyFabricTableChanged(); + } // Gets called when a fabric in FabricTable is persisted to storage void OnFabricCommitted(const FabricTable & fabricTable, FabricIndex fabricIndex) override diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 2b78cb8f4c9388..d1c77de9bb1d95 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -335,6 +335,7 @@ class TestReadInteraction static void TestSubscribeSendUnknownMessage(nlTestSuite * apSuite, void * apContext); static void TestSubscribeSendInvalidStatusReport(nlTestSuite * apSuite, void * apContext); static void TestReadHandlerInvalidSubscribeRequest(nlTestSuite * apSuite, void * apContext); + static void TestSubscribeInvalidateFabric(nlTestSuite * apSuite, void * apContext); private: static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, @@ -3651,6 +3652,71 @@ void TestReadInteraction::TestReadHandlerInvalidSubscribeRequest(nlTestSuite * a NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); } +// Create the subscription, then remove the corresponding fabric in client and handler, the corresponding +// client and handler would be released as well. +void TestReadInteraction::TestSubscribeInvalidateFabric(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + CHIP_ERROR err = CHIP_NO_ERROR; + + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + // Shouldn't have anything in the retransmit table when starting the test. + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + + GenerateEvents(apSuite, apContext); + + MockInteractionModelApp delegate; + auto * engine = chip::app::InteractionModelEngine::GetInstance(); + err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); + + ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); + readPrepareParams.mEventPathParamsListSize = 0; + + readPrepareParams.mpAttributePathParamsList = new chip::app::AttributePathParams[2]; + readPrepareParams.mAttributePathParamsListSize = 2; + + readPrepareParams.mMinIntervalFloorSeconds = 0; + readPrepareParams.mMaxIntervalCeilingSeconds = 0; + + { + app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, + chip::app::ReadClient::InteractionType::Subscribe); + + delegate.mGotReport = false; + + err = readClient.SendAutoResubscribeRequest(std::move(readPrepareParams)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, delegate.mGotReport); + + // We have 29 attributes in our mock attribute storage. And we subscribed twice. + // And attribute 3/2/4 is a list with 6 elements and list chunking is applied to it, thus we should receive ( 29 + 6 ) * 2 = + // 70 attribute data in total. + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 70); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 1); + NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); + delegate.mpReadHandler = engine->ActiveHandlerAt(0); + + ctx.GetFabricTable().Delete(ctx.GetAliceFabricIndex()); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers(ReadHandler::InteractionType::Subscribe) == 0); + ctx.GetFabricTable().Delete(ctx.GetBobFabricIndex()); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0); + ctx.ExpireSessionAliceToBob(); + ctx.ExpireSessionBobToAlice(); + ctx.CreateAliceFabric(); + ctx.CreateBobFabric(); + ctx.CreateSessionAliceToBob(); + ctx.CreateSessionBobToAlice(); + } + + engine->Shutdown(); + NL_TEST_ASSERT(apSuite, ctx.GetExchangeManager().GetNumActiveExchanges() == 0); +} + } // namespace app } // namespace chip @@ -3695,6 +3761,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestSubscribeSendUnknownMessage", chip::app::TestReadInteraction::TestSubscribeSendUnknownMessage), NL_TEST_DEF("TestSubscribeSendInvalidStatusReport", chip::app::TestReadInteraction::TestSubscribeSendInvalidStatusReport), NL_TEST_DEF("TestReadHandlerInvalidSubscribeRequest", chip::app::TestReadInteraction::TestReadHandlerInvalidSubscribeRequest), + NL_TEST_DEF("TestSubscribeInvalidateFabric", chip::app::TestReadInteraction::TestSubscribeInvalidateFabric), NL_TEST_DEF("TestSubscribeUrgentWildcardEvent", chip::app::TestReadInteraction::TestSubscribeUrgentWildcardEvent), NL_TEST_DEF("TestSubscribeWildcard", chip::app::TestReadInteraction::TestSubscribeWildcard), NL_TEST_DEF("TestSubscribePartialOverlap", chip::app::TestReadInteraction::TestSubscribePartialOverlap), diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index dcd13e596859ea..65f6e8b5d300d1 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -69,6 +69,7 @@ class TestWriteInteraction static void TestWriteRoundtripWithClusterObjectsVersionMismatch(nlTestSuite * apSuite, void * apContext); #if CONFIG_BUILD_FOR_HOST_UNIT_TEST static void TestWriteHandlerReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext); + static void TestWriteHandlerInvalidateFabric(nlTestSuite * apSuite, void * apContext); #endif private: static void AddAttributeDataIB(nlTestSuite * apSuite, void * apContext, WriteClient & aWriteClient); @@ -626,6 +627,56 @@ void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * a ctx.CreateSessionAliceToBob(); ctx.CreateSessionBobToAlice(); } + +// This test is to create Chunked write requests, we drop the message since the 3rd message, then remove fabrics for client and +// handler, the corresponding client and handler would be released as well. +void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuite, void * apContext) +{ + TestContext & ctx = *static_cast(apContext); + auto sessionHandle = ctx.GetSessionBobToAlice(); + + app::AttributePathParams attributePath(2, 3, 4); + + CHIP_ERROR err = CHIP_NO_ERROR; + Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + // Shouldn't have anything in the retransmit table when starting the test. + NL_TEST_ASSERT(apSuite, rm->TestGetCountRetransTable() == 0); + + TestWriteClientCallback writeCallback; + auto * engine = chip::app::InteractionModelEngine::GetInstance(); + err = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable()); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional::Missing(), + static_cast(900) /* reserved buffer size */); + + ByteSpan list[5]; + + err = writeClient.EncodeAttribute(attributePath, app::DataModel::List(list, 5)); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + ctx.GetLoopback().mDroppedMessageCount = 0; + ctx.GetLoopback().mSentMessageCount = 0; + ctx.GetLoopback().mNumMessagesToDrop = 1; + ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 2; + err = writeClient.SendWriteRequest(sessionHandle); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + ctx.DrainAndServiceIO(); + + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 1); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 3); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); + + ctx.GetFabricTable().Delete(ctx.GetAliceFabricIndex()); + NL_TEST_ASSERT(apSuite, InteractionModelEngine::GetInstance()->GetNumActiveWriteHandlers() == 0); + engine->Shutdown(); + ctx.ExpireSessionAliceToBob(); + ctx.ExpireSessionBobToAlice(); + ctx.CreateAliceFabric(); + ctx.CreateSessionAliceToBob(); + ctx.CreateSessionBobToAlice(); +} + #endif // Write Client sends a write request, receives an unexpected message type, sends a status response to that. @@ -917,6 +968,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestWriteRoundtripWithClusterObjectsVersionMismatch", chip::app::TestWriteInteraction::TestWriteRoundtripWithClusterObjectsVersionMismatch), #if CONFIG_BUILD_FOR_HOST_UNIT_TEST NL_TEST_DEF("TestWriteHandlerReceiveInvalidMessage", chip::app::TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage), + NL_TEST_DEF("TestWriteHandlerInvalidateFabric", chip::app::TestWriteInteraction::TestWriteHandlerInvalidateFabric), #endif NL_TEST_DEF("TestWriteInvalidMessage1", chip::app::TestWriteInteraction::TestWriteInvalidMessage1), NL_TEST_DEF("TestWriteInvalidMessage2", chip::app::TestWriteInteraction::TestWriteInvalidMessage2), diff --git a/src/messaging/tests/MessagingContext.cpp b/src/messaging/tests/MessagingContext.cpp index 127b1a152d7bf2..c5519892690071 100644 --- a/src/messaging/tests/MessagingContext.cpp +++ b/src/messaging/tests/MessagingContext.cpp @@ -53,13 +53,8 @@ CHIP_ERROR MessagingContext::Init(TransportMgrBase * transport, IOContext * ioCo if (mInitializeNodes) { - ReturnErrorOnFailure(mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, - GetNodeA1CertAsset().mCert, - GetNodeA1CertAsset().mKey, &mAliceFabricIndex)); - - ReturnErrorOnFailure(mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, - GetNodeA2CertAsset().mCert, - GetNodeA2CertAsset().mKey, &mBobFabricIndex)); + ReturnErrorOnFailure(CreateAliceFabric()); + ReturnErrorOnFailure(CreateBobFabric()); ReturnErrorOnFailure(CreateSessionBobToAlice()); ReturnErrorOnFailure(CreateSessionAliceToBob()); @@ -98,6 +93,20 @@ void MessagingContext::ShutdownAndRestoreExisting(MessagingContext & existing) existing.mTransport->SetSessionManager(&existing.GetSecureSessionManager()); } +CHIP_ERROR MessagingContext::CreateAliceFabric() +{ + return mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, + GetNodeA1CertAsset().mCert, GetNodeA1CertAsset().mKey, + &mAliceFabricIndex); +} + +CHIP_ERROR MessagingContext::CreateBobFabric() +{ + return mFabricTable.AddNewFabricForTestIgnoringCollisions(GetRootACertAsset().mCert, GetIAA1CertAsset().mCert, + GetNodeA2CertAsset().mCert, GetNodeA2CertAsset().mKey, + &mBobFabricIndex); +} + CHIP_ERROR MessagingContext::CreateSessionBobToAlice() { return mSessionManager.InjectPaseSessionWithTestKey(mSessionBobToAlice, kBobKeyId, GetAliceFabric()->GetNodeId(), kAliceKeyId, diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index 8c13fdd4f62a85..43aa164df1865d 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -133,6 +133,9 @@ class MessagingContext : public PlatformMemoryUser SessionHandle GetSessionDavidToCharlie(); SessionHandle GetSessionBobToFriends(); + CHIP_ERROR CreateAliceFabric(); + CHIP_ERROR CreateBobFabric(); + const Transport::PeerAddress & GetAliceAddress() { return mAliceAddress; } const Transport::PeerAddress & GetBobAddress() { return mBobAddress; }