Skip to content

Commit

Permalink
Don't call into AttributeAccessInterface for attributes that don't ex…
Browse files Browse the repository at this point in the history
…ist.

In particular, writes already had this check, but we need it for reads.

Fixes project-chip#10389
  • Loading branch information
bzbarsky-apple committed Nov 24, 2021
1 parent 2e85d48 commit 9fb46bb
Show file tree
Hide file tree
Showing 5 changed files with 90 additions and 16 deletions.
7 changes: 7 additions & 0 deletions src/app/tests/suites/TestCluster.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -2033,3 +2033,10 @@ tests:
attribute: "nullable_char_string"
response:
value: ""

- label: "Read nonexistent attribute."
endpoint: 200
command: "readAttribute"
attribute: "list_int8u"
response:
error: 1
46 changes: 33 additions & 13 deletions src/app/util/ember-compatibility-functions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,28 @@ bool ServerClusterCommandExists(const ConcreteCommandPath & aCommandPath)
return emberAfContainsServer(aCommandPath.mEndpointId, aCommandPath.mClusterId);
}

namespace {
CHIP_ERROR SendFailureStatus(const ConcreteAttributePath & aPath, AttributeReportIB::Builder & aAttributeReport,
Protocols::InteractionModel::Status aStatus, const TLV::TLVWriter & aReportCheckpoint)
{
aAttributeReport.Rollback(aReportCheckpoint);
AttributeStatusIB::Builder attributeStatusIBBuilder = aAttributeReport.CreateAttributeStatus();
AttributePathIB::Builder attributePathIBBuilder = attributeStatusIBBuilder.CreatePath();
attributePathIBBuilder.Endpoint(aPath.mEndpointId)
.Cluster(aPath.mClusterId)
.Attribute(aPath.mAttributeId)
.EndOfAttributePathIB();
ReturnErrorOnFailure(attributePathIBBuilder.GetError());
StatusIB::Builder statusIBBuilder = attributeStatusIBBuilder.CreateErrorStatus();
statusIBBuilder.EncodeStatusIB(StatusIB(aStatus));
ReturnErrorOnFailure(statusIBBuilder.GetError());
attributeStatusIBBuilder.EndOfAttributeStatusIB();
ReturnErrorOnFailure(attributeStatusIBBuilder.GetError());
return CHIP_NO_ERROR;
}

} // anonymous namespace

CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const ConcreteReadAttributePath & aPath,
AttributeReportIB::Builder & aAttributeReport)
{
Expand All @@ -234,6 +256,16 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
AttributeAccessInterface * attrOverride = findAttributeAccessOverride(aPath.mEndpointId, aPath.mClusterId);
if (attrOverride != nullptr)
{
const EmberAfAttributeMetadata * attributeMetadata =
emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);

if (attributeMetadata == nullptr)
{
// This attribute (or even this cluster) is not actually supported
// on this endpoint.
return SendFailureStatus(aPath, aAttributeReport, Protocols::InteractionModel::Status::UnsupportedAttribute, backup);
}

// TODO: We should probably clone the writer and convert failures here
// into status responses, unless our caller already does that.
writer = attributeDataIBBuilder.GetWriter();
Expand Down Expand Up @@ -433,19 +465,7 @@ CHIP_ERROR ReadSingleClusterData(FabricIndex aAccessingFabricIndex, const Concre
}
else
{
aAttributeReport.Rollback(backup);
attributeStatusIBBuilder = aAttributeReport.CreateAttributeStatus();
attributePathIBBuilder = attributeStatusIBBuilder.CreatePath();
attributePathIBBuilder.Endpoint(aPath.mEndpointId)
.Cluster(aPath.mClusterId)
.Attribute(aPath.mAttributeId)
.EndOfAttributePathIB();
ReturnErrorOnFailure(attributePathIBBuilder.GetError());
StatusIB::Builder statusIBBuilder = attributeStatusIBBuilder.CreateErrorStatus();
statusIBBuilder.EncodeStatusIB(StatusIB(imStatus));
ReturnErrorOnFailure(statusIBBuilder.GetError());
attributeStatusIBBuilder.EndOfAttributeStatusIB();
ReturnErrorOnFailure(attributeStatusIBBuilder.GetError());
ReturnErrorOnFailure(SendFailureStatus(aPath, aAttributeReport, imStatus, backup));
}
return CHIP_NO_ERROR;
}
Expand Down
5 changes: 3 additions & 2 deletions src/app/zap-templates/common/ClusterTestGeneration.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ function setDefaultResponse(test)
setDefault(test[kResponseName], kConstraintsName, defaultResponseConstraints);

const hasResponseValue = 'value' in test[kResponseName];
const hasResponseError = 'error' in test[kResponseName];
const hasResponseConstraints = 'constraints' in test[kResponseName] && Object.keys(test[kResponseName].constraints).length;
const hasResponseValueOrConstraints = hasResponseValue || hasResponseConstraints;

Expand Down Expand Up @@ -233,10 +234,10 @@ function setDefaultResponse(test)
return;
}

if (!hasResponseValueOrConstraints) {
if (!hasResponseValueOrConstraints && !hasResponseError) {
console.log(test);
console.log(test[kResponseName]);
const errorStr = 'Test does not have a "value" or a "constraints" defined.';
const errorStr = 'Test does not have a "value" or a "constraints" defined and is not expecting an error.';
throwError(test, errorStr);
}

Expand Down
18 changes: 18 additions & 0 deletions src/darwin/Framework/CHIPTests/CHIPClustersTests.m

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

30 changes: 29 additions & 1 deletion zzz_generated/chip-tool/zap-generated/test/Commands.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

0 comments on commit 9fb46bb

Please sign in to comment.