From 9830f0d27348124949e3ba85fc270ddd7440c6c9 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 27 Jun 2024 08:03:42 -0400 Subject: [PATCH 1/7] ReadClient: Truncate data version list during encoding if necessary The existing code made the assumption that if a list of versions was able to fit into the request packet when generating the first subscribe request, then any resubscribe containing data versions for the same clusters would also fit. However the data version numbers themselves can be updated when we receive reports, and this can cause the list to no longer fit the request packet, leaving us in a state where every resubscribe attempt would fail. Note that this change means even an initial subscribe request with a data version list that is too long will no longer fail; ReadClient will simply truncate the list as needed in all cases. --- src/app/ReadClient.cpp | 49 ++++++++++++++++++++++++++++++------------ src/app/ReadClient.h | 5 +++++ 2 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 78b0a56fdd7c26..142fb65e726b45 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -423,33 +423,54 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder continue; } - DataVersionFilterIB::Builder & filterIB = aDataVersionFilterIBsBuilder.CreateDataVersionFilter(); - ReturnErrorOnFailure(aDataVersionFilterIBsBuilder.GetError()); - ClusterPathIB::Builder & path = filterIB.CreatePath(); - ReturnErrorOnFailure(filterIB.GetError()); - ReturnErrorOnFailure(path.Endpoint(filter.mEndpointId).Cluster(filter.mClusterId).EndOfClusterPathIB()); - VerifyOrReturnError(filter.mDataVersion.HasValue(), CHIP_ERROR_INVALID_ARGUMENT); - ReturnErrorOnFailure(filterIB.DataVersion(filter.mDataVersion.Value()).EndOfDataVersionFilterIB()); - aEncodedDataVersionList = true; + TLV::TLVWriter backup; + aDataVersionFilterIBsBuilder.Checkpoint(backup); + CHIP_ERROR err = EncodeDataVersionFilter(aDataVersionFilterIBsBuilder, filter); + if (err == CHIP_NO_ERROR) + { + aEncodedDataVersionList = true; + } + else if (err == CHIP_ERROR_NO_MEMORY) + { + // Packet is full, ignore the rest of the list + aDataVersionFilterIBsBuilder.Rollback(backup); + return CHIP_NO_ERROR; + } + else + { + return err; + } } return CHIP_NO_ERROR; } +CHIP_ERROR ReadClient::EncodeDataVersionFilter(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder, + DataVersionFilter const & aFilter) +{ + // Caller has checked aFilter.IsValidDataVersionFilter() + DataVersionFilterIB::Builder & filterIB = aDataVersionFilterIBsBuilder.CreateDataVersionFilter(); + ReturnErrorOnFailure(aDataVersionFilterIBsBuilder.GetError()); + ClusterPathIB::Builder & path = filterIB.CreatePath(); + ReturnErrorOnFailure(filterIB.GetError()); + ReturnErrorOnFailure(path.Endpoint(aFilter.mEndpointId).Cluster(aFilter.mClusterId).EndOfClusterPathIB()); + ReturnErrorOnFailure(filterIB.DataVersion(aFilter.mDataVersion.Value()).EndOfDataVersionFilterIB()); + return CHIP_NO_ERROR; +} + CHIP_ERROR ReadClient::GenerateDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder, const Span & aAttributePaths, const Span & aDataVersionFilters, bool & aEncodedDataVersionList) { - if (!aDataVersionFilters.empty()) + // Give the callback a chance first, otherwise use the list we have. + ReturnErrorOnFailure( + mpCallback.OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList)); + + if (!aEncodedDataVersionList) { ReturnErrorOnFailure(BuildDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aDataVersionFilters, aEncodedDataVersionList)); } - else - { - ReturnErrorOnFailure( - mpCallback.OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList)); - } return CHIP_NO_ERROR; } diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index 219a5be73f0b09..e2a072767f289a 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -339,6 +339,9 @@ class ReadClient : public Messaging::ExchangeDelegate * This will send either a Read Request or a Subscribe Request depending on * the InteractionType this read client was initialized with. * + * If the requests contains more data version filters than can fit in the request packet + * the list will be truncated as needed, i.e. filter inclusion is on a best effort basis. + * * @retval #others fail to send read request * @retval #CHIP_NO_ERROR On success. */ @@ -559,6 +562,8 @@ class ReadClient : public Messaging::ExchangeDelegate CHIP_ERROR BuildDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder, const Span & aAttributePaths, const Span & aDataVersionFilters, bool & aEncodedDataVersionList); + CHIP_ERROR EncodeDataVersionFilter(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder, + DataVersionFilter const & aFilter); CHIP_ERROR ReadICDOperatingModeFromAttributeDataIB(TLV::TLVReader && aReader, PeerType & aType); CHIP_ERROR ProcessAttributeReportIBs(TLV::TLVReader & aAttributeDataIBsReader); CHIP_ERROR ProcessEventReportIBs(TLV::TLVReader & aEventReportIBsReader); From 15376cc719aa0dca436132a93bb7679f59f8db6c Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:57:09 -0400 Subject: [PATCH 2/7] Apply comment suggestions from code review Co-authored-by: Boris Zbarsky --- src/app/ReadClient.cpp | 2 +- src/app/ReadClient.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 142fb65e726b45..e6953ac04efc9b 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -462,7 +462,7 @@ CHIP_ERROR ReadClient::GenerateDataVersionFilterList(DataVersionFilterIBs::Build const Span & aDataVersionFilters, bool & aEncodedDataVersionList) { - // Give the callback a chance first, otherwise use the list we have. + // Give the callback a chance first, otherwise use the list we have, if any. ReturnErrorOnFailure( mpCallback.OnUpdateDataVersionFilterList(aDataVersionFilterIBsBuilder, aAttributePaths, aEncodedDataVersionList)); diff --git a/src/app/ReadClient.h b/src/app/ReadClient.h index e2a072767f289a..9bfb628b47e252 100644 --- a/src/app/ReadClient.h +++ b/src/app/ReadClient.h @@ -339,7 +339,7 @@ class ReadClient : public Messaging::ExchangeDelegate * This will send either a Read Request or a Subscribe Request depending on * the InteractionType this read client was initialized with. * - * If the requests contains more data version filters than can fit in the request packet + * If the params contain more data version filters than can fit in the request packet * the list will be truncated as needed, i.e. filter inclusion is on a best effort basis. * * @retval #others fail to send read request From 6944997294e1f2f415832ce67cf077e6c7abcab8 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Thu, 27 Jun 2024 14:58:04 -0400 Subject: [PATCH 3/7] Treat CHIP_ERROR_BUFFER_TOO_SMALL the same --- src/app/ReadClient.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index e6953ac04efc9b..470c875fbaac30 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -430,7 +430,7 @@ CHIP_ERROR ReadClient::BuildDataVersionFilterList(DataVersionFilterIBs::Builder { aEncodedDataVersionList = true; } - else if (err == CHIP_ERROR_NO_MEMORY) + else if (err == CHIP_ERROR_NO_MEMORY || err == CHIP_ERROR_BUFFER_TOO_SMALL) { // Packet is full, ignore the rest of the list aDataVersionFilterIBsBuilder.Rollback(backup); From c697cad9c3e2ce7ad038acdc493e9427060dbe16 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Sat, 29 Jun 2024 19:08:28 -0700 Subject: [PATCH 4/7] Switch data_model tests to pw_unit_test --- src/controller/tests/data_model/BUILD.gn | 1 + src/controller/tests/data_model/TestCommands.cpp | 3 ++- src/controller/tests/data_model/TestRead.cpp | 3 ++- src/controller/tests/data_model/TestWrite.cpp | 3 ++- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/controller/tests/data_model/BUILD.gn b/src/controller/tests/data_model/BUILD.gn index 017980c4cd11f7..d8ce821ed837af 100644 --- a/src/controller/tests/data_model/BUILD.gn +++ b/src/controller/tests/data_model/BUILD.gn @@ -35,6 +35,7 @@ chip_test_suite("data_model") { "${chip_root}/src/app/tests:helpers", "${chip_root}/src/app/util/mock:mock_ember", "${chip_root}/src/controller", + "${chip_root}/src/lib/core:string-builder-adapters", "${chip_root}/src/messaging/tests:helpers", "${chip_root}/src/transport/raw/tests:helpers", ] diff --git a/src/controller/tests/data_model/TestCommands.cpp b/src/controller/tests/data_model/TestCommands.cpp index b6d39ae41978bc..11b71415ad6ec5 100644 --- a/src/controller/tests/data_model/TestCommands.cpp +++ b/src/controller/tests/data_model/TestCommands.cpp @@ -22,7 +22,8 @@ * */ -#include +#include +#include #include "app/data-model/NullObject.h" #include diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index a27eb61fcb2901..5c5a20da8c8e8b 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -16,7 +16,8 @@ * limitations under the License. */ -#include +#include +#include #include "system/SystemClock.h" #include "transport/SecureSession.h" diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp index e0f2c19480a1b0..da5505ccd613a1 100644 --- a/src/controller/tests/data_model/TestWrite.cpp +++ b/src/controller/tests/data_model/TestWrite.cpp @@ -16,7 +16,8 @@ * limitations under the License. */ -#include +#include +#include #include "app-common/zap-generated/ids/Clusters.h" #include From fbf6bab2c43505998220505ac3232a47d4cc6881 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Sat, 29 Jun 2024 19:08:57 -0700 Subject: [PATCH 5/7] Add WillSendMessage to loopback delegate and make source addresses more plausible --- src/messaging/tests/MessagingContext.h | 2 +- src/transport/raw/tests/NetworkTestHelpers.h | 23 +++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h index 51525f967d1475..f0e950a2cfc2a9 100644 --- a/src/messaging/tests/MessagingContext.h +++ b/src/messaging/tests/MessagingContext.h @@ -94,7 +94,7 @@ class MessagingContext : public PlatformMemoryUser MessagingContext() : mInitialized(false), mAliceAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT + 1)), - mBobAddress(Transport::PeerAddress::UDP(GetAddress(), CHIP_PORT)) + mBobAddress(LoopbackTransport::LoopbackPeer(mAliceAddress)) {} // TODO Replace VerifyOrDie with Pigweed assert after transition app/tests to Pigweed. // TODO Currently src/app/icd/server/tests is using MessagingConetext as dependency. diff --git a/src/transport/raw/tests/NetworkTestHelpers.h b/src/transport/raw/tests/NetworkTestHelpers.h index 49890406bc6afb..fca2b6b04cfd7c 100644 --- a/src/transport/raw/tests/NetworkTestHelpers.h +++ b/src/transport/raw/tests/NetworkTestHelpers.h @@ -64,6 +64,10 @@ class LoopbackTransportDelegate public: virtual ~LoopbackTransportDelegate() {} + // Called by the loopback transport when a message is requested to be sent. + // This is called even if the message is subsequently rejected or dropped. + virtual void WillSendMessage(const Transport::PeerAddress & peer, const System::PacketBufferHandle & message) {} + // Called by the loopback transport when it drops one of a configurable number of messages (mDroppedMessageCount) after a // configurable allowed number of messages (mNumMessagesToAllowBeforeDropping) virtual void OnMessageDropped() {} @@ -72,6 +76,18 @@ class LoopbackTransportDelegate class LoopbackTransport : public Transport::Base { public: + // In test scenarios using the loopback transport, we're only ever given + // the address we're sending to, but we don't have any information about + // what our local address is. Assume our fake addresses come in pairs of + // even and odd port numbers, so we can calculate one from the other by + // flipping the LSB of the port number. + static Transport::PeerAddress LoopbackPeer(const Transport::PeerAddress & address) + { + Transport::PeerAddress other(address); + other.SetPort(address.GetPort() ^ 1); + return other; + } + void InitLoopbackTransport(System::Layer * systemLayer) { Reset(); @@ -100,7 +116,7 @@ class LoopbackTransport : public Transport::Base { auto item = std::move(_this->mPendingMessageQueue.front()); _this->mPendingMessageQueue.pop(); - _this->HandleMessageReceived(item.mDestinationAddress, std::move(item.mPendingMessage)); + _this->HandleMessageReceived(LoopbackPeer(item.mDestinationAddress), std::move(item.mPendingMessage)); } } @@ -108,6 +124,11 @@ class LoopbackTransport : public Transport::Base CHIP_ERROR SendMessage(const Transport::PeerAddress & address, System::PacketBufferHandle && msgBuf) override { + if (mDelegate != nullptr) + { + mDelegate->WillSendMessage(address, msgBuf); + } + if (mNumMessagesToAllowBeforeError == 0) { ReturnErrorOnFailure(mMessageSendError); From 0d9cbc2ef28d316c89f6aab83aae24b4e66eb926 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Sat, 29 Jun 2024 19:10:56 -0700 Subject: [PATCH 6/7] Add test for ReadClient data version truncation --- src/controller/tests/data_model/TestRead.cpp | 80 ++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index 5c5a20da8c8e8b..f449e41613b453 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -3094,6 +3095,85 @@ TEST_F(TestRead, TestReadHandler_MultipleSubscriptionsWithDataVersionFilter) EXPECT_EQ(mpContext->GetExchangeManager().GetNumActiveExchanges(), 0u); } +TEST_F(TestRead, TestReadHandler_DataVersionFiltersTruncated) +{ + struct : public chip::Test::LoopbackTransportDelegate + { + size_t requestSize = 0; + void WillSendMessage(const Transport::PeerAddress & peer, const System::PacketBufferHandle & message) override + { + // We only care about the messages we (Alice) send to Bob, not the responses. + // Assume the first message we see in an iteration is the request. + if (peer == mpContext->GetBobAddress() && requestSize == 0) + { + requestSize = message->TotalLength(); + } + } + } loopbackDelegate; + mpContext->GetLoopback().SetLoopbackTransportDelegate(&loopbackDelegate); + + // Note that on the server side, wildcard expansion does not actually work for kTestEndpointId due + // to lack of meta-data, but we don't care about the reports we get back in this test. + AttributePathParams wildcardPath(kTestEndpointId, kInvalidClusterId, kInvalidAttributeId); + constexpr size_t maxDataVersionFilterCount = 100; + DataVersionFilter dataVersionFilters[maxDataVersionFilterCount]; + ClusterId nextClusterId = 0; + for (auto & dv : dataVersionFilters) + { + dv.mEndpointId = wildcardPath.mEndpointId; + dv.mClusterId = nextClusterId++; + dv.mDataVersion = MakeOptional(0x01000000u); + } + + // Keep increasing the number of data version filters until we see truncation kick in. + size_t lastRequestSize; + for (size_t count = 1; count <= maxDataVersionFilterCount; count++) + { + lastRequestSize = loopbackDelegate.requestSize; + loopbackDelegate.requestSize = 0; // reset + + ReadPrepareParams read(mpContext->GetSessionAliceToBob()); + read.mpAttributePathParamsList = &wildcardPath; + read.mAttributePathParamsListSize = 1; + read.mpDataVersionFilterList = dataVersionFilters; + read.mDataVersionFilterListSize = count; + + struct : public ReadClient::Callback + { + CHIP_ERROR error = CHIP_NO_ERROR; + bool done = false; + void OnError(CHIP_ERROR aError) override { error = aError; } + void OnDone(ReadClient * apReadClient) override { done = true; }; + + } readCallback; + + ReadClient readClient(app::InteractionModelEngine::GetInstance(), &mpContext->GetExchangeManager(), readCallback, + ReadClient::InteractionType::Read); + + EXPECT_EQ(readClient.SendRequest(read), CHIP_NO_ERROR); + + mpContext->GetIOContext().DriveIOUntil(System::Clock::Seconds16(5), [&]() { return readCallback.done; }); + EXPECT_EQ(readCallback.error, CHIP_NO_ERROR); + EXPECT_EQ(mpContext->GetExchangeManager().GetNumActiveExchanges(), 0u); + + EXPECT_NE(loopbackDelegate.requestSize, 0u); + EXPECT_GE(loopbackDelegate.requestSize, lastRequestSize); + if (loopbackDelegate.requestSize == lastRequestSize) + { + ChipLogProgress(DataManagement, "Data Version truncation detected after %zu elements", count - 1); + // With the parameters used in this test and current encoding rules we can fit 68 data versions + // into a packet. If we're seeing substantially less then something is likely gone wrong. + EXPECT_GE(count, 60u); + ExitNow(); + } + } + ChipLogProgress(DataManagement, "Unable to detect Data Version truncation, maxDataVersionFilterCount too small?"); + ADD_FAILURE(); + +exit: + mpContext->GetLoopback().SetLoopbackTransportDelegate(nullptr); +} + TEST_F(TestRead, TestReadHandlerResourceExhaustion_MultipleReads) { auto sessionHandle = mpContext->GetSessionBobToAlice(); From 4d8ea7f869e942668340bd0a752b9a62f129dca4 Mon Sep 17 00:00:00 2001 From: Karsten Sperling Date: Tue, 2 Jul 2024 12:23:52 +1200 Subject: [PATCH 7/7] Make the linter happy --- src/controller/tests/data_model/TestRead.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp index f449e41613b453..906ba82a213042 100644 --- a/src/controller/tests/data_model/TestRead.cpp +++ b/src/controller/tests/data_model/TestRead.cpp @@ -3160,7 +3160,8 @@ TEST_F(TestRead, TestReadHandler_DataVersionFiltersTruncated) EXPECT_GE(loopbackDelegate.requestSize, lastRequestSize); if (loopbackDelegate.requestSize == lastRequestSize) { - ChipLogProgress(DataManagement, "Data Version truncation detected after %zu elements", count - 1); + ChipLogProgress(DataManagement, "Data Version truncation detected after %llu elements", + static_cast(count - 1)); // With the parameters used in this test and current encoding rules we can fit 68 data versions // into a packet. If we're seeing substantially less then something is likely gone wrong. EXPECT_GE(count, 60u);