Release ObjectPool objects in Session management (#12906)

#### Problem

Fixes #12505 ObjectPool leaks in Session management

#### Change overview

- Call `ReleaseAll()` on the various object pools in their
  owners' destructors.
- Fix `TestSessionManager` to call `SessionManager::Shutdown()`
- Remove the now-unused `OnObjectPoolDestruction` option from
  object pools. Destroying a pool with live objects will now
  fail unconditionally.

#### Testing

CI; fixed unit tests.

src/app/CASEClientPool.h

@@ -36,12 +36,14 @@ template <size_t N>
 class CASEClientPool : public CASEClientPoolDelegate
 {
 public:
+ ~CASEClientPool() { mClientPool.ReleaseAll(); }
+
  CASEClient * Allocate(CASEClientInitParams params) override { return mClientPool.CreateObject(params); }
 
  void Release(CASEClient * client) override { mClientPool.ReleaseObject(client); }
 
 private:
- BitMapObjectPool<CASEClient, N, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mClientPool;
+ BitMapObjectPool<CASEClient, N> mClientPool;
 };
 
 }; // namespace chip

src/app/OperationalDeviceProxyPool.h

@@ -44,6 +44,8 @@ template <size_t N>
 class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate
 {
 public:
+ ~OperationalDeviceProxyPool() { mDevicePool.ReleaseAll(); }
+
  OperationalDeviceProxy * Allocate(DeviceProxyInitParams & params, PeerId peerId) override
  {
  return mDevicePool.CreateObject(params, peerId);
@@ -88,7 +90,7 @@ class OperationalDeviceProxyPool : public OperationalDeviceProxyPoolDelegate
  }
 
 private:
- BitMapObjectPool<OperationalDeviceProxy, N, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mDevicePool;
+ BitMapObjectPool<OperationalDeviceProxy, N> mDevicePool;
 };
 
 }; // namespace chip

src/lib/dnssd/minimal_mdns/Server.h

@@ -228,8 +228,7 @@ class ServerBase
 
 // The PoolImpl impl is used as a base class because its destructor must be called after ServerBase's destructor.
 template <size_t kCount>
-class Server : private chip::PoolImpl<ServerBase::EndpointInfo, kCount, chip::OnObjectPoolDestruction::Die,
- ServerBase::EndpointInfoPoolType::Interface>,
+class Server : private chip::PoolImpl<ServerBase::EndpointInfo, kCount, ServerBase::EndpointInfoPoolType::Interface>,
  public ServerBase
 {
 public:

src/lib/dnssd/minimal_mdns/tests/CheckOnlyServer.h

@@ -71,8 +71,7 @@ void MakePrintableName(char (&location)[N], FullQName name)
 
 } // namespace
 
-class CheckOnlyServer : private chip::PoolImpl<ServerBase::EndpointInfo, 0, chip::OnObjectPoolDestruction::Die,
- ServerBase::EndpointInfoPoolType::Interface>,
+class CheckOnlyServer : private chip::PoolImpl<ServerBase::EndpointInfo, 0, ServerBase::EndpointInfoPoolType::Interface>,
  public ServerBase,
  public ParserDelegate,
  public TxtRecordDelegate

src/lib/support/Pool.h

@@ -162,16 +162,6 @@ 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.
- IgnoreUnsafeDoNotUseInNewCode, ///< Do nothing; keep historical behaviour until leaks are fixed.
-};
-
 /**
  * @class ObjectPool
  *
@@ -204,25 +194,12 @@ enum class OnObjectPoolDestruction
  * @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, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
+template <class T, size_t N>
 class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal::PoolCommon<T>
 {
 public:
  BitMapObjectPool() : StaticAllocatorBitmap(mData.mMemory, mUsage, N, sizeof(T)) {}
- ~BitMapObjectPool()
- {
- switch (Action)
- {
- case OnObjectPoolDestruction::AutoRelease:
- ReleaseAll();
- break;
- case OnObjectPoolDestruction::Die:
- VerifyOrDie(Allocated() == 0);
- break;
- case OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode:
- break;
- }
- }
+ ~BitMapObjectPool() { VerifyOrDie(Allocated() == 0); }
 
  template <typename... Args>
  T * CreateObject(Args &&... args)
@@ -289,25 +266,12 @@ class BitMapObjectPool : public internal::StaticAllocatorBitmap, public internal
  *
  * @tparam T type to be allocated.
  */
