ReadClient: Truncate data version list during encoding if necessary (#34111)
* 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.
* Apply comment suggestions from code review
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
* Treat CHIP_ERROR_BUFFER_TOO_SMALL the same
* Switch data_model tests to pw_unit_test
* Add WillSendMessage to loopback delegate and make source addresses more plausible
* Add test for ReadClient data version truncation
* Make the linter happy
---------
Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
index 78b0a56..470c875 100644
--- a/src/app/ReadClient.cpp
+++ b/src/app/ReadClient.cpp
@@ -423,33 +423,54 @@
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 || err == CHIP_ERROR_BUFFER_TOO_SMALL)
+ {
+ // 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<AttributePathParams> & aAttributePaths,
const Span<DataVersionFilter> & aDataVersionFilters,
bool & aEncodedDataVersionList)
{
- if (!aDataVersionFilters.empty())
+ // Give the callback a chance first, otherwise use the list we have, if any.
+ 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 219a5be..9bfb628 100644
--- a/src/app/ReadClient.h
+++ b/src/app/ReadClient.h
@@ -339,6 +339,9 @@
* This will send either a Read Request or a Subscribe Request depending on
* the InteractionType this read client was initialized with.
*
+ * 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
* @retval #CHIP_NO_ERROR On success.
*/
@@ -559,6 +562,8 @@
CHIP_ERROR BuildDataVersionFilterList(DataVersionFilterIBs::Builder & aDataVersionFilterIBsBuilder,
const Span<AttributePathParams> & aAttributePaths,
const Span<DataVersionFilter> & 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);
diff --git a/src/controller/tests/data_model/BUILD.gn b/src/controller/tests/data_model/BUILD.gn
index 017980c..d8ce821 100644
--- a/src/controller/tests/data_model/BUILD.gn
+++ b/src/controller/tests/data_model/BUILD.gn
@@ -35,6 +35,7 @@
"${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 b6d39ae..11b7141 100644
--- a/src/controller/tests/data_model/TestCommands.cpp
+++ b/src/controller/tests/data_model/TestCommands.cpp
@@ -22,7 +22,8 @@
*
*/
-#include <gtest/gtest.h>
+#include <lib/core/StringBuilderAdapters.h>
+#include <pw_unit_test/framework.h>
#include "app/data-model/NullObject.h"
#include <app-common/zap-generated/cluster-objects.h>
diff --git a/src/controller/tests/data_model/TestRead.cpp b/src/controller/tests/data_model/TestRead.cpp
index a27eb61..906ba82 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 <gtest/gtest.h>
+#include <lib/core/StringBuilderAdapters.h>
+#include <pw_unit_test/framework.h>
#include "system/SystemClock.h"
#include "transport/SecureSession.h"
@@ -25,6 +26,7 @@
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteEventPath.h>
#include <app/InteractionModelEngine.h>
+#include <app/ReadClient.h>
#include <app/tests/AppTestContext.h>
#include <app/util/mock/Constants.h>
#include <app/util/mock/Functions.h>
@@ -3093,6 +3095,86 @@
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 %llu elements",
+ static_cast<unsigned long long>(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();
diff --git a/src/controller/tests/data_model/TestWrite.cpp b/src/controller/tests/data_model/TestWrite.cpp
index e0f2c19..da5505c 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 <gtest/gtest.h>
+#include <lib/core/StringBuilderAdapters.h>
+#include <pw_unit_test/framework.h>
#include "app-common/zap-generated/ids/Clusters.h"
#include <app-common/zap-generated/cluster-objects.h>
diff --git a/src/messaging/tests/MessagingContext.h b/src/messaging/tests/MessagingContext.h
index 51525f9..f0e950a 100644
--- a/src/messaging/tests/MessagingContext.h
+++ b/src/messaging/tests/MessagingContext.h
@@ -94,7 +94,7 @@
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 4989040..fca2b6b 100644
--- a/src/transport/raw/tests/NetworkTestHelpers.h
+++ b/src/transport/raw/tests/NetworkTestHelpers.h
@@ -64,6 +64,10 @@
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 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 @@
{
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 @@
CHIP_ERROR SendMessage(const Transport::PeerAddress & address, System::PacketBufferHandle && msgBuf) override
{
+ if (mDelegate != nullptr)
+ {
+ mDelegate->WillSendMessage(address, msgBuf);
+ }
+
if (mNumMessagesToAllowBeforeError == 0)
{
ReturnErrorOnFailure(mMessageSendError);
