Skip to content

Commit

Permalink
Fix crashes when last controller is shut down while a PASE session is…
Browse files Browse the repository at this point in the history
… outstanding.

Commissioner shutdown shuts down the CASE sessions associated with the
commissioner, but not the PASE sessions.  Those sessions then get shut down
much later in shutdown, at which point various objects that are needed to
handle the shutdown are no longer present.

The fix is to shut down PASE sessions when we destroy CommissioneeDeviceProxy
objects, and ensure that we always destroy CommissioneeDeviceProxy via
ReleaseCommissioneeDevice, so we don't end up with dangling pointers to the
objects.

Fixes project-chip#16440

Should vastly improve, if not completely fix,
project-chip#20880
  • Loading branch information
bzbarsky-apple committed Aug 30, 2022
1 parent 5f46524 commit aa82747
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 16 deletions.
27 changes: 12 additions & 15 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -489,19 +489,13 @@ void DeviceCommissioner::Shutdown()
}
#endif // CHIP_DEVICE_CONFIG_ENABLE_COMMISSIONER_DISCOVERY

// If we have a commissionee device for the device being commissioned,
// release it now, before we release our whole commissionee pool.
if (mDeviceBeingCommissioned != nullptr)
{
auto * commissionee = FindCommissioneeDevice(mDeviceBeingCommissioned->GetDeviceId());
if (commissionee)
{
ReleaseCommissioneeDevice(commissionee);
}
}

// Release everything from the commissionee device pool here. DeviceController::Shutdown releases operational.
mCommissioneeDevicePool.ReleaseAll();
// Release everything from the commissionee device pool here.
// Make sure to use ReleaseCommissioneeDevice so we don't keep dangling
// pointers to the device objects.
mCommissioneeDevicePool.ForEachActiveObject([this](auto * commissioneeDevice) {
ReleaseCommissioneeDevice(commissioneeDevice);
return Loop::Continue;
});

DeviceController::Shutdown();
}
Expand Down Expand Up @@ -539,8 +533,6 @@ CommissioneeDeviceProxy * DeviceCommissioner::FindCommissioneeDevice(const Trans

void DeviceCommissioner::ReleaseCommissioneeDevice(CommissioneeDeviceProxy * device)
{
// TODO: Call CloseSession here see #16440 and #16805 (blocking)

#if CONFIG_NETWORK_LAYER_BLE
if (mSystemState->BleLayer() != nullptr && device->GetDeviceTransportType() == Transport::Type::kBle)
{
Expand All @@ -559,6 +551,11 @@ void DeviceCommissioner::ReleaseCommissioneeDevice(CommissioneeDeviceProxy * dev
{
mDeviceBeingCommissioned = nullptr;
}

// Release the commissionee device after we have nulled out our pointers,
// because that can call back in to us with error notifications as the
// session is released.
mCommissioneeDevicePool.ReleaseObject(device);
}

CHIP_ERROR DeviceCommissioner::GetDeviceBeingCommissioned(NodeId deviceId, CommissioneeDeviceProxy ** out_device)
Expand Down
9 changes: 8 additions & 1 deletion src/controller/CommissioneeDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,14 @@ CHIP_ERROR CommissioneeDeviceProxy::SetConnected(const SessionHandle & session)
return CHIP_NO_ERROR;
}

CommissioneeDeviceProxy::~CommissioneeDeviceProxy() {}
CommissioneeDeviceProxy::~CommissioneeDeviceProxy()
{
auto session = GetSecureSession();
if (session.HasValue())
{
session.Value()->AsSecureSession()->MarkForEviction();
}
}

CHIP_ERROR CommissioneeDeviceProxy::SetPeerId(ByteSpan rcac, ByteSpan noc)
{
Expand Down

0 comments on commit aa82747

Please sign in to comment.