From 07f4b236814a5bdf9a16d0760f5ec12382b5f2af Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 4 Jun 2021 19:37:14 +0800 Subject: [PATCH] Resolve comments --- src/messaging/ApplicationExchangeDispatch.cpp | 6 +++--- src/messaging/ExchangeMessageDispatch.cpp | 21 ++++++++----------- src/messaging/ExchangeMessageDispatch.h | 12 ++++++++++- .../tests/TestReliableMessageProtocol.cpp | 4 ++-- .../SessionEstablishmentExchangeDispatch.cpp | 4 ++-- .../SessionEstablishmentExchangeDispatch.h | 2 +- src/transport/SecureSessionMgr.cpp | 13 ++++++------ src/transport/SecureSessionMgr.h | 15 +++++-------- src/transport/tests/TestSecureSessionMgr.cpp | 12 ++++++----- 9 files changed, 47 insertions(+), 42 deletions(-) diff --git a/src/messaging/ApplicationExchangeDispatch.cpp b/src/messaging/ApplicationExchangeDispatch.cpp index ce6f42f9fd8752..d76f4b779fc2e6 100644 --- a/src/messaging/ApplicationExchangeDispatch.cpp +++ b/src/messaging/ApplicationExchangeDispatch.cpp @@ -30,13 +30,13 @@ CHIP_ERROR ApplicationExchangeDispatch::PrepareMessage(SecureSessionHandle sessi System::PacketBufferHandle && message, EncryptedPacketBufferHandle & preparedMessage) { - return mSessionMgr->PrepareMessage(session, payloadHeader, std::move(message), preparedMessage); + return mSessionMgr->BuildEncryptedMessagePayload(session, payloadHeader, std::move(message), preparedMessage); } CHIP_ERROR ApplicationExchangeDispatch::SendPreparedMessage(SecureSessionHandle session, - const EncryptedPacketBufferHandle & message) const + const EncryptedPacketBufferHandle & preparedMessage) const { - return mSessionMgr->SendPreparedMessage(session, message); + return mSessionMgr->SendPreparedMessage(session, preparedMessage); } bool ApplicationExchangeDispatch::MessagePermitted(uint16_t protocol, uint8_t type) diff --git a/src/messaging/ExchangeMessageDispatch.cpp b/src/messaging/ExchangeMessageDispatch.cpp index ae7ebf1146b06e..fdd924c217f763 100644 --- a/src/messaging/ExchangeMessageDispatch.cpp +++ b/src/messaging/ExchangeMessageDispatch.cpp @@ -29,6 +29,7 @@ #endif #include +#include #include #include @@ -76,18 +77,14 @@ CHIP_ERROR ExchangeMessageDispatch::SendMessage(SecureSessionHandle session, uin // Add to Table for subsequent sending ReturnErrorOnFailure(reliableMessageMgr->AddToRetransTable(reliableMessageContext, &entry)); - - CHIP_ERROR err = PrepareMessage(session, payloadHeader, std::move(message), entry->retainedBuf); - if (err != CHIP_NO_ERROR) - { - // Remove from table - ChipLogError(ExchangeManager, "Failed to send message with err %s", ::chip::ErrorStr(err)); - reliableMessageMgr->ClearRetransTable(*entry); - return err; - } - - ReturnErrorOnFailure(SendPreparedMessage(session, entry->retainedBuf)); - reliableMessageMgr->StartRetransmision(entry); + auto deleter = [reliableMessageMgr](ReliableMessageMgr::RetransTableEntry * e) { + reliableMessageMgr->ClearRetransTable(*e); + }; + std::unique_ptr entryOwner(entry, deleter); + + ReturnErrorOnFailure(PrepareMessage(session, payloadHeader, std::move(message), entryOwner->retainedBuf)); + ReturnErrorOnFailure(SendPreparedMessage(session, entryOwner->retainedBuf)); + reliableMessageMgr->StartRetransmision(entryOwner.release()); } else { diff --git a/src/messaging/ExchangeMessageDispatch.h b/src/messaging/ExchangeMessageDispatch.h index fe249a5727ffe3..e0d9dd60322f7a 100644 --- a/src/messaging/ExchangeMessageDispatch.h +++ b/src/messaging/ExchangeMessageDispatch.h @@ -44,9 +44,19 @@ class ExchangeMessageDispatch : public ReferenceCounted ReliableMessageContext * reliableMessageContext, bool isReliableTransmission, Protocols::Id protocol, uint8_t type, System::PacketBufferHandle && message); + /** + * @brief + * This interface takes the payload and returns the prepared message which can be send multiple times. + * + * @param session Peer node to which the payload to be sent + * @param payloadHeader The payloadHeader to be encoded into the packet + * @param message The payload to be sent + * @param preparedMessage The handle to hold the prepared message + */ virtual CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && message, EncryptedPacketBufferHandle & preparedMessage) = 0; - virtual CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const = 0; + virtual CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, + const EncryptedPacketBufferHandle & preparedMessage) const = 0; virtual CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, const Transport::PeerAddress & peerAddress, diff --git a/src/messaging/tests/TestReliableMessageProtocol.cpp b/src/messaging/tests/TestReliableMessageProtocol.cpp index f3d0d5641a6550..580f21578b6674 100644 --- a/src/messaging/tests/TestReliableMessageProtocol.cpp +++ b/src/messaging/tests/TestReliableMessageProtocol.cpp @@ -129,9 +129,9 @@ class MockSessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessa return CHIP_NO_ERROR; } - CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const override + CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & preparedMessage) const override { - return gTransportMgr.SendMessage(Transport::PeerAddress(), message.Retain()); + return gTransportMgr.SendMessage(Transport::PeerAddress(), preparedMessage.CastToWritable()); } bool IsReliableTransmissionAllowed() const override { return mRetainMessageOnSend; } diff --git a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp index da0f56c7b1b911..3593427ec08acf 100644 --- a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp +++ b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.cpp @@ -41,10 +41,10 @@ CHIP_ERROR SessionEstablishmentExchangeDispatch::PrepareMessage(SecureSessionHan } CHIP_ERROR SessionEstablishmentExchangeDispatch::SendPreparedMessage(SecureSessionHandle session, - const EncryptedPacketBufferHandle & message) const + const EncryptedPacketBufferHandle & preparedMessage) const { ReturnErrorCodeIf(mTransportMgr == nullptr, CHIP_ERROR_INCORRECT_STATE); - return mTransportMgr->SendMessage(mPeerAddress, message.Retain()); + return mTransportMgr->SendMessage(mPeerAddress, preparedMessage.CastToWritable()); } CHIP_ERROR SessionEstablishmentExchangeDispatch::OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, diff --git a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h index c667f81f9c1404..e59bfb92630fce 100644 --- a/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h +++ b/src/protocols/secure_channel/SessionEstablishmentExchangeDispatch.h @@ -45,7 +45,7 @@ class SessionEstablishmentExchangeDispatch : public Messaging::ExchangeMessageDi CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && message, EncryptedPacketBufferHandle & out) override; - CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & message) const override; + CHIP_ERROR SendPreparedMessage(SecureSessionHandle session, const EncryptedPacketBufferHandle & preparedMessage) const override; CHIP_ERROR OnMessageReceived(const PayloadHeader & payloadHeader, uint32_t messageId, const Transport::PeerAddress & peerAddress, diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp index f44ec39037910b..4f58c60244d842 100644 --- a/src/transport/SecureSessionMgr.cpp +++ b/src/transport/SecureSessionMgr.cpp @@ -122,8 +122,9 @@ Transport::Type SecureSessionMgr::GetTransportType(NodeId peerNodeId) return Transport::Type::kUndefined; } -CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, - System::PacketBufferHandle && msgBuf, EncryptedPacketBufferHandle & preparedMessage) +CHIP_ERROR SecureSessionMgr::BuildEncryptedMessagePayload(SecureSessionHandle session, PayloadHeader & payloadHeader, + System::PacketBufferHandle && msgBuf, + EncryptedPacketBufferHandle & encryptedMessage) { PacketHeader packetHeader; if (IsControlMessage(payloadHeader)) @@ -135,7 +136,7 @@ CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, Payload if (state == nullptr) { return CHIP_ERROR_NOT_CONNECTED; - }; + } Transport::AdminPairingInfo * admin = mAdmins->FindAdminWithId(state->GetAdminId()); if (admin == nullptr) @@ -149,8 +150,8 @@ CHIP_ERROR SecureSessionMgr::PrepareMessage(SecureSessionHandle session, Payload ReturnErrorOnFailure(packetHeader.EncodeBeforeData(msgBuf)); - preparedMessage = EncryptedPacketBufferHandle::MarkEncrypted(std::move(msgBuf)); - ChipLogProgress(Inet, "Prepared msg %p from 0x" ChipLogFormatX64 " to 0x" ChipLogFormatX64 ".", &preparedMessage, + encryptedMessage = EncryptedPacketBufferHandle::MarkEncrypted(std::move(msgBuf)); + ChipLogProgress(Inet, "Prepared msg %p from 0x" ChipLogFormatX64 " to 0x" ChipLogFormatX64 ".", &encryptedMessage, ChipLogValueX64(localNodeId), ChipLogValueX64(state->GetPeerNodeId())); return CHIP_NO_ERROR; @@ -164,7 +165,7 @@ CHIP_ERROR SecureSessionMgr::SendPreparedMessage(SecureSessionHandle session, co VerifyOrExit(mState == State::kInitialized, err = CHIP_ERROR_INCORRECT_STATE); VerifyOrExit(!preparedMessage.IsNull(), err = CHIP_ERROR_INVALID_ARGUMENT); - msgBuf = preparedMessage.Retain(); + msgBuf = preparedMessage.CastToWritable(); VerifyOrExit(!msgBuf.IsNull(), err = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(!msgBuf->HasChainedBuffer(), err = CHIP_ERROR_INVALID_MESSAGE_LENGTH); diff --git a/src/transport/SecureSessionMgr.h b/src/transport/SecureSessionMgr.h index ec102c2f5c3c28..861f13f8f2f18c 100644 --- a/src/transport/SecureSessionMgr.h +++ b/src/transport/SecureSessionMgr.h @@ -68,8 +68,6 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle uint32_t GetMsgId() const; - PacketBufferHandle Retain() const { return PacketBufferHandle::Retain(); } - /** * Creates a copy of the data in this packet. * @@ -108,11 +106,8 @@ class EncryptedPacketBufferHandle final : private System::PacketBufferHandle * Get a handle to the data that allows mutating the bytes. This should * only be used if absolutely necessary, because EncryptedPacketBufferHandle * represents a buffer that we want to resend as-is. - * - * We only allow doing this with an rvalue reference, so the fact that we - * are moving out of the EncryptedPacketBufferHandle is clear. */ - PacketBufferHandle CastToWritable() && { return PacketBufferHandle(std::move(*this)); } + PacketBufferHandle CastToWritable() const { return PacketBufferHandle::Retain(); } private: EncryptedPacketBufferHandle(PacketBufferHandle && aBuffer) : PacketBufferHandle(std::move(aBuffer)) {} @@ -184,17 +179,17 @@ class DLL_EXPORT SecureSessionMgr : public TransportMgrDelegate /** * @brief - * This function takes the payload and returns the final message which can be send multiple times. + * This function takes the payload and returns encrypted message which can be sent multiple times. * * @details * It contains following preparation: * 1. Encrypt the msgBuf * 2. construct the packet header * 3. Encode the packet header and prepend it to message. - * Returns a prepared message in preparedMessage. + * Returns a encrypted message in encryptedMessage. */ - CHIP_ERROR PrepareMessage(SecureSessionHandle session, PayloadHeader & payloadHeader, System::PacketBufferHandle && msgBuf, - EncryptedPacketBufferHandle & preparedMessage); + CHIP_ERROR BuildEncryptedMessagePayload(SecureSessionHandle session, PayloadHeader & payloadHeader, + System::PacketBufferHandle && msgBuf, EncryptedPacketBufferHandle & encryptedMessage); /** * @brief diff --git a/src/transport/tests/TestSecureSessionMgr.cpp b/src/transport/tests/TestSecureSessionMgr.cpp index c149baaa19952b..b9d0d42c76a0c7 100644 --- a/src/transport/tests/TestSecureSessionMgr.cpp +++ b/src/transport/tests/TestSecureSessionMgr.cpp @@ -221,7 +221,7 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext) payloadHeader.SetMessageType(chip::Protocols::Echo::MsgType::EchoRequest); EncryptedPacketBufferHandle preparedMessage; - err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); + err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage); @@ -235,7 +235,8 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext) callback.LargeMessageSent = true; - err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(large_buffer), preparedMessage); + err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(large_buffer), + preparedMessage); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage); @@ -251,7 +252,8 @@ void CheckMessageTest(nlTestSuite * inSuite, void * inContext) callback.LargeMessageSent = true; - err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(extra_large_buffer), preparedMessage); + err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(extra_large_buffer), + preparedMessage); NL_TEST_ASSERT(inSuite, err == CHIP_ERROR_MESSAGE_TOO_LONG); } @@ -319,7 +321,7 @@ void SendEncryptedPacketTest(nlTestSuite * inSuite, void * inContext) payloadHeader.SetInitiator(true); - err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); + err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage); @@ -401,7 +403,7 @@ void SendBadEncryptedPacketTest(nlTestSuite * inSuite, void * inContext) payloadHeader.SetInitiator(true); - err = secureSessionMgr.PrepareMessage(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); + err = secureSessionMgr.BuildEncryptedMessagePayload(localToRemoteSession, payloadHeader, std::move(buffer), preparedMessage); NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); err = secureSessionMgr.SendPreparedMessage(localToRemoteSession, preparedMessage);