From aa3d70952aef94eeddb641bf073fb8d89f09cd0f Mon Sep 17 00:00:00 2001 From: Kevin Schoedel Date: Tue, 7 Dec 2021 15:40:00 -0500 Subject: [PATCH] Fix ObjectPool leaks in InteractionModelEngine #### 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 mCommandHandlerObjs; - BitMapObjectPool - mTimedHandlers; + BitMapObjectPool 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 * 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(mElements) + mElementSize * index; } size_t IndexOf(void * element);