Stop using explicit BitMapObjectPool. (#20058)

There were only two consumers (the retrans table and exchanges) left, which both
used to fail unit tests if using ObjectPool due to use-after-free.

One of those tests is now fixed; the other is fixed in this PR.  So we can move
to using ObjectPool everywhere.

Fixes #14509
Fixes #14446

src/app/CommandSender.cpp

@@ -96,6 +96,9 @@ CHIP_ERROR CommandSender::SendGroupCommandRequest(const SessionHandle & session)
 
     ReturnErrorOnFailure(SendInvokeRequest());
 
+    // Exchange is gone now, since it closed itself on successful send.
+    mpExchangeCtx = nullptr;
+
     Close();
     return CHIP_NO_ERROR;
 }

src/app/WriteClient.cpp

@@ -425,7 +425,8 @@ CHIP_ERROR WriteClient::SendWriteRequest()
 
     System::PacketBufferHandle data = mChunks.PopHead();
 
-    if (!mChunks.IsNull() && mpExchangeCtx->IsGroupExchangeContext())
+    bool isGroupWrite = mpExchangeCtx->IsGroupExchangeContext();
+    if (!mChunks.IsNull() && isGroupWrite)
     {
         // Reject this request if we have more than one chunk (mChunks is not null after PopHead()), and this is a group
         // exchange context.
@@ -434,6 +435,12 @@ CHIP_ERROR WriteClient::SendWriteRequest()
 
     // kExpectResponse is ignored by ExchangeContext in case of groupcast
     ReturnErrorOnFailure(mpExchangeCtx->SendMessage(MsgType::WriteRequest, std::move(data), SendMessageFlags::kExpectResponse));
+    if (isGroupWrite)
+    {
+        // Exchange is closed now, since there are no group responses.  Drop our
+        // ref to it.
+        mpExchangeCtx = nullptr;
+    }
     MoveToState(State::AwaitingResponse);
     return CHIP_NO_ERROR;
 }

src/messaging/ExchangeMgr.h

@@ -226,7 +226,7 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate
 
     FabricIndex mFabricIndex = 0;
 
-    BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;
+    ObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> mContextPool;
 
     SessionManager * mSessionManager;
     ReliableMessageMgr mReliableMessageMgr;

src/messaging/ReliableMessageMgr.cpp

@@ -51,7 +51,7 @@ ReliableMessageMgr::RetransTableEntry::~RetransTableEntry()
     ec->SetMessageNotAcked(false);
 }
 
-ReliableMessageMgr::ReliableMessageMgr(BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool) :
+ReliableMessageMgr::ReliableMessageMgr(ObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool) :
     mContextPool(contextPool), mSystemLayer(nullptr)
 {}
 

src/messaging/ReliableMessageMgr.h

@@ -66,7 +66,7 @@ class ReliableMessageMgr
                                                        including both successfully and failure send. */
     };
 
-    ReliableMessageMgr(BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool);
+    ReliableMessageMgr(ObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & contextPool);
     ~ReliableMessageMgr();
 
     void Init(chip::System::Layer * systemLayer);
@@ -177,7 +177,7 @@ class ReliableMessageMgr
 #endif // CHIP_CONFIG_TEST
 
 private:
-    BitMapObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & mContextPool;
+    ObjectPool<ExchangeContext, CHIP_CONFIG_MAX_EXCHANGE_CONTEXTS> & mContextPool;
     chip::System::Layer * mSystemLayer;
 
     /* Placeholder function to run a function for all exchanges */
@@ -193,7 +193,7 @@ class ReliableMessageMgr
     void TicklessDebugDumpRetransTable(const char * log);
 
     // ReliableMessageProtocol Global tables for timer context
-    BitMapObjectPool<RetransTableEntry, CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE> mRetransTable;
+    ObjectPool<RetransTableEntry, CHIP_CONFIG_RMP_RETRANS_TABLE_SIZE> mRetransTable;
 };
 
 } // namespace Messaging

src/messaging/tests/TestReliableMessageProtocol.cpp

@@ -1569,6 +1569,9 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)
     // We now have just the received message waiting for an ack.
     NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1);
 
+    // And receiver still has no ack pending.
+    NL_TEST_ASSERT(inSuite, !receiverRc->IsAckPending());
+
     // Reset various state so we can measure things again.
     mockReceiver.IsOnMessageReceivedCalled = false;
     mockReceiver.mReceivedPiggybackAck     = false;
@@ -1594,8 +1597,8 @@ void CheckLostStandaloneAck(nlTestSuite * inSuite, void * inContext)
     NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled);
     NL_TEST_ASSERT(inSuite, mockReceiver.mReceivedPiggybackAck);
 
-    // And that receiver has no ack pending.
-    NL_TEST_ASSERT(inSuite, !receiverRc->IsAckPending());
+    // At this point all our exchanges and reliable message contexts should be
+    // dead, so we can't test anything about their state.
 
     // And that there are no un-acked messages left.
     NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0);