Skip to content

Commit 0d16348

Browse files
bzbarsky-appleandy31415
bzbarsky-apple
and
andy31415
authored
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 <[email protected]>
1 parent 8dce277 commit 0d16348

File tree

6 files changed

+111
-18
lines changed

6 files changed

+111
-18
lines changed

‎src/app/ConcreteAttributePath.h

+2
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ struct ConcreteAttributePath : public ConcreteClusterPath
4848
mExpanded = false;
4949
}
5050

51+
bool IsValid() const { return ConcreteClusterPath::HasValidIds() && IsValidAttributeId(mAttributeId); }
52+
5153
bool operator==(const ConcreteAttributePath & aOther) const
5254
{
5355
return ConcreteClusterPath::operator==(aOther) && (mAttributeId == aOther.mAttributeId);

‎src/app/ConcreteClusterPath.h

+2
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,8 @@ struct ConcreteClusterPath
3838

3939
bool IsValidConcreteClusterPath() const { return !(mEndpointId == kInvalidEndpointId || mClusterId == kInvalidClusterId); }
4040

41+
bool HasValidIds() const { return IsValidEndpointId(mEndpointId) && IsValidClusterId(mClusterId); }
42+
4143
bool operator==(const ConcreteClusterPath & aOther) const
4244
{
4345
return mEndpointId == aOther.mEndpointId && mClusterId == aOther.mClusterId;

‎src/app/MessageDef/AttributePathIB.cpp

+11-6
Original file line numberDiff line numberDiff line change
@@ -171,13 +171,17 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(DataModel::Nullable<ListIndex>
171171
return GetNullableUnsignedInteger(to_underlying(Tag::kListIndex), apListIndex);
172172
}
173173

174-
CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const
174+
CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath,
175+
ValidateIdRanges aValidateRanges) const
175176
{
176177
ReturnErrorOnFailure(GetCluster(&aAttributePath.mClusterId));
177-
VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
178-
179178
ReturnErrorOnFailure(GetAttribute(&aAttributePath.mAttributeId));
180-
VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
179+
180+
if (aValidateRanges == ValidateIdRanges::kYes)
181+
{
182+
VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
183+
VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
184+
}
181185

182186
CHIP_ERROR err = CHIP_NO_ERROR;
183187
DataModel::Nullable<ListIndex> listIndex;
@@ -204,9 +208,10 @@ CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributeP
204208
return err;
205209
}
206210

207-
CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const
211+
CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath,
212+
ValidateIdRanges aValidateRanges) const
208213
{
209-
ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath));
214+
ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath, aValidateRanges));
210215

211216
// And now read our endpoint.
212217
return GetEndpoint(&aAttributePath.mEndpointId);

‎src/app/MessageDef/AttributePathIB.h

+14-2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,12 @@ enum class Tag : uint8_t
4545
kListIndex = 5,
4646
};
4747

