Skip to content

Commit

Permalink
Fix CHIPDeviceControllerFactory shutdown (#19022)
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 authored Jun 23, 2022
1 parent 5398d78 commit 20e65bf
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 39 deletions.
44 changes: 23 additions & 21 deletions src/controller/CHIPDeviceControllerFactory.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,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 @@ -154,14 +153,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 @@ -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<ControllerFabricDelegate>();
ReturnErrorOnFailure(delegate->Init(stateParams.sessionMgr, stateParams.groupDataProvider));
Expand All @@ -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));

//
Expand Down Expand Up @@ -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,
Expand All @@ -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<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 @@ -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;

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

return CHIP_NO_ERROR;
}

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

Expand All @@ -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(); }

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

private:
DeviceControllerFactory(){};
DeviceControllerFactory() {}
void PopulateInitParams(ControllerInitParams & controllerParams, const SetupParams & params);
CHIP_ERROR InitSystemState(FactoryInitParams params);
CHIP_ERROR InitSystemState();
Expand Down
43 changes: 27 additions & 16 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 @@ -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;
Expand All @@ -113,41 +117,47 @@ 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),
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());
};

// 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<uint32_t>::max());
++mRefCount;
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()
{
Expand All @@ -173,7 +183,7 @@ class DeviceControllerSystemState
void SetTempFabricTable(FabricTable * tempFabricTable) { mTempFabricTable = tempFabricTable; }

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

System::Layer * mSystemLayer = nullptr;
Inet::EndPointManager<Inet::TCPEndPoint> * mTCPEndPointManager = nullptr;
Expand All @@ -193,17 +203,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 mHaveShutDown = false;

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

} // namespace Controller
Expand Down

0 comments on commit 20e65bf

Please sign in to comment.