Skip to content

Commit

Permalink
Fix ObjectPool leaks in InteractionModelEngine (#12702)
Browse files Browse the repository at this point in the history
#### 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.
  • Loading branch information
kpschoedel authored and pull[bot] committed May 16, 2022
1 parent bcd7bf5 commit a807296
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 24 deletions.
25 changes: 5 additions & 20 deletions src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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,
Expand All @@ -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());
Expand All @@ -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());
Expand Down
3 changes: 1 addition & 2 deletions src/app/InteractionModelEngine.h
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
4 changes: 2 additions & 2 deletions src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down

0 comments on commit a807296

Please sign in to comment.