Skip to content

Commit

Permalink
Fix CHIPDeviceControllerFactory shutdown
Browse files Browse the repository at this point in the history 
- 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())
  • Loading branch information
@mspang
mspang committed Jun 21, 2022
1 parent 98a1c6b commit 56d3d04
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 36 deletions.
43 changes: 23 additions & 20 deletions src/controller/CHIPDeviceControllerFactory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,7 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)

if (mSystemState != nullptr)
{
mSystemState->Release();
chip::Platform::Delete(mSystemState);
Platform::Delete(mSystemState);
mSystemState = nullptr;
}

Expand Down Expand Up @@ -150,14 +149,18 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
#endif
));

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;
stateParams.sessionMgr = chip::Platform::New<SessionManager>();
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;
Expand All @@ -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<ControllerFabricDelegate>();
ReturnErrorOnFailure(delegate->Init(stateParams.sessionMgr, stateParams.groupDataProvider));
Expand All @@ -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));

//
Expand Down Expand Up @@ -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,
Expand All @@ -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<DeviceControllerSystemState>(stateParams);
mSystemState = chip::Platform::New<DeviceControllerSystemState>(std::move(stateParams));
mSystemState->SetTempFabricTable(tempFabricTable);
ChipLogDetail(Controller, "System State Initialized...");
return CHIP_NO_ERROR;
Expand Down Expand Up @@ -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");

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -447,8 +452,6 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
// so that SetupController/Commissioner can use it
mFabrics = nullptr;
}

return CHIP_NO_ERROR;
}

} // namespace Controller
Expand Down
5 changes: 3 additions & 2 deletions src/controller/CHIPDeviceControllerFactory.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }

//
Expand Down Expand Up @@ -212,7 +213,7 @@ class DeviceControllerFactory
};

private:
DeviceControllerFactory(){};
DeviceControllerFactory() {}
void PopulateInitParams(ControllerInitParams & controllerParams, const SetupParams & params);
CHIP_ERROR InitSystemState(FactoryInitParams params);
CHIP_ERROR InitSystemState();
Expand Down
32 changes: 18 additions & 14 deletions src/controller/CHIPDeviceControllerSystemState.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> sessionResumptionStorage;
Credentials::CertificateValidityPolicy * certificateValidityPolicy = nullptr;
SessionManager * sessionMgr = nullptr;
Protocols::SecureChannel::UnsolicitedStatusHandler * unsolicitedStatusHandler = nullptr;
Expand All @@ -106,19 +106,25 @@ 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),
mUnsolicitedStatusHandler(params.unsolicitedStatusHandler), mExchangeMgr(params.exchangeMgr),
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()
Expand All @@ -132,15 +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();
mWasShutdown = true;
}
};
bool IsInitialized()
Expand All @@ -167,7 +168,7 @@ class DeviceControllerSystemState
void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; }

private:
DeviceControllerSystemState(){};
DeviceControllerSystemState() {}

System::Layer * mSystemLayer = nullptr;
Inet::EndPointManager<Inet::TCPEndPoint> * mTCPEndPointManager = nullptr;
Expand All @@ -187,15 +188,18 @@ 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
// freed during shutdown
FabricTable * mTempFabricTable = nullptr;

std::atomic<uint32_t> mRefCount{ 1 };
std::atomic<uint32_t> mRefCount{ 0 };

bool mWasShutdown = false;

CHIP_ERROR Shutdown();
void Shutdown();
};

} // namespace Controller
Expand Down

0 comments on commit 56d3d04

Please sign in to comment.