From 6bb052ecea465ef66dd4c04be9a05bd1f4349bdd Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 10 Jun 2022 13:38:33 +0800 Subject: [PATCH 1/3] StandaloneAck handling: Untangle code path for UMH and StandaloneAck --- src/messaging/ExchangeContext.cpp | 23 ++++++- src/messaging/ExchangeContext.h | 6 +- src/messaging/ExchangeMgr.cpp | 72 ++++++++++++++-------- src/messaging/ReliableMessageContext.h | 11 ++++ src/messaging/StandaloneExchangeDispatch.h | 61 ++++++++++++++++++ 5 files changed, 142 insertions(+), 31 deletions(-) create mode 100644 src/messaging/StandaloneExchangeDispatch.h diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index cb67cdbbe03a9b..eb27388a69c41b 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -41,6 +41,7 @@ #include #include #include +#include #include #include @@ -290,8 +291,8 @@ void ExchangeContextDeletor::Release(ExchangeContext * ec) } ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, const SessionHandle & session, bool Initiator, - ExchangeDelegate * delegate) : - mDispatch((delegate != nullptr) ? delegate->GetMessageDispatch() : ApplicationExchangeDispatch::Instance()), + ExchangeDelegate * delegate, bool isStandaloneExchange) : + mDispatch(GetMessageDispatch(isStandaloneExchange, delegate)), mSession(*this) { VerifyOrDie(mExchangeMgr == nullptr); @@ -300,6 +301,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons mExchangeId = ExchangeId; mSession.Grab(session); mFlags.Set(Flags::kFlagInitiator, Initiator); + mFlags.Set(Flags::kFlagStandaloneExchange, isStandaloneExchange); mDelegate = delegate; SetAckPending(false); @@ -494,6 +496,12 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload return CHIP_NO_ERROR; } + if (IsStandaloneExchange()) + { + // The StandaloneExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. + return CHIP_NO_ERROR; + } + // Since we got the response, cancel the response timer. CancelResponseTimer(); @@ -527,5 +535,16 @@ void ExchangeContext::MessageHandled() Close(); } +ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isStandaloneExchange, ExchangeDelegate * delegate) +{ + if (isStandaloneExchange) + return StandaloneExchangeDispatch::Instance(); + + if (delegate != nullptr) + return delegate->GetMessageDispatch(); + + return ApplicationExchangeDispatch::Instance(); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index d1a8c943644561..895aa9b046a18f 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -66,7 +66,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, typedef System::Clock::Timeout Timeout; // Type used to express the timeout in this ExchangeContext ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, const SessionHandle & session, bool Initiator, - ExchangeDelegate * delegate); + ExchangeDelegate * delegate, bool isStandaloneExchange = false); ~ExchangeContext() override; @@ -154,8 +154,6 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, ReliableMessageContext * GetReliableMessageContext() { return static_cast(this); }; - ExchangeMessageDispatch & GetMessageDispatch() { return mDispatch; } - SessionHandle GetSessionHandle() const { VerifyOrDie(mSession); @@ -273,6 +271,8 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, * exchange nor other component requests the active mode. */ void UpdateSEDIntervalMode(bool activeMode); + + static ExchangeMessageDispatch & GetMessageDispatch(bool isStandaloneExchange, ExchangeDelegate * delegate); }; } // namespace Messaging diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 1a7a3ca4cc1fb1..771da40c13cb37 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -270,22 +270,18 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const return; } - // If we found a handler or we need to send an ack, create an exchange to - // handle the message. - if (matchingUMH != nullptr || payloadHeader.NeedsAck()) + // If we found a handler, create an exchange to handle the message. + if (matchingUMH != nullptr) { ExchangeDelegate * delegate = nullptr; // Fetch delegate from the handler - if (matchingUMH != nullptr) + CHIP_ERROR err = matchingUMH->Handler->OnUnsolicitedMessageReceived(payloadHeader, delegate); + if (err != CHIP_NO_ERROR) { - CHIP_ERROR err = matchingUMH->Handler->OnUnsolicitedMessageReceived(payloadHeader, delegate); - if (err != CHIP_NO_ERROR) - { - // Using same error message for all errors to reduce code size. - ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); - return; - } + // Using same error message for all errors to reduce code size. + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); + return; } // If rcvd msg is from initiator then this exchange is created as not Initiator. @@ -297,7 +293,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const if (ec == nullptr) { - if (matchingUMH != nullptr && delegate != nullptr) + if (delegate != nullptr) { matchingUMH->Handler->OnExchangeCreationFailed(delegate); } @@ -310,24 +306,45 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const ChipLogDetail(ExchangeManager, "Handling via exchange: " ChipLogFormatExchange ", Delegate: %p", ChipLogValueExchange(ec), ec->GetDelegate()); + if (ec->IsEncryptionRequired() != packetHeader.IsEncrypted()) + { + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE)); + ec->Close(); + return; + } + + err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf)); + if (err != CHIP_NO_ERROR) + { + // Using same error message for all errors to reduce code size. + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); + } + return; + } + + // If we need to send a StandaloneAck, create a StandaloneExchange for the purpose to send the StandaloneAck + if (payloadHeader.NeedsAck()) + { + // If rcvd msg is from initiator then this exchange is created as not Initiator. + // If rcvd msg is not from initiator then this exchange is created as Initiator. + // Create a StandaloneExchange to generate a StandaloneAck + ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), + nullptr, true /* IsStandaloneExchange */); + + if (ec == nullptr) + { + // Using same error message for all errors to reduce code size. + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY)); + return; + } + + ChipLogDetail(ExchangeManager, "Generating StandaloneAck via exchange: " ChipLogFormatExchange, ChipLogValueExchange(ec)); + // Make sure the exchange stays alive through the code below even if we // close it before calling HandleMessage. ExchangeHandle ref(*ec); - // Ignore encryption-required mismatches for emphemeral exchanges, - // because those never have delegates anyway. - if (matchingUMH != nullptr && ec->IsEncryptionRequired() != packetHeader.IsEncrypted()) - { - // We want to still to do MRP processing for this message, but we do - // not want to deliver it to the application. Just close the - // exchange (which will notify the delegate, null it out, etc), then - // go ahead and call HandleMessage() on it to do the MRP - // processing.null out the delegate on the exchange, pretend to - // matchingUMH that exchange creation failed, so it cleans up the - // delegate, then tell the exchagne to handle the message. - ChipLogProgress(ExchangeManager, "OnMessageReceived encryption mismatch"); - ec->Close(); - } + // No need to verify packet encryption type, the StandaloneExchange can handle both secure and insecure messages. CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf)); if (err != CHIP_NO_ERROR) @@ -335,6 +352,9 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const // Using same error message for all errors to reduce code size. ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); } + + // The exchange should be closed inside HandleMessage function. So don't bother close it here. + return; } } diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index 6ebc8227f87a4a..279e50f68837d5 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -121,6 +121,9 @@ class ReliableMessageContext /// Determine whether this exchange is requesting Sleepy End Device active mode bool IsRequestingActiveMode() const; + /// Determine whether this exchange is a StandaloneExchange for replying a StandaloneAck + bool IsStandaloneExchange() const; + /** * Get the reliable message manager that corresponds to this reliable * message context. @@ -158,6 +161,9 @@ class ReliableMessageContext /// When set, signifies that the exchange is requesting Sleepy End Device active mode. kFlagActiveMode = (1u << 8), + + /// When set, signifies that the exchange created sorely for replying a StandaloneAck + kFlagStandaloneExchange = (1u << 9), }; BitFlags mFlags; // Internal state flags @@ -234,5 +240,10 @@ inline void ReliableMessageContext::SetRequestingActiveMode(bool activeMode) mFlags.Set(Flags::kFlagActiveMode, activeMode); } +inline bool ReliableMessageContext::IsStandaloneExchange() const +{ + return mFlags.Has(Flags::kFlagStandaloneExchange); +} + } // namespace Messaging } // namespace chip diff --git a/src/messaging/StandaloneExchangeDispatch.h b/src/messaging/StandaloneExchangeDispatch.h new file mode 100644 index 00000000000000..b2b007d6af65d8 --- /dev/null +++ b/src/messaging/StandaloneExchangeDispatch.h @@ -0,0 +1,61 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * This file defines Application Channel class. The object of this + * class can be used by CHIP data model cluster applications to send + * and receive messages. The messages are encrypted using session keys. + */ + +#pragma once + +#include +#include + +namespace chip { +namespace Messaging { + +class StandaloneExchangeDispatch : public ExchangeMessageDispatch +{ +public: + static ExchangeMessageDispatch & Instance() + { + static StandaloneExchangeDispatch instance; + return instance; + } + + StandaloneExchangeDispatch() {} + ~StandaloneExchangeDispatch() override {} + +protected: + bool MessagePermitted(Protocols::Id protocol, uint8_t type) override + { + // Only permit StandaloneAck + return (protocol == Protocols::SecureChannel::Id && + type == static_cast(Protocols::SecureChannel::MsgType::StandaloneAck)); + } + + bool IsEncryptionRequired() const override + { + // This function should not be called at all + VerifyOrDie(false); + return false; + } +}; + +} // namespace Messaging +} // namespace chip From 01d544b3752ccd67a7c897f761a16a2b30f45727 Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 10 Jun 2022 14:37:55 +0800 Subject: [PATCH 2/3] Fix comments from Boris --- ...eDispatch.h => EphemeralExchangeDispatch.h} | 8 ++++---- src/messaging/ExchangeContext.cpp | 18 +++++++++--------- src/messaging/ExchangeContext.h | 4 ++-- src/messaging/ExchangeMgr.cpp | 16 ++++++++++++---- src/messaging/ExchangeMgr.h | 2 ++ src/messaging/ReliableMessageContext.h | 10 +++++----- 6 files changed, 34 insertions(+), 24 deletions(-) rename src/messaging/{StandaloneExchangeDispatch.h => EphemeralExchangeDispatch.h} (89%) diff --git a/src/messaging/StandaloneExchangeDispatch.h b/src/messaging/EphemeralExchangeDispatch.h similarity index 89% rename from src/messaging/StandaloneExchangeDispatch.h rename to src/messaging/EphemeralExchangeDispatch.h index b2b007d6af65d8..9b81faf3ab6581 100644 --- a/src/messaging/StandaloneExchangeDispatch.h +++ b/src/messaging/EphemeralExchangeDispatch.h @@ -29,17 +29,17 @@ namespace chip { namespace Messaging { -class StandaloneExchangeDispatch : public ExchangeMessageDispatch +class EphemeralExchangeDispatch : public ExchangeMessageDispatch { public: static ExchangeMessageDispatch & Instance() { - static StandaloneExchangeDispatch instance; + static EphemeralExchangeDispatch instance; return instance; } - StandaloneExchangeDispatch() {} - ~StandaloneExchangeDispatch() override {} + EphemeralExchangeDispatch() {} + ~EphemeralExchangeDispatch() override {} protected: bool MessagePermitted(Protocols::Id protocol, uint8_t type) override diff --git a/src/messaging/ExchangeContext.cpp b/src/messaging/ExchangeContext.cpp index eb27388a69c41b..e5008e4bd91fa1 100644 --- a/src/messaging/ExchangeContext.cpp +++ b/src/messaging/ExchangeContext.cpp @@ -39,9 +39,9 @@ #include #include #include +#include #include #include -#include #include #include @@ -291,8 +291,8 @@ void ExchangeContextDeletor::Release(ExchangeContext * ec) } ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, const SessionHandle & session, bool Initiator, - ExchangeDelegate * delegate, bool isStandaloneExchange) : - mDispatch(GetMessageDispatch(isStandaloneExchange, delegate)), + ExchangeDelegate * delegate, bool isEphemeralExchange) : + mDispatch(GetMessageDispatch(isEphemeralExchange, delegate)), mSession(*this) { VerifyOrDie(mExchangeMgr == nullptr); @@ -301,7 +301,7 @@ ExchangeContext::ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, cons mExchangeId = ExchangeId; mSession.Grab(session); mFlags.Set(Flags::kFlagInitiator, Initiator); - mFlags.Set(Flags::kFlagStandaloneExchange, isStandaloneExchange); + mFlags.Set(Flags::kFlagEphemeralExchange, isEphemeralExchange); mDelegate = delegate; SetAckPending(false); @@ -496,9 +496,9 @@ CHIP_ERROR ExchangeContext::HandleMessage(uint32_t messageCounter, const Payload return CHIP_NO_ERROR; } - if (IsStandaloneExchange()) + if (IsEphemeralExchange()) { - // The StandaloneExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. + // The EphemeralExchange has done its job, since StandaloneAck is sent in previous FlushAcks() call. return CHIP_NO_ERROR; } @@ -535,10 +535,10 @@ void ExchangeContext::MessageHandled() Close(); } -ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isStandaloneExchange, ExchangeDelegate * delegate) +ExchangeMessageDispatch & ExchangeContext::GetMessageDispatch(bool isEphemeralExchange, ExchangeDelegate * delegate) { - if (isStandaloneExchange) - return StandaloneExchangeDispatch::Instance(); + if (isEphemeralExchange) + return EphemeralExchangeDispatch::Instance(); if (delegate != nullptr) return delegate->GetMessageDispatch(); diff --git a/src/messaging/ExchangeContext.h b/src/messaging/ExchangeContext.h index 895aa9b046a18f..db3d4e1489572c 100644 --- a/src/messaging/ExchangeContext.h +++ b/src/messaging/ExchangeContext.h @@ -66,7 +66,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, typedef System::Clock::Timeout Timeout; // Type used to express the timeout in this ExchangeContext ExchangeContext(ExchangeManager * em, uint16_t ExchangeId, const SessionHandle & session, bool Initiator, - ExchangeDelegate * delegate, bool isStandaloneExchange = false); + ExchangeDelegate * delegate, bool isEphemeralExchange = false); ~ExchangeContext() override; @@ -272,7 +272,7 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext, */ void UpdateSEDIntervalMode(bool activeMode); - static ExchangeMessageDispatch & GetMessageDispatch(bool isStandaloneExchange, ExchangeDelegate * delegate); + static ExchangeMessageDispatch & GetMessageDispatch(bool isEphemeralExchange, ExchangeDelegate * delegate); }; } // namespace Messaging diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 771da40c13cb37..7c7c5489d5e751 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -310,6 +310,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const { ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE)); ec->Close(); + ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); return; } @@ -322,14 +323,21 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const return; } - // If we need to send a StandaloneAck, create a StandaloneExchange for the purpose to send the StandaloneAck + ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); + return; +} + +void ExchangeManager::ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, + const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf) +{ + // If we need to send a StandaloneAck, create a EphemeralExchange for the purpose to send the StandaloneAck if (payloadHeader.NeedsAck()) { // If rcvd msg is from initiator then this exchange is created as not Initiator. // If rcvd msg is not from initiator then this exchange is created as Initiator. - // Create a StandaloneExchange to generate a StandaloneAck + // Create a EphemeralExchange to generate a StandaloneAck ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), - nullptr, true /* IsStandaloneExchange */); + nullptr, true /* IsEphemeralExchange */); if (ec == nullptr) { @@ -344,7 +352,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const // close it before calling HandleMessage. ExchangeHandle ref(*ec); - // No need to verify packet encryption type, the StandaloneExchange can handle both secure and insecure messages. + // No need to verify packet encryption type, the EphemeralExchange can handle both secure and insecure messages. CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf)); if (err != CHIP_NO_ERROR) diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index 1c0a783703a343..6b9f6c8ad02bc7 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -240,6 +240,8 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session, DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf) override; + void ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session, + MessageFlags msgFlags, System::PacketBufferHandle && msgBuf); }; } // namespace Messaging diff --git a/src/messaging/ReliableMessageContext.h b/src/messaging/ReliableMessageContext.h index 279e50f68837d5..e67b2d596e963e 100644 --- a/src/messaging/ReliableMessageContext.h +++ b/src/messaging/ReliableMessageContext.h @@ -121,8 +121,8 @@ class ReliableMessageContext /// Determine whether this exchange is requesting Sleepy End Device active mode bool IsRequestingActiveMode() const; - /// Determine whether this exchange is a StandaloneExchange for replying a StandaloneAck - bool IsStandaloneExchange() const; + /// Determine whether this exchange is a EphemeralExchange for replying a StandaloneAck + bool IsEphemeralExchange() const; /** * Get the reliable message manager that corresponds to this reliable @@ -163,7 +163,7 @@ class ReliableMessageContext kFlagActiveMode = (1u << 8), /// When set, signifies that the exchange created sorely for replying a StandaloneAck - kFlagStandaloneExchange = (1u << 9), + kFlagEphemeralExchange = (1u << 9), }; BitFlags mFlags; // Internal state flags @@ -240,9 +240,9 @@ inline void ReliableMessageContext::SetRequestingActiveMode(bool activeMode) mFlags.Set(Flags::kFlagActiveMode, activeMode); } -inline bool ReliableMessageContext::IsStandaloneExchange() const +inline bool ReliableMessageContext::IsEphemeralExchange() const { - return mFlags.Has(Flags::kFlagStandaloneExchange); + return mFlags.Has(Flags::kFlagEphemeralExchange); } } // namespace Messaging From 124d90913dc15630c8189910552c823e451d97a4 Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 10 Jun 2022 15:21:58 +0800 Subject: [PATCH 3/3] Resolve comments --- src/messaging/EphemeralExchangeDispatch.h | 9 +-- src/messaging/ExchangeMgr.cpp | 68 +++++++++++------------ src/messaging/ExchangeMgr.h | 4 +- 3 files changed, 34 insertions(+), 47 deletions(-) diff --git a/src/messaging/EphemeralExchangeDispatch.h b/src/messaging/EphemeralExchangeDispatch.h index 9b81faf3ab6581..2f58587c8c103d 100644 --- a/src/messaging/EphemeralExchangeDispatch.h +++ b/src/messaging/EphemeralExchangeDispatch.h @@ -14,13 +14,6 @@ * limitations under the License. */ -/** - * @file - * This file defines Application Channel class. The object of this - * class can be used by CHIP data model cluster applications to send - * and receive messages. The messages are encrypted using session keys. - */ - #pragma once #include @@ -46,7 +39,7 @@ class EphemeralExchangeDispatch : public ExchangeMessageDispatch { // Only permit StandaloneAck return (protocol == Protocols::SecureChannel::Id && - type == static_cast(Protocols::SecureChannel::MsgType::StandaloneAck)); + type == to_underlying(Protocols::SecureChannel::MsgType::StandaloneAck)); } bool IsEncryptionRequired() const override diff --git a/src/messaging/ExchangeMgr.cpp b/src/messaging/ExchangeMgr.cpp index 7c7c5489d5e751..2c1242aff55c1c 100644 --- a/src/messaging/ExchangeMgr.cpp +++ b/src/messaging/ExchangeMgr.cpp @@ -281,15 +281,11 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const { // Using same error message for all errors to reduce code size. ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); + SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); return; } - // If rcvd msg is from initiator then this exchange is created as not Initiator. - // If rcvd msg is not from initiator then this exchange is created as Initiator. - // Note that if matchingUMH is not null then rcvd msg if from initiator. - // TODO: Figure out which channel to use for the received message - ExchangeContext * ec = - mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), delegate); + ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, false, delegate); if (ec == nullptr) { @@ -300,6 +296,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const // Using same error message for all errors to reduce code size. ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY)); + // No resource for creating new exchange, SendStandaloneAckIfNeeded probably also fails, so do not try it here return; } @@ -310,7 +307,7 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const { ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_INVALID_MESSAGE_TYPE)); ec->Close(); - ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); + SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); return; } @@ -323,47 +320,44 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const return; } - ReplyStandaloneAck(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); + SendStandaloneAckIfNeeded(packetHeader, payloadHeader, session, msgFlags, std::move(msgBuf)); return; } -void ExchangeManager::ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, - const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf) +void ExchangeManager::SendStandaloneAckIfNeeded(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, + const SessionHandle & session, MessageFlags msgFlags, + System::PacketBufferHandle && msgBuf) { // If we need to send a StandaloneAck, create a EphemeralExchange for the purpose to send the StandaloneAck - if (payloadHeader.NeedsAck()) - { - // If rcvd msg is from initiator then this exchange is created as not Initiator. - // If rcvd msg is not from initiator then this exchange is created as Initiator. - // Create a EphemeralExchange to generate a StandaloneAck - ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), - nullptr, true /* IsEphemeralExchange */); - - if (ec == nullptr) - { - // Using same error message for all errors to reduce code size. - ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY)); - return; - } + if (!payloadHeader.NeedsAck()) + return; - ChipLogDetail(ExchangeManager, "Generating StandaloneAck via exchange: " ChipLogFormatExchange, ChipLogValueExchange(ec)); + // If rcvd msg is from initiator then this exchange is created as not Initiator. + // If rcvd msg is not from initiator then this exchange is created as Initiator. + // Create a EphemeralExchange to generate a StandaloneAck + ExchangeContext * ec = mContextPool.CreateObject(this, payloadHeader.GetExchangeID(), session, !payloadHeader.IsInitiator(), + nullptr, true /* IsEphemeralExchange */); - // Make sure the exchange stays alive through the code below even if we - // close it before calling HandleMessage. - ExchangeHandle ref(*ec); + if (ec == nullptr) + { + // Using same error message for all errors to reduce code size. + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(CHIP_ERROR_NO_MEMORY)); + return; + } - // No need to verify packet encryption type, the EphemeralExchange can handle both secure and insecure messages. + ChipLogDetail(ExchangeManager, "Generating StandaloneAck via exchange: " ChipLogFormatExchange, ChipLogValueExchange(ec)); - CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf)); - if (err != CHIP_NO_ERROR) - { - // Using same error message for all errors to reduce code size. - ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); - } + // No need to verify packet encryption type, the EphemeralExchange can handle both secure and insecure messages. - // The exchange should be closed inside HandleMessage function. So don't bother close it here. - return; + CHIP_ERROR err = ec->HandleMessage(packetHeader.GetMessageCounter(), payloadHeader, msgFlags, std::move(msgBuf)); + if (err != CHIP_NO_ERROR) + { + // Using same error message for all errors to reduce code size. + ChipLogError(ExchangeManager, "OnMessageReceived failed, err = %s", ErrorStr(err)); } + + // The exchange should be closed inside HandleMessage function. So don't bother close it here. + return; } void ExchangeManager::CloseAllContextsForDelegate(const ExchangeDelegate * delegate) diff --git a/src/messaging/ExchangeMgr.h b/src/messaging/ExchangeMgr.h index 6b9f6c8ad02bc7..d6b775436799f4 100644 --- a/src/messaging/ExchangeMgr.h +++ b/src/messaging/ExchangeMgr.h @@ -240,8 +240,8 @@ class DLL_EXPORT ExchangeManager : public SessionMessageDelegate void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session, DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf) override; - void ReplyStandaloneAck(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session, - MessageFlags msgFlags, System::PacketBufferHandle && msgBuf); + void SendStandaloneAckIfNeeded(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, + const SessionHandle & session, MessageFlags msgFlags, System::PacketBufferHandle && msgBuf); }; } // namespace Messaging