Catch or release ObjectPool leaks (#12499)

* 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

- Add an `ObjectPool` template argument specifying what to do if
  it is destroyed with live objects: abort with an error message
  (the default), release the live objects (which calls their
  destructors), or ignore the condition (transitionally, until
  all leaks are fixed).

- Fix shutdown ordering in `DeviceControllerSystemState`.

- For existing pools in `SessionManager`, `InteractionModelEngine`,
  and `GroupDataProviderImpl`, ignore the condition.

#### Testing

CI; no changes to functionality.

* new uses in #12417

src/app/CASESessionManager.h

@@ -103,7 +103,9 @@ class CASESessionManager : public SessionReleaseDelegate, public Dnssd::Resolver
     OperationalDeviceProxy * FindSession(SessionHandle session);
     void ReleaseSession(OperationalDeviceProxy * device);
 
-    BitMapObjectPool<OperationalDeviceProxy, CHIP_CONFIG_CONTROLLER_MAX_ACTIVE_DEVICES> mActiveSessions;
+    BitMapObjectPool<OperationalDeviceProxy, CHIP_CONFIG_CONTROLLER_MAX_ACTIVE_DEVICES,
+                     OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
+        mActiveSessions;
 
     CASESessionManagerConfig mConfig;
 };

src/app/InteractionModelEngine.h

@@ -281,7 +281,8 @@ class InteractionModelEngine : public Messaging::ExchangeDelegate, public Comman
     // TODO(#8006): investgate if we can disable some IM functions on some compact accessories.
     // TODO(#8006): investgate if we can provide more flexible object management on devices with more resources.
     BitMapObjectPool<CommandHandler, CHIP_IM_MAX_NUM_COMMAND_HANDLER> mCommandHandlerObjs;
-    BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> mTimedHandlers;
+    BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
+        mTimedHandlers;
     ReadClient mReadClients[CHIP_IM_MAX_NUM_READ_CLIENT];
     ReadHandler mReadHandlers[CHIP_IM_MAX_NUM_READ_HANDLER];
     WriteClient mWriteClients[CHIP_IM_MAX_NUM_WRITE_CLIENT];

src/controller/CHIPDeviceControllerFactory.cpp

@@ -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(),
@@ -247,11 +256,6 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()
 
     mSystemLayer = nullptr;
     mInetLayer   = nullptr;
-    if (mTransportMgr != nullptr)
-    {
-        chip::Platform::Delete(mTransportMgr);
-        mTransportMgr = nullptr;
-    }
 
     if (mMessageCounterManager != nullptr)
     {

src/credentials/GroupDataProviderImpl.h

@@ -138,7 +138,7 @@ class GroupDataProviderImpl : public GroupDataProvider
     BitMapObjectPool<GroupMappingIteratorImpl, kIteratorsMax> mEndpointIterators;
     BitMapObjectPool<AllStatesIterator, kIteratorsMax> mAllStatesIterators;
     BitMapObjectPool<FabricStatesIterator, kIteratorsMax> mFabricStatesIterators;
-    BitMapObjectPool<KeySetIteratorImpl, kIteratorsMax> mKeySetIterators;
+    BitMapObjectPool<KeySetIteratorImpl, kIteratorsMax, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mKeySetIterators;
 };
 
 } // namespace Credentials

src/lib/dnssd/minimal_mdns/Server.h

