Fix support for 0-length hex-encoded octet strings in chip-tool. (#24097

) * 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 #24054 * Apply suggestion from review comment. Co-authored-by: Jonathan Mégevand <[email protected]> * Address review comment. Co-authored-by: Jonathan Mégevand <[email protected]>
project-chip · May 12, 2023 · bf97d0c · bf97d0c
1 parent 5e3c765
commit bf97d0c
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 24 deletions.
diff --git 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
@@ -36,6 +36,7 @@
 #include <app-common/zap-generated/cluster-objects.h>
 #include <app/data-model/List.h>
 #include <app/data-model/Nullable.h>
+#include <commands/common/HexConversion.h>
 #include <json/json.h>
 #include <lib/core/Optional.h>
 #include <lib/support/BytesToHex.h>
@@ -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<uint8_t *>(chip::Platform::MemoryCalloc(allocSize, sizeof(uint8_t)));
+                    return buffer;
+                },
+                &size);
 
-            buffer = static_cast<uint8_t *>(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
@@ -19,6 +19,7 @@
 #pragma once
 
 #include <app-common/zap-generated/cluster-objects.h>
+#include <commands/common/HexConversion.h>
 #include <lib/support/BytesToHex.h>
 #include <lib/support/CHIPMemString.h>
 #include <lib/support/SafeInt.h>
@@ -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<uint8_t> 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
@@ -18,6 +18,7 @@
 
 #include "Command.h"
 #include "CustomStringPrefix.h"
+#include "HexConversion.h"
 #include "platform/PlatformManager.h"
 
 #include <functional>
@@ -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<uint8_t> 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
@@ -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 <lib/core/CHIPError.h>
+#include <lib/support/BytesToHex.h>
+#include <lib/support/Span.h>
+#include <lib/support/logging/CHIPLogging.h>
+
+/**
+ * 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 <typename F>
+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<int>(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<unsigned long long>(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<int>(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
@@ -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
@@ -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",