Don't call into AttributeAccessInterface for attributes that don't ex…

…ist. (#11997) In particular, writes already had this check, but we need it for reads. Fixes #10389
project-chip · Feb 7, 2022 · 01936fc · 01936fc
1 parent d88e7c9
commit 01936fc
Show file tree

Hide file tree

Showing 12 changed files with 1,268 additions and 1,081 deletions.
diff --git a/src/app/tests/suites/TestCluster.yaml b/src/app/tests/suites/TestCluster.yaml
@@ -2033,3 +2033,10 @@ tests:
       attribute: "nullable_char_string"
       response:
           value: ""
+
+    - label: "Read nonexistent attribute."
+      endpoint: 200
+      command: "readAttribute"
+      attribute: "list_int8u"
+      response:
+          error: 0x86 # UNSUPPORTED_ATTRIBUTE
diff --git a/src/app/util/ember-compatibility-functions.cpp b/src/app/util/ember-compatibility-functions.cpp
@@ -211,6 +211,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)
 {
@@ -235,6 +257,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();
@@ -482,19 +514,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
@@ -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/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj b/src/darwin/Framework/CHIP.xcodeproj/project.pbxproj
@@ -60,6 +60,7 @@
 		51B22C222740CB1D008D5055 /* CHIPCommandPayloadsObjc.h in Headers */ = {isa = PBXBuildFile; fileRef = 51B22C212740CB1D008D5055 /* CHIPCommandPayloadsObjc.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		51B22C262740CB32008D5055 /* CHIPStructsObjc.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51B22C252740CB32008D5055 /* CHIPStructsObjc.mm */; };
 		51B22C2A2740CB47008D5055 /* CHIPCommandPayloadsObjc.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51B22C292740CB47008D5055 /* CHIPCommandPayloadsObjc.mm */; };
+		51E24E73274E0DAC007CCF6E /* CHIPErrorTestUtils.mm in Sources */ = {isa = PBXBuildFile; fileRef = 51E24E72274E0DAC007CCF6E /* CHIPErrorTestUtils.mm */; };
 		991DC0842475F45400C13860 /* CHIPDeviceController.h in Headers */ = {isa = PBXBuildFile; fileRef = 991DC0822475F45400C13860 /* CHIPDeviceController.h */; settings = {ATTRIBUTES = (Public, ); }; };
 		991DC0892475F47D00C13860 /* CHIPDeviceController.mm in Sources */ = {isa = PBXBuildFile; fileRef = 991DC0872475F47D00C13860 /* CHIPDeviceController.mm */; };
 		991DC08B247704DC00C13860 /* CHIPLogging.h in Headers */ = {isa = PBXBuildFile; fileRef = 991DC08A247704DC00C13860 /* CHIPLogging.h */; };
@@ -151,6 +152,7 @@
 		51B22C212740CB1D008D5055 /* CHIPCommandPayloadsObjc.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = CHIPCommandPayloadsObjc.h; path = "zap-generated/CHIPCommandPayloadsObjc.h"; sourceTree = "<group>"; };
 		51B22C252740CB32008D5055 /* CHIPStructsObjc.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = CHIPStructsObjc.mm; path = "zap-generated/CHIPStructsObjc.mm"; sourceTree = "<group>"; };
 		51B22C292740CB47008D5055 /* CHIPCommandPayloadsObjc.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; name = CHIPCommandPayloadsObjc.mm; path = "zap-generated/CHIPCommandPayloadsObjc.mm"; sourceTree = "<group>"; };
+		51E24E72274E0DAC007CCF6E /* CHIPErrorTestUtils.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = CHIPErrorTestUtils.mm; sourceTree = "<group>"; };
 		991DC0822475F45400C13860 /* CHIPDeviceController.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CHIPDeviceController.h; sourceTree = "<group>"; };
 		991DC0872475F47D00C13860 /* CHIPDeviceController.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = CHIPDeviceController.mm; sourceTree = "<group>"; };
 		991DC08A247704DC00C13860 /* CHIPLogging.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CHIPLogging.h; sourceTree = "<group>"; };
@@ -321,6 +323,7 @@
 		B202529A2459E34F00F97062 /* CHIPTests */ = {
 			isa = PBXGroup;
 			children = (
+				51E24E72274E0DAC007CCF6E /* CHIPErrorTestUtils.mm */,
 				1EB41B7A263C4CC60048E4C1 /* CHIPClustersTests.m */,
 				99C65E0F267282F1003402F6 /* CHIPControllerTests.m */,
 				B2F53AF1245B0DCF0010745E /* CHIPSetupPayloadParserTests.m */,
@@ -551,6 +554,7 @@
 				997DED1A26955D0200975E97 /* CHIPThreadOperationalDatasetTests.mm in Sources */,
 				99C65E10267282F1003402F6 /* CHIPControllerTests.m in Sources */,
 				B2F53AF2245B0DCF0010745E /* CHIPSetupPayloadParserTests.m in Sources */,
+				51E24E73274E0DAC007CCF6E /* CHIPErrorTestUtils.mm in Sources */,
 			);
 			runOnlyForDeploymentPostprocessing = 0;
 		};