-template <class T, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
+template <class T>
 class HeapObjectPool : public internal::Statistics, public internal::PoolCommon<T>
 {
 public:
  HeapObjectPool() {}
- ~HeapObjectPool()
- {
- switch (Action)
- {
- case OnObjectPoolDestruction::AutoRelease:
- ReleaseAll();
- break;
- case OnObjectPoolDestruction::Die:
- VerifyOrDie(Allocated() == 0);
- break;
- case OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode:
- break;
- }
- }
+ ~HeapObjectPool() { VerifyOrDie(Allocated() == 0); }
 
  template <typename... Args>
  T * CreateObject(Args &&... args)
@@ -373,11 +337,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, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
-using ObjectPool = HeapObjectPool<T, Action>;
+template <typename T, unsigned int N>
+using ObjectPool = HeapObjectPool<T>;
 #else // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
-template <typename T, unsigned int N, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
-using ObjectPool = BitMapObjectPool<T, N, Action>;
+template <typename T, unsigned int N>
+using ObjectPool = BitMapObjectPool<T, N>;
 #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
 
 enum class ObjectPoolMem
@@ -388,17 +352,17 @@ enum class ObjectPoolMem
 #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
 };
 
-template <typename T, size_t N, ObjectPoolMem P, OnObjectPoolDestruction Action = OnObjectPoolDestruction::Die>
+template <typename T, size_t N, ObjectPoolMem P>
 class MemTypeObjectPool;
 
-template <typename T, size_t N, OnObjectPoolDestruction Action>
-class MemTypeObjectPool<T, N, ObjectPoolMem::kStatic, Action> : public BitMapObjectPool<T, N, Action>
+template <typename T, size_t N>
+class MemTypeObjectPool<T, N, ObjectPoolMem::kStatic> : public BitMapObjectPool<T, N>
 {
 };
 
 #if CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
-template <typename T, size_t N, OnObjectPoolDestruction Action>
-class MemTypeObjectPool<T, N, ObjectPoolMem::kDynamic, Action> : public HeapObjectPool<T, Action>
+template <typename T, size_t N>
+class MemTypeObjectPool<T, N, ObjectPoolMem::kDynamic> : public HeapObjectPool<T>
 {
 };
 #endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP

src/lib/support/PoolWrapper.h

@@ -35,6 +35,7 @@ class PoolInterface
 
  virtual U * CreateObject(ConstructorArguments... args) = 0;
  virtual void ReleaseObject(U * element) = 0;
+ virtual void ReleaseAll() = 0;
  virtual void ResetObject(U * element, ConstructorArguments... args) = 0;
 
  template <typename Function>
@@ -52,11 +53,11 @@ class PoolInterface
  virtual Loop ForEachActiveObjectInner(void * context, Lambda lambda) = 0;
 };
 
-template <class T, size_t N, OnObjectPoolDestruction A, typename Interface>
+template <class T, size_t N, typename Interface>
 class PoolProxy;
 
-template <class T, size_t N, OnObjectPoolDestruction A, typename U, typename... ConstructorArguments>
-class PoolProxy<T, N, A, std::tuple<U, ConstructorArguments...>> : public PoolInterface<U, ConstructorArguments...>
+template <class T, size_t N, typename U, typename... ConstructorArguments>
+class PoolProxy<T, N, std::tuple<U, ConstructorArguments...>> : public PoolInterface<U, ConstructorArguments...>
 {
 public:
  static_assert(std::is_base_of<U, T>::value, "Interface type is not derived from Pool type");
@@ -68,6 +69,8 @@ class PoolProxy<T, N, A, std::tuple<U, ConstructorArguments...>> : public PoolIn
 
  virtual void ReleaseObject(U * element) override { Impl().ReleaseObject(static_cast<T *>(element)); }
 
+ virtual void ReleaseAll() override { Impl().ReleaseAll(); }
+
  virtual void ResetObject(U * element, ConstructorArguments... args) override
  {
  return Impl().ResetObject(static_cast<T *>(element), std::move(args)...);
@@ -80,7 +83,7 @@ class PoolProxy<T, N, A, std::tuple<U, ConstructorArguments...>> : public PoolIn
  return Impl().ForEachActiveObject([&](T * target) { return lambda(context, static_cast<U *>(target)); });
  }
 
- virtual BitMapObjectPool<T, N, A> & Impl() = 0;
+ virtual BitMapObjectPool<T, N> & Impl() = 0;
 };
 
 /*
@@ -94,18 +97,18 @@ class PoolProxy<T, N, A, std::tuple<U, ConstructorArguments...>> : public PoolIn
  * PoolInterface<U, ConstructorArguments...>, the PoolImpl can be converted to the interface type
  * and passed around
  */
-template <class T, size_t N, OnObjectPoolDestruction A, typename... Interfaces>
-class PoolImpl : public PoolProxy<T, N, A, Interfaces>...
+template <class T, size_t N, typename... Interfaces>
+class PoolImpl : public PoolProxy<T, N, Interfaces>...
 {
 public:
  PoolImpl() {}
  virtual ~PoolImpl() override {}
 
 protected:
- virtual BitMapObjectPool<T, N, A> & Impl() override { return mImpl; }
+ virtual BitMapObjectPool<T, N> & Impl() override { return mImpl; }
 
 private:
- BitMapObjectPool<T, N, A> mImpl;
+ BitMapObjectPool<T, N> mImpl;
 };
 
 } // namespace chip

