Release ObjectPool objects in Session management

#### Problem

Fixes project-chip#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