Enforce spec range restrictions on most nullable integer types for co…

…mmand/struct fields. (#13072) This does not enforce restrictions for the "odd-sized" (3-, 5-, 6-, 7-byte) integers, because at the point when we do TLV decoding for command/struct fields we don't know we're dealing with one of those. Those should generally not be used in command/struct fields anyway.
project-chip · Jan 10, 2022 · 19f61eb · 19f61eb
1 parent 7d2771f
commit 19f61eb
Show file tree

Hide file tree

Showing 10 changed files with 70 additions and 18 deletions.
diff --git a/src/app/data-model/Decode.h b/src/app/data-model/Decode.h
@@ -158,7 +158,12 @@ CHIP_ERROR Decode(TLV::TLVReader & reader, Nullable<X> & x)
     }
 
     // We have a value; decode it.
-    return Decode(reader, x.SetNonNull());
+    ReturnErrorOnFailure(Decode(reader, x.SetNonNull()));
+    if (!x.HasValidValue())
+    {
+        return CHIP_ERROR_IM_CONSTRAINT_ERROR;
+    }
+    return CHIP_NO_ERROR;
 }
 
 } // namespace DataModel

diff --git a/src/app/data-model/Encode.h b/src/app/data-model/Encode.h
@@ -22,6 +22,8 @@
 #include <lib/core/CHIPTLV.h>
 #include <lib/core/Optional.h>
 
