Skip to content

Commit

Permalink
Enforce spec range restrictions on most nullable integer types for co…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Jan 20, 2022
1 parent 7bf71a4 commit 4980557
Show file tree
Hide file tree
Showing 10 changed files with 70 additions and 18 deletions.
7 changes: 6 additions & 1 deletion src/app/data-model/Decode.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 11 additions & 0 deletions src/app/data-model/Encode.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include <lib/core/CHIPTLV.h>
#include <lib/core/Optional.h>

#include <type_traits>

namespace chip {
namespace app {
namespace DataModel {
Expand Down Expand Up @@ -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());
}

Expand Down
21 changes: 21 additions & 0 deletions src/app/data-model/Nullable.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(), ^{
Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 2 additions & 1 deletion src/darwin/Framework/CHIP/CHIPError.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -39,5 +39,6 @@ typedef NS_ERROR_ENUM(CHIPErrorDomain, CHIPErrorCode){
CHIPErrorCodeUnsupportedWrite = 0x88,
CHIPErrorCodeUnsupportedCluster = 0xC3,
};
// clang-format on

NS_ASSUME_NONNULL_END
23 changes: 15 additions & 8 deletions src/darwin/Framework/CHIP/CHIPError.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/CHIPError_Internal.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
2 changes: 0 additions & 2 deletions src/darwin/Framework/CHIPTests/CHIPErrorTestUtils.mm
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
9 changes: 9 additions & 0 deletions src/lib/core/CHIPError.h
Original file line number Diff line number Diff line change
Expand Up @@ -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)

/**
* @}
*/
Expand Down

0 comments on commit 4980557

Please sign in to comment.