@@ -228,7 +228,8 @@ 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, ServerBase::EndpointInfoPoolType::Interface>,
+class Server : private chip::PoolImpl<ServerBase::EndpointInfo, kCount, chip::OnObjectPoolDestruction::Die,
+                                      ServerBase::EndpointInfoPoolType::Interface>,
                public ServerBase
 {
 public:

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

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

src/lib/support/Pool.h

@@ -22,6 +22,7 @@
 
 #pragma once
 
+#include <lib/support/CodeUtils.h>
 #include <system/SystemConfig.h>
 
 #include <lib/support/Iterators.h>
@@ -161,6 +162,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.
+    IgnoreUnsafeDoNotUseInNewCode, ///< Do nothing; keep historical behaviour until leaks are fixed.
+};
+
 /**
  * @class ObjectPool
  *
@@ -193,14 +204,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::IgnoreUnsafeDoNotUseInNewCode:
+            break;
+        }
     }
 
     template <typename... Args>
@@ -268,15 +289,24 @@ 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();
+        switch (Action)
+        {
+        case OnObjectPoolDestruction::AutoRelease:
+            ReleaseAll();
+            break;
+        case OnObjectPoolDestruction::Die:
+            VerifyOrDie(Allocated() == 0);
+            break;
+        case OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode:
+            break;
+        }
     }
 
     template <typename... Args>
@@ -343,11 +373,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
@@ -358,17 +388,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

src/lib/support/PoolWrapper.h

@@ -52,11 +52,11 @@ class PoolInterface
     virtual Loop ForEachActiveObjectInner(void * context, Lambda lambda) = 0;
 };
 
-template <class T, size_t N, typename Interface>
+template <class T, size_t N, OnObjectPoolDestruction A, typename Interface>
 class PoolProxy;
 
-template <class T, size_t N, typename U, typename... ConstructorArguments>
-class PoolProxy<T, N, std::tuple<U, ConstructorArguments...>> : public PoolInterface<U, ConstructorArguments...>
+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...>
 {
 public:
     static_assert(std::is_base_of<U, T>::value, "Interface type is not derived from Pool type");
@@ -80,7 +80,7 @@ class PoolProxy<T, N, std::tuple<U, ConstructorArguments...>> : public PoolInter
         return Impl().ForEachActiveObject([&](T * target) { return lambda(context, static_cast<U *>(target)); });
     }
 
-    virtual BitMapObjectPool<T, N> & Impl() = 0;
+    virtual BitMapObjectPool<T, N, A> & Impl() = 0;
 };
 
 /*
@@ -94,18 +94,18 @@ class PoolProxy<T, N, std::tuple<U, ConstructorArguments...>> : public PoolInter
  *                     PoolInterface<U, ConstructorArguments...>, the PoolImpl can be converted to the interface type
  *                     and passed around
  */
-template <class T, size_t N, typename... Interfaces>
-class PoolImpl : public PoolProxy<T, N, Interfaces>...
+template <class T, size_t N, OnObjectPoolDestruction A, typename... Interfaces>
+class PoolImpl : public PoolProxy<T, N, A, Interfaces>...
 {
 public:
     PoolImpl() {}
     virtual ~PoolImpl() override {}
 
 protected:
-    virtual BitMapObjectPool<T, N> & Impl() override { return mImpl; }
+    virtual BitMapObjectPool<T, N, A> & Impl() override { return mImpl; }
 
 private:
-    BitMapObjectPool<T, N> mImpl;
+    BitMapObjectPool<T, N, A> mImpl;
 };
 
 } // namespace chip

src/lib/support/tests/TestPool.cpp

@@ -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
@@ -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)

src/transport/SecureSessionTable.h

@@ -124,7 +124,7 @@ class SecureSessionTable
 
 private:
     Time::TimeSource<kTimeSource> mTimeSource;
-    BitMapObjectPool<SecureSession, kMaxSessionCount> mEntries;
+    BitMapObjectPool<SecureSession, kMaxSessionCount, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mEntries;
 };
 
 } // namespace Transport

src/transport/SessionManager.h

@@ -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::IgnoreUnsafeDoNotUseInNewCode>
         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::IgnoreUnsafeDoNotUseInNewCode>
         mSessionReleaseDelegates;
 
-    BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES>
+    BitMapObjectPool<std::reference_wrapper<SessionRecoveryDelegate>, CHIP_CONFIG_MAX_SESSION_RECOVERY_DELEGATES,
+                     OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
         mSessionRecoveryDelegates;
 
     TransportMgrBase * mTransportMgr                                   = nullptr;

src/transport/UnauthenticatedSessionTable.h

@@ -223,7 +223,7 @@ class UnauthenticatedSessionTable
     }
 
     Time::TimeSource<Time::Source::kSystem> mTimeSource;
-    BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount> mEntries;
+    BitMapObjectPool<UnauthenticatedSession, kMaxSessionCount, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode> mEntries;
 };
 
 } // namespace Transport

src/transport/raw/TCP.h

@@ -280,7 +280,9 @@ class TCP : public TCPBase
 private:
     friend class TCPTest;
     TCPBase::ActiveConnectionState mConnectionsBuffer[kActiveConnectionsSize];
-    PoolImpl<PendingPacket, kPendingPacketSize, PendingPacketPoolType::Interface> mPendingPackets;
+    PoolImpl<PendingPacket, kPendingPacketSize, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode,
+             PendingPacketPoolType::Interface>
+        mPendingPackets;
 };
 
 } // namespace Transport