Shut down subscription clients when DeviceController shuts down. (#22323

)

* 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 #22319

* Suppress now-visible LSan leak and have IM engine shut down any remaining subscriptions.

* Fix lifetime management in subscribeAttributeWithEndpointId.

* Address review comment: reduce duplication in read client iteration methods.

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:]

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
@@ -253,24 +258,37 @@ CHIP_ERROR InteractionModelEngine::ShutdownSubscription(const ScopedNodeId & aPe
 
 void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex, NodeId aPeerNodeId)
 {
- for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
- {
- if (readClient->IsSubscriptionType() && readClient->GetFabricIndex() == aFabricIndex &&
- readClient->GetPeerNodeId() == aPeerNodeId)
- {
- readClient->Close(CHIP_NO_ERROR);
- }
- }
+ ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex), MakeOptional(aPeerNodeId));
+}
+void InteractionModelEngine::ShutdownSubscriptions(FabricIndex aFabricIndex)
+{
+ ShutdownMatchingSubscriptions(MakeOptional(aFabricIndex));
 }
 
 void InteractionModelEngine::ShutdownAllSubscriptions()
 {
- for (auto * readClient = mpActiveReadClientList; readClient != nullptr; readClient = readClient->GetNextClient())
+ ShutdownMatchingSubscriptions();
+}
+
+void InteractionModelEngine::ShutdownMatchingSubscriptions(const Optional<FabricIndex> & aFabricIndex,
+ const Optional<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->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;
  }
 }
 

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.
  */
@@ -504,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<FabricIndex> & aFabricIndex = NullOptional,
+ const Optional<NodeId> & aPeerNodeId = NullOptional);
+
  template <typename T, size_t N>
  void ReleasePool(ObjectList<T> *& aObjectList, ObjectPool<ObjectList<T>, N> & aObjectPool);
  template <typename T, size_t N>

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

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

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<MTRDataValueDictionaryDecodableType> * 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<BufferedReadAttributeCallback<MTRDataValueDictionaryDecodableType>>(