From 365fa9f6bf922bb7760978b987eaeaf3c476f0c5 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Wed, 31 Aug 2022 17:37:55 -0400 Subject: [PATCH 1/4] 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 | 2 ++ src/controller/CHIPDeviceController.cpp | 3 +++ 4 files changed, 38 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..c4c26cdc034611 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -190,6 +190,8 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription, bool allowOn StopResubscription(); } + mExchange.Release(); + if (allowOnDone) { mpCallback.OnDone(this); diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index b62ae41e823d8e..7e72bb2b6f2a17 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); From d143f8e07bedafe5690110a12abf7ad0ca54ea52 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 1 Sep 2022 00:09:41 -0400 Subject: [PATCH 2/4] Suppress now-visible LSan leak and have IM engine shut down any remaining subscriptions. --- scripts/tests/chiptest/lsan-mac-suppressions.txt | 3 +++ src/app/InteractionModelEngine.cpp | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/scripts/tests/chiptest/lsan-mac-suppressions.txt b/scripts/tests/chiptest/lsan-mac-suppressions.txt index 68ac691dec325d..1b52efb24f056d 100644 --- a/scripts/tests/chiptest/lsan-mac-suppressions.txt +++ b/scripts/tests/chiptest/lsan-mac-suppressions.txt @@ -33,3 +33,6 @@ leak:[CHIPToolKeypair signMessageECDSA_RAW:] # TODO: Figure out how we are managing to leak NSData while using ARC, though # this may just be a leak deep inside the CFPreferences stuff somewhere. leak:[CHIPToolPersistentStorageDelegate storageDataForKey:] + +# TODO: https://github.com/project-chip/connectedhomeip/issues/22333 +leak:[MTRBaseCluster* subscribeAttribute*WithMinInterval:maxInterval:params:subscriptionEstablished:reportHandler:] diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index abd99824eab361..9888395e562ad3 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -98,6 +98,11 @@ void InteractionModelEngine::Shutdown() mReadHandlers.ReleaseAll(); + // Shut down any subscription clients that are still around. They won't be + // able to work after this point anyway, since we're about to drop our refs + // to them. + ShutdownAllSubscriptions(); + // // We hold weak references to ReadClient objects. The application ultimately // actually owns them, so it's on them to eventually shut them down and free them From 57f9c08a14c7158c44ee031e2312a3b4fd513638 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 1 Sep 2022 09:29:01 -0400 Subject: [PATCH 3/4] Fix lifetime management in subscribeAttributeWithEndpointId. --- src/darwin/Framework/CHIP/MTRBaseDevice.mm | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/darwin/Framework/CHIP/MTRBaseDevice.mm b/src/darwin/Framework/CHIP/MTRBaseDevice.mm index d323d15cd5c31e..668e1412be3864 100644 --- a/src/darwin/Framework/CHIP/MTRBaseDevice.mm +++ b/src/darwin/Framework/CHIP/MTRBaseDevice.mm @@ -1203,8 +1203,10 @@ - (void)subscribeAttributeWithEndpointId:(NSNumber * _Nullable)endpointId = (params != nil && params.keepPreviousSubscriptions != nil && [params.keepPreviousSubscriptions boolValue]); auto onDone = [container](BufferedReadAttributeCallback * callback) { - chip::Platform::Delete(callback); [container onDone]; + // Make sure we delete callback last, because doing that actually destroys our + // lambda, so we can't access captured values after that. + chip::Platform::Delete(callback); }; auto callback = chip::Platform::MakeUnique>( From 10fb972de767353dc9cb0af78d4f1e621fc0a083 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 1 Sep 2022 10:14:44 -0400 Subject: [PATCH 4/4] Address review comment: reduce duplication in read client iteration methods. --- src/app/InteractionModelEngine.cpp | 41 ++++++++++-------------------- src/app/InteractionModelEngine.h | 7 +++++ 2 files changed, 21 insertions(+), 27 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 9888395e562ad3..3f88c40f5f9910 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -258,38 +258,20 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscription(const ScopedNodeId & aPe void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId) { - // 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; - } + ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex), MakeOptional(aPeerNodeId)); } - 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; - } + ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex)); } void InteractionModelEngine::ShutdownAllSubscriptions() +{ + ShutdownMatchingSubscriptions(); +} + +void InteractionModelEngine::ShutdownMatchingSubscriptions(const Optional & aFabricIndex, + const Optional & aPeerNodeId) { // This is assuming that ReadClient::Close will not affect any other // ReadClients in the list. @@ -299,7 +281,12 @@ void InteractionModelEngine::ShutdownAllSubscriptions() auto * nextClient = readClient->GetNextClient(); if (readClient->IsSubscriptionType()) { - readClient->Close(CHIP_NO_ERROR); + bool fabricMatches = !aFabricIndex.HasValue() || (aFabricIndex.Value() == readClient->GetFabricIndex()); + bool nodeIdMatches = !aPeerNodeId.HasValue() || (aPeerNodeId.Value() == readClient->GetPeerNodeId()); + if (fabricMatches && nodeIdMatches) + { + readClient->Close(CHIP_NO_ERROR); + } } readClient = nextClient; } diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h index f1547341a7ade4..0bd2c6738a3818 100644 --- a/src/app/InteractionModelEngine.h +++ b/src/app/InteractionModelEngine.h @@ -509,6 +509,13 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler, */ Status EnsureResourceForRead(FabricIndex aFabricIndex, size_t aRequestedAttributePathCount, size_t aRequestedEventPathCount); + /** + * Helper for various ShutdownSubscriptions functions. The subscriptions + * that match all the provided arguments will be shut down. + */ + void ShutdownMatchingSubscriptions(const Optional & aFabricIndex = NullOptional, + const Optional & aPeerNodeId = NullOptional); + template void ReleasePool(ObjectList *& aObjectList, ObjectPool, N> & aObjectPool); template