From 4257633619d162fbde58d353eba5bbbad0901d53 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 3 Jan 2023 13:49:08 -0500 Subject: [PATCH] Fix support for 0-length hex-encoded octet strings in chip-tool. (#24097) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix support for 0-length hex-encoded octet strings in chip-tool. Without this fix, these commands all fail to parse: chip-tool unittesting write struct-attr '{"a": 1, "b": true, "c": 0, "d":"", "e": "abc", "f": 0, "g": 0, "h": 0}' 17 1 chip-tool unittesting write struct-attr '{"a": 1, "b": true, "c": 0, "d":"hex:", "e": "abc", "f": 0, "g": 0, "h": 0}' 17 1 chip-tool unittesting write octet-string "hex:" 17 1 chip-tool unittesting write-by-id 0 "hex:" 17 1 and with this fix they parse and send correct payloads. Fixes https://github.com/project-chip/connectedhomeip/issues/24054 * Apply suggestion from review comment. Co-authored-by: Jonathan Mégevand <77852424+jmeg-sfy@users.noreply.github.com> * Address review comment. Co-authored-by: Jonathan Mégevand <77852424+jmeg-sfy@users.noreply.github.com> --- examples/chip-tool/BUILD.gn | 1 + .../commands/clusters/ComplexArgument.h | 25 ++++--- .../commands/clusters/CustomArgument.h | 19 ++++-- .../chip-tool/commands/common/Command.cpp | 17 +++-- .../chip-tool/commands/common/HexConversion.h | 65 +++++++++++++++++++ examples/darwin-framework-tool/BUILD.gn | 1 + .../tv-casting-app/tv-casting-common/BUILD.gn | 1 + 7 files changed, 105 insertions(+), 24 deletions(-) create mode 100644 examples/chip-tool/commands/common/HexConversion.h diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index d870c1d5cf92f2..86e003a69201c4 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -58,6 +58,7 @@ static_library("chip-tool-utils") { "commands/common/Commands.cpp", "commands/common/Commands.h", "commands/common/CredentialIssuerCommands.h", + "commands/common/HexConversion.h", "commands/discover/DiscoverCommand.cpp", "commands/discover/DiscoverCommissionablesCommand.cpp", "commands/discover/DiscoverCommissionersCommand.cpp", diff --git a/examples/chip-tool/commands/clusters/ComplexArgument.h b/examples/chip-tool/commands/clusters/ComplexArgument.h index 2af80e5b3eef3e..e624aeb2cf583c 100644 --- a/examples/chip-tool/commands/clusters/ComplexArgument.h +++ b/examples/chip-tool/commands/clusters/ComplexArgument.h @@ -36,6 +36,7 @@ #include #include #include +#include #include #include #include @@ -210,18 +211,22 @@ class ComplexArgumentParser size = str.size(); } - if (size % 2 != 0) - { - ChipLogError(chipTool, "Error while encoding %s as a hex string: Odd number of characters.", label); - return CHIP_ERROR_INVALID_STRING_LENGTH; - } + CHIP_ERROR err = HexToBytes( + chip::CharSpan(str.c_str(), size), + [&buffer](size_t allocSize) { + buffer = static_cast(chip::Platform::MemoryCalloc(allocSize, sizeof(uint8_t))); + return buffer; + }, + &size); - buffer = static_cast(chip::Platform::MemoryCalloc(size / 2, sizeof(uint8_t))); - size = chip::Encoding::HexToBytes(str.c_str(), size, buffer, size / 2); - if (size == 0) + if (err != CHIP_NO_ERROR) { - ChipLogError(chipTool, "Error while encoding %s as a hex string.", label); - return CHIP_ERROR_INTERNAL; + if (buffer != nullptr) + { + chip::Platform::MemoryFree(buffer); + } + + return err; } } diff --git a/examples/chip-tool/commands/clusters/CustomArgument.h b/examples/chip-tool/commands/clusters/CustomArgument.h index 63fe7d7ade387e..ab6ce40752ae76 100644 --- a/examples/chip-tool/commands/clusters/CustomArgument.h +++ b/examples/chip-tool/commands/clusters/CustomArgument.h @@ -19,6 +19,7 @@ #pragma once #include +#include #include #include #include @@ -140,14 +141,18 @@ class CustomArgumentParser static CHIP_ERROR PutOctetString(chip::TLV::TLVWriter * writer, chip::TLV::Tag tag, Json::Value & value) { - size_t size = strlen(value.asCString()); - VerifyOrReturnError(size % 2 == 0, CHIP_ERROR_INVALID_STRING_LENGTH); - + const char * hexData = value.asCString() + kPayloadHexPrefixLen; + size_t hexDataLen = strlen(hexData); chip::Platform::ScopedMemoryBuffer buffer; - VerifyOrReturnError(buffer.Calloc(size / 2), CHIP_ERROR_NO_MEMORY); - size_t octetCount = chip::Encoding::HexToBytes(value.asCString() + kPayloadHexPrefixLen, size - kPayloadHexPrefixLen, - buffer.Get(), (size - kPayloadHexPrefixLen) / 2); - VerifyOrReturnError(octetCount != 0, CHIP_ERROR_NO_MEMORY); + + size_t octetCount; + ReturnErrorOnFailure(HexToBytes( + chip::CharSpan(hexData, hexDataLen), + [&buffer](size_t allocSize) { + buffer.Calloc(allocSize); + return buffer.Get(); + }, + &octetCount)); return chip::app::DataModel::Encode(*writer, tag, chip::ByteSpan(buffer.Get(), octetCount)); } diff --git a/examples/chip-tool/commands/common/Command.cpp b/examples/chip-tool/commands/common/Command.cpp index c4161b6ac59908..cd39293954e4f5 100644 --- a/examples/chip-tool/commands/common/Command.cpp +++ b/examples/chip-tool/commands/common/Command.cpp @@ -18,6 +18,7 @@ #include "Command.h" #include "CustomStringPrefix.h" +#include "HexConversion.h" #include "platform/PlatformManager.h" #include @@ -377,14 +378,16 @@ bool Command::InitArgument(size_t argIndex, char * argValue) // representation is always longer than the octet string it encodes, // so we have enough space in argValue for the decoded version. chip::Platform::ScopedMemoryBuffer buffer; - if (!buffer.Calloc(argLen)) // Bigger than needed, but it's fine. - { - return false; - } - size_t octetCount = - chip::Encoding::HexToBytes(argValue + kHexStringPrefixLen, argLen - kHexStringPrefixLen, buffer.Get(), argLen); - if (octetCount == 0) + size_t octetCount; + CHIP_ERROR err = HexToBytes( + chip::CharSpan(argValue + kHexStringPrefixLen, argLen - kHexStringPrefixLen), + [&buffer](size_t allocSize) { + buffer.Calloc(allocSize); + return buffer.Get(); + }, + &octetCount); + if (err != CHIP_NO_ERROR) { return false; } diff --git a/examples/chip-tool/commands/common/HexConversion.h b/examples/chip-tool/commands/common/HexConversion.h new file mode 100644 index 00000000000000..b31568101f2a2d --- /dev/null +++ b/examples/chip-tool/commands/common/HexConversion.h @@ -0,0 +1,65 @@ +/* + * Copyright (c) 2022 Project CHIP Authors + * All rights reserved. + * + * 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. + */ + +#pragma once + +#include +#include +#include +#include + +/** + * Utility for converting a hex string to bytes, with the right error checking + * and allocation size computation. + * + * Takes a functor to allocate the buffer to use for the hex bytes. The functor + * is expected to return uint8_t *. The caller is responsible for cleaning up + * this buffer as needed. + * + * On success, *octetCount is filled with the number of octets placed in the + * buffer. On failure, the value of *octetCount is undefined. + */ +template +CHIP_ERROR HexToBytes(chip::CharSpan hex, F bufferAllocator, size_t * octetCount) +{ + *octetCount = 0; + + if (hex.size() % 2 != 0) + { + ChipLogError(chipTool, "Error while encoding '%.*s' as an octet string: Odd number of characters.", + static_cast(hex.size()), hex.data()); + return CHIP_ERROR_INVALID_STRING_LENGTH; + } + + const size_t bufferSize = hex.size() / 2; + uint8_t * buffer = bufferAllocator(bufferSize); + if (buffer == nullptr && bufferSize != 0) + { + ChipLogError(chipTool, "Failed to allocate buffer of size: %llu", static_cast(bufferSize)); + return CHIP_ERROR_NO_MEMORY; + } + + size_t byteCount = chip::Encoding::HexToBytes(hex.data(), hex.size(), buffer, bufferSize); + if (byteCount == 0 && hex.size() != 0) + { + ChipLogError(chipTool, "Error while encoding '%.*s' as an octet string.", static_cast(hex.size()), hex.data()); + return CHIP_ERROR_INTERNAL; + } + + *octetCount = byteCount; + return CHIP_NO_ERROR; +} diff --git a/examples/darwin-framework-tool/BUILD.gn b/examples/darwin-framework-tool/BUILD.gn index a677fc669d1d55..3e59a3c9264342 100644 --- a/examples/darwin-framework-tool/BUILD.gn +++ b/examples/darwin-framework-tool/BUILD.gn @@ -124,6 +124,7 @@ executable("darwin-framework-tool") { "${chip_root}/examples/chip-tool/commands/common/Command.h", "${chip_root}/examples/chip-tool/commands/common/Commands.cpp", "${chip_root}/examples/chip-tool/commands/common/Commands.h", + "${chip_root}/examples/chip-tool/commands/common/HexConversion.h", "${chip_root}/zzz_generated/chip-tool/zap-generated/cluster/ComplexArgumentParser.cpp", "commands/clusters/ClusterCommandBridge.h", "commands/clusters/ModelCommandBridge.mm", diff --git a/examples/tv-casting-app/tv-casting-common/BUILD.gn b/examples/tv-casting-app/tv-casting-common/BUILD.gn index 37572da1d394e4..4d797a9c5b02cf 100644 --- a/examples/tv-casting-app/tv-casting-common/BUILD.gn +++ b/examples/tv-casting-app/tv-casting-common/BUILD.gn @@ -43,6 +43,7 @@ chip_data_model("tv-casting-common") { "${chip_root}/examples/chip-tool/commands/common/Commands.cpp", "${chip_root}/examples/chip-tool/commands/common/Commands.h", "${chip_root}/examples/chip-tool/commands/common/CredentialIssuerCommands.h", + "${chip_root}/examples/chip-tool/commands/common/HexConversion.h", "${chip_root}/src/controller/ExamplePersistentStorage.cpp", "${chip_root}/src/controller/ExamplePersistentStorage.h", "${chip_root}/zzz_generated/chip-tool/zap-generated/cluster/ComplexArgumentParser.cpp",