From 3592001bfdd70c094c2017691c366936696a8322 Mon Sep 17 00:00:00 2001
From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com>
Date: Wed, 8 Dec 2021 09:42:43 -0500
Subject: [PATCH] Fix ObjectPool leaks in InteractionModelEngine (#12702)

#### Problem

Fixes #12504 ObjectPool leaks in InteractionModelEngine

#### Change overview

Call `ReleaseAll()` in `InteractionModelEngine::Shutdown()`;
since the comments there indicate that was the intent.

#### Testing

CI; no changes to external functionality.
---
 src/app/InteractionModelEngine.cpp | 25 +++++--------------------
 src/app/InteractionModelEngine.h   |  3 +--
 src/lib/support/Pool.h             |  4 ++--
 3 files changed, 8 insertions(+), 24 deletions(-)

diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp
index ba3310fdbd215e..471ac33b2b115b 100644
--- a/src/app/InteractionModelEngine.cpp
+++ b/src/app/InteractionModelEngine.cpp
@@ -81,28 +81,13 @@ void InteractionModelEngine::Shutdown()
     // Increase magic number to invalidate all Handle-s.
     mMagic++;
 
-    mCommandHandlerObjs.ForEachActiveObject([this](CommandHandler * obj) -> Loop {
-        // Modifying the pool during iteration is generally frowned upon.
-        // This is almost safe since mCommandHandlerObjs is a BitMapObjectPool which won't malfunction when modifying the inner
-        // record while during traversal. But this behavior is not guranteed, so we should fix this by implementing DeallocateAll.
-        //
-        // Deallocate an CommandHandler will call its destructor (and abort the exchange context it holds) without calling
-        // Shutdown().
-        //
-        // TODO(@kghost, #10332) Implement DeallocateAll and replace this.
-
-        mCommandHandlerObjs.Deallocate(obj);
-        return Loop::Continue;
-    });
+    mCommandHandlerObjs.ReleaseAll();
 
     mTimedHandlers.ForEachActiveObject([this](TimedHandler * obj) -> Loop {
-        // This calls back into us and deallocates |obj|.  As above, this is not
-        // really guaranteed, and we should do something better here (like
-        // ignoring the calls to OnTimedInteractionFailed and then doing a
-        // DeallocateAll.
         mpExchangeMgr->CloseAllContextsForDelegate(obj);
         return Loop::Continue;
     });
+    mTimedHandlers.ReleaseAll();
 
     for (auto & readClient : mReadClients)
     {
@@ -702,7 +687,7 @@ CommandHandlerInterface * InteractionModelEngine::FindCommandHandler(EndpointId
 
 void InteractionModelEngine::OnTimedInteractionFailed(TimedHandler * apTimedHandler)
 {
-    mTimedHandlers.Deallocate(apTimedHandler);
+    mTimedHandlers.ReleaseObject(apTimedHandler);
 }
 
 void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messaging::ExchangeContext * apExchangeContext,
@@ -713,7 +698,7 @@ void InteractionModelEngine::OnTimedInvoke(TimedHandler * apTimedHandler, Messag
     // Reset the ourselves as the exchange delegate for now, to match what we'd
     // do with an initial unsolicited invoke.
     apExchangeContext->SetDelegate(this);
-    mTimedHandlers.Deallocate(apTimedHandler);
+    mTimedHandlers.ReleaseObject(apTimedHandler);
 
     VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::InvokeCommandRequest));
     VerifyOrDie(!apExchangeContext->IsGroupExchangeContext());
@@ -734,7 +719,7 @@ void InteractionModelEngine::OnTimedWrite(TimedHandler * apTimedHandler, Messagi
     // Reset the ourselves as the exchange delegate for now, to match what we'd
     // do with an initial unsolicited write.
     apExchangeContext->SetDelegate(this);
-    mTimedHandlers.Deallocate(apTimedHandler);
+    mTimedHandlers.ReleaseObject(apTimedHandler);
 
     VerifyOrDie(aPayloadHeader.HasMessageType(MsgType::WriteRequest));
     VerifyOrDie(!apExchangeContext->IsGroupExchangeContext());
diff --git a/src/app/InteractionModelEngine.h b/src/app/InteractionModelEngine.h
index fbfcb283e12688..a9ed2dd46e848a 100644
--- a/src/app/InteractionModelEngine.h
+++ b/src/app/InteractionModelEngine.h
@@ -283,8 +283,7 @@ 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, OnObjectPoolDestruction::IgnoreUnsafeDoNotUseInNewCode>
-        mTimedHandlers;
+    BitMapObjectPool<TimedHandler, CHIP_IM_MAX_NUM_TIMED_HANDLER> 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];
diff --git a/src/lib/support/Pool.h b/src/lib/support/Pool.h
index 1b4e77253b6563..15000182005e83 100644
--- a/src/lib/support/Pool.h
+++ b/src/lib/support/Pool.h
@@ -83,10 +83,10 @@ class StaticAllocatorBitmap : public internal::StaticAllocatorBase
 
 public:
     StaticAllocatorBitmap(void * storage, std::atomic<tBitChunkType> * usage, size_t capacity, size_t elementSize);
-    void * Allocate();
-    void Deallocate(void * element);
 
 protected:
+    void * Allocate();
+    void Deallocate(void * element);
     void * At(size_t index) { return static_cast<uint8_t *>(mElements) + mElementSize * index; }
     size_t IndexOf(void * element);