Skip to content

Commit

Permalink
Fix invariant violation if we get a message without piggyback ack whi…
Browse files Browse the repository at this point in the history 
…le 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 #22854

* Fix tests.

* Address review comment.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Nov 6, 2023
1 parent 7f3dbc3 commit 70f9286
Show file tree
Hide file tree
Showing 6 changed files with 225 additions and 37 deletions.
65 changes: 56 additions & 9 deletions src/app/tests/TestCommandInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
110 changes: 86 additions & 24 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
Loading

0 comments on commit 70f9286

Please sign in to comment.