48+
enum class ValidateIdRanges : uint8_t
49+
{
50+
kYes,
51+
kNo,
52+
};
53+
4854
class Parser : public ListParser
4955
{
5056
public:
@@ -136,10 +142,13 @@ class Parser : public ListParser
136142
* as ReplaceAll if that's appropriate to their context.
137143
*
138144
* @param [in] aAttributePath The attribute path object to write to.
145+
* @param [in] aValidateRanges Whether to validate that the cluster/attribute
146+
* IDs in the path are in the right ranges.
139147
*
140148
* @return #CHIP_NO_ERROR on success
141149
*/
142-
CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const;
150+
CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath,
151+
ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const;
143152

144153
/**
145154
* @brief Get a group attribute path. This will set the ListOp to
@@ -148,10 +157,13 @@ class Parser : public ListParser
148157
* endpoint id of the resulting path might have any value.
149158
*
150159
* @param [in] aAttributePath The attribute path object to write to.
160+
* @param [in] aValidateRanges Whether to validate that the cluster/attribute
161+
* IDs in the path are in the right ranges.
151162
*
152163
* @return #CHIP_NO_ERROR on success
153164
*/
154-
CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const;
165+
CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath,
166+
ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const;
155167

156168
// TODO(#14934) Add a function to get ConcreteDataAttributePath from AttributePathIB::Parser directly.
157169

‎src/app/ReadClient.cpp

+26-1
Original file line numberDiff line numberDiff line change
@@ -641,9 +641,12 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
641641
CHIP_ERROR ReadClient::ProcessAttributePath(AttributePathIB::Parser & aAttributePathParser,
642642
ConcreteDataAttributePath & aAttributePath)
643643
{
644+
// The ReportData must contain a concrete attribute path. Don't validate ID
645+
// ranges here, so we can tell apart "malformed data" and "out of range
646+
// IDs".
644647
CHIP_ERROR err = CHIP_NO_ERROR;
645648
// The ReportData must contain a concrete attribute path
646-
err = aAttributePathParser.GetConcreteAttributePath(aAttributePath);
649+
err = aAttributePathParser.GetConcreteAttributePath(aAttributePath, AttributePathIB::ValidateIdRanges::kNo);
647650
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB);
648651
return CHIP_NO_ERROR;
649652
}
@@ -679,6 +682,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
679682
StatusIB::Parser errorStatus;
680683
ReturnErrorOnFailure(status.GetPath(&path));
681684
ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
685+
if (!attributePath.IsValid())
686+
{
687+
// Don't fail the entire read or subscription when there is an
688+
// out-of-range ID. Just skip that one AttributeReportIB.
689+
ChipLogError(DataManagement,
690+
"Skipping AttributeStatusIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ",
691+
attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId),
692+
ChipLogValueMEI(attributePath.mAttributeId));
693+
continue;
694+
}
695+
682696
ReturnErrorOnFailure(status.GetErrorStatus(&errorStatus));
683697
ReturnErrorOnFailure(errorStatus.DecodeStatusIB(statusIB));
684698
NoteReportingData();
@@ -689,6 +703,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
689703
ReturnErrorOnFailure(report.GetAttributeData(&data));
690704
ReturnErrorOnFailure(data.GetPath(&path));
691705
ReturnErrorOnFailure(ProcessAttributePath(path, attributePath));
706+
if (!attributePath.IsValid())
707+
{
708+
// Don't fail the entire read or subscription when there is an
709+
// out-of-range ID. Just skip that one AttributeReportIB.
710+
ChipLogError(DataManagement,
711+
"Skipping AttributeDataIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ",
712+
attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId),
713+
ChipLogValueMEI(attributePath.mAttributeId));
714+
continue;
715+
}
716+
692717
DataVersion version = 0;
693718
ReturnErrorOnFailure(data.GetDataVersion(&version));
694719
attributePath.mDataVersion.SetValue(version);

‎src/app/tests/TestReadInteraction.cpp

+56-9
Original file line numberDiff line numberDiff line change
@@ -333,6 +333,7 @@ class TestReadInteraction
333333
static void TestReadClientGenerateOneEventPaths(nlTestSuite * apSuite, void * apContext);
334334
static void TestReadClientGenerateTwoEventPaths(nlTestSuite * apSuite, void * apContext);
335335
static void TestReadClientInvalidReport(nlTestSuite * apSuite, void * apContext);
336+
static void TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext);
336337
static void TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext);
337338
static void TestProcessSubscribeRequest(nlTestSuite * apSuite, void * apContext);
338339
#if CHIP_CONFIG_ENABLE_ICD_SERVER
@@ -390,12 +391,19 @@ class TestReadInteraction
390391
static void TestReadHandlerMalformedSubscribeRequest(nlTestSuite * apSuite, void * apContext);
391392

392393
private:
394+
enum class ReportType : uint8_t
395+
{
396+
kValid,
397+
kInvalidNoAttributeId,
398+
kInvalidOutOfRangeAttributeId,
399+
};
400+
393401
static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
394-
bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId);
402+
ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId);
395403
};
396404

