From 5f31784aaf8379f4cfea0cc47224c4b804b9f460 Mon Sep 17 00:00:00 2001 From: Pradip De Date: Tue, 4 Jun 2024 09:24:04 -0700 Subject: [PATCH 1/3] ReadHandler changes for large payloads When sending reports, if the session established with the peer supports large payloads, the ReadHandler will allocate a large buffer to, potentially, fit more attribute and event data. --- src/app/ReadHandler.cpp | 12 ++++++++++++ src/app/ReadHandler.h | 8 ++++++++ src/app/StatusResponse.h | 3 ++- src/app/reporting/Engine.cpp | 13 ++++++++++--- 4 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index dd4c928a1bafa1..6cad305603dedd 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -919,5 +919,17 @@ void ReadHandler::ClearStateFlag(ReadHandlerFlags aFlag) SetStateFlag(aFlag, false); } +size_t ReadHandler::GetReportBufferMaxSize() +{ + size_t maxBufSize = chip::app::kMaxSecureSduLengthBytes; + Transport::SecureSession * session = GetSession(); + if (session && session->AllowsLargePayload()) + { + maxBufSize = chip::app::kMaxLargeSecureSduLengthBytes; + } + + return maxBufSize; +} + } // namespace app } // namespace chip diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index a30f1f8974ad25..ea74f0e2af7833 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -333,6 +333,14 @@ class ReadHandler : public Messaging::ExchangeDelegate */ CHIP_ERROR SendReportData(System::PacketBufferHandle && aPayload, bool aMoreChunks); + /* + * Get the appropriate size of a packet buffer to allocate for encoding a Report message. + * Depending on the underlying session, which may or may not support large + * payloads, a buffer with the corresponding max size would be allocated. + * + */ + size_t GetReportBufferMaxSize(); + /** * Returns whether this ReadHandler represents a subscription that was created by the other side of the provided exchange. */ diff --git a/src/app/StatusResponse.h b/src/app/StatusResponse.h index 93f2557b679854..e823c33d84f22b 100644 --- a/src/app/StatusResponse.h +++ b/src/app/StatusResponse.h @@ -26,7 +26,8 @@ namespace chip { namespace app { -static constexpr size_t kMaxSecureSduLengthBytes = kMaxAppMessageLen + kMaxTagLen; +static constexpr size_t kMaxSecureSduLengthBytes = kMaxAppMessageLen + kMaxTagLen; +static constexpr size_t kMaxLargeSecureSduLengthBytes = kMaxLargeAppMessageLen + kMaxTagLen; class StatusResponse { diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index 39c582f3e45f0b..beafc68ad6f2bf 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -492,10 +492,11 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) CHIP_ERROR err = CHIP_NO_ERROR; chip::System::PacketBufferTLVWriter reportDataWriter; ReportDataMessage::Builder reportDataBuilder; - chip::System::PacketBufferHandle bufHandle = System::PacketBufferHandle::New(chip::app::kMaxSecureSduLengthBytes); + chip::System::PacketBufferHandle bufHandle = nullptr; uint16_t reservedSize = 0; bool hasMoreChunks = false; bool needCloseReadHandler = false; + size_t maxSduSize = 0; // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag. const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1; @@ -512,11 +513,17 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) VerifyOrExit(apReadHandler != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(apReadHandler->GetSession() != nullptr, err = CHIP_ERROR_INCORRECT_STATE); + + // Depending on whether the session supports large payload or not, the + // appropriate max size would be returned for the Report buffer. + maxSduSize = apReadHandler->GetReportBufferMaxSize(); + + bufHandle = System::PacketBufferHandle::New(maxSduSize); VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY); - if (bufHandle->AvailableDataLength() > kMaxSecureSduLengthBytes) + if (bufHandle->AvailableDataLength() > maxSduSize) { - reservedSize = static_cast(bufHandle->AvailableDataLength() - kMaxSecureSduLengthBytes); + reservedSize = static_cast(bufHandle->AvailableDataLength() - maxSduSize); } reportDataWriter.Init(std::move(bufHandle)); From b0f4ca02457853d1ed99214a0a2a6784698f8339 Mon Sep 17 00:00:00 2001 From: Pradip De Date: Tue, 11 Jun 2024 16:17:30 -0700 Subject: [PATCH 2/3] Address comments and apply suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/ReadHandler.cpp | 4 ++-- src/app/ReadHandler.h | 5 +++-- src/app/reporting/Engine.cpp | 16 +++++++--------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 6cad305603dedd..87dfe111709141 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -921,11 +921,11 @@ void ReadHandler::ClearStateFlag(ReadHandlerFlags aFlag) size_t ReadHandler::GetReportBufferMaxSize() { - size_t maxBufSize = chip::app::kMaxSecureSduLengthBytes; + size_t maxBufSize = kMaxSecureSduLengthBytes; Transport::SecureSession * session = GetSession(); if (session && session->AllowsLargePayload()) { - maxBufSize = chip::app::kMaxLargeSecureSduLengthBytes; + maxBufSize = kMaxLargeSecureSduLengthBytes; } return maxBufSize; diff --git a/src/app/ReadHandler.h b/src/app/ReadHandler.h index ea74f0e2af7833..c973432953275b 100644 --- a/src/app/ReadHandler.h +++ b/src/app/ReadHandler.h @@ -335,9 +335,10 @@ class ReadHandler : public Messaging::ExchangeDelegate /* * Get the appropriate size of a packet buffer to allocate for encoding a Report message. - * Depending on the underlying session, which may or may not support large - * payloads, a buffer with the corresponding max size would be allocated. + * This size might depend on the underlying session used by the ReadHandler. * + * The size returned here is the size not including the various prepended headers + * (what System::PacketBuffer calls the "available size"). */ size_t GetReportBufferMaxSize(); diff --git a/src/app/reporting/Engine.cpp b/src/app/reporting/Engine.cpp index beafc68ad6f2bf..dfa64cf3b8f9de 100644 --- a/src/app/reporting/Engine.cpp +++ b/src/app/reporting/Engine.cpp @@ -496,7 +496,7 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) uint16_t reservedSize = 0; bool hasMoreChunks = false; bool needCloseReadHandler = false; - size_t maxSduSize = 0; + size_t reportBufferMaxSize = 0; // Reserved size for the MoreChunks boolean flag, which takes up 1 byte for the control tag and 1 byte for the context tag. const uint32_t kReservedSizeForMoreChunksFlag = 1 + 1; @@ -514,16 +514,14 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) VerifyOrExit(apReadHandler != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT); VerifyOrExit(apReadHandler->GetSession() != nullptr, err = CHIP_ERROR_INCORRECT_STATE); - // Depending on whether the session supports large payload or not, the - // appropriate max size would be returned for the Report buffer. - maxSduSize = apReadHandler->GetReportBufferMaxSize(); + reportBufferMaxSize = apReadHandler->GetReportBufferMaxSize(); - bufHandle = System::PacketBufferHandle::New(maxSduSize); + bufHandle = System::PacketBufferHandle::New(reportBufferMaxSize); VerifyOrExit(!bufHandle.IsNull(), err = CHIP_ERROR_NO_MEMORY); - if (bufHandle->AvailableDataLength() > maxSduSize) + if (bufHandle->AvailableDataLength() > reportBufferMaxSize) { - reservedSize = static_cast(bufHandle->AvailableDataLength() - maxSduSize); + reservedSize = static_cast(bufHandle->AvailableDataLength() - reportBufferMaxSize); } reportDataWriter.Init(std::move(bufHandle)); @@ -532,8 +530,8 @@ CHIP_ERROR Engine::BuildAndSendSingleReportData(ReadHandler * apReadHandler) reportDataWriter.ReserveBuffer(mReservedSize); #endif - // Always limit the size of the generated packet to fit within kMaxSecureSduLengthBytes regardless of the available buffer - // capacity. + // Always limit the size of the generated packet to fit within the max size returned by the ReadHandler regardless + // of the available buffer capacity. // Also, we need to reserve some extra space for the MIC field. reportDataWriter.ReserveBuffer(static_cast(reservedSize + chip::Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES)); From 821b65f8b511eeedc9afeba87ba37783fca3808d Mon Sep 17 00:00:00 2001 From: Pradip De Date: Wed, 12 Jun 2024 09:59:15 -0700 Subject: [PATCH 3/3] Update src/app/ReadHandler.cpp Co-authored-by: Andrei Litvin --- src/app/ReadHandler.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index 87dfe111709141..487acd27ed883e 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -921,14 +921,12 @@ void ReadHandler::ClearStateFlag(ReadHandlerFlags aFlag) size_t ReadHandler::GetReportBufferMaxSize() { - size_t maxBufSize = kMaxSecureSduLengthBytes; Transport::SecureSession * session = GetSession(); if (session && session->AllowsLargePayload()) { - maxBufSize = kMaxLargeSecureSduLengthBytes; + return kMaxLargeSecureSduLengthBytes; } - - return maxBufSize; + return kMaxSecureSduLengthBytes; } } // namespace app