Skip to content

Commit

Permalink
Catch or release ObjectPool leaks
Browse files Browse the repository at this point in the history
#### Problem

No action is taken if an `ObjectPool` is destroyed while objects
remain live. For the `BitMapObjectPool` case, such objects can't
be safely touched. For `HeapObjectPool` such objects can be used
but can't be released.

Some leaks have been fixed (project-chip#11983, project-chip#12031), but there are still
a few remaining.

Fixes project-chip#11880 _Possible use of destroyed pool objects_

#### Change overview

- Add a `BitMapObjectPool` template argument indicating what to
  do if it is destroyed with live objects: abort  with an error
  message, release all objects (which calls their destructors),
  or (transitionally) ignore the condition.
- Fix shutdown ordering in `DeviceControllerSystemState`.
- For existing pools in `SessionManager`, ignore the condition.

#### Testing

CI; no changes to functionality.
  • Loading branch information
kpschoedel committed Dec 1, 2021
1 parent 31156c4 commit e5e5352
Show file tree
Hide file tree
Showing 6 changed files with 61 additions and 24 deletions.
14 changes: 9 additions & 5 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,15 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
// Shut down the interaction model
app::InteractionModelEngine::GetInstance()->Shutdown();

// Shut down the TransportMgr. This holds Inet::UDPEndPoints so it must be shut down
// before PlatformMgr().Shutdown() shuts down Inet.
if (mTransportMgr != nullptr)
{
mTransportMgr->Close();
chip::Platform::Delete(mTransportMgr);
mTransportMgr = nullptr;
}

#if CONFIG_DEVICE_LAYER
//
// We can safely call PlatformMgr().Shutdown(), which like DeviceController::Shutdown(),
Expand All @@ -247,11 +256,6 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()

mSystemLayer = nullptr;
mInetLayer = nullptr;
if (mTransportMgr != nullptr)
{
chip::Platform::Delete(mTransportMgr);
mTransportMgr = nullptr;
}

if (mMessageCounterManager != nullptr)
{
Expand Down
54 changes: 40 additions & 14 deletions src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,16 @@ struct HeapObjectList : HeapObjectListNode

} // namespace internal

/**
* Action taken if objects remain allocated when a pool is destroyed.
*/
enum class OnObjectPoolDestruction
{
AutoRelease, ///< Release any objects still allocated.
Die, ///< Abort if any objects remain allocated.
UnsafeDoNotUse, ///< Do nothing; keep historical behaviour until leaks are fixed.
};

/**
* @class ObjectPool
*
Expand Down Expand Up @@ -191,14 +201,24 @@ struct HeapObjectList : HeapObjectListNode
* @tparam T type of element to be allocated.
* @tparam N a positive integer max number of elements the pool provides.
*/
template <class T, size_t N>
template <class T, size_t N, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal::PoolCommon<T>
{
public:
BitMapObjectPool() : StaticAllocatorBitmap(mData.mMemory, mUsage, N, sizeof(T)) {}
~BitMapObjectPool()
{
// ReleaseAll();
switch (Action)
{
case OnObjectPoolDestruction::AutoRelease:
ReleaseAll();
break;
case OnObjectPoolDestruction::Die:
VerifyOrDie(Allocated() == 0);
break;
case OnObjectPoolDestruction::UnsafeDoNotUse:
break;
}
}

template <typename... Args>
Expand Down Expand Up @@ -264,15 +284,21 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal
*
* @tparam T type to be allocated.
*/
template <class T>
template <class T, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
class HeapObjectPool : public internal::Statistics, public internal::PoolCommon<T>
{
public:
HeapObjectPool() {}
~HeapObjectPool()
{
// TODO(#11880): Release all active objects (or verify that none are active) when destroying the pool.
// ReleaseAll();
if (Action == OnObjectPoolDestruction::AutoRelease)
{
ReleaseAll();
}
else
{
VerifyOrDie(Allocated() == 0);
}
}

template <typename... Args>
Expand Down Expand Up @@ -338,11 +364,11 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon<
#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP

#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
template <typename T, unsigned int N>
using ObjectPool = HeapObjectPool<T>;
template <typename T, unsigned int N, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
using ObjectPool = HeapObjectPool<T, Action>;
#else // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
template <typename T, unsigned int N>
using ObjectPool = BitMapObjectPool<T, N>;
template <typename T, unsigned int N, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
using ObjectPool = BitMapObjectPool<T, N, Action>;
#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP

enum class ObjectPoolMem
Expand All @@ -353,17 +379,17 @@ enum class ObjectPoolMem
#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
};

template <typename T, size_t N, ObjectPoolMem P>
template <typename T, size_t N, ObjectPoolMem P, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
class MemTypeObjectPool;

template <typename T, size_t N>
class MemTypeObjectPool<T, N, ObjectPoolMem::kStatic> : public BitMapObjectPool<T, N>
template <typename T, size_t N, OnObjectPoolDestruction Action>
class MemTypeObjectPool<T, N, ObjectPoolMem::kStatic, Action> : public BitMapObjectPool<T, N, Action>
{
};

#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
template <typename T, size_t N>
class MemTypeObjectPool<T, N, ObjectPoolMem::kDynamic> : public HeapObjectPool<T>
template <typename T, size_t N, OnObjectPoolDestruction Action>
class MemTypeObjectPool<T, N, ObjectPoolMem::kDynamic, Action> : public HeapObjectPool<T, Action>
{
};
#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
Expand Down
4 changes: 4 additions & 0 deletions src/lib/support/tests/TestPool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ void TestCreateReleaseObjectStatic(nlTestSuite * inSuite, void * inContext)
NL_TEST_ASSERT(inSuite, GetNumObjectsInUse(pool) == kSize);
NL_TEST_ASSERT(inSuite, pool.Allocated() == kSize);
NL_TEST_ASSERT(inSuite, pool.Exhausted());

pool.ReleaseAll();
}

#if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
Expand Down Expand Up @@ -325,6 +327,8 @@ void TestForEachActiveObject(nlTestSuite * inSuite, void * inContext)
}
NL_TEST_ASSERT(inSuite, count >= kSize / 2);
NL_TEST_ASSERT(inSuite, count <= kSize);

