diff --git a/src/app/CASESessionManager.cpp b/src/app/CASESessionManager.cpp index 8690f036f6353f..80d66a50b81cc0 100644 --- a/src/app/CASESessionManager.cpp +++ b/src/app/CASESessionManager.cpp @@ -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) diff --git a/src/app/CASESessionManager.h b/src/app/CASESessionManager.h index 2e499595426342..a3c0dbe81dfe2f 100644 --- a/src/app/CASESessionManager.h +++ b/src/app/CASESessionManager.h @@ -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 diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp index 29a1b37a775b72..ac800bc14d6821 100644 --- a/src/app/OperationalDeviceProxy.cpp +++ b/src/app/OperationalDeviceProxy.cpp @@ -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 diff --git a/src/app/OperationalDeviceProxyPool.h b/src/app/OperationalDeviceProxyPool.h index aaa46bbf5773c8..cb93c29aebd40d 100644 --- a/src/app/OperationalDeviceProxyPool.h +++ b/src/app/OperationalDeviceProxyPool.h @@ -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() {} }; @@ -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) @@ -102,6 +104,14 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate }); } + void ReleaseAllDevices() override + { + mDevicePool.ForEachActiveObject([&](auto * activeDevice) { + Release(activeDevice); + return Loop::Continue; + }); + } + private: ObjectPool mDevicePool; }; diff --git a/src/app/clusters/bindings/BindingManager.cpp b/src/app/clusters/bindings/BindingManager.cpp index 0fdaf555782dcf..35967d50ab1016 100644 --- a/src/app/clusters/bindings/BindingManager.cpp +++ b/src/app/clusters/bindings/BindingManager.cpp @@ -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) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index c4c68fc88dc8d6..f33d456dffbc71 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -240,6 +240,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) @@ -1433,6 +1448,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); diff --git a/src/protocols/secure_channel/CASESession.cpp b/src/protocols/secure_channel/CASESession.cpp index 3eff7c7561974e..bb2161d94dedc2 100644 --- a/src/protocols/secure_channel/CASESession.cpp +++ b/src/protocols/secure_channel/CASESession.cpp @@ -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; } } diff --git a/src/protocols/secure_channel/CASESession.h b/src/protocols/secure_channel/CASESession.h index 82fa60c9fd56ec..e813eaf6c1cdfa 100644 --- a/src/protocols/secure_channel/CASESession.h +++ b/src/protocols/secure_channel/CASESession.h @@ -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