From bba2fbeafb5f55c4878bd2853f8ec1d515dbcd5a Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 28 Jun 2022 19:10:26 -0400 Subject: [PATCH] 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 https://github.com/project-chip/connectedhomeip/issues/14509 Fixes https://github.com/project-chip/connectedhomeip/issues/14446 --- src/app/CommandSender.cpp | 3 +++ src/app/WriteClient.cpp | 9 ++++++++- src/messaging/ExchangeMgr.h | 2 +- src/messaging/ReliableMessageMgr.cpp | 2 +- src/messaging/ReliableMessageMgr.h | 6 +++--- src/messaging/tests/TestReliableMessageProtocol.cpp | 7 +++++-- 6 files changed, 21 insertions(+), 8 deletions(-) diff --git a/src/app/CommandSender.cpp b/src/app/CommandSender.cpp index 1e04fb58bc75db..63053751240eb7 100644 --- a/src/app/CommandSender.cpp +++ b/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; } diff --git a/src/app/WriteClient.cpp b/src/app/WriteClient.cpp index 106e99633b9db8..c17c63af372500 100644 --- a/src/app/WriteClient.cpp +++ b/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; } diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index ac796b8bd4e33b..e2ad4e8f20e388 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -226,7 +226,7 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate FabricIndex mFabricIndex = 0; - BitMapObjectPool mContextPool; + ObjectPool mContextPool; SessionManager * mSessionManager; ReliableMessageMgr mReliableMessageMgr; diff --git a/src/messaging/ReliableMessageMgr.cpp b/src/messaging/ReliableMessageMgr.cpp index e6bb6cb8f64a62..c85e3f952a6cdb 100644 --- a/src/messaging/ReliableMessageMgr.cpp +++ b/src/messaging/ReliableMessageMgr.cpp @@ -51,7 +51,7 @@ ReliableMessageMgr::RetransTableEntry::~RetransTableEntry() ec->SetMessageNotAcked(false); } -ReliableMessageMgr::ReliableMessageMgr(BitMapObjectPool & contextPool) : +ReliableMessageMgr::ReliableMessageMgr(ObjectPool & contextPool) : mContextPool(contextPool), mSystemLayer(nullptr) {} diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index 9538ad3f5e0c4c..ca3a9252e2f255 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -66,7 +66,7 @@ class ReliableMessageMgr including both successfully and failure send. */ }; - ReliableMessageMgr(BitMapObjectPool & contextPool); + ReliableMessageMgr(ObjectPool & contextPool); ~ReliableMessageMgr(); void Init(chip::System::Layer * systemLayer); @@ -177,7 +177,7 @@ class ReliableMessageMgr #endif // CHIP_CONFIG_TEST private: - BitMapObjectPool & mContextPool; + ObjectPool & 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 mRetransTable; + ObjectPool mRetransTable; }; } // namespace Messaging diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index dc2118d3bb1c86..68c0ad717693f4 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/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);