From 29a2387a96e007d479f8d85708551abd6771994d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 18 Nov 2021 17:20:36 -0500 Subject: [PATCH] Don't call into AttributeAccessInterface for attributes that don't exist. In particular, writes already had this check, but we need it for reads. Fixes https://github.com/project-chip/connectedhomeip/issues/10389 --- .../tests/suites/TestClusterComplexTypes.yaml | 7 +++ .../util/ember-compatibility-functions.cpp | 46 +++++++++++++------ .../common/ClusterTestGeneration.js | 5 +- .../chip-tool/zap-generated/test/Commands.h | 30 +++++++++++- 4 files changed, 72 insertions(+), 16 deletions(-) diff --git a/src/app/tests/suites/TestClusterComplexTypes.yaml b/src/app/tests/suites/TestClusterComplexTypes.yaml index f1d0025e735c4d..7add22f6fa13dc 100644 --- a/src/app/tests/suites/TestClusterComplexTypes.yaml +++ b/src/app/tests/suites/TestClusterComplexTypes.yaml @@ -767,3 +767,10 @@ tests: attribute: "nullable_char_string" response: value: "" + + - label: "Read nonexistent attribute." + endpoint: 200 + command: "readAttribute" + attribute: "list_int8u" + response: + error: 1 diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp index 08a0a8ef868ac9..86db8e59e513ef 100644 --- a/src/app/util/ember-compatibility-functions.cpp +++ b/src/app/util/ember-compatibility-functions.cpp @@ -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 ConcreteAttributePath & aPath, AttributeReportIB::Builder & aAttributeReport) { @@ -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); + } + ConcreteReadAttributePath readPath(aPath); // TODO: We should probably clone the writer and convert failures here @@ -435,19 +467,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; } diff --git a/src/app/zap-templates/common/ClusterTestGeneration.js b/src/app/zap-templates/common/ClusterTestGeneration.js index 44bdc0429bed2e..bef6bd81c74266 100644 --- a/src/app/zap-templates/common/ClusterTestGeneration.js +++ b/src/app/zap-templates/common/ClusterTestGeneration.js @@ -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; @@ -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); } diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 61cd845e815edc..22bd72bfc74874 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -33528,6 +33528,10 @@ class TestClusterComplexTypes : public TestCommand ChipLogProgress(chipTool, " ***** Test Step 107 : Read attribute NULLABLE_CHAR_STRING\n"); err = TestReadAttributeNullableCharString_107(); break; + case 108: + ChipLogProgress(chipTool, " ***** Test Step 108 : Read nonexistent attribute.\n"); + err = TestReadNonexistentAttribute_108(); + break; } if (CHIP_NO_ERROR != err) @@ -33539,7 +33543,7 @@ class TestClusterComplexTypes : public TestCommand private: std::atomic_uint16_t mTestIndex; - const uint16_t mTestCount = 108; + const uint16_t mTestCount = 109; static void OnFailureCallback_1(void * context, EmberAfStatus status) { @@ -34467,6 +34471,16 @@ class TestClusterComplexTypes : public TestCommand (static_cast(context))->OnSuccessResponse_107(nullableCharString); } + static void OnFailureCallback_108(void * context, EmberAfStatus status) + { + (static_cast(context))->OnFailureResponse_108(chip::to_underlying(status)); + } + + static void OnSuccessCallback_108(void * context, const chip::app::DataModel::DecodableList & listInt8u) + { + (static_cast(context))->OnSuccessResponse_108(listInt8u); + } + // // Tests methods // @@ -36491,6 +36505,20 @@ class TestClusterComplexTypes : public TestCommand VerifyOrReturn(CheckValueAsString("nullableCharString.Value()", nullableCharString.Value(), chip::CharSpan("", 0))); NextTest(); } + + CHIP_ERROR TestReadNonexistentAttribute_108() + { + const chip::EndpointId endpoint = mEndpointId.HasValue() ? mEndpointId.Value() : 200; + chip::Controller::TestClusterClusterTest cluster; + cluster.Associate(mDevice, endpoint); + + return cluster.ReadAttribute(this, OnSuccessCallback_108, + OnFailureCallback_108); + } + + void OnFailureResponse_108(uint8_t status) { NextTest(); } + + void OnSuccessResponse_108(const chip::app::DataModel::DecodableList & listInt8u) { ThrowSuccessResponse(); } }; class TestConstraints : public TestCommand