pool.ReleaseAll();
}

void TestForEachActiveObjectStatic(nlTestSuite * inSuite, void * inContext)
Expand Down
2 changes: 1 addition & 1 deletion src/transport/SecureSessionTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ class SecureSessionTable

private:
Time::TimeSource<kTimeSource> mTimeSource;
BitMapObjectPool<SecureSession, kMaxSessionCount> mEntries;
BitMapObjectPool<SecureSession, kMaxSessionCount, OnObjectPoolDestruction::UnsafeDoNotUse> mEntries;
};

} // namespace Transport
Expand Down
9 changes: 6 additions & 3 deletions src/transport/SessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -286,16 +286,19 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
State mState; // < Initialization state of the object

SessionMessageDelegate * mCB = nullptr;
BitMapObjectPool<std::reference_wrapper<SessionCreationDelegate>, CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES>
BitMapObjectPool<std::reference_wrapper<SessionCreationDelegate>, CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES,
OnObjectPoolDestruction::UnsafeDoNotUse>
mSessionCreationDelegates;

// TODO: This is a temporary solution to release sessions, in the near future, SessionReleaseDelegate will be
// directly associated with the every SessionHandle. Then the callback function is called on over the handle
// delegate directly, in order to prevent dangling handles.
BitMapObjectPool<std::reference_wrapper<SessionReleaseDelegate>, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES>
BitMapObjectPool<std::reference_wrapper<SessionReleaseDelegate>, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES,
OnObjectPoolDestruction::UnsafeDoNotUse>
mSessionReleaseDelegates;

BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES,
OnObjectPoolDestruction::UnsafeDoNotUse>
mSessionRecoveryDelegates;

TransportMgrBase * mTransportMgr = nullptr;
Expand Down
2 changes: 1 addition & 1 deletion src/transport/UnauthenticatedSessionTable.h
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ class UnauthenticatedSessionTable
}

Time::TimeSource<Time::Source::kSystem> mTimeSource;
BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount> mEntries;
BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount, OnObjectPoolDestruction::UnsafeDoNotUse> mEntries;
};

} // namespace Transport
Expand Down

0 comments on commit e5e5352

Please sign in to comment.