From 383180632ba1e2e0a79a44957f6546adb8281576 Mon Sep 17 00:00:00 2001 From: Pankaj Garg Date: Mon, 28 Jun 2021 10:07:30 -0700 Subject: [PATCH] Duplicate MRP message handling and test (#7893) * Duplicate CRMP message handling and test * address review comments * update test with review comments --- src/lib/core/CHIPError.h | 8 + src/messaging/ExchangeContext.cpp | 13 +- src/messaging/ExchangeContext.h | 5 +- src/messaging/ExchangeMessageDispatch.cpp | 6 +- src/messaging/ExchangeMessageDispatch.h | 3 +- src/messaging/ExchangeMgr.cpp | 41 ++--- src/messaging/ExchangeMgr.h | 16 +- src/messaging/tests/TestExchangeMgr.cpp | 4 +- .../tests/TestReliableMessageProtocol.cpp | 149 ++++++++++++++---- .../SessionEstablishmentExchangeDispatch.cpp | 3 +- .../SessionEstablishmentExchangeDispatch.h | 2 +- src/transport/PeerMessageCounter.h | 2 +- src/transport/SecureSessionMgr.cpp | 26 ++- src/transport/SecureSessionMgr.h | 21 +-- src/transport/tests/TestSecureSessionMgr.cpp | 8 +- 15 files changed, 207 insertions(+), 100 deletions(-) diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 36d792da372e7f..849cfb1a9449f3 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -1785,6 +1785,14 @@ typedef CHIP_CONFIG_ERROR_TYPE CHIP_ERROR; */ #define CHIP_ERROR_INTERMEDIATE_CA_NOT_REQUIRED _CHIP_ERROR(190) +/** + * @def CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED + * + * @brief + * The received message is a duplicate of a previously received message. + */ +#define CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED _CHIP_ERROR(191) + /** * @} */ diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index 22b55dcac13a46..1b11ee5bced895 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -370,7 +370,8 @@ void ExchangeContext::NotifyResponseTimeout() } CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - const Transport::PeerAddress & peerAddress, PacketBufferHandle && msgBuf) + const Transport::PeerAddress & peerAddress, MessageFlags msgFlags, + PacketBufferHandle && msgBuf) { // We hold a reference to the ExchangeContext here to // guard against Close() calls(decrementing the reference @@ -378,8 +379,8 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con // layer has completed its work on the ExchangeContext. Retain(); - CHIP_ERROR err = - mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, GetReliableMessageContext()); + CHIP_ERROR err = mDispatch->OnMessageReceived(payloadHeader, packetHeader.GetMessageId(), peerAddress, msgFlags, + GetReliableMessageContext()); SuccessOrExit(err); // The SecureChannel::StandaloneAck message type is only used for MRP; do not pass such messages to the application layer. @@ -388,6 +389,12 @@ CHIP_ERROR ExchangeContext::HandleMessage(const PacketHeader & packetHeader, con ExitNow(err = CHIP_NO_ERROR); } + // Since the message is duplicate, let's not forward it up the stack + if (msgFlags.Has(MessageFlagValues::kDuplicateMessage)) + { + ExitNow(err = CHIP_NO_ERROR); + } + // Since we got the response, cancel the response timer. CancelResponseTimer(); diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 1918b190a86251..8c8c6032a36c3e 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -118,6 +118,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen * * @param[in] peerAddress The address of the sender * + * @param[in] msgFlags The message flags corresponding to the received message + * * @param[in] msgBuf A handle to the packet buffer holding the CHIP message. * * @retval #CHIP_ERROR_INVALID_ARGUMENT if an invalid argument was passed to this HandleMessage API. @@ -126,7 +128,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, public Referen * protocol layer. */ CHIP_ERROR HandleMessage(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - const Transport::PeerAddress & peerAddress, System::PacketBufferHandle && msgBuf); + const Transport::PeerAddress & peerAddress, MessageFlags msgFlags, + System::PacketBufferHandle && msgBuf); ExchangeDelegate * GetDelegate() const { return mDelegate; } void SetDelegate(ExchangeDelegate * delegate) { mDelegate = delegate; } diff --git a/src/messaging/ExchangeMessageDispatch.cpp b/src/messaging/ExchangeMessageDispatch.cpp index fdd924c217f763..277b0454a35f28 100644 --- a/src/messaging/ExchangeMessageDispatch.cpp +++ b/src/messaging/ExchangeMessageDispatch.cpp @@ -99,7 +99,7 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SecureSessionHandle session, uin } CHIP_ERROR ExchangeMessageDispatch::OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, - const Transport::PeerAddress & peerAddress, + const Transport::PeerAddress & peerAddress, MessageFlags msgFlags, ReliableMessageContext * reliableMessageContext) { ReturnErrorCodeIf(!MessagePermitted(payloadHeader.GetProtocolID().GetProtocolId(), payloadHeader.GetMessageType()), @@ -107,15 +107,13 @@ CHIP_ERROR ExchangeMessageDispatch::OnMessageReceived(const PayloadHeader & payl if (IsReliableTransmissionAllowed()) { - if (payloadHeader.IsAckMsg() && payloadHeader.GetAckId().HasValue()) + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsAckMsg() && payloadHeader.GetAckId().HasValue()) { ReturnErrorOnFailure(reliableMessageContext->HandleRcvdAck(payloadHeader.GetAckId().Value())); } if (payloadHeader.NeedsAck()) { - MessageFlags msgFlags; - // An acknowledgment needs to be sent back to the peer for this message on this exchange, // Set the flag in message header indicating an ack requested by peer; msgFlags.Set(MessageFlagValues::kPeerRequestedAck); diff --git a/src/messaging/ExchangeMessageDispatch.h b/src/messaging/ExchangeMessageDispatch.h index e0d9dd60322f7a..4bf43fa7b293ac 100644 --- a/src/messaging/ExchangeMessageDispatch.h +++ b/src/messaging/ExchangeMessageDispatch.h @@ -24,6 +24,7 @@ #pragma once #include +#include #include namespace chip { @@ -59,7 +60,7 @@ class ExchangeMessageDispatch : public ReferenceCounted const EncryptedPacketBufferHandle & preparedMessage) const = 0; virtual CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, - const Transport::PeerAddress & peerAddress, + const Transport::PeerAddress & peerAddress, MessageFlags msgFlags, ReliableMessageContext * reliableMessageContext); protected: diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 4f5509357e0994..a769574354bbd4 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -140,7 +140,7 @@ CHIP_ERROR ExchangeManager::UnregisterUnsolicitedMessageHandlerForType(Protocols return UnregisterUMH(protocolId, static_cast(msgType)); } -void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer) +void ExchangeManager::OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source) { #if CHIP_ERROR_LOGGING char srcAddressStr[Transport::PeerAddress::kMaxToStringSize]; @@ -197,7 +197,7 @@ CHIP_ERROR ExchangeManager::UnregisterUMH(Protocols::Id protocolId, int16_t msgT void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, SecureSessionHandle session, const Transport::PeerAddress & source, - System::PacketBufferHandle && msgBuf, SecureSessionMgr * msgLayer) + DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf) { CHIP_ERROR err = CHIP_NO_ERROR; UnsolicitedMessageHandler * matchingUMH = nullptr; @@ -207,6 +207,12 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const payloadHeader.GetMessageType(), payloadHeader.GetProtocolID().ToFullyQualifiedSpecForm(), payloadHeader.GetExchangeID()); + MessageFlags msgFlags; + if (isDuplicate == DuplicateMessage::Yes) + { + msgFlags.Set(MessageFlagValues::kDuplicateMessage); + } + // Search for an existing exchange that the message applies to. If a match is found... bool found = false; mContextPool.ForEachActiveObject([&](auto * ec) { @@ -220,7 +226,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const } // Matched ExchangeContext; send to message handler. - ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf)); + ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf)); found = true; return false; } @@ -232,10 +238,10 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ExitNow(err = CHIP_NO_ERROR); } - // Search for an unsolicited message handler if it marked as being sent by an initiator. Since we didn't - // find an existing exchange that matches the message, it must be an unsolicited message. However all + // If it's not a duplicate message, search for an unsolicited message handler if it is marked as being sent by an initiator. + // Since we didn't find an existing exchange that matches the message, it must be an unsolicited message. However all // unsolicited messages must be marked as being from an initiator. - if (payloadHeader.IsInitiator()) + if (!msgFlags.Has(MessageFlagValues::kDuplicateMessage) && payloadHeader.IsInitiator()) { // Search for an unsolicited message handler that can handle the message. Prefer handlers that can explicitly // handle the message type over handlers that handle all messages for a profile. @@ -288,7 +294,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ChipLogDetail(ExchangeManager, "ec id: %d, Delegate: 0x%p", ec->GetExchangeId(), ec->GetDelegate()); - ec->HandleMessage(packetHeader, payloadHeader, source, std::move(msgBuf)); + ec->HandleMessage(packetHeader, payloadHeader, source, msgFlags, std::move(msgBuf)); // Close exchange if it was created only to send ack for a duplicate message. if (sendAckAndCloseExchange) @@ -302,7 +308,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const } } -void ExchangeManager::OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) +void ExchangeManager::OnNewConnection(SecureSessionHandle session) { if (mDelegate != nullptr) { @@ -310,7 +316,7 @@ void ExchangeManager::OnNewConnection(SecureSessionHandle session, SecureSession } } -void ExchangeManager::OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) +void ExchangeManager::OnConnectionExpired(SecureSessionHandle session) { if (mDelegate != nullptr) { @@ -328,23 +334,6 @@ void ExchangeManager::OnConnectionExpired(SecureSessionHandle session, SecureSes }); } -void ExchangeManager::OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf) -{ - PacketHeader header; - - ReturnOnFailure(header.DecodeAndConsume(msgBuf)); - - Optional peer = header.GetSourceNodeId(); - if (!peer.HasValue()) - { - char addrBuffer[Transport::PeerAddress::kMaxToStringSize]; - source.ToString(addrBuffer, sizeof(addrBuffer)); - ChipLogError(ExchangeManager, "Unencrypted message from %s is dropped since no source node id in packet header.", - addrBuffer); - return; - } -} - void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * delegate) { mContextPool.ForEachActiveObject([&](auto * ec) { diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index ddeb26f98a2426..6a83ff5afacb20 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -49,7 +49,7 @@ static constexpr int16_t kAnyMessageType = -1; * It works on be behalf of higher layers, creating ExchangeContexts and * handling the registration/unregistration of unsolicited message handlers. */ -class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public TransportMgrDelegate +class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate { friend class ExchangeContext; @@ -242,21 +242,17 @@ class DLL_EXPORT ExchangeManager : public SecureSessionMgrDelegate, public Trans CHIP_ERROR RegisterUMH(Protocols::Id protocolId, int16_t msgType, ExchangeDelegate * delegate); CHIP_ERROR UnregisterUMH(Protocols::Id protocolId, int16_t msgType); - void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * msgLayer) override; + void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source) override; void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, SecureSessionHandle session, - const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf, - SecureSessionMgr * msgLayer) override; + const Transport::PeerAddress & source, DuplicateMessage isDuplicate, + System::PacketBufferHandle && msgBuf) override; - void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) override; + void OnNewConnection(SecureSessionHandle session) override; #if CHIP_CONFIG_TEST public: // Allow OnConnectionExpired to be called directly from tests. #endif // CHIP_CONFIG_TEST - void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) override; - - // TransportMgrDelegate interface for rendezvous sessions -private: - void OnMessageReceived(const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf) override; + void OnConnectionExpired(SecureSessionHandle session) override; }; } // namespace Messaging diff --git a/src/messaging/tests/TestExchangeMgr.cpp b/src/messaging/tests/TestExchangeMgr.cpp index b3ae773a5f756a..fb26006ed1040b 100644 --- a/src/messaging/tests/TestExchangeMgr.cpp +++ b/src/messaging/tests/TestExchangeMgr.cpp @@ -141,7 +141,7 @@ void CheckSessionExpirationBasics(nlTestSuite * inSuite, void * inContext) ExchangeContext * ec1 = ctx.NewExchangeToLocal(&sendDelegate); // Expire the session this exchange is supposedly on. - ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession(), &ctx.GetSecureSessionManager()); + ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession()); MockAppDelegate receiveDelegate; CHIP_ERROR err = @@ -171,7 +171,7 @@ void CheckSessionExpirationTimeout(nlTestSuite * inSuite, void * inContext) // Expire the session this exchange is supposedly on. This should close the // exchange. - ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession(), &ctx.GetSecureSessionManager()); + ctx.GetExchangeManager().OnConnectionExpired(ec1->GetSecureSession()); NL_TEST_ASSERT(inSuite, sendDelegate.IsOnResponseTimeoutCalled); } diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index 7f511ae0286d8e..7ef5d2bad47a2f 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -99,7 +99,19 @@ class MockAppDelegate : public ExchangeDelegate { ec->GetReliableMessageContext()->SetAckPending(false); } - ec->Close(); + + if (mExchange != ec) + { + CloseExchangeIfNeeded(); + } + + if (!mRetainExchange) + { + ec->Close(); + ec = nullptr; + } + mExchange = ec; + if (mTestSuite != nullptr) { NL_TEST_ASSERT(mTestSuite, buffer->TotalLength() == sizeof(PAYLOAD)); @@ -110,8 +122,19 @@ class MockAppDelegate : public ExchangeDelegate void OnResponseTimeout(ExchangeContext * ec) override {} + void CloseExchangeIfNeeded() + { + if (mExchange != nullptr) + { + mExchange->Close(); + mExchange = nullptr; + } + } + bool IsOnMessageReceivedCalled = false; bool mDropAckResponse = false; + bool mRetainExchange = false; + ExchangeContext * mExchange = nullptr; nlTestSuite * mTestSuite = nullptr; }; @@ -481,7 +504,7 @@ void CheckResendApplicationMessageWithPeerExchange(nlTestSuite * inSuite, void * rm->ClearRetransTable(rc); } -void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inContext) +void CheckDuplicateMessageClosedExchange(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); @@ -512,13 +535,15 @@ void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inC 1, // CHIP_CONFIG_RMP_DEFAULT_ACTIVE_RETRY_INTERVAL }); - mockReceiver.mDropAckResponse = true; - - // Let's not drop any messages. We'll drop acks for this test. + // Let's not drop the message. Expectation is that it is received by the peer, but the ack is dropped gLoopback.mSendMessageCount = 0; gLoopback.mNumMessagesToDrop = 0; gLoopback.mDroppedMessageCount = 0; + // Drop the ack, and also close the peer exchange + mockReceiver.mDropAckResponse = true; + mockReceiver.mRetainExchange = false; + // Ensure the retransmit table is empty right now NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); @@ -526,44 +551,29 @@ void CheckResendApplicationMessageWithLostAcks(nlTestSuite * inSuite, void * inC NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); exchange->Close(); - // Ensure the message was not dropped, and was added to retransmit table + // Ensure the message was sent + // The ack was dropped, and message was added to the retransmit table NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 1); NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - // 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit - test_os_sleep_ms(65); - ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR); - - // Ensure the retransmit message was also not dropped, and is still there in the retransmit table - NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 2); - NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); - NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - - // Let's not drop the ack on the next retry + // Let's not drop the duplicate message mockReceiver.mDropAckResponse = false; + err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + // 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit test_os_sleep_ms(65); ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR); - // Ensure the message was retransmitted, and is no longer in the retransmit table - NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount >= 3); + // Ensure the retransmit message was sent and the ack was sent + // and retransmit table was cleared + NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 3); NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); - // TODO - Enable test for lost CRMP ack messages - // The following check is commented out because of https://github.com/project-chip/connectedhomeip/issues/7292 - // NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); - NL_TEST_ASSERT(inSuite, mockReceiver.IsOnMessageReceivedCalled); - - mockReceiver.mTestSuite = nullptr; - - err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); - NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); - - rm->ClearRetransTable(rc); + mockReceiver.CloseExchangeIfNeeded(); } void CheckResendSessionEstablishmentMessageWithPeerExchange(nlTestSuite * inSuite, void * inContext) @@ -654,6 +664,80 @@ void CheckResendSessionEstablishmentMessageWithPeerExchange(nlTestSuite * inSuit gTransportMgr.SetSecureSessionMgr(&inctx.GetSecureSessionManager()); } +void CheckDuplicateMessage(nlTestSuite * inSuite, void * inContext) +{ + TestContext & ctx = *reinterpret_cast(inContext); + + ctx.GetInetLayer().SystemLayer()->Init(nullptr); + + chip::System::PacketBufferHandle buffer = chip::MessagePacketBuffer::NewWithData(PAYLOAD, sizeof(PAYLOAD)); + NL_TEST_ASSERT(inSuite, !buffer.IsNull()); + + CHIP_ERROR err = CHIP_NO_ERROR; + + MockAppDelegate mockReceiver; + err = ctx.GetExchangeManager().RegisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest, &mockReceiver); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + mockReceiver.mTestSuite = inSuite; + + MockAppDelegate mockSender; + ExchangeContext * exchange = ctx.NewExchangeToPeer(&mockSender); + NL_TEST_ASSERT(inSuite, exchange != nullptr); + + ReliableMessageMgr * rm = ctx.GetExchangeManager().GetReliableMessageMgr(); + ReliableMessageContext * rc = exchange->GetReliableMessageContext(); + NL_TEST_ASSERT(inSuite, rm != nullptr); + NL_TEST_ASSERT(inSuite, rc != nullptr); + + rc->SetConfig({ + 1, // CHIP_CONFIG_RMP_DEFAULT_INITIAL_RETRY_INTERVAL + 1, // CHIP_CONFIG_RMP_DEFAULT_ACTIVE_RETRY_INTERVAL + }); + + // Let's not drop the message. Expectation is that it is received by the peer, but the ack is dropped + gLoopback.mSendMessageCount = 0; + gLoopback.mNumMessagesToDrop = 0; + gLoopback.mDroppedMessageCount = 0; + + // Drop the ack, and keep the exchange around to receive the duplicate message + mockReceiver.mDropAckResponse = true; + mockReceiver.mRetainExchange = true; + + // Ensure the retransmit table is empty right now + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); + + err = exchange->SendMessage(Echo::MsgType::EchoRequest, std::move(buffer)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + exchange->Close(); + + // Ensure the message was sent + // The ack was dropped, and message was added to the retransmit table + NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 1); + NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 1); + + err = ctx.GetExchangeManager().UnregisterUnsolicitedMessageHandlerForType(Echo::MsgType::EchoRequest); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // Let's not drop the duplicate message + mockReceiver.mDropAckResponse = false; + mockReceiver.mRetainExchange = false; + + // 1 tick is 64 ms, sleep 65 ms to trigger first re-transmit + test_os_sleep_ms(65); + ReliableMessageMgr::Timeout(&ctx.GetSystemLayer(), rm, CHIP_SYSTEM_NO_ERROR); + + // Ensure the retransmit message was sent and the ack was sent + // and retransmit table was cleared + NL_TEST_ASSERT(inSuite, gLoopback.mSendMessageCount == 3); + NL_TEST_ASSERT(inSuite, gLoopback.mDroppedMessageCount == 0); + NL_TEST_ASSERT(inSuite, rm->TestGetCountRetransTable() == 0); + + mockReceiver.CloseExchangeIfNeeded(); +} + void CheckSendStandaloneAckMessage(nlTestSuite * inSuite, void * inContext) { TestContext & ctx = *reinterpret_cast(inContext); @@ -688,8 +772,9 @@ const nlTest sTests[] = NL_TEST_DEF("Test ReliableMessageMgr::CheckCloseExchangeAndResendApplicationMessage", CheckCloseExchangeAndResendApplicationMessage), NL_TEST_DEF("Test ReliableMessageMgr::CheckFailedMessageRetainOnSend", CheckFailedMessageRetainOnSend), NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithPeerExchange", CheckResendApplicationMessageWithPeerExchange), - NL_TEST_DEF("Test ReliableMessageMgr::CheckResendApplicationMessageWithLostAcks", CheckResendApplicationMessageWithLostAcks), NL_TEST_DEF("Test ReliableMessageMgr::CheckResendSessionEstablishmentMessageWithPeerExchange", CheckResendSessionEstablishmentMessageWithPeerExchange), + NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessage", CheckDuplicateMessage), + NL_TEST_DEF("Test ReliableMessageMgr::CheckDuplicateMessageClosedExchange", CheckDuplicateMessageClosedExchange), NL_TEST_DEF("Test ReliableMessageMgr::CheckSendStandaloneAckMessage", CheckSendStandaloneAckMessage), NL_TEST_SENTINEL() diff --git a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp index 3593427ec08acf..59914b415231d9 100644 --- a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp +++ b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp @@ -49,10 +49,11 @@ CHIP_ERROR SessionEstablishmentExchangeDispatch::SendPreparedMessage(SecureSessi CHIP_ERROR SessionEstablishmentExchangeDispatch::OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, const Transport::PeerAddress & peerAddress, + Messaging::MessageFlags msgFlags, ReliableMessageContext * reliableMessageContext) { mPeerAddress = peerAddress; - return ExchangeMessageDispatch::OnMessageReceived(payloadHeader, messageId, peerAddress, reliableMessageContext); + return ExchangeMessageDispatch::OnMessageReceived(payloadHeader, messageId, peerAddress, msgFlags, reliableMessageContext); } bool SessionEstablishmentExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type) diff --git a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h index e59bfb92630fce..1a58d00f98e859 100644 --- a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h +++ b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h @@ -48,7 +48,7 @@ class SessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessageDi CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & preparedMessage) const override; CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, - const Transport::PeerAddress & peerAddress, + const Transport::PeerAddress & peerAddress, Messaging::MessageFlags msgFlags, Messaging::ReliableMessageContext * reliableMessageContext) override; const Transport::PeerAddress & GetPeerAddress() const { return mPeerAddress; } diff --git a/src/transport/PeerMessageCounter.h b/src/transport/PeerMessageCounter.h index 2048c98b0af26d..6e62dc0cc4f114 100644 --- a/src/transport/PeerMessageCounter.h +++ b/src/transport/PeerMessageCounter.h @@ -101,7 +101,7 @@ class PeerMessageCounter } if (mSynced.mWindow.test(offset)) { - return CHIP_ERROR_INVALID_ARGUMENT; // duplicated, in window + return CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED; // duplicated, in window } } diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index b13e23670ce4a5..6d981c46717c69 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -287,7 +287,7 @@ CHIP_ERROR SecureSessionMgr::NewPairing(const Optional & if (mCB != nullptr) { state->GetSessionMessageCounter().GetPeerMessageCounter().SetCounter(pairing->GetPeerCounter()); - mCB->OnNewConnection({ state->GetPeerNodeId(), state->GetPeerKeyID(), admin }, this); + mCB->OnNewConnection({ state->GetPeerNodeId(), state->GetPeerKeyID(), admin }); } return CHIP_NO_ERROR; @@ -332,7 +332,8 @@ void SecureSessionMgr::MessageDispatch(const PacketHeader & packetHeader, const { PayloadHeader payloadHeader; ReturnOnFailure(payloadHeader.DecodeAndConsume(msg)); - mCB->OnMessageReceived(packetHeader, payloadHeader, SecureSessionHandle(), peerAddress, std::move(msg), this); + mCB->OnMessageReceived(packetHeader, payloadHeader, SecureSessionHandle(), peerAddress, + SecureSessionMgrDelegate::DuplicateMessage::No, std::move(msg)); } } @@ -351,6 +352,8 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, NodeId localNodeId; FabricId fabricId; + SecureSessionMgrDelegate::DuplicateMessage isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::No; + VerifyOrExit(!msg.IsNull(), ChipLogError(Inet, "Secure transport received NULL packet, discarding")); if (state == nullptr) @@ -389,6 +392,12 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, } err = state->GetSessionMessageCounter().GetPeerMessageCounter().Verify(packetHeader.GetMessageId()); + if (err == CHIP_ERROR_DUPLICATE_MESSAGE_RECEIVED) + { + ChipLogDetail(Inet, "Received a duplicate message"); + isDuplicate = SecureSessionMgrDelegate::DuplicateMessage::Yes; + err = CHIP_NO_ERROR; + } if (err != CHIP_NO_ERROR) { ChipLogError(Inet, "Message counter verify failed, err = %" PRId32, err); @@ -428,6 +437,13 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, VerifyOrExit(CHIP_NO_ERROR == SecureMessageCodec::Decode(state, payloadHeader, packetHeader, msg), ChipLogError(Inet, "Secure transport received message, but failed to decode it, discarding")); + if (isDuplicate == SecureSessionMgrDelegate::DuplicateMessage::Yes && !payloadHeader.NeedsAck()) + { + // If it's a duplicate message, but doesn't require an ack, let's drop it right here to save CPU + // cycles on further message processing. + ExitNow(err = CHIP_NO_ERROR); + } + if (packetHeader.GetFlags().Has(Header::FlagValues::kSecureSessionControlMessage)) { // TODO: control message counter is not implemented yet @@ -491,13 +507,13 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader, if (mCB != nullptr) { SecureSessionHandle session(state->GetPeerNodeId(), state->GetPeerKeyID(), state->GetAdminId()); - mCB->OnMessageReceived(packetHeader, payloadHeader, session, peerAddress, std::move(msg), this); + mCB->OnMessageReceived(packetHeader, payloadHeader, session, peerAddress, isDuplicate, std::move(msg)); } exit: if (err != CHIP_NO_ERROR && mCB != nullptr) { - mCB->OnReceiveError(err, peerAddress, this); + mCB->OnReceiveError(err, peerAddress); } } @@ -510,7 +526,7 @@ void SecureSessionMgr::HandleConnectionExpired(const Transport::PeerConnectionSt if (mCB != nullptr) { - mCB->OnConnectionExpired({ state.GetPeerNodeId(), state.GetPeerKeyID(), state.GetAdminId() }, this); + mCB->OnConnectionExpired({ state.GetPeerNodeId(), state.GetPeerKeyID(), state.GetAdminId() }); } mTransportMgr->Disconnect(state.GetPeerAddress()); diff --git a/src/transport/SecureSessionMgr.h b/src/transport/SecureSessionMgr.h index ac29c1b30614ed..ca0ff7e069a0e8 100644 --- a/src/transport/SecureSessionMgr.h +++ b/src/transport/SecureSessionMgr.h @@ -123,6 +123,12 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle class DLL_EXPORT SecureSessionMgrDelegate { public: + enum class DuplicateMessage : uint8_t + { + Yes, + No, + }; + /** * @brief * Called when a new message is received. The function must internally release the @@ -132,12 +138,12 @@ class DLL_EXPORT SecureSessionMgrDelegate * @param payloadHeader The payload header * @param session The handle to the secure session * @param source The sender's address + * @param isDuplicate The message is a duplicate of previously received message * @param msgBuf The received message - * @param mgr A pointer to the SecureSessionMgr */ virtual void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - SecureSessionHandle session, const Transport::PeerAddress & source, - System::PacketBufferHandle && msgBuf, SecureSessionMgr * mgr) + SecureSessionHandle session, const Transport::PeerAddress & source, DuplicateMessage isDuplicate, + System::PacketBufferHandle && msgBuf) {} /** @@ -146,27 +152,24 @@ class DLL_EXPORT SecureSessionMgrDelegate * * @param error error code * @param source network entity that sent the message - * @param mgr A pointer to the SecureSessionMgr */ - virtual void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source, SecureSessionMgr * mgr) {} + virtual void OnReceiveError(CHIP_ERROR error, const Transport::PeerAddress & source) {} /** * @brief * Called when a new pairing is being established * * @param session The handle to the secure session - * @param mgr A pointer to the SecureSessionMgr */ - virtual void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) {} + virtual void OnNewConnection(SecureSessionHandle session) {} /** * @brief * Called when a new connection is closing * * @param session The handle to the secure session - * @param mgr A pointer to the SecureSessionMgr */ - virtual void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) {} + virtual void OnConnectionExpired(SecureSessionHandle session) {} virtual ~SecureSessionMgrDelegate() {} }; diff --git a/src/transport/tests/TestSecureSessionMgr.cpp b/src/transport/tests/TestSecureSessionMgr.cpp index b9d0d42c76a0c7..0974d28d523ed7 100644 --- a/src/transport/tests/TestSecureSessionMgr.cpp +++ b/src/transport/tests/TestSecureSessionMgr.cpp @@ -94,8 +94,8 @@ class TestSessMgrCallback : public SecureSessionMgrDelegate { public: void OnMessageReceived(const PacketHeader & header, const PayloadHeader & payloadHeader, SecureSessionHandle session, - const Transport::PeerAddress & source, System::PacketBufferHandle && msgBuf, - SecureSessionMgr * mgr) override + const Transport::PeerAddress & source, DuplicateMessage isDuplicate, + System::PacketBufferHandle && msgBuf) override { NL_TEST_ASSERT(mSuite, header.GetSourceNodeId() == Optional::Value(kSourceNodeId)); NL_TEST_ASSERT(mSuite, header.GetDestinationNodeId() == Optional::Value(kDestinationNodeId)); @@ -117,7 +117,7 @@ class TestSessMgrCallback : public SecureSessionMgrDelegate ReceiveHandlerCallCount++; } - void OnNewConnection(SecureSessionHandle session, SecureSessionMgr * mgr) override + void OnNewConnection(SecureSessionHandle session) override { // Preset the MessageCounter if (NewConnectionHandlerCallCount == 0) @@ -126,7 +126,7 @@ class TestSessMgrCallback : public SecureSessionMgrDelegate mLocalToRemoteSession = session; NewConnectionHandlerCallCount++; } - void OnConnectionExpired(SecureSessionHandle session, SecureSessionMgr * mgr) override {} + void OnConnectionExpired(SecureSessionHandle session) override {} nlTestSuite * mSuite = nullptr; SecureSessionHandle mRemoteToLocalSession;