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())
project-chip · Jun 20, 2022 · 7c5a9b5 · 7c5a9b5
1 parent 98a1c6b
commit 7c5a9b5
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 9 deletions.
diff --git 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<SessionManager>();
-    SimpleSessionResumptionStorage * sessionResumptionStorage = chip::Platform::New<SimpleSessionResumptionStorage>();
-    stateParams.sessionResumptionStorage                      = sessionResumptionStorage;
     stateParams.certificateValidityPolicy                     = params.certificateValidityPolicy;
     stateParams.unsolicitedStatusHandler                      = Platform::New<Protocols::SecureChannel::UnsolicitedStatusHandler>();
     stateParams.exchangeMgr                                   = chip::Platform::New<Messaging::ExchangeManager>();
     stateParams.messageCounterManager                         = chip::Platform::New<secure_channel::MessageCounterManager>();
     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<SimpleSessionResumptionStorage, Platform::Deleter<chip::SessionResumptionStorage>>(
+            Platform::New<SimpleSessionResumptionStorage>());
+
     // 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<ControllerFabricDelegate>();
     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<DeviceControllerSystemState>(stateParams);
+    mSystemState = chip::Platform::New<DeviceControllerSystemState>(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
@@ -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> 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<SessionResumptionStorage> mSessionResumptionStorage;
 
     // If mTempFabricTable is not null, it was created during
     // DeviceControllerFactory::InitSystemState and needs to be