Skip to content

Commit

Permalink
Ensure that we shut down open exchanges when controller shuts down.
Browse files Browse the repository at this point in the history
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 #15760
  • Loading branch information
bzbarsky-apple committed Mar 3, 2022
1 parent 4ab9f60 commit 527126c
Show file tree
Hide file tree
Showing 8 changed files with 64 additions and 11 deletions.
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

0 comments on commit 527126c

Please sign in to comment.