Make client handling of invalid IDs a bit more lenient. (#30452)
Before this change, if a server happened to send an attribute report with an
invalid cluster or attribute id (very common for vendor-prefixed things that
people set up incorrectly), the entire read or subscription would fail and no
reports would be processed after the invalid id. This causes wildcard
subscriptions to completely fail if the device happens to have an invalid path
configured somewhere.
The new behavior is to completely skip that one AttributeReportIB and process
the other ones in the list.
Co-authored-by: Andrei Litvin <andy314@gmail.com>
diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h
index 312311f..b06a4b8 100644
--- a/src/app/ConcreteAttributePath.h
+++ b/src/app/ConcreteAttributePath.h
@@ -48,6 +48,8 @@
mExpanded = false;
}
+ bool IsValid() const { return ConcreteClusterPath::HasValidIds() && IsValidAttributeId(mAttributeId); }
+
bool operator==(const ConcreteAttributePath & aOther) const
{
return ConcreteClusterPath::operator==(aOther) && (mAttributeId == aOther.mAttributeId);
diff --git a/src/app/ConcreteClusterPath.h b/src/app/ConcreteClusterPath.h
index 6865fdb..58b2f5b 100644
--- a/src/app/ConcreteClusterPath.h
+++ b/src/app/ConcreteClusterPath.h
@@ -38,6 +38,8 @@
bool IsValidConcreteClusterPath() const { return !(mEndpointId == kInvalidEndpointId || mClusterId == kInvalidClusterId); }
+ bool HasValidIds() const { return IsValidEndpointId(mEndpointId) && IsValidClusterId(mClusterId); }
+
bool operator==(const ConcreteClusterPath & aOther) const
{
return mEndpointId == aOther.mEndpointId && mClusterId == aOther.mClusterId;
diff --git a/src/app/MessageDef/AttributePathIB.cpp b/src/app/MessageDef/AttributePathIB.cpp
index db9e4cd..52e7c6b 100644
--- a/src/app/MessageDef/AttributePathIB.cpp
+++ b/src/app/MessageDef/AttributePathIB.cpp
@@ -171,13 +171,17 @@
return GetNullableUnsignedInteger(to_underlying(Tag::kListIndex), apListIndex);
}
-CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const
+CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath,
+ ValidateIdRanges aValidateRanges) const
{
ReturnErrorOnFailure(GetCluster(&aAttributePath.mClusterId));
- VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
-
ReturnErrorOnFailure(GetAttribute(&aAttributePath.mAttributeId));
- VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+
+ if (aValidateRanges == ValidateIdRanges::kYes)
+ {
+ VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+ VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+ }
CHIP_ERROR err = CHIP_NO_ERROR;
DataModel::Nullable<ListIndex> listIndex;
@@ -204,9 +208,10 @@
return err;
}
-CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const
+CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath,
+ ValidateIdRanges aValidateRanges) const
{
- ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath));
+ ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath, aValidateRanges));
// And now read our endpoint.
return GetEndpoint(&aAttributePath.mEndpointId);
diff --git a/src/app/MessageDef/AttributePathIB.h b/src/app/MessageDef/AttributePathIB.h
index 5035bbd..e1740d3 100644
--- a/src/app/MessageDef/AttributePathIB.h
+++ b/src/app/MessageDef/AttributePathIB.h
@@ -45,6 +45,12 @@
kListIndex = 5,
};
+enum class ValidateIdRanges : uint8_t
+{
+ kYes,
+ kNo,
+};
+
class Parser : public ListParser
{
public:
@@ -136,10 +142,13 @@
* as ReplaceAll if that's appropriate to their context.
*
* @param [in] aAttributePath The attribute path object to write to.
+ * @param [in] aValidateRanges Whether to validate that the cluster/attribute
+ * IDs in the path are in the right ranges.
*
* @return #CHIP_NO_ERROR on success
*/
- CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const;
+ CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath,
+ ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const;
/**
* @brief Get a group attribute path. This will set the ListOp to
@@ -148,10 +157,13 @@
* endpoint id of the resulting path might have any value.
*
* @param [in] aAttributePath The attribute path object to write to.
+ * @param [in] aValidateRanges Whether to validate that the cluster/attribute
+ * IDs in the path are in the right ranges.
*
* @return #CHIP_NO_ERROR on success
*/
- CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const;
+ CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath,
+ ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const;
// TODO(#14934) Add a function to get ConcreteDataAttributePath from AttributePathIB::Parser directly.
diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp
index 373d205..bbc1c68 100644
--- a/src/app/ReadClient.cpp
+++ b/src/app/ReadClient.cpp
@@ -641,9 +641,12 @@
CHIP_ERROR ReadClient::ProcessAttributePath(AttributePathIB::Parser & aAttributePathParser,
ConcreteDataAttributePath & aAttributePath)
{
+ // The ReportData must contain a concrete attribute path. Don't validate ID
+ // ranges here, so we can tell apart "malformed data" and "out of range
+ // IDs".
CHIP_ERROR err = CHIP_NO_ERROR;
// The ReportData must contain a concrete attribute path
- err = aAttributePathParser.GetConcreteAttributePath(aAttributePath);
+ err = aAttributePathParser.GetConcreteAttributePath(aAttributePath, AttributePathIB::ValidateIdRanges::kNo);
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB);
return CHIP_NO_ERROR;
}
@@ -679,6 +682,17 @@
StatusIB::Parser errorStatus;
ReturnErrorOnFailure(status.GetPath(&path));
ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
+ if (!attributePath.IsValid())
+ {
+ // Don't fail the entire read or subscription when there is an
+ // out-of-range ID. Just skip that one AttributeReportIB.
+ ChipLogError(DataManagement,
+ "Skipping AttributeStatusIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ",
+ attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId),
+ ChipLogValueMEI(attributePath.mAttributeId));
+ continue;
+ }
+
ReturnErrorOnFailure(status.GetErrorStatus(&errorStatus));
ReturnErrorOnFailure(errorStatus.DecodeStatusIB(statusIB));
NoteReportingData();
@@ -689,6 +703,17 @@
ReturnErrorOnFailure(report.GetAttributeData(&data));
ReturnErrorOnFailure(data.GetPath(&path));
ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
+ if (!attributePath.IsValid())
+ {
+ // Don't fail the entire read or subscription when there is an
+ // out-of-range ID. Just skip that one AttributeReportIB.
+ ChipLogError(DataManagement,
+ "Skipping AttributeDataIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ",
+ attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId),
+ ChipLogValueMEI(attributePath.mAttributeId));
+ continue;
+ }
+
DataVersion version = 0;
ReturnErrorOnFailure(data.GetDataVersion(&version));
attributePath.mDataVersion.SetValue(version);
diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp
index 25f2611..dd163bc 100644
--- a/src/app/tests/TestReadInteraction.cpp
+++ b/src/app/tests/TestReadInteraction.cpp
@@ -333,6 +333,7 @@
static void TestReadClientGenerateOneEventPaths(nlTestSuite * apSuite, void * apContext);
static void TestReadClientGenerateTwoEventPaths(nlTestSuite * apSuite, void * apContext);
static void TestReadClientInvalidReport(nlTestSuite * apSuite, void * apContext);
+ static void TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext);
static void TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext);
static void TestProcessSubscribeRequest(nlTestSuite * apSuite, void * apContext);
#if CHIP_CONFIG_ENABLE_ICD_SERVER
@@ -390,12 +391,19 @@
static void TestReadHandlerMalformedSubscribeRequest(nlTestSuite * apSuite, void * apContext);
private:
+ enum class ReportType : uint8_t
+ {
+ kValid,
+ kInvalidNoAttributeId,
+ kInvalidOutOfRangeAttributeId,
+ };
+
static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
- bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId);
+ ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId);
};
void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
- bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId = false)
+ ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId = false)
{
CHIP_ERROR err = CHIP_NO_ERROR;
System::PacketBufferTLVWriter writer;
@@ -428,12 +436,17 @@
AttributePathIB::Builder & attributePathBuilder = attributeDataIBBuilder.CreatePath();
NL_TEST_ASSERT(apSuite, attributeDataIBBuilder.GetError() == CHIP_NO_ERROR);
- if (aNeedInvalidReport)
+ if (aReportType == ReportType::kInvalidNoAttributeId)
{
attributePathBuilder.Node(1).Endpoint(2).Cluster(3).ListIndex(5).EndOfAttributePathIB();
}
+ else if (aReportType == ReportType::kInvalidOutOfRangeAttributeId)
+ {
+ attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(0xFFF18000).EndOfAttributePathIB();
+ }
else
{
+ NL_TEST_ASSERT(apSuite, aReportType == ReportType::kValid);
attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(4).EndOfAttributePathIB();
}
@@ -496,7 +509,7 @@
ctx.GetLoopback().mNumMessagesToDrop = 1;
ctx.DrainAndServiceIO();
- GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/);
+ GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/);
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
}
@@ -521,8 +534,7 @@
ctx.DrainAndServiceIO();
// For read, we don't expect there is subscription id in report data.
- GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/,
- true /*aHasSubscriptionId*/);
+ GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/, true /*aHasSubscriptionId*/);
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT);
}
@@ -547,7 +559,7 @@
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read,
app::reporting::GetDefaultReportScheduler());
- GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
+ GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/);
err = readHandler.SendReportData(std::move(reportDatabuf), false);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);
@@ -665,12 +677,46 @@
ctx.GetLoopback().mNumMessagesToDrop = 1;
ctx.DrainAndServiceIO();
- GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/, true /* aSuppressResponse*/);
+ GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidNoAttributeId, true /* aSuppressResponse*/);
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB);
}
+void TestReadInteraction::TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext)
+{
+ CHIP_ERROR err = CHIP_NO_ERROR;
+ TestContext & ctx = *static_cast<TestContext *>(apContext);
+ MockInteractionModelApp delegate;
+
+ System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
+
+ app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate,
+ chip::app::ReadClient::InteractionType::Read);
+
+ ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice());
+ err = readClient.SendRequest(readPrepareParams);
+ NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+ // We don't actually want to deliver that message, because we want to
+ // synthesize the read response. But we don't want it hanging around
+ // forever either.
+ ctx.GetLoopback().mNumMessagesToDrop = 1;
+ ctx.DrainAndServiceIO();
+
+ GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidOutOfRangeAttributeId, true /* aSuppressResponse*/);
+
+ err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
+ // Overall processing should succeed.
+ NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
+
+ // We should not have gotten any attribute reports or errors.
+ NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);
+ NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0);
+ NL_TEST_ASSERT(apSuite, !delegate.mGotReport);
+ NL_TEST_ASSERT(apSuite, !delegate.mReadError);
+}
+
void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext)
{
CHIP_ERROR err = CHIP_NO_ERROR;
@@ -691,7 +737,7 @@
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read,
app::reporting::GetDefaultReportScheduler());
- GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
+ GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/);
err = readHandler.SendReportData(std::move(reportDatabuf), false);
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);
@@ -4941,6 +4987,7 @@
NL_TEST_DEF("TestReadClientGenerateOneEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateOneEventPaths),
NL_TEST_DEF("TestReadClientGenerateTwoEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateTwoEventPaths),
NL_TEST_DEF("TestReadClientInvalidReport", chip::app::TestReadInteraction::TestReadClientInvalidReport),
+ NL_TEST_DEF("TestReadClientInvalidAttributeId", chip::app::TestReadInteraction::TestReadClientInvalidAttributeId),
NL_TEST_DEF("TestReadHandlerInvalidAttributePath", chip::app::TestReadInteraction::TestReadHandlerInvalidAttributePath),
NL_TEST_DEF("TestProcessSubscribeRequest", chip::app::TestReadInteraction::TestProcessSubscribeRequest),
/*