+#include <type_traits>
+
 namespace chip {
 namespace app {
 namespace DataModel {
@@ -115,6 +117,15 @@ CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, const Nullable<X> & x)
     {
         return writer.PutNull(tag);
     }
+    // Allow sending invalid values for nullables when
+    // CONFIG_IM_BUILD_FOR_UNIT_TEST is true, so we can test how the other side
+    // responds.
+#if !CONFIG_IM_BUILD_FOR_UNIT_TEST
+    if (!x.HasValidValue())
+    {
+        return CHIP_ERROR_IM_CONSTRAINT_ERROR;
+    }
+#endif // !CONFIG_IM_BUILD_FOR_UNIT_TEST
     return Encode(writer, tag, x.Value());
 }
 

diff --git a/src/app/data-model/Nullable.h b/src/app/data-model/Nullable.h
@@ -18,8 +18,11 @@
 
 #pragma once
 
+#include <app/util/attribute-storage-null-handling.h>
 #include <lib/core/Optional.h>
 
+#include <type_traits>
+
 namespace chip {
 namespace app {
 namespace DataModel {
@@ -52,6 +55,24 @@ struct Nullable : protected Optional<T>
     {
         return Optional<T>::Emplace(std::forward<Args>(args)...);
     }
+
+    // For integer types, being nullable involves a range restriction.
+    template <
+        typename U = std::decay_t<T>,
+        typename std::enable_if_t<(std::is_integral<U>::value && !std::is_same<U, bool>::value) || std::is_enum<U>::value, int> = 0>
+    constexpr bool HasValidValue() const
+    {
+        return NumericAttributeTraits<T>::CanRepresentValue(/* isNullable = */ true, Value());
+    }
+
+    // For all other types, all values are valid.
+    template <typename U                     = std::decay_t<T>,
+              typename std::enable_if_t<(!std::is_integral<U>::value || std::is_same<U, bool>::value) && !std::is_enum<U>::value,
+                                        int> = 0>
+    constexpr bool HasValidValue() const
+    {
+        return true;
+    }
 };
 
 } // namespace DataModel

diff --git a/src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.m b/src/darwin/CHIPTool/CHIPTool/Framework Helpers/DefaultsUtils.m
@@ -119,7 +119,7 @@ BOOL CHIPIsDevicePaired(uint64_t deviceId)
 
     NSError * error;
     bool paired = [controller isDevicePaired:deviceId error:&error];
-    if (error.code != CHIPSuccess) {
+    if (error != nil) {
         NSLog(@"Error retrieving device info for deviceId %llu", deviceId);
         paired = NO;
     }

diff --git a/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m b/src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m
@@ -474,9 +474,9 @@ - (void)setVendorIDOnAccessory
 }
 
 // MARK: CHIPDevicePairingDelegate
-- (void)onPairingComplete:(NSError *)error
+- (void)onPairingComplete:(NSError * _Nullable)error
 {
-    if (error.code != CHIPSuccess) {
+    if (error != nil) {
         NSLog(@"Got pairing error back %@", error);
     } else {
         dispatch_async(dispatch_get_main_queue(), ^{
@@ -721,9 +721,9 @@ - (void)onConnectNetworkResponse:(NSError *)error
     [controller updateDevice:deviceId fabricId:0];
 }
 
-- (void)onAddressUpdated:(NSError *)error
+- (void)onAddressUpdated:(NSError * _Nullable)error
 {
-    if (error.code != CHIPSuccess) {
+    if (error != nil) {
         NSLog(@"Error retrieving device informations over Mdns: %@", error);
         return;
     }

diff --git a/src/darwin/Framework/CHIP/CHIPError.h b/src/darwin/Framework/CHIP/CHIPError.h
@@ -20,8 +20,8 @@
 NS_ASSUME_NONNULL_BEGIN
 FOUNDATION_EXPORT NSErrorDomain const CHIPErrorDomain;
 
+// clang-format off
 typedef NS_ERROR_ENUM(CHIPErrorDomain, CHIPErrorCode){
-    CHIPSuccess                       = 0,
     CHIPErrorCodeUndefinedError       = 1,
     CHIPErrorCodeInvalidStringLength  = 2,
     CHIPErrorCodeInvalidIntegerValue  = 3,
@@ -39,5 +39,6 @@ typedef NS_ERROR_ENUM(CHIPErrorDomain, CHIPErrorCode){
     CHIPErrorCodeUnsupportedWrite     = 0x88,
     CHIPErrorCodeUnsupportedCluster   = 0xC3,
 };
+// clang-format on
 
 NS_ASSUME_NONNULL_END
diff --git a/src/darwin/Framework/CHIP/CHIPError.mm b/src/darwin/Framework/CHIP/CHIPError.mm
@@ -63,10 +63,14 @@ + (NSError *)errorForCHIPErrorCode:(CHIP_ERROR)errorCode
                                userInfo:@{ NSLocalizedDescriptionKey : NSLocalizedString(@"Integrity check failed.", nil) }];
     }
 
-    if (errorCode == CHIP_NO_ERROR) {
+    if (errorCode == CHIP_ERROR_IM_CONSTRAINT_ERROR) {
         return [NSError errorWithDomain:CHIPErrorDomain
-                                   code:CHIPSuccess
-                               userInfo:@{ NSLocalizedDescriptionKey : NSLocalizedString(@"Success.", nil) }];
+                                   code:CHIPErrorCodeConstraintError
+                               userInfo:@{ NSLocalizedDescriptionKey : NSLocalizedString(@"Value out of range.", nil) }];
+    }
+
+    if (errorCode == CHIP_NO_ERROR) {
+        return nil;
     }
 
     return [NSError errorWithDomain:CHIPErrorDomain
@@ -132,8 +136,12 @@ + (NSError *)errorForZCLErrorCode:(uint8_t)errorCode
     }
 }
 
-+ (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)error
++ (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error
 {
+    if (error == nil) {
+        return CHIP_NO_ERROR;
+    }
+
     if (error.domain != CHIPErrorDomain) {
         return CHIP_ERROR_INTERNAL;
     }
@@ -151,8 +159,8 @@ + (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)error
         return CHIP_ERROR_INCORRECT_STATE;
     case CHIPErrorCodeIntegrityCheckFailed:
         return CHIP_ERROR_INTEGRITY_CHECK_FAILED;
-    case CHIPSuccess:
-        return CHIP_NO_ERROR;
+    case CHIPErrorCodeConstraintError:
+        return CHIP_ERROR_IM_CONSTRAINT_ERROR;
     default:
         return CHIP_ERROR_INTERNAL;
     }
@@ -165,6 +173,7 @@ + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error
     if (error == nil) {
         return EMBER_ZCL_STATUS_SUCCESS;
     }
+
     if (error.domain != CHIPErrorDomain) {
         return EMBER_ZCL_STATUS_FAILURE;
     }
@@ -186,8 +195,6 @@ + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error
         return EMBER_ZCL_STATUS_UNSUPPORTED_WRITE;
     case CHIPErrorCodeUnsupportedCluster:
         return EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER;
-    case CHIPSuccess:
-        return EMBER_ZCL_STATUS_SUCCESS;
     default:
         return EMBER_ZCL_STATUS_FAILURE;
     }

diff --git a/src/darwin/Framework/CHIP/CHIPError_Internal.h b/src/darwin/Framework/CHIP/CHIPError_Internal.h
@@ -24,7 +24,7 @@ NS_ASSUME_NONNULL_BEGIN
 @interface CHIPError : NSObject
 + (nullable NSError *)errorForCHIPErrorCode:(CHIP_ERROR)errorCode;
 + (nullable NSError *)errorForZCLErrorCode:(uint8_t)errorCode;
-+ (CHIP_ERROR)errorToCHIPErrorCode:(NSError *)error;
++ (CHIP_ERROR)errorToCHIPErrorCode:(NSError * _Nullable)error;
 + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error;
 @end
 

diff --git a/src/darwin/Framework/CHIPTests/CHIPErrorTestUtils.mm b/src/darwin/Framework/CHIPTests/CHIPErrorTestUtils.mm
@@ -54,8 +54,6 @@ + (uint8_t)errorToZCLErrorCode:(NSError * _Nullable)error
         return EMBER_ZCL_STATUS_UNSUPPORTED_WRITE;
     case CHIPErrorCodeUnsupportedCluster:
         return EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER;
-    case CHIPSuccess:
-        return EMBER_ZCL_STATUS_SUCCESS;
     default:
         return EMBER_ZCL_STATUS_FAILURE;
     }

diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h
@@ -2357,6 +2357,15 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_MISSING_URI_SEPARATOR             CHIP_CORE_ERROR(0xd7)
 
+/**
+ * @def CHIP_ERROR_IM_CONSTRAINT_ERROR
+ *
+ * @brief
+ *   The equivalent of a CONSTRAINT_ERROR status: a value was out of the valid
+ *   range.
+ */
+#define CHIP_ERROR_IM_CONSTRAINT_ERROR               CHIP_CORE_ERROR(0xd8)
+
 /**
  *  @}
  */