397405
void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload,
398-
bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId = false)
406+
ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId = false)
399407
{
400408
CHIP_ERROR err = CHIP_NO_ERROR;
401409
System::PacketBufferTLVWriter writer;
@@ -428,12 +436,17 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon
428436
AttributePathIB::Builder & attributePathBuilder = attributeDataIBBuilder.CreatePath();
429437
NL_TEST_ASSERT(apSuite, attributeDataIBBuilder.GetError() == CHIP_NO_ERROR);
430438

431-
if (aNeedInvalidReport)
439+
if (aReportType == ReportType::kInvalidNoAttributeId)
432440
{
433441
attributePathBuilder.Node(1).Endpoint(2).Cluster(3).ListIndex(5).EndOfAttributePathIB();
434442
}
443+
else if (aReportType == ReportType::kInvalidOutOfRangeAttributeId)
444+
{
445+
attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(0xFFF18000).EndOfAttributePathIB();
446+
}
435447
else
436448
{
449+
NL_TEST_ASSERT(apSuite, aReportType == ReportType::kValid);
437450
attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(4).EndOfAttributePathIB();
438451
}
439452

@@ -496,7 +509,7 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext
496509
ctx.GetLoopback().mNumMessagesToDrop = 1;
497510
ctx.DrainAndServiceIO();
498511

499-
GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/);
512+
GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/);
500513
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
501514
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
502515
}
@@ -521,8 +534,7 @@ void TestReadInteraction::TestReadUnexpectedSubscriptionId(nlTestSuite * apSuite
521534
ctx.DrainAndServiceIO();
522535

523536
// For read, we don't expect there is subscription id in report data.
524-
GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/,
525-
true /*aHasSubscriptionId*/);
537+
GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/, true /*aHasSubscriptionId*/);
526538
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
527539
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT);
528540
}
@@ -547,7 +559,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
547559
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read,
548560
app::reporting::GetDefaultReportScheduler());
549561

550-
GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
562+
GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/);
551563
err = readHandler.SendReportData(std::move(reportDatabuf), false);
552564
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);
553565

@@ -665,12 +677,46 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi
665677
ctx.GetLoopback().mNumMessagesToDrop = 1;
666678
ctx.DrainAndServiceIO();
667679

668-
GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/, true /* aSuppressResponse*/);
680+
GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidNoAttributeId, true /* aSuppressResponse*/);
669681

670682
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
671683
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB);
672684
}
673685

686+
void TestReadInteraction::TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext)
687+
{
688+
CHIP_ERROR err = CHIP_NO_ERROR;
689+
TestContext & ctx = *static_cast<TestContext *>(apContext);
690+
MockInteractionModelApp delegate;
691+
692+
System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize);
693+
694+
app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate,
695+
chip::app::ReadClient::InteractionType::Read);
696+
697+
ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice());
698+
err = readClient.SendRequest(readPrepareParams);
699+
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
700+
701+
// We don't actually want to deliver that message, because we want to
702+
// synthesize the read response. But we don't want it hanging around
703+
// forever either.
704+
ctx.GetLoopback().mNumMessagesToDrop = 1;
705+
ctx.DrainAndServiceIO();
706+
707+
GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidOutOfRangeAttributeId, true /* aSuppressResponse*/);
708+
709+
err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction);
710+
// Overall processing should succeed.
711+
NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
712+
713+
// We should not have gotten any attribute reports or errors.
714+
NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);
715+
NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0);
716+
NL_TEST_ASSERT(apSuite, !delegate.mGotReport);
717+
NL_TEST_ASSERT(apSuite, !delegate.mReadError);
718+
}
719+
674720
void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext)
675721
{
676722
CHIP_ERROR err = CHIP_NO_ERROR;
@@ -691,7 +737,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu
691737
ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read,
692738
app::reporting::GetDefaultReportScheduler());
693739

694-
GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/);
740+
GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/);
695741
err = readHandler.SendReportData(std::move(reportDatabuf), false);
696742
NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE);
697743

@@ -4941,6 +4987,7 @@ const nlTest sTests[] =
49414987
NL_TEST_DEF("TestReadClientGenerateOneEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateOneEventPaths),
49424988
NL_TEST_DEF("TestReadClientGenerateTwoEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateTwoEventPaths),
49434989
NL_TEST_DEF("TestReadClientInvalidReport", chip::app::TestReadInteraction::TestReadClientInvalidReport),
4990+
NL_TEST_DEF("TestReadClientInvalidAttributeId", chip::app::TestReadInteraction::TestReadClientInvalidAttributeId),
49444991
NL_TEST_DEF("TestReadHandlerInvalidAttributePath", chip::app::TestReadInteraction::TestReadHandlerInvalidAttributePath),
49454992
NL_TEST_DEF("TestProcessSubscribeRequest", chip::app::TestReadInteraction::TestProcessSubscribeRequest),
49464993
/*

0 commit comments

Comments
 (0)