From 20e65bf3edae8d7926b1be73903f41ee1bac1ddf Mon Sep 17 00:00:00 2001 From: Michael Spang Date: Wed, 22 Jun 2022 21:18:21 -0400 Subject: [PATCH] Fix CHIPDeviceControllerFactory shutdown (#19022) - SessionResumptionStorage is leaked - Ownership is passed using raw pointers. This is bad form, and there are many more instances of it (not fixed here). - DeviceControllerSystemState destructor is called twice (once by Release(), then again by Delete()) --- .../CHIPDeviceControllerFactory.cpp | 44 ++++++++++--------- src/controller/CHIPDeviceControllerFactory.h | 10 ++++- .../CHIPDeviceControllerSystemState.h | 43 +++++++++++------- 3 files changed, 58 insertions(+), 39 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 09a0c732804336..f61e3f92170d1b 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -101,8 +101,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) if (mSystemState != nullptr) { - mSystemState->Release(); - chip::Platform::Delete(mSystemState); + Platform::Delete(mSystemState); mSystemState = nullptr; } @@ -154,14 +153,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) #endif )); - stateParams.sessionMgr = chip::Platform::New(); - SimpleSessionResumptionStorage * sessionResumptionStorage = chip::Platform::New(); - stateParams.sessionResumptionStorage = sessionResumptionStorage; - stateParams.certificateValidityPolicy = params.certificateValidityPolicy; - stateParams.unsolicitedStatusHandler = Platform::New(); - stateParams.exchangeMgr = chip::Platform::New(); - stateParams.messageCounterManager = chip::Platform::New(); - stateParams.groupDataProvider = params.groupDataProvider; + stateParams.sessionMgr = chip::Platform::New(); + stateParams.certificateValidityPolicy = params.certificateValidityPolicy; + stateParams.unsolicitedStatusHandler = Platform::New(); + stateParams.exchangeMgr = chip::Platform::New(); + stateParams.messageCounterManager = chip::Platform::New(); + stateParams.groupDataProvider = params.groupDataProvider; + + // This is constructed with a base class deleter so we can std::move it into + // stateParams without a manual conversion below. + auto sessionResumptionStorage = + std::unique_ptr>( + Platform::New()); // if no fabricTable was provided, create one and track it in stateParams for cleanup FabricTable * tempFabricTable = nullptr; @@ -172,6 +175,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) ReturnErrorOnFailure(stateParams.fabricTable->Init(params.fabricIndependentStorage, params.operationalKeystore)); } ReturnErrorOnFailure(sessionResumptionStorage->Init(params.fabricIndependentStorage)); + stateParams.sessionResumptionStorage = std::move(sessionResumptionStorage); auto delegate = chip::Platform::MakeUnique(); ReturnErrorOnFailure(delegate->Init(stateParams.sessionMgr, stateParams.groupDataProvider)); @@ -198,7 +202,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) // Enable listening for session establishment messages. ReturnErrorOnFailure(stateParams.caseServer->ListenForSessionEstablishment( - stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, stateParams.sessionResumptionStorage, + stateParams.exchangeMgr, stateParams.sessionMgr, stateParams.fabricTable, stateParams.sessionResumptionStorage.get(), stateParams.certificateValidityPolicy, stateParams.groupDataProvider)); // @@ -232,7 +236,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) DeviceProxyInitParams deviceInitParams = { .sessionManager = stateParams.sessionMgr, - .sessionResumptionStorage = stateParams.sessionResumptionStorage, + .sessionResumptionStorage = stateParams.sessionResumptionStorage.get(), .exchangeMgr = stateParams.exchangeMgr, .fabricTable = stateParams.fabricTable, .clientPool = stateParams.caseClientPool, @@ -250,7 +254,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) ReturnErrorOnFailure(stateParams.caseSessionManager->Init(stateParams.systemLayer, sessionManagerConfig)); // store the system state - mSystemState = chip::Platform::New(stateParams); + mSystemState = chip::Platform::New(std::move(stateParams)); mSystemState->SetTempFabricTable(tempFabricTable); ChipLogDetail(Controller, "System State Initialized..."); return CHIP_NO_ERROR; @@ -321,21 +325,20 @@ void DeviceControllerFactory::Shutdown() { if (mSystemState != nullptr) { - mSystemState->Release(); - chip::Platform::Delete(mSystemState); + Platform::Delete(mSystemState); mSystemState = nullptr; } mFabricIndependentStorage = nullptr; mOperationalKeystore = nullptr; } -CHIP_ERROR DeviceControllerSystemState::Shutdown() +void DeviceControllerSystemState::Shutdown() { - VerifyOrReturnError(mRefCount <= 1, CHIP_ERROR_INCORRECT_STATE); + VerifyOrDie(mRefCount == 0); if (mHaveShutDown) { // Nothing else to do here. - return CHIP_NO_ERROR; + return; } mHaveShutDown = true; @@ -405,7 +408,8 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() // Consumers are expected to call PlaformMgr().StopEventLoopTask() before calling // DeviceController::Shutdown() in the CONFIG_DEVICE_LAYER configuration // - ReturnErrorOnFailure(DeviceLayer::PlatformMgr().Shutdown()); + CHIP_ERROR error = DeviceLayer::PlatformMgr().Shutdown(); + VerifyOrDie(error == CHIP_NO_ERROR); #endif if (mExchangeMgr != nullptr) @@ -459,8 +463,6 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() // so that SetupController/Commissioner can use it mFabrics = nullptr; } - - return CHIP_NO_ERROR; } } // namespace Controller diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 5acbd03fb1193b..b760d574272787 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -116,7 +116,12 @@ class DeviceControllerFactory } CHIP_ERROR Init(FactoryInitParams params); + + // Shuts down matter and frees the system state. + // + // Must not be called while any controllers are alive. void Shutdown(); + CHIP_ERROR SetupController(SetupParams params, DeviceController & controller); CHIP_ERROR SetupCommissioner(SetupParams params, DeviceCommissioner & commissioner); @@ -135,8 +140,9 @@ class DeviceControllerFactory // // Some clients do not prefer a complete shutdown of the stack being initiated if // all device controllers have ceased to exist. To avoid that, this method has been - // created to permit retention of the underlying system state to avoid that. + // created to permit retention of the underlying system state. // + // NB: The system state will still be freed in Shutdown() regardless of this call. void RetainSystemState() { (void) mSystemState->Retain(); } // @@ -212,7 +218,7 @@ class DeviceControllerFactory }; private: - DeviceControllerFactory(){}; + DeviceControllerFactory() {} void PopulateInitParams(ControllerInitParams & controllerParams, const SetupParams & params); CHIP_ERROR InitSystemState(FactoryInitParams params); CHIP_ERROR InitSystemState(); diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index 390339f7fc33ed..28c80b888e75b3 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -83,8 +83,8 @@ struct DeviceControllerSystemStateParams // Params that will be deallocated via Platform::Delete in // DeviceControllerSystemState::Shutdown. - DeviceTransportMgr * transportMgr = nullptr; - SessionResumptionStorage * sessionResumptionStorage = nullptr; + DeviceTransportMgr * transportMgr = nullptr; + Platform::UniquePtr sessionResumptionStorage; Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr; SessionManager * sessionMgr = nullptr; Protocols::SecureChannel::UnsolicitedStatusHandler * unsolicitedStatusHandler = nullptr; @@ -97,9 +97,13 @@ struct DeviceControllerSystemStateParams FabricTable::Delegate * fabricTableDelegate = nullptr; }; -// A representation of the internal state maintained by the DeviceControllerFactory -// and refcounted by Device Controllers. -// Expects that the creator of this object is the last one to release it. +// A representation of the internal state maintained by the DeviceControllerFactory. +// +// This class automatically maintains a count of active device controllers and +// shuts down Matter when there are none remaining. +// +// NB: Lifetime of the object itself is not managed by reference counting; it is +// owned by DeviceControllerFactory. class DeviceControllerSystemState { using OperationalDevicePool = DeviceControllerSystemStateParams::OperationalDevicePool; @@ -113,6 +117,7 @@ class DeviceControllerSystemState // above 1. In that case we need to make sure we call Shutdown(). Shutdown(); }; + DeviceControllerSystemState(DeviceControllerSystemStateParams params) : mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager), mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr), @@ -120,13 +125,18 @@ class DeviceControllerSystemState mMessageCounterManager(params.messageCounterManager), mFabrics(params.fabricTable), mCASEServer(params.caseServer), mCASESessionManager(params.caseSessionManager), mOperationalDevicePool(params.operationalDevicePool), mCASEClientPool(params.caseClientPool), mGroupDataProvider(params.groupDataProvider), - mFabricTableDelegate(params.fabricTableDelegate) + mFabricTableDelegate(params.fabricTableDelegate), mSessionResumptionStorage(std::move(params.sessionResumptionStorage)) { #if CONFIG_NETWORK_LAYER_BLE mBleLayer = params.bleLayer; #endif + VerifyOrDie(IsInitialized()); }; + // Acquires a reference to the system state. + // + // While a reference is held, the shared state is kept alive. Release() + // should be called to release the reference once it is no longer needed. DeviceControllerSystemState * Retain() { VerifyOrDie(mRefCount < std::numeric_limits::max()); @@ -134,20 +144,20 @@ class DeviceControllerSystemState return this; }; + // Releases a reference to the system state. + // + // The stack will shut down when all references are released. + // + // NB: The system state is owned by the factory; Relase() will not free it + // but will free its members (Shutdown()). void Release() { VerifyOrDie(mRefCount > 0); - mRefCount--; - if (mRefCount == 1) + if (--mRefCount == 0) { - // Only the factory should have a ref now, shutdown and release the underlying objects Shutdown(); } - else if (mRefCount == 0) - { - this->~DeviceControllerSystemState(); - } }; bool IsInitialized() { @@ -173,7 +183,7 @@ class DeviceControllerSystemState void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; } private: - DeviceControllerSystemState(){}; + DeviceControllerSystemState() {} System::Layer * mSystemLayer = nullptr; Inet::EndPointManager * mTCPEndPointManager = nullptr; @@ -193,17 +203,18 @@ class DeviceControllerSystemState CASEClientPool * mCASEClientPool = nullptr; Credentials::GroupDataProvider * mGroupDataProvider = nullptr; FabricTable::Delegate * mFabricTableDelegate = nullptr; + Platform::UniquePtr mSessionResumptionStorage; // If mTempFabricTable is not null, it was created during // DeviceControllerFactory::InitSystemState and needs to be // freed during shutdown FabricTable * mTempFabricTable = nullptr; - std::atomic mRefCount{ 1 }; + std::atomic mRefCount{ 0 }; bool mHaveShutDown = false; - CHIP_ERROR Shutdown(); + void Shutdown(); }; } // namespace Controller