From dd05faec16edc455aef3a49012acc7b0ad53372e Mon Sep 17 00:00:00 2001 From: Michael Spang Date: Mon, 20 Jun 2022 10:38:00 -0400 Subject: [PATCH] Fix CHIPDeviceControllerFactory shutdown - 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 | 43 +++++++++++-------- src/controller/CHIPDeviceControllerFactory.h | 5 ++- .../CHIPDeviceControllerSystemState.h | 31 +++++++------ 3 files changed, 44 insertions(+), 35 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 1ed14c805406a8..fec5b7d7543163 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -99,8 +99,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) if (mSystemState != nullptr) { - mSystemState->Release(); - chip::Platform::Delete(mSystemState); + Platform::Delete(mSystemState); mSystemState = nullptr; } @@ -150,14 +149,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; @@ -168,6 +171,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)); @@ -194,7 +198,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)); // @@ -228,7 +232,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, @@ -246,7 +250,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; @@ -317,17 +321,17 @@ 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); + VerifyOrDie(!mWasShutdown); ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack"); @@ -395,7 +399,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) @@ -448,7 +453,7 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown() mFabrics = nullptr; } - return CHIP_NO_ERROR; + mWasShutdown = true; } } // namespace Controller diff --git a/src/controller/CHIPDeviceControllerFactory.h b/src/controller/CHIPDeviceControllerFactory.h index 5acbd03fb1193b..99950b57e3e62d 100644 --- a/src/controller/CHIPDeviceControllerFactory.h +++ b/src/controller/CHIPDeviceControllerFactory.h @@ -135,8 +135,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 +213,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 e0c6c8723d3d50..1bf24b3756d08b 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; @@ -106,7 +106,12 @@ class DeviceControllerSystemState using CASEClientPool = DeviceControllerSystemStateParams::CASEClientPool; public: - ~DeviceControllerSystemState(){}; + ~DeviceControllerSystemState() + { + VerifyOrDie(mRefCount == 0); + VerifyOrDie(mWasShutdown); + } + DeviceControllerSystemState(DeviceControllerSystemStateParams params) : mSystemLayer(params.systemLayer), mTCPEndPointManager(params.tcpEndPointManager), mUDPEndPointManager(params.udpEndPointManager), mTransportMgr(params.transportMgr), mSessionMgr(params.sessionMgr), @@ -114,11 +119,12 @@ 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()); }; DeviceControllerSystemState * Retain() @@ -132,16 +138,10 @@ class DeviceControllerSystemState { 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() { @@ -167,7 +167,7 @@ class DeviceControllerSystemState void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; } private: - DeviceControllerSystemState(){}; + DeviceControllerSystemState() {} System::Layer * mSystemLayer = nullptr; Inet::EndPointManager * mTCPEndPointManager = nullptr; @@ -187,15 +187,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 mWasShutdown = false; - CHIP_ERROR Shutdown(); + void Shutdown(); }; } // namespace Controller