@@ -691,6 +695,7 @@
 			isa = XCBuildConfiguration;
 			buildSettings = {
 				CODE_SIGN_STYLE = Automatic;
+				"HEADER_SEARCH_PATHS[arch=*]" = "$(PROJECT_DIR)/../../../src";
 				INFOPLIST_FILE = CHIPTests/Info.plist;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",
@@ -814,6 +819,7 @@
 			buildSettings = {
 				CODE_SIGN_STYLE = Automatic;
 				DEVELOPMENT_TEAM = "";
+				"HEADER_SEARCH_PATHS[arch=*]" = "$(PROJECT_DIR)/../../../src";
 				INFOPLIST_FILE = CHIPTests/Info.plist;
 				LD_RUNPATH_SEARCH_PATHS = (
 					"$(inherited)",

diff --git a/src/darwin/Framework/CHIP/CHIPError.mm b/src/darwin/Framework/CHIP/CHIPError.mm
@@ -125,4 +125,27 @@ + (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)error
         return CHIP_ERROR_INTERNAL;
     }
 }
+
++ (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error
+{
+    // If this is changed, change CHIPErrorTestUtils' version of
+    // errorToZCLErrorCode too.
+    if (error == nil) {
+        return EMBER_ZCL_STATUS_SUCCESS;
+    }
+    if (error.domain != CHIPErrorDomain) {
+        return EMBER_ZCL_STATUS_FAILURE;
+    }
+
+    switch (error.code) {
+    case CHIPErrorCodeDuplicateExists:
+        return EMBER_ZCL_STATUS_DUPLICATE_EXISTS;
+    case CHIPErrorCodeUnsupportedAttribute:
+        return EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE;
+    case CHIPSuccess:
+        return EMBER_ZCL_STATUS_SUCCESS;
+    default:
+        return EMBER_ZCL_STATUS_FAILURE;
+    }
+}
 @end
diff --git a/src/darwin/Framework/CHIP/CHIPError_Internal.h b/src/darwin/Framework/CHIP/CHIPError_Internal.h
@@ -24,7 +24,8 @@ NS_ASSUME_NONNULL_BEGIN
 @interface CHIPError : NSObject
 + (nullable NSError *)errorForCHIPErrorCode:(CHIP_ERROR)errorCode;
 + (nullable NSError *)errorForZCLErrorCode:(uint8_t)errorCode;
-+ (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)errorCode;
++ (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)error;
++ (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error;
 @end
 
 NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIP/templates/clusters-tests.zapt b/src/darwin/Framework/CHIP/templates/clusters-tests.zapt
@@ -7,6 +7,8 @@
 #import <CHIP/CHIP.h>
 #import <CHIP/CHIPTestClustersObjc.h>
 
+#import "CHIPErrorTestUtils.h"
+
 // system dependencies
 #import <XCTest/XCTest.h>
 

diff --git a/src/darwin/Framework/CHIP/templates/partials/test_cluster.zapt b/src/darwin/Framework/CHIP/templates/partials/test_cluster.zapt
@@ -71,7 +71,7 @@ bool testSendCluster{{parent.filename}}_{{asTestIndex index}}_{{asUpperCamelCase
         }
         {{/if}}
 
-        XCTAssertEqual(err.code, {{response.error}});
+        XCTAssertEqual([CHIPErrorTestUtils errorToZCLErrorCode:err], {{response.error}});
         {{#unless (isStrEqual "0" response.error)}}
         [expectation fulfill];
         {{else}}