From e885999c4fc81fb7560d1e86aea258113ace0852 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 25 Oct 2022 08:23:30 -0400 Subject: [PATCH] Fix invariant violation if we get a message without piggyback ack while expecting an ack. (#23282) * Fix invariant violation if we get a message without piggyback ack while expecting an ack. Such messages are not allowed per spec, so we should just ignore them. Fixes https://github.com/project-chip/connectedhomeip/issues/22854 * Fix tests. * Address review comment. --- src/app/tests/TestCommandInteraction.cpp | 65 ++++++++++++-- src/app/tests/TestReadInteraction.cpp | 110 ++++++++++++++++++----- src/app/tests/TestWriteInteraction.cpp | 58 +++++++++++- src/messaging/ExchangeContext.cpp | 12 +++ src/messaging/ReliableMessageContext.h | 8 ++ src/messaging/ReliableMessageMgr.h | 9 ++ 6 files changed, 225 insertions(+), 37 deletions(-) diff --git a/src/app/tests/TestCommandInteraction.cpp b/src/app/tests/TestCommandInteraction.cpp index fde62b54ecd3bd..2ad2bc751be526 100644 --- a/src/app/tests/TestCommandInteraction.cpp +++ b/src/app/tests/TestCommandInteraction.cpp @@ -714,6 +714,40 @@ void TestCommandInteraction::TestCommandHandlerCommandEncodeFailure(nlTestSuite exchange->Close(); } +/** + * Helper macro we can use to pretend we got a reply from the server in cases + * when the reply was actually dropped due to us not wanting the client's state + * machine to advance. + * + * When this macro is used, the client has sent a message and is waiting for an + * ack+response, and the server has sent a response that got dropped and is + * waiting for an ack (and maybe a response). + * + * What this macro then needs to do is: + * + * 1. Pretend that the client got an ack (and clear out the corresponding ack + * state). + * 2. Pretend that the client got a message from the server, with the id of the + * message that was dropped, which requires an ack, so the client will send + * that ack in its next message. + * + * This is a macro so we get useful line numbers on assertion failures + */ +#define PretendWeGotReplyFromServer(aSuite, aContext, aClientExchange) \ + { \ + Messaging::ReliableMessageMgr * localRm = (aContext).GetExchangeManager().GetReliableMessageMgr(); \ + Messaging::ExchangeContext * localExchange = aClientExchange; \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 2); \ + \ + localRm->ClearRetransTable(localExchange); \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 1); \ + \ + localRm->EnumerateRetransTable([localExchange](auto * entry) { \ + localExchange->SetPendingPeerAckMessageCounter(entry->retainedBuf.GetMessageCounter()); \ + return Loop::Break; \ + }); \ + } + // Command Sender sends invoke request, command handler drops invoke response, then test injects status response message with // busy to client, client sends out a status response with invalid action. void TestCommandInteraction::TestCommandInvalidMessage1(nlTestSuite * apSuite, void * apContext) @@ -757,8 +791,11 @@ void TestCommandInteraction::TestCommandInvalidMessage1(nlTestSuite * apSuite, v Test::MessageCapturer messageLog(ctx); messageLog.mCaptureStandaloneAcks = false; - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - rm->ClearRetransTable(commandSender.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up our + // MRP state to look like what it would have looked like if the packet had + // not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, commandSender.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -823,9 +860,13 @@ void TestCommandInteraction::TestCommandInvalidMessage2(nlTestSuite * apSuite, v payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData); Test::MessageCapturer messageLog(ctx); - messageLog.mCaptureStandaloneAcks = false; - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - rm->ClearRetransTable(commandSender.mExchangeCtx.Get()); + messageLog.mCaptureStandaloneAcks = false; + + // Since we are dropping packets, things are not getting acked. Set up our + // MRP state to look like what it would have looked like if the packet had + // not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, commandSender.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -892,8 +933,11 @@ void TestCommandInteraction::TestCommandInvalidMessage3(nlTestSuite * apSuite, v Test::MessageCapturer messageLog(ctx); messageLog.mCaptureStandaloneAcks = false; - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - rm->ClearRetransTable(commandSender.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up our + // MRP state to look like what it would have looked like if the packet had + // not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, commandSender.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -960,8 +1004,11 @@ void TestCommandInteraction::TestCommandInvalidMessage4(nlTestSuite * apSuite, v Test::MessageCapturer messageLog(ctx); messageLog.mCaptureStandaloneAcks = false; - Messaging::ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); - rm->ClearRetransTable(commandSender.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up our + // MRP state to look like what it would have looked like if the packet had + // not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, commandSender.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index fc9e37964ef9dc..9a27ba771b9497 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -2926,6 +2926,40 @@ void CheckForInvalidAction(nlTestSuite * apSuite, Test::MessageCapturer & messag } // anonymous namespace +/** + * Helper macro we can use to pretend we got a reply from the server in cases + * when the reply was actually dropped due to us not wanting the client's state + * machine to advance. + * + * When this macro is used, the client has sent a message and is waiting for an + * ack+response, and the server has sent a response that got dropped and is + * waiting for an ack (and maybe a response). + * + * What this macro then needs to do is: + * + * 1. Pretend that the client got an ack (and clear out the corresponding ack + * state). + * 2. Pretend that the client got a message from the server, with the id of the + * message that was dropped, which requires an ack, so the client will send + * that ack in its next message. + * + * This is a macro so we get useful line numbers on assertion failures + */ +#define PretendWeGotReplyFromServer(aSuite, aContext, aClientExchange) \ + { \ + Messaging::ReliableMessageMgr * localRm = (aContext).GetExchangeManager().GetReliableMessageMgr(); \ + Messaging::ExchangeContext * localExchange = aClientExchange; \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 2); \ + \ + localRm->ClearRetransTable(localExchange); \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 1); \ + \ + localRm->EnumerateRetransTable([localExchange](auto * entry) { \ + localExchange->SetPendingPeerAckMessageCounter(entry->retainedBuf.GetMessageCounter()); \ + return Loop::Break; \ + }); \ + } + // Read Client sends the read request, Read Handler drops the response, then test injects unknown status reponse message for Read // Client. void TestReadInteraction::TestReadClientReceiveInvalidMessage(nlTestSuite * apSuite, void * apContext) @@ -2957,8 +2991,7 @@ void TestReadInteraction::TestReadClientReceiveInvalidMessage(nlTestSuite * apSu readPrepareParams.mAttributePathParamsListSize = 2; { - app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, - chip::app::ReadClient::InteractionType::Read); + app::ReadClient readClient(engine, &ctx.GetExchangeManager(), delegate, chip::app::ReadClient::InteractionType::Read); ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 1; @@ -2968,6 +3001,9 @@ void TestReadInteraction::TestReadClientReceiveInvalidMessage(nlTestSuite * apSu NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); + System::PacketBufferHandle msgBuf = System::PacketBufferHandle::New(kMaxSecureSduLengthBytes); NL_TEST_ASSERT(apSuite, !msgBuf.IsNull()); System::PacketBufferTLVWriter writer; @@ -2983,9 +3019,11 @@ void TestReadInteraction::TestReadClientReceiveInvalidMessage(nlTestSuite * apSu Test::MessageCapturer messageLog(ctx); messageLog.mCaptureStandaloneAcks = false; - rm->ClearRetransTable(readClient.mExchange.Get()); - NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); - NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -3065,13 +3103,16 @@ void TestReadInteraction::TestSubscribeClientReceiveInvalidStatusResponse(nlTest payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse); - rm->ClearRetransTable(readClient.mExchange.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); - NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -3155,13 +3196,16 @@ void TestReadInteraction::TestSubscribeClientReceiveWellFormedStatusResponse(nlT payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse); - rm->ClearRetransTable(readClient.mExchange.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); - NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -3243,12 +3287,15 @@ void TestReadInteraction::TestSubscribeClientReceiveInvalidReportMessage(nlTestS payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData); - rm->ClearRetransTable(readClient.mExchange.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; @@ -3407,12 +3454,15 @@ void TestReadInteraction::TestSubscribeClientReceiveInvalidSubscribeResponseMess payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::SubscribeResponse); - rm->ClearRetransTable(readClient.mExchange.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 4); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; @@ -3574,7 +3624,11 @@ void TestReadInteraction::TestReadChunkingInvalidSubscriptionId(nlTestSuite * ap NL_TEST_ASSERT(apSuite, writer.Finalize(&msgBuf) == CHIP_NO_ERROR); - rm->ClearRetransTable(readClient.mExchange.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 4); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); @@ -3780,17 +3834,21 @@ void TestReadInteraction::TestSubscribeSendUnknownMessage(nlTestSuite * apSuite, NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); - - ctx.GetLoopback().mSentMessageCount = 0; - rm->ClearRetransTable(readClient.mExchange.Get()); NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); + + ctx.GetLoopback().mSentMessageCount = 0; + // Server sends out status report, client should send status report along with Piggybacking ack, but we don't do that - // Instead, we send out unknown message to server,but server is still waiting for ack, so we need - // clear out retranstable - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); + // Instead, we send out unknown message to server + System::PacketBufferHandle msgBuf; ReadRequestMessage::Builder request; System::PacketBufferTLVWriter writer; @@ -3853,13 +3911,17 @@ void TestReadInteraction::TestSubscribeSendInvalidStatusReport(nlTestSuite * apS NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); ctx.DrainAndServiceIO(); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mSentMessageCount == 2); NL_TEST_ASSERT(apSuite, ctx.GetLoopback().mDroppedMessageCount == 1); ctx.GetLoopback().mSentMessageCount = 0; - rm->ClearRetransTable(readClient.mExchange.Get()); + NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadHandlers() == 1); NL_TEST_ASSERT(apSuite, engine->ActiveHandlerAt(0) != nullptr); - rm->ClearRetransTable(engine->ActiveHandlerAt(0)->mExchangeCtx.Get()); System::PacketBufferHandle msgBuf; StatusResponseMessage::Builder request; diff --git a/src/app/tests/TestWriteInteraction.cpp b/src/app/tests/TestWriteInteraction.cpp index 65f6e8b5d300d1..6437b251cbf17f 100644 --- a/src/app/tests/TestWriteInteraction.cpp +++ b/src/app/tests/TestWriteInteraction.cpp @@ -679,6 +679,40 @@ void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuit #endif +/** + * Helper macro we can use to pretend we got a reply from the server in cases + * when the reply was actually dropped due to us not wanting the client's state + * machine to advance. + * + * When this macro is used, the client has sent a message and is waiting for an + * ack+response, and the server has sent a response that got dropped and is + * waiting for an ack (and maybe a response). + * + * What this macro then needs to do is: + * + * 1. Pretend that the client got an ack (and clear out the corresponding ack + * state). + * 2. Pretend that the client got a message from the server, with the id of the + * message that was dropped, which requires an ack, so the client will send + * that ack in its next message. + * + * This is a macro so we get useful line numbers on assertion failures + */ +#define PretendWeGotReplyFromServer(aSuite, aContext, aClientExchange) \ + { \ + Messaging::ReliableMessageMgr * localRm = (aContext).GetExchangeManager().GetReliableMessageMgr(); \ + Messaging::ExchangeContext * localExchange = aClientExchange; \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 2); \ + \ + localRm->ClearRetransTable(localExchange); \ + NL_TEST_ASSERT(aSuite, localRm->TestGetCountRetransTable() == 1); \ + \ + localRm->EnumerateRetransTable([localExchange](auto * entry) { \ + localExchange->SetPendingPeerAckMessageCounter(entry->retainedBuf.GetMessageCounter()); \ + return Loop::Break; \ + }); \ + } + // Write Client sends a write request, receives an unexpected message type, sends a status response to that. void TestWriteInteraction::TestWriteInvalidMessage1(nlTestSuite * apSuite, void * apContext) { @@ -724,7 +758,11 @@ void TestWriteInteraction::TestWriteInvalidMessage1(nlTestSuite * apSuite, void payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::ReportData); - rm->ClearRetransTable(writeClient.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, writeClient.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -791,7 +829,11 @@ void TestWriteInteraction::TestWriteInvalidMessage2(nlTestSuite * apSuite, void payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::WriteResponse); - rm->ClearRetransTable(writeClient.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, writeClient.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -857,7 +899,11 @@ void TestWriteInteraction::TestWriteInvalidMessage3(nlTestSuite * apSuite, void payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse); - rm->ClearRetransTable(writeClient.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, writeClient.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; @@ -925,7 +971,11 @@ void TestWriteInteraction::TestWriteInvalidMessage4(nlTestSuite * apSuite, void payloadHeader.SetExchangeID(0); payloadHeader.SetMessageType(chip::Protocols::InteractionModel::MsgType::StatusResponse); - rm->ClearRetransTable(writeClient.mExchangeCtx.Get()); + // Since we are dropping packets, things are not getting acked. Set up + // our MRP state to look like what it would have looked like if the + // packet had not gotten dropped. + PretendWeGotReplyFromServer(apSuite, ctx, writeClient.mExchangeCtx.Get()); + ctx.GetLoopback().mSentMessageCount = 0; ctx.GetLoopback().mNumMessagesToDrop = 0; ctx.GetLoopback().mNumMessagesToAllowBeforeDropping = 0; diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 9d3c8ec9a2f401..080b73b82ca337 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -573,6 +573,18 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload return CHIP_NO_ERROR; } + if (IsMessageNotAcked()) + { + // The only way we can get here is a spec violation on the other side: + // we sent a message that needs an ack, and the other side responded + // with a message that does not contain an ack for the message we sent. + // Just drop this message; if we delivered it to our delegate it might + // try to send another message-needing-an-ack in response, which would + // violate our internal invariants. + ChipLogError(ExchangeManager, "Dropping message without piggyback ack when we are waiting for an ack."); + return CHIP_ERROR_INCORRECT_STATE; + } + // Since we got the response, cancel the response timer. CancelResponseTimer(); diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index 886ed5c3ba341b..57ec4561740376 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -36,6 +36,11 @@ #include namespace chip { +namespace app { +class TestCommandInteraction; +class TestReadInteraction; +class TestWriteInteraction; +} // namespace app namespace Messaging { class ChipMessageInfo; @@ -193,6 +198,9 @@ class ReliableMessageContext friend class ReliableMessageMgr; friend class ExchangeContext; friend class ExchangeMessageDispatch; + friend class ::chip::app::TestCommandInteraction; + friend class ::chip::app::TestReadInteraction; + friend class ::chip::app::TestWriteInteraction; System::Clock::Timestamp mNextAckTime; // Next time for triggering Solo Ack uint32_t mPendingPeerAckMessageCounter; diff --git a/src/messaging/ReliableMessageMgr.h b/src/messaging/ReliableMessageMgr.h index c50ca12c275983..e2fc1e21897d8c 100644 --- a/src/messaging/ReliableMessageMgr.h +++ b/src/messaging/ReliableMessageMgr.h @@ -192,6 +192,15 @@ class ReliableMessageMgr #if CHIP_CONFIG_TEST // Functions for testing int TestGetCountRetransTable(); + + // Enumerate the retransmission table. Clearing an entry while enumerating + // that entry is allowed. F must take a RetransTableEntry as an argument + // and return Loop::Continue or Loop::Break. + template + void EnumerateRetransTable(F && functor) + { + mRetransTable.ForEachActiveObject(std::forward(functor)); + } #endif // CHIP_CONFIG_TEST private: