Skip to content

Commit

Permalink
Fix HeapObjectPool node leaks. (#20943)
Browse files Browse the repository at this point in the history
Leaking scenario 1 (which was discovered in practice): An iteration on a pool
that removes the element it's iterating and then does a Loop::Break.  In that
case we would break out of the loop in ForEachNode before setting anyReleased to
true, which would cause the cleanup code to not run when the iteration depth
went to 0 and the node to leak.  This might have been fixed by reordering the
setting of anyReleased and the break from the loop, except:

Leaking scenario 2: Outer iteration iterates over the whole pool.  When
iterating the last element of the pool, it starts an inner iteration, which
releases all but that last element.  Then the two iterations complete.  In this
situation, the inner iteration would have anyReleased true, but not do any
cleanup because it's a nested iteration, while the outer iteration would have
anyReleased false (because the releases all happened behind its cursor, so it
did not see any of them) and hence would not do any cleanup either.

The fix is to just keep track of the "we need to do post-iteration cleanup"
state in a member.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Aug 1, 2023
1 parent 7250533 commit f0ed3e4
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 8 deletions.
9 changes: 3 additions & 6 deletions src/lib/support/Pool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ Loop HeapObjectList::ForEachNode(void * context, Lambda lambda)
{
++mIterationDepth;
Loop result = Loop::Finish;
bool anyReleased = false;
HeapObjectListNode * p = mNext;
while (p != this)
{
Expand All @@ -130,14 +129,10 @@ Loop HeapObjectList::ForEachNode(void * context, Lambda lambda)
break;
}
}
if (p->mObject == nullptr)
{
anyReleased = true;
}
p = p->mNext;
}
--mIterationDepth;
if (mIterationDepth == 0 && anyReleased)
if (mIterationDepth == 0 && mHaveDeferredNodeRemovals)
{
// Remove nodes for released objects.
p = mNext;
Expand All @@ -151,6 +146,8 @@ Loop HeapObjectList::ForEachNode(void * context, Lambda lambda)
}
p = next;
}

mHaveDeferredNodeRemovals = false;
}
return result;
}
Expand Down
9 changes: 7 additions & 2 deletions src/lib/support/Pool.h
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ struct HeapObjectListNode

struct HeapObjectList : HeapObjectListNode
{
HeapObjectList() : mIterationDepth(0) { mNext = mPrev = this; }
HeapObjectList() { mNext = mPrev = this; }

void Append(HeapObjectListNode * node)
{
Expand All @@ -170,7 +170,8 @@ struct HeapObjectList : HeapObjectListNode
return const_cast<HeapObjectList *>(this)->ForEachNode(context, reinterpret_cast<Lambda>(lambda));
}

size_t mIterationDepth;
size_t mIterationDepth = 0;
bool mHaveDeferredNodeRemovals = false;
};

#endif // CHIP_SYSTEM_CONFIG_POOL_USE_HEAP
Expand Down Expand Up @@ -365,6 +366,10 @@ class HeapObjectPool : public internal::Statistics, public internal::PoolCommon<
node->Remove();
Platform::Delete(node);
}
else
{
mObjects.mHaveDeferredNodeRemovals = true;
}

DecreaseUsage();
}
Expand Down

0 comments on commit f0ed3e4

Please sign in to comment.