From 11830ff2f822fbbf5655a064351253c072109c36 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 29 Aug 2022 18:46:21 -0400 Subject: [PATCH] Fix crashes when last controller is shut down while a PASE session is 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 https://github.com/project-chip/connectedhomeip/issues/16440 Should vastly improve, if not completely fix, https://github.com/project-chip/connectedhomeip/issues/20880 --- src/controller/CHIPDeviceController.cpp | 27 ++++++++++------------ src/controller/CommissioneeDeviceProxy.cpp | 9 +++++++- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/controller/CHIPDeviceController.cpp b/src/controller/CHIPDeviceController.cpp index 0bf9ee911359d9..b62ae41e823d8e 100644 --- a/src/controller/CHIPDeviceController.cpp +++ b/src/controller/CHIPDeviceController.cpp @@ -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(); } @@ -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) { @@ -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) diff --git a/src/controller/CommissioneeDeviceProxy.cpp b/src/controller/CommissioneeDeviceProxy.cpp index af312f7cf27ca4..45a14eed62a356 100644 --- a/src/controller/CommissioneeDeviceProxy.cpp +++ b/src/controller/CommissioneeDeviceProxy.cpp @@ -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) {