From 7c5a9b50c1b5e49155660246cf4734dec38c9c9d 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()) --- src/controller/CHIPDeviceControllerFactory.cpp | 16 ++++++++++------ src/controller/CHIPDeviceControllerSystemState.h | 8 +++++--- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/controller/CHIPDeviceControllerFactory.cpp b/src/controller/CHIPDeviceControllerFactory.cpp index 1ed14c805406a8..b3e5a91eee5838 100644 --- a/src/controller/CHIPDeviceControllerFactory.cpp +++ b/src/controller/CHIPDeviceControllerFactory.cpp @@ -151,14 +151,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params) )); 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; + // 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; stateParams.fabricTable = params.fabricTable; @@ -168,6 +172,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 +199,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 +233,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 +251,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; @@ -318,7 +323,6 @@ void DeviceControllerFactory::Shutdown() if (mSystemState != nullptr) { mSystemState->Release(); - chip::Platform::Delete(mSystemState); mSystemState = nullptr; } mFabricIndependentStorage = nullptr; diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index e0c6c8723d3d50..12d96a6696ecc7 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -84,7 +84,7 @@ struct DeviceControllerSystemStateParams // Params that will be deallocated via Platform::Delete in // DeviceControllerSystemState::Shutdown. DeviceTransportMgr * transportMgr = nullptr; - SessionResumptionStorage * sessionResumptionStorage = nullptr; + Platform::UniquePtr sessionResumptionStorage; Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr; SessionManager * sessionMgr = nullptr; Protocols::SecureChannel::UnsolicitedStatusHandler * unsolicitedStatusHandler = nullptr; @@ -114,7 +114,8 @@ 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; @@ -140,7 +141,7 @@ class DeviceControllerSystemState } else if (mRefCount == 0) { - this->~DeviceControllerSystemState(); + Platform::Delete(this); } }; bool IsInitialized() @@ -187,6 +188,7 @@ 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