From fb7de2a73c17b84b9f10c6410a8916cf4683c3ab Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 14 Jun 2022 10:34:08 -0400 Subject: [PATCH] Improve custom/complex argument JSON parsing for chip-tool. (#19530) Specific changes: 1) Use CharReaderBuilder/CharReader, because that allows us to set 'failIfExtra' to true, so we fail out on trailing garbage. 2) Since now we can, set 'allowSingleQuotes' to true, so people can use single quotes in their JSON if that's more convenient. 3) Add checking for duplicated keys by setting rejectDupKeys to true. 4) For complex arguments, enforce that the value is in fact an array or object, not a primitive value. 5) Factour out the parsing logic from ComplexArgument so we can use it in CustomArgument too. 6) Make CustomArgument fail out on JSON parse errors instead of silently doing unexpected things. 7) For CustomArgument, detect an argument starting with one of our type prefixes ("hex:", "i:", "u:", "f:", "d:") and if so just treat it as a string value instead of trying to do a full JSON parse (which would fail anyway). 8) For CustomArgument, detect an argument starting with "0x" and treat it as a string starting "u:0x", so it will be parsed as a hex number instead of just failing to parse. This leads to the following behavior: ./chip-tool any write-by-id 0x130AFC01 0x130A0001 123 17 1 Before: sends 123 as signed int. After: same. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 0x123 17 1 Before: sends 0. After: sends 291 as unsigned int. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 u:123 17 1 Before: sends null. After: sends 123 as unsigned int. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 hex:121314 17 1 Before: sends null. After: sends byte string with bytes 0x12, 0x13, 0x14. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 "[1, 2, 3], [4, 5, 6]" 17 1 Before: sends list [1, 2, 3]. After: errors out on trailing garbage. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 "[1 2, 3]" 17 1 Before: sends list [1]. After: errors out with "Missing ',' or ']' in array declaration". ./chip-tool accesscontrol write acl '[{"privilege": 5, "authMode": 2, "subjects": [112233], "targets": [{"cluster": null, "endpoint": null, "deviceType": 0x100}]}]' 17 0 Before: Errors out on the invalid hex number not in quotes, shows the error location. After: same. ./chip-tool accesscontrol write acl '[{"privilege": 5, "authMode": 2, "subjects": [112233], "targets": []}], [{"targets": [{"cluster": null, "endpoint": null, "deviceType": 0x100}]}]' 17 0 Before: sends the first array and ignores the second one. After: errors out on trailing garbage. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 '{"1": 0x100 }' 17 1 Before: Sends {"1": 0} After: Parse error on invalid hex. ./chip-tool any write-by-id 0x130AFC01 0x130A0001 '{"1": "u:0x100" }' 17 1 Before: sends {"1": 256} After: same. Fixes https://github.com/project-chip/connectedhomeip/issues/19529 --- .../commands/clusters/ComplexArgument.h | 53 +----- .../commands/clusters/CustomArgument.h | 27 ++- .../chip-tool/commands/clusters/JsonParser.h | 162 ++++++++++++++++++ 3 files changed, 190 insertions(+), 52 deletions(-) create mode 100644 examples/chip-tool/commands/clusters/JsonParser.h diff --git a/examples/chip-tool/commands/clusters/ComplexArgument.h b/examples/chip-tool/commands/clusters/ComplexArgument.h index 689b2e6f9a217e..d02afd9ee98741 100644 --- a/examples/chip-tool/commands/clusters/ComplexArgument.h +++ b/examples/chip-tool/commands/clusters/ComplexArgument.h @@ -26,6 +26,8 @@ #include #include +#include "JsonParser.h" + constexpr uint8_t kMaxLabelLength = 100; class ComplexArgumentParser @@ -314,57 +316,8 @@ class TypedComplexArgument : public ComplexArgument CHIP_ERROR Parse(const char * label, const char * json) { Json::Value value; - Json::Reader reader; - if (!reader.parse(json, value)) + if (!JsonParser::ParseComplexArgument(label, json, value)) { - std::vector errors = reader.getStructuredErrors(); - ChipLogError(chipTool, "Error parsing JSON for %s:", label); - for (auto & error : errors) - { - ChipLogError(chipTool, " %s", error.message.c_str()); - ptrdiff_t error_start = error.offset_start; - ptrdiff_t error_end = error.offset_limit; - const char * sourceText = json; - // The whole JSON string might be too long to fit in our log - // messages. Just include 30 chars before the error. - constexpr ptrdiff_t kMaxContext = 30; - std::string errorMsg; - if (error_start > kMaxContext) - { - sourceText += (error_start - kMaxContext); - error_end = kMaxContext + (error_end - error_start); - error_start = kMaxContext; - ChipLogError(chipTool, "... %s", sourceText); - // Add markers corresponding to the "... " above. - errorMsg += "----"; - } - else - { - ChipLogError(chipTool, "%s", sourceText); - } - for (ptrdiff_t i = 0; i < error_start; ++i) - { - errorMsg += "-"; - } - errorMsg += "^"; - if (error_start + 1 < error_end) - { - for (ptrdiff_t i = error_start + 1; i < error_end; ++i) - { - errorMsg += "-"; - } - errorMsg += "^"; - } - ChipLogError(chipTool, "%s", errorMsg.c_str()); - - if (error.message == "Missing ',' or '}' in object declaration" && error.offset_start > 0 && - json[error.offset_start - 1] == '0' && (json[error.offset_start] == 'x' || json[error.offset_start] == 'X')) - { - ChipLogError(chipTool, - "NOTE: JSON does not allow hex syntax beginning with 0x for numbers. Try putting the hex number " - "in quotes (like {\"name\": \"0x100\"})."); - } - } return CHIP_ERROR_INVALID_ARGUMENT; } diff --git a/examples/chip-tool/commands/clusters/CustomArgument.h b/examples/chip-tool/commands/clusters/CustomArgument.h index 2362171aa06da9..6755703fa0b7d5 100644 --- a/examples/chip-tool/commands/clusters/CustomArgument.h +++ b/examples/chip-tool/commands/clusters/CustomArgument.h @@ -22,6 +22,8 @@ #include #include +#include "JsonParser.h" + namespace { static constexpr char kPayloadHexPrefix[] = "hex:"; static constexpr char kPayloadSignedPrefix[] = "s:"; @@ -230,9 +232,30 @@ class CustomArgument CHIP_ERROR Parse(const char * label, const char * json) { - Json::Reader reader; Json::Value value; - reader.parse(json, value); + constexpr const char kHexNumPrefix[] = "0x"; + constexpr size_t kHexNumPrefixLen = ArraySize(kHexNumPrefix) - 1; + if (strncmp(json, kPayloadHexPrefix, kPayloadHexPrefixLen) == 0 || + strncmp(json, kPayloadSignedPrefix, kPayloadSignedPrefixLen) == 0 || + strncmp(json, kPayloadUnsignedPrefix, kPayloadUnsignedPrefixLen) == 0 || + strncmp(json, kPayloadFloatPrefix, kPayloadFloatPrefixLen) == 0 || + strncmp(json, kPayloadDoublePrefix, kPayloadDoublePrefixLen) == 0) + { + value = Json::Value(json); + } + else if (strncmp(json, kHexNumPrefix, kHexNumPrefixLen) == 0) + { + // Assume that hex numbers are unsigned. Prepend + // kPayloadUnsignedPrefix and then let the rest of the logic handle + // things. + std::string str(kPayloadUnsignedPrefix); + str += json; + value = Json::Value(str); + } + else if (!JsonParser::ParseCustomArgument(label, json, value)) + { + return CHIP_ERROR_INVALID_ARGUMENT; + } mData = static_cast(chip::Platform::MemoryCalloc(sizeof(uint8_t), mDataMaxLen)); VerifyOrReturnError(mData != nullptr, CHIP_ERROR_NO_MEMORY); diff --git a/examples/chip-tool/commands/clusters/JsonParser.h b/examples/chip-tool/commands/clusters/JsonParser.h new file mode 100644 index 00000000000000..2b74aee6fffb7c --- /dev/null +++ b/examples/chip-tool/commands/clusters/JsonParser.h @@ -0,0 +1,162 @@ +/* + * 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 +#include +#include + +class JsonParser +{ +public: + // Returns whether the parse succeeded. + static bool ParseComplexArgument(const char * label, const char * json, Json::Value & value) + { + return Parse(label, json, /* strictRoot = */ true, value); + } + + // Returns whether the parse succeeded. + static bool ParseCustomArgument(const char * label, const char * json, Json::Value & value) + { + return Parse(label, json, /* strictRoot = */ false, value); + } + +private: + static bool Parse(const char * label, const char * json, bool strictRoot, Json::Value & value) + { + Json::CharReaderBuilder readerBuilder; + readerBuilder.settings_["strictRoot"] = strictRoot; + readerBuilder.settings_["allowSingleQuotes"] = true; + readerBuilder.settings_["failIfExtra"] = true; + readerBuilder.settings_["rejectDupKeys"] = true; + + auto reader = std::unique_ptr(readerBuilder.newCharReader()); + std::string errors; + if (reader->parse(json, json + strlen(json), &value, &errors)) + { + return true; + } + + // The CharReader API allows us to set failIfExtra, unlike Reader, but does + // not allow us to get structured errors. We get to try to manually undo + // the work it did to create a string from the structured errors it had. + ChipLogError(chipTool, "Error parsing JSON for %s:", label); + + // For each error "errors" has the following: + // + // 1) A line starting with "* " that has line/column info + // 2) A line with the error message. + // 3) An optional line with some extra info. + // + // We keep track of the last error column, in case the error message + // reporting needs it. + std::istringstream stream(errors); + std::string error; + chip::Optional errorColumn; + while (getline(stream, error)) + { + if (error.rfind("* ", 0) == 0) + { + // Flush out any pending error location. + LogErrorLocation(errorColumn, json); + + // The format of this line is: + // + // * Line N, Column M + // + // Unfortunately it does not indicate end of error, so we can only + // show its start. + unsigned errorLine; // ignored in practice + if (sscanf(error.c_str(), "* Line %u, Column %u", &errorLine, &errorColumn.Emplace()) != 2) + { + ChipLogError(chipTool, "Unexpected location string: %s\n", error.c_str()); + // We don't know how to make sense of this thing anymore. + break; + } + if (errorColumn.Value() == 0) + { + ChipLogError(chipTool, "Expected error column to be at least 1"); + // We don't know how to make sense of this thing anymore. + break; + } + // We are using our column numbers as offsets, so want them to be + // 0-based. + --errorColumn.Value(); + } + else + { + ChipLogError(chipTool, " %s", error.c_str()); + if (error == " Missing ',' or '}' in object declaration" && errorColumn.HasValue() && errorColumn.Value() > 0 && + json[errorColumn.Value() - 1] == '0' && (json[errorColumn.Value()] == 'x' || json[errorColumn.Value()] == 'X')) + { + // Log the error location marker before showing the NOTE + // message. + LogErrorLocation(errorColumn, json); + ChipLogError(chipTool, + "NOTE: JSON does not allow hex syntax beginning with 0x for numbers. Try putting the hex number " + "in quotes (like {\"name\": \"0x100\"})."); + } + } + } + + // Write out the marker for our last error. + LogErrorLocation(errorColumn, json); + + return false; + } + +private: + static void LogErrorLocation(chip::Optional & errorColumn, const char * json) + { + if (!errorColumn.HasValue()) + { + return; + } + + const char * sourceText = json; + unsigned error_start = errorColumn.Value(); + // The whole JSON string might be too long to fit in our log + // messages. Just include 30 chars before the error. + constexpr ptrdiff_t kMaxContext = 30; + std::string errorMarker; + if (error_start > kMaxContext) + { + sourceText += (error_start - kMaxContext); + error_start = kMaxContext; + ChipLogError(chipTool, "... %s", sourceText); + // Add markers corresponding to the "... " above. + errorMarker += "----"; + } + else + { + ChipLogError(chipTool, "%s", sourceText); + } + for (unsigned i = 0; i < error_start; ++i) + { + errorMarker += "-"; + } + errorMarker += "^"; + ChipLogError(chipTool, "%s", errorMarker.c_str()); + errorColumn.ClearValue(); + } +};