src/transport/SecureSessionTable.h

@@ -41,6 +41,8 @@ template <size_t kMaxSessionCount, Time::Source kTimeSource = Time::Source::kSys
 class SecureSessionTable
 {
 public:
+ ~SecureSessionTable() { mEntries.ReleaseAll(); }
+
  /**
  * Allocates a new secure session out of the internal resource pool.
  *
@@ -124,7 +126,7 @@ class SecureSessionTable
 
 private:
  Time::TimeSource<kTimeSource> mTimeSource;
- BitMapObjectPool<SecureSession, kMaxSessionCount, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mEntries;
+ BitMapObjectPool<SecureSession, kMaxSessionCount> mEntries;
 };
 
 } // namespace Transport

src/transport/SessionManager.cpp

@@ -95,6 +95,9 @@ void SessionManager::Shutdown()
 {
  CancelExpiryTimer();
 
+ mSessionReleaseDelegates.ReleaseAll();
+ mSessionRecoveryDelegates.ReleaseAll();
+
  mMessageCounterManager = nullptr;
 
  mState = State::kNotReady;

src/transport/SessionManager.h

@@ -272,12 +272,10 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate
  // TODO: This is a temporary solution to release sessions, in the near future, SessionReleaseDelegate will be
  // directly associated with the every SessionHolder. 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,
- OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
+ BitMapObjectPool<std::reference_wrapper<SessionReleaseDelegate>, CHIP_CONFIG_MAX_SESSION_RELEASE_DELEGATES>
  mSessionReleaseDelegates;
 
- BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES,
- OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
+ BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
  mSessionRecoveryDelegates;
 
  TransportMgrBase * mTransportMgr = nullptr;

src/transport/UnauthenticatedSessionTable.h

@@ -88,6 +88,8 @@ template <size_t kMaxSessionCount, Time::Source kTimeSource = Time::Source::kSys
 class UnauthenticatedSessionTable
 {
 public:
+ ~UnauthenticatedSessionTable() { mEntries.ReleaseAll(); }
+
  /**
  * Get a session given the peer address. If the session doesn't exist in the cache, allocate a new entry for it.
  *
@@ -223,7 +225,7 @@ class UnauthenticatedSessionTable
  }
 
  Time::TimeSource<Time::Source::kSystem> mTimeSource;
- BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mEntries;
+ BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount> mEntries;
 };
 
 } // namespace Transport

src/transport/raw/TCP.h

@@ -276,13 +276,12 @@ class TCP : public TCPBase
  mConnectionsBuffer[i].Init(nullptr);
  }
  }
+ ~TCP() { mPendingPackets.ReleaseAll(); }
 
 private:
  friend class TCPTest;
  TCPBase::ActiveConnectionState mConnectionsBuffer[kActiveConnectionsSize];
- PoolImpl<PendingPacket, kPendingPacketSize, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode,
- PendingPacketPoolType::Interface>
- mPendingPackets;
+ PoolImpl<PendingPacket, kPendingPacketSize, PendingPacketPoolType::Interface> mPendingPackets;
 };
 
 } // namespace Transport

src/transport/tests/TestSessionManager.cpp

@@ -202,6 +202,8 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext)
 
  err = sessionManager.PrepareMessage(localToRemoteSession, payloadHeader, std::move(extra_large_buffer), preparedMessage);
  NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_MESSAGE_TOO_LONG);
+
+ sessionManager.Shutdown();
 }
 
 void SendEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
@@ -278,6 +280,8 @@ void SendEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
  NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
  NL_TEST_ASSERT(inSuite, callback.ReceiveHandlerCallCount == 2);
+
+ sessionManager.Shutdown();
 }
 
 void SendBadEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
@@ -392,6 +396,8 @@ void SendBadEncryptedPacketTest(nlTestSuite * inSuite, void * inContext)
  NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
  NL_TEST_ASSERT(inSuite, callback.ReceiveHandlerCallCount == 2);
+
+ sessionManager.Shutdown();
 }
 
 void StaleConnectionDropTest(nlTestSuite * inSuite, void * inContext)
@@ -459,6 +465,8 @@ void StaleConnectionDropTest(nlTestSuite * inSuite, void * inContext)
  CryptoContext::SessionRole::kResponder, 0);
  NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
  NL_TEST_ASSERT(inSuite, callback.mOldConnectionDropped);
+
+ sessionManager.Shutdown();
 }
 
 // Test Suite