From a849b6b5434f22ac888271d7911568f01cdf2c29 Mon Sep 17 00:00:00 2001 From: Kevin Schoedel Date: Mon, 22 Nov 2021 11:01:24 -0500 Subject: [PATCH] Catch or release ObjectPool leaks #### 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 (#11983, #12031), but there are still a few remaining. Fixes #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. **XXX file issue** #### Testing CI; no changes to functionality. --- src/lib/support/Pool.h | 52 +++++++++++++++------ src/lib/support/tests/TestPool.cpp | 4 ++ src/transport/SecureSessionTable.h | 2 +- src/transport/SessionManager.h | 4 +- src/transport/UnauthenticatedSessionTable.h | 2 +- 5 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/lib/support/Pool.h b/src/lib/support/Pool.h index 47df3efd9ef729..781833419d3dcb 100644 --- a/src/lib/support/Pool.h +++ b/src/lib/support/Pool.h @@ -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 * @@ -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 +template class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal::PoolCommon { 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 @@ -265,15 +282,22 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal * * @tparam T type to be allocated. */ -template +template class HeapObjectPool : public internal::Statistics, public internal::PoolCommon { 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 @@ -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 -using ObjectPool = HeapObjectPool; +template +using ObjectPool = HeapObjectPool; #else // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP -template -using ObjectPool = BitMapObjectPool; +template +using ObjectPool = BitMapObjectPool; #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP enum class ObjectPoolMem @@ -354,17 +378,17 @@ enum class ObjectPoolMem #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP }; -template +template class MemTypeObjectPool; -template -class MemTypeObjectPool : public BitMapObjectPool +template +class MemTypeObjectPool : public BitMapObjectPool { }; #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP -template -class MemTypeObjectPool : public HeapObjectPool +template +class MemTypeObjectPool : public HeapObjectPool { }; #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP diff --git a/src/lib/support/tests/TestPool.cpp b/src/lib/support/tests/TestPool.cpp index 613bb87d00d578..9aec0da01e5056 100644 --- a/src/lib/support/tests/TestPool.cpp +++ b/src/lib/support/tests/TestPool.cpp @@ -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 @@ -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) diff --git a/src/transport/SecureSessionTable.h b/src/transport/SecureSessionTable.h index 85a709bc81b027..2f6129633df977 100644 --- a/src/transport/SecureSessionTable.h +++ b/src/transport/SecureSessionTable.h @@ -120,7 +120,7 @@ class SecureSessionTable private: Time::TimeSource mTimeSource; - BitMapObjectPool mEntries; + BitMapObjectPool mEntries; }; } // namespace Transport diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 7112542593c784..dccbf34b4a1612 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -282,13 +282,13 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate State mState; // < Initialization state of the object SessionMessageDelegate * mCB = nullptr; - BitMapObjectPool, CHIP_CONFIG_MAX_SESSION_CREATION_DELEGATES> + BitMapObjectPool, 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, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES> + BitMapObjectPool, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES, OnObjectPoolDestruction::AutoRelease> mSessionReleaseDelegates; TransportMgrBase * mTransportMgr = nullptr; diff --git a/src/transport/UnauthenticatedSessionTable.h b/src/transport/UnauthenticatedSessionTable.h index c265d43fe249e7..3a83b5bd4f399d 100644 --- a/src/transport/UnauthenticatedSessionTable.h +++ b/src/transport/UnauthenticatedSessionTable.h @@ -223,7 +223,7 @@ class UnauthenticatedSessionTable } Time::TimeSource mTimeSource; - BitMapObjectPool mEntries; + BitMapObjectPool mEntries; }; } // namespace Transport