Skip to content

Commit

Permalink
Make client handling of invalid IDs a bit more lenient. (project-chip…
Browse files Browse the repository at this point in the history 
…#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]>
  • Loading branch information
@bzbarsky-apple @andy31415
bzbarsky-apple and andy31415 authored Nov 20, 2023
1 parent 8dce277 commit 0d16348
Show file tree
Hide file tree
Showing 6 changed files with 111 additions and 18 deletions.
2 changes: 2 additions & 0 deletions src/app/ConcreteAttributePath.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,8 @@ struct ConcreteAttributePath : public ConcreteClusterPath
mExpanded = false;
}

bool IsValid() const { return ConcreteClusterPath::HasValidIds() && IsValidAttributeId(mAttributeId); }

bool operator==(const ConcreteAttributePath & aOther) const
{
return ConcreteClusterPath::operator==(aOther) && (mAttributeId == aOther.mAttributeId);
Expand Down
2 changes: 2 additions & 0 deletions src/app/ConcreteClusterPath.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ struct ConcreteClusterPath

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;
Expand Down
17 changes: 11 additions & 6 deletions src/app/MessageDef/AttributePathIB.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -171,13 +171,17 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(DataModel::Nullable<ListIndex>
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;
Expand All @@ -204,9 +208,10 @@ CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributeP
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);
Expand Down
16 changes: 14 additions & 2 deletions src/app/MessageDef/AttributePathIB.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,12 @@ enum class Tag : uint8_t
kListIndex = 5,
};

enum class ValidateIdRanges : uint8_t
{
kYes,
kNo,
};

class Parser : public ListParser
{
public:
Expand Down Expand Up @@ -136,10 +142,13 @@ class Parser : public ListParser
* 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
Expand All @@ -148,10 +157,13 @@ class Parser : public ListParser
* 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.

Expand Down
27 changes: 26 additions & 1 deletion src/app/ReadClient.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -641,9 +641,12 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex
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;
}
Expand Down Expand Up @@ -679,6 +682,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
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();
Expand All @@ -689,6 +703,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo
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);
Expand Down
65 changes: 56 additions & 9 deletions src/app/tests/TestReadInteraction.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ class TestReadInteraction
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
Expand Down Expand Up @@ -390,12 +391,19 @@ class TestReadInteraction
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;
Expand Down Expand Up @@ -428,12 +436,17 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon
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();
}

Expand Down Expand Up @@ -496,7 +509,7 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext
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);
}
Expand All @@ -521,8 +534,7 @@ void TestReadInteraction::TestReadUnexpectedSubscriptionId(nlTestSuite * apSuite
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);
}
Expand All @@ -547,7 +559,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex
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);

Expand Down Expand Up @@ -665,12 +677,46 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi
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;
Expand All @@ -691,7 +737,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu
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);

Expand Down Expand Up @@ -4941,6 +4987,7 @@ const nlTest sTests[] =
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),
/*
Expand Down

0 comments on commit 0d16348

Please sign in to comment.