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

- Adds a template argument to choose between two options when an
  `ObjectPool` is destroyed with live objects: to either release
  all objects (which calls their destructors), or to die with an
  error message.
- For existing cases in `SessionManager`, free leftover objects.
- Fix shutdown ordering in `DeviceControllerSystemState`.

#### Testing

CI; no changes to functionality.
  • Loading branch information
kpschoedel committed Nov 30, 2021
1 parent fe681d9 commit 87f9d78
Show file tree
Hide file tree
Showing 6 changed files with 56 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
52 changes: 38 additions & 14 deletions src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,15 @@ 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.
};

/**
* @class ObjectPool
*
Expand Down Expand Up @@ -192,14 +201,22 @@ 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();
if (Action == OnObjectPoolDestruction::AutoRelease)
{
ReleaseAll();
}
else
{
ForEachActiveObject([&](void * object) -> bool { ChipLogError(Support, "Leaked %p from %p", object, this); return true; });
VerifyOrDie(Allocated() == 0);
}
}

template <typename... Args>
Expand Down Expand Up @@ -265,15 +282,22 @@ 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
{
ForEachActiveObject([&](void * object) -> bool { ChipLogError(Support, "Leaked %p from %p", object, this); return true; });
VerifyOrDie(Allocated() == 0);
}
}

template <typename... Args>
Expand Down Expand Up @@ -339,11 +363,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 @@ -354,17 +378,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 @@ -135,6 +135,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 @@ -324,6 +326,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::AutoRelease> mEntries;
};

} // namespace Transport
Expand Down
6 changes: 3 additions & 3 deletions src/transport/SessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -286,16 +286,16 @@ 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::AutoRelease>
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::AutoRelease>
mSessionReleaseDelegates;

BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES, OnObjectPoolDestruction::AutoRelease>
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::AutoRelease> mEntries;
};

} // namespace Transport
Expand Down

0 comments on commit 87f9d78

Please sign in to comment.