Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that we shut down open exchanges when controller shuts down. #15816

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,14 @@ void CASESessionManager::ReleaseSession(PeerId peerId)
ReleaseSession(FindExistingSession(peerId));
}

void CASESessionManager::ReleaseSessionForFabric(CompressedFabricId compressedFabricId)
void CASESessionManager::ReleaseSessionsForFabric(CompressedFabricId compressedFabricId)
{
mConfig.devicePool->ReleaseDeviceForFabric(compressedFabricId);
mConfig.devicePool->ReleaseDevicesForFabric(compressedFabricId);
}

void CASESessionManager::ReleaseAllSessions()
{
mConfig.devicePool->ReleaseAllDevices();
}

CHIP_ERROR CASESessionManager::ResolveDeviceAddress(FabricInfo * fabric, NodeId nodeId)
Expand Down
4 changes: 3 additions & 1 deletion src/app/CASESessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,9 @@ class CASESessionManager : public Dnssd::OperationalResolveDelegate

void ReleaseSession(PeerId peerId);

void ReleaseSessionForFabric(CompressedFabricId compressedFabricId);
void ReleaseSessionsForFabric(CompressedFabricId compressedFabricId);

void ReleaseAllSessions();

/**
* This API triggers the DNS-SD resolution for the given node ID. The node ID will be looked up
Expand Down
9 changes: 8 additions & 1 deletion src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,13 @@ CHIP_ERROR OperationalDeviceProxy::ShutdownSubscriptions()
return app::InteractionModelEngine::GetInstance()->ShutdownSubscriptions(mFabricInfo->GetFabricIndex(), GetDeviceId());
}

OperationalDeviceProxy::~OperationalDeviceProxy() {}
OperationalDeviceProxy::~OperationalDeviceProxy()
{
if (mCASEClient)
{
// Make sure we don't leak it.
mInitParams.clientPool->Release(mCASEClient);
}
}

} // namespace chip
14 changes: 12 additions & 2 deletions src/app/OperationalDeviceProxyPool.h
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,9 @@ class OperationalDeviceProxyPoolDelegate

virtual OperationalDeviceProxy * FindDevice(PeerId peerId) = 0;

virtual void ReleaseDeviceForFabric(CompressedFabricId compressedFabricId) = 0;
virtual void ReleaseDevicesForFabric(CompressedFabricId compressedFabricId) = 0;

virtual void ReleaseAllDevices() = 0;

virtual ~OperationalDeviceProxyPoolDelegate() {}
};
Expand Down Expand Up @@ -91,7 +93,7 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate
return foundDevice;
}

void ReleaseDeviceForFabric(CompressedFabricId compressedFabricId) override
void ReleaseDevicesForFabric(CompressedFabricId compressedFabricId) override
{
mDevicePool.ForEachActiveObject([&](auto * activeDevice) {
if (activeDevice->GetPeerId().GetCompressedFabricId() == compressedFabricId)
Expand All @@ -102,6 +104,14 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate
});
}

void ReleaseAllDevices() override
{
mDevicePool.ForEachActiveObject([&](auto * activeDevice) {
Release(activeDevice);
return Loop::Continue;
});
}

private:
ObjectPool<OperationalDeviceProxy, N> mDevicePool;
};
Expand Down
2 changes: 1 addition & 1 deletion src/app/clusters/bindings/BindingManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ void BindingManager::HandleDeviceConnectionFailure(PeerId peerId, CHIP_ERROR err
void BindingManager::FabricRemoved(CompressedFabricId compressedFabricId, FabricIndex fabricIndex)
{
mPendingNotificationMap.RemoveAllEntriesForFabric(fabricIndex);
mAppServer->GetCASESessionManager()->ReleaseSessionForFabric(compressedFabricId);
mAppServer->GetCASESessionManager()->ReleaseSessionsForFabric(compressedFabricId);
}

CHIP_ERROR BindingManager::NotifyBindingAdded(const EmberBindingTableEntry & binding)
Expand Down
25 changes: 25 additions & 0 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,6 +246,21 @@ CHIP_ERROR DeviceController::Shutdown()

mState = State::NotInitialized;

if (mFabricInfo != nullptr)
{
// Shut down any ongoing CASE session activity we have. Note that we
// own the mCASESessionManager, so shutting down everything on it is
// fine. If we ever end up sharing the CASE session manager with other
// DeviceController instances we may need to be more targeted here.
mCASESessionManager->ReleaseAllSessions();

// TODO: The CASE session manager does not shut down existing CASE
// sessions. It just shuts down any ongoing CASE session establishment
// we're in the middle of as initiator. Maybe it should shut down
// existing sessions too?
mSystemState->SessionMgr()->ExpireAllPairingsForFabric(mFabricInfo->GetFabricIndex());
}

mStorageDelegate = nullptr;

if (mFabricInfo != nullptr)
Expand Down Expand Up @@ -1452,6 +1467,16 @@ void DeviceCommissioner::OnOperationalNodeResolved(const chip::Dnssd::ResolvedNo
ChipLogValueX64(nodeData.mPeerId.GetNodeId()));
VerifyOrReturn(mState == State::Initialized);

// TODO: minimal mdns is buggy and violates the API contract for the
// resolver proxy by handing us results for all sorts of things we did not
// ask it to resolve, including results that don't even match our fabric.
// Reject at least those mis-matching results, since we can detect those
// easily.
if (nodeData.mPeerId.GetCompressedFabricId() != GetCompressedFabricId())
{
return;
}

mDNSCache.Insert(nodeData);

mCASESessionManager->FindOrEstablishSession(nodeData.mPeerId, &mOnDeviceConnectedCallback, &mOnDeviceConnectionFailureCallback);
Expand Down
10 changes: 7 additions & 3 deletions src/protocols/secure_channel/CASESession.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -110,14 +110,18 @@ void CASESession::Clear()

mState = kInitialized;

CloseExchange();
AbortExchange();
}

void CASESession::CloseExchange()
void CASESession::AbortExchange()
{
if (mExchangeCtxt != nullptr)
{
mExchangeCtxt->Close();
// The only time we reach this is if we are getting destroyed in the
// middle of our handshake. In that case, there is no point trying to
// do MRP resends of the last message we sent, so abort the exchange
// instead of just closing it.
mExchangeCtxt->Abort();
mExchangeCtxt = nullptr;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/protocols/secure_channel/CASESession.h
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ class DLL_EXPORT CASESession : public Messaging::ExchangeDelegate, public Pairin
void OnSuccessStatusReport() override;
CHIP_ERROR OnFailureStatusReport(Protocols::SecureChannel::GeneralStatusCode generalCode, uint16_t protocolCode) override;

void CloseExchange();
void AbortExchange();

/**
* Clear our reference to our exchange context pointer so that it can close
Expand Down