From ab6410a7512dea1292989d7b620a78921a15b42c Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 3 Mar 2022 17:07:06 -0500 Subject: [PATCH] Ensure that we shut down open exchanges when controller shuts down. (#15816) We already handled shutdown of any ongoing PASE bits. This PR adds two more things: 1) Shutting down any ongoing CASE session establishment exchanges for which we are the initiator. This is done by shutting down all the operational device proxies on our mCASESessionManager (since we own all of those anyway) and fixing operational device proxy shutdown/destruction to actually clean up the CASEClient if we're still in the middle of CASE establishment. 2) Expiring the SecureSessions for our fabric, so that any still-open operational exchanges for those sessions get closed correctly (with a timeout). This is needed because our client cluster APIs don't give us any way to cancel the operation (invoke, read, write, etc) and we need to make sure those get cleaned up when we shut down. In addition to that: * Reject wrong-fabric results in DeviceCommissioner::OnOperationalNodeResolved (due to buggy minimal mdns), so if we start sharing a CASESessionManager across controllers we will not be in a position where we are ending up with CASE sessions we create but can't tear down. * Fix CASE shutdown to not leave a dangling MRP entry after it shuts down the exchange. Fixes https://github.com/project-chip/connectedhomeip/issues/15760 --- src/app/CASESessionManager.cpp | 9 +++++-- src/app/CASESessionManager.h | 4 +++- src/app/OperationalDeviceProxy.cpp | 9 ++++++- src/app/OperationalDeviceProxyPool.h | 14 +++++++++-- src/app/clusters/bindings/BindingManager.cpp | 2 +- src/controller/CHIPDeviceController.cpp | 25 ++++++++++++++++++++ src/protocols/secure_channel/CASESession.cpp | 10 +++++--- src/protocols/secure_channel/CASESession.h | 2 +- 8 files changed, 64 insertions(+), 11 deletions(-) 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