From 530244152733dc7f34e8d0e4096a1e7b2026a7e9 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Sat, 29 Apr 2023 12:32:47 -0400 Subject: [PATCH] Ensure that DataModel::Encode does not encode kUnknownEnumValue for enums. (#26299) This should help prevent those values leaking out on the wire. Fixes https://github.com/project-chip/connectedhomeip/issues/24368 --- .../chip-tool/include/CHIPProjectAppConfig.h | 5 ++- examples/darwin-framework-tool/BUILD.gn | 6 ++++ examples/darwin-framework-tool/args.gni | 11 +++++++ .../include/CHIPProjectAppConfig.h | 31 +++++++++++++++++++ scripts/build/build_darwin_framework.py | 5 +++ src/app/data-model/Encode.h | 27 +++++++++++++++- src/app/tests/suites/TestCluster.yaml | 7 ++--- src/app/tests/suites/TestEvents.yaml | 6 ++-- .../Framework/chip_xcode_build_connector.sh | 9 ++++++ src/lib/core/CHIPConfig.h | 11 +++++++ .../chip-tool/zap-generated/test/Commands.h | 14 +++------ .../zap-generated/test/Commands.h | 19 ++++-------- 12 files changed, 118 insertions(+), 33 deletions(-) create mode 100644 examples/darwin-framework-tool/include/CHIPProjectAppConfig.h diff --git a/examples/chip-tool/include/CHIPProjectAppConfig.h b/examples/chip-tool/include/CHIPProjectAppConfig.h index 36ce69c2007feb..bc8ee25a03645e 100644 --- a/examples/chip-tool/include/CHIPProjectAppConfig.h +++ b/examples/chip-tool/include/CHIPProjectAppConfig.h @@ -1,6 +1,6 @@ /* * - * Copyright (c) 2020-2022 Project CHIP Authors + * Copyright (c) 2020-2023 Project CHIP Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -63,4 +63,7 @@ // Enable some test-only interaction model APIs. #define CONFIG_BUILD_FOR_HOST_UNIT_TEST 1 +// Allow us, for test purposes, to encode invalid enum values. +#define CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES 1 + #endif /* CHIPPROJECTCONFIG_H */ diff --git a/examples/darwin-framework-tool/BUILD.gn b/examples/darwin-framework-tool/BUILD.gn index 67939fcb311f28..b920356a29b3a3 100644 --- a/examples/darwin-framework-tool/BUILD.gn +++ b/examples/darwin-framework-tool/BUILD.gn @@ -100,6 +100,12 @@ action("build-darwin-framework") { args += [ "--no-clang" ] } + if (config_enable_yaml_tests) { + args += [ "--enable_encoding_sentinel_enum_values" ] + } else { + args += [ "--no-enable_encoding_sentinel_enum_values" ] + } + output_name = "Matter.framework" outputs = [ "${root_out_dir}/macos_framework_output/Build/Products/${output_sdk_type}/${output_name}", diff --git a/examples/darwin-framework-tool/args.gni b/examples/darwin-framework-tool/args.gni index 0cd237a8409fa7..8308b2a9bcffaa 100644 --- a/examples/darwin-framework-tool/args.gni +++ b/examples/darwin-framework-tool/args.gni @@ -16,4 +16,15 @@ import("//build_overrides/chip.gni") import("${chip_root}/config/standalone/args.gni") +import("${chip_root}/examples/chip-tool/chip-tool.gni") + chip_crypto = "boringssl" + +if (config_enable_yaml_tests) { + chip_device_project_config_include = "" + chip_project_config_include = "" + + chip_project_config_include_dirs = + [ "${chip_root}/examples/darwin-framework-tool/include" ] + chip_project_config_include_dirs += [ "${chip_root}/config/standalone" ] +} diff --git a/examples/darwin-framework-tool/include/CHIPProjectAppConfig.h b/examples/darwin-framework-tool/include/CHIPProjectAppConfig.h new file mode 100644 index 00000000000000..6f1764a52df56b --- /dev/null +++ b/examples/darwin-framework-tool/include/CHIPProjectAppConfig.h @@ -0,0 +1,31 @@ +/* + * Copyright (c) 2023 Project CHIP Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * @file + * Project configuration for Darwin Framework Tool. + * + */ +#ifndef CHIPPROJECTCONFIG_H +#define CHIPPROJECTCONFIG_H + +// Enable some test-only interaction model APIs. +#define CONFIG_BUILD_FOR_HOST_UNIT_TEST 1 + +// Allow us, for test purposes, to encode invalid enum values. +#define CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES 1 + +#endif /* CHIPPROJECTCONFIG_H */ diff --git a/scripts/build/build_darwin_framework.py b/scripts/build/build_darwin_framework.py index af7e428c27be07..edb854d7d173ab 100644 --- a/scripts/build/build_darwin_framework.py +++ b/scripts/build/build_darwin_framework.py @@ -82,6 +82,7 @@ def build_darwin_framework(args): 'CHIP_IS_ASAN': args.asan, 'CHIP_IS_BLE': args.ble, 'CHIP_IS_CLANG': args.clang, + 'CHIP_ENABLE_ENCODING_SENTINEL_ENUM_VALUES': args.enable_encoding_sentinel_enum_values } for option in options: command += ["{}={}".format(option, "YES" if options[option] else "NO")] @@ -116,6 +117,9 @@ def build_darwin_framework(args): get_file_from_pigweed("libclang_rt.asan_osx_dynamic.dylib") ] + if args.enable_encoding_sentinel_enum_values: + cflags += ["-DCHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES=1"] + command += ["OTHER_CFLAGS=" + ' '.join(cflags), "OTHER_LDFLAGS=" + ' '.join(ldflags)] command_result = run_command(command) print("Build Framework Result: {}".format(command_result)) @@ -158,6 +162,7 @@ def build_darwin_framework(args): parser.add_argument('--asan', action=argparse.BooleanOptionalAction) parser.add_argument('--ble', action=argparse.BooleanOptionalAction) parser.add_argument('--clang', action=argparse.BooleanOptionalAction) + parser.add_argument('--enable_encoding_sentinel_enum_values', action=argparse.BooleanOptionalAction) args = parser.parse_args() build_darwin_framework(args) diff --git a/src/app/data-model/Encode.h b/src/app/data-model/Encode.h index 3e930a1c0ec24a..0546098a74184d 100644 --- a/src/app/data-model/Encode.h +++ b/src/app/data-model/Encode.h @@ -31,6 +31,18 @@ namespace chip { namespace app { namespace DataModel { +namespace detail { +// A way to detect whether an enum has a kUnknownEnumValue value, for use in enable_if. +template +using VoidType = void; + +template +constexpr bool HasUnknownValue = false; + +template +constexpr bool HasUnknownValue> = true; +} // namespace detail + /* * @brief * Set of overloaded encode methods that based on the type of cluster element passed in, @@ -48,9 +60,22 @@ CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, X x) return writer.Put(tag, x); } -template ::value, int> = 0> +template ::value && !detail::HasUnknownValue, int> = 0> +CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, X x) +{ + return writer.Put(tag, x); +} + +template ::value && detail::HasUnknownValue, int> = 0> CHIP_ERROR Encode(TLV::TLVWriter & writer, TLV::Tag tag, X x) { +#if !CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES + if (x == X::kUnknownEnumValue) + { + return CHIP_IM_GLOBAL_STATUS(ConstraintError); + } +#endif // !CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES + return writer.Put(tag, x); } diff --git a/src/app/tests/suites/TestCluster.yaml b/src/app/tests/suites/TestCluster.yaml index 5f127d112aeca9..e89a892dd32ee5 100644 --- a/src/app/tests/suites/TestCluster.yaml +++ b/src/app/tests/suites/TestCluster.yaml @@ -1068,11 +1068,8 @@ tests: - name: "arg2" value: 101 response: - values: - - name: "arg1" - value: 20003 - - name: "arg2" - value: 4 + # Attempting to echo back the invalid enum value should fail. + error: FAILURE # Tests for Struct diff --git a/src/app/tests/suites/TestEvents.yaml b/src/app/tests/suites/TestEvents.yaml index e43dc0f5423c75..03f314fea3bdbe 100644 --- a/src/app/tests/suites/TestEvents.yaml +++ b/src/app/tests/suites/TestEvents.yaml @@ -80,7 +80,7 @@ tests: - name: "arg1" value: 3 - name: "arg2" - value: 4 + value: 1 - name: "arg3" value: false response: @@ -95,7 +95,7 @@ tests: - values: - value: { arg1: 1, arg2: 2, arg3: true } - values: - - value: { arg1: 3, arg2: 4, arg3: false } + - value: { arg1: 3, arg2: 1, arg3: false } - label: "Subscribe to the event" command: "subscribeEvent" @@ -106,7 +106,7 @@ tests: - values: - value: { arg1: 1, arg2: 2, arg3: true } - values: - - value: { arg1: 3, arg2: 4, arg3: false } + - value: { arg1: 3, arg2: 1, arg3: false } - label: "Generate a third event on the accessory" command: "TestEmitTestEventRequest" diff --git a/src/darwin/Framework/chip_xcode_build_connector.sh b/src/darwin/Framework/chip_xcode_build_connector.sh index 55227ed2b5330f..eee0afeed97506 100755 --- a/src/darwin/Framework/chip_xcode_build_connector.sh +++ b/src/darwin/Framework/chip_xcode_build_connector.sh @@ -58,6 +58,9 @@ for define in "${defines[@]}"; do esac target_defines+=,\"${define//\"/\\\"}\" done +[[ $CHIP_ENABLE_ENCODING_SENTINEL_ENUM_VALUES == YES ]] && { + target_defines+=,\"CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES=1\" +} target_defines=[${target_defines:1}] declare target_arch= @@ -142,6 +145,12 @@ declare -a args=( ) } +[[ $CHIP_ENABLE_ENCODING_SENTINEL_ENUM_VALUES == YES ]] && { + args+=( + 'enable_encoding_sentinel_enum_values=true' + ) +} + # search current (or $2) and its parent directories until # a name match is found, which is output on stdout find_in_ancestors() { diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index 4052c17a446bfa..961db5250e7d63 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -914,6 +914,17 @@ extern const char CHIP_NON_PRODUCTION_MARKER[]; #define CONFIG_BUILD_FOR_HOST_UNIT_TEST 0 #endif +/** + * @def CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES + * + * @brief Defines whether encoding the "not a known enum value" enum values is + * allowed. Should only be enabled in certain test applications. This + * flag must not be enabled on actual devices. + */ +#ifndef CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES +#define CHIP_CONFIG_IM_ENABLE_ENCODING_SENTINEL_ENUM_VALUES 0 +#endif + /** * @def CHIP_CONFIG_LAMBDA_EVENT_SIZE * diff --git a/zzz_generated/chip-tool/zap-generated/test/Commands.h b/zzz_generated/chip-tool/zap-generated/test/Commands.h index 1666169997e780..1696aa2afc2543 100644 --- a/zzz_generated/chip-tool/zap-generated/test/Commands.h +++ b/zzz_generated/chip-tool/zap-generated/test/Commands.h @@ -58751,13 +58751,7 @@ class TestClusterSuite : public TestCommand } break; case 156: - VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); - { - chip::app::Clusters::UnitTesting::Commands::TestEnumsResponse::DecodableType value; - VerifyOrReturn(CheckDecodeValue(chip::app::DataModel::Decode(*data, value))); - VerifyOrReturn(CheckValue("arg1", value.arg1, 20003U)); - VerifyOrReturn(CheckValue("arg2", value.arg2, 4U)); - } + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); break; case 157: VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); @@ -66197,7 +66191,7 @@ class TestEventsSuite : public TestCommand chip::app::Clusters::UnitTesting::Events::TestEvent::DecodableType value; VerifyOrReturn(CheckDecodeValue(chip::app::DataModel::Decode(*data, value))); VerifyOrReturn(CheckValue("testEvent.arg1", value.arg1, 3U)); - VerifyOrReturn(CheckValue("testEvent.arg2", value.arg2, 4U)); + VerifyOrReturn(CheckValue("testEvent.arg2", value.arg2, 1U)); VerifyOrReturn(CheckValue("testEvent.arg3", value.arg3, false)); } mTestSubStepIndex++; @@ -66227,7 +66221,7 @@ class TestEventsSuite : public TestCommand chip::app::Clusters::UnitTesting::Events::TestEvent::DecodableType value; VerifyOrReturn(CheckDecodeValue(chip::app::DataModel::Decode(*data, value))); VerifyOrReturn(CheckValue("testEvent.arg1", value.arg1, 3U)); - VerifyOrReturn(CheckValue("testEvent.arg2", value.arg2, 4U)); + VerifyOrReturn(CheckValue("testEvent.arg2", value.arg2, 1U)); VerifyOrReturn(CheckValue("testEvent.arg3", value.arg3, false)); } mTestSubStepIndex++; @@ -66334,7 +66328,7 @@ class TestEventsSuite : public TestCommand ListFreer listFreer; chip::app::Clusters::UnitTesting::Commands::TestEmitTestEventRequest::Type value; value.arg1 = 3U; - value.arg2 = static_cast(4); + value.arg2 = static_cast(1); value.arg3 = false; return SendCommand(kIdentityAlpha, GetEndpoint(1), UnitTesting::Id, UnitTesting::Commands::TestEmitTestEventRequest::Id, value, chip::NullOptional diff --git a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h index 391eef964a706e..bfccaf83dbe46f 100644 --- a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h +++ b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h @@ -82389,7 +82389,7 @@ class TestCluster : public TestCommandBridge { VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); break; case 156: - VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); + VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), EMBER_ZCL_STATUS_FAILURE)); break; case 157: VerifyOrReturn(CheckValue("status", chip::to_underlying(status.mStatus), 0)); @@ -87068,18 +87068,11 @@ class TestCluster : public TestCommandBridge { completion:^(MTRUnitTestingClusterTestEnumsResponseParams * _Nullable values, NSError * _Nullable err) { NSLog(@"Send a command with a vendor_id and invalid enum Error: %@", err); - VerifyOrReturn(CheckValue("status", err ? err.code : 0, 0)); - - { - id actualValue = values.arg1; - VerifyOrReturn(CheckValue("arg1", actualValue, 20003U)); - } - - { - id actualValue = values.arg2; - VerifyOrReturn(CheckValue("arg2", actualValue, 4U)); - } - + VerifyOrReturn(CheckValue("status", + err ? ([err.domain isEqualToString:MTRInteractionErrorDomain] ? err.code + : EMBER_ZCL_STATUS_FAILURE) + : 0, + EMBER_ZCL_STATUS_FAILURE)); NextTest(); }];