Skip to content

Commit

Permalink
Shut down subscription clients when DeviceController shuts down.
Browse files Browse the repository at this point in the history
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 project-chip#22319
  • Loading branch information
bzbarsky-apple committed Sep 1, 2022
1 parent 08830a5 commit e87751e
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 2 deletions.
30 changes: 28 additions & 2 deletions src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down
5 changes: 5 additions & 0 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down
2 changes: 2 additions & 0 deletions src/app/ReadClient.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,8 @@ void ReadClient::Close(CHIP_ERROR aError, bool allowResubscription, bool allowOn
StopResubscription();
}

mExchange.Release();

if (allowOnDone)
{
mpCallback.OnDone(this);
Expand Down
3 changes: 3 additions & 0 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit e87751e

Please sign in to comment.