diff --git a/include/behaviortree_cpp/json_export.h b/include/behaviortree_cpp/json_export.h index c14447091..cbf12d331 100644 --- a/include/behaviortree_cpp/json_export.h +++ b/include/behaviortree_cpp/json_export.h @@ -3,6 +3,9 @@ #include "behaviortree_cpp/basic_types.h" #include "behaviortree_cpp/utils/safe_any.hpp" +#include +#include + // Use the version of nlohmann::json embedded in BT.CPP, // or the system one if BT.CPP was built with USE_VENDORED_JSON=OFF #ifdef BTCPP_SYSTEM_JSON @@ -250,6 +253,45 @@ inline void RegisterJsonDefinition() JsonExporter::get().addConverter(); } +/** + * @brief Read a JSON integer that must be representable as T. + * + * json.get() converts a floating point value or an out-of-range integer + * with a plain static_cast, which truncates, wraps, or (for a value outside + * the range of T) is undefined behavior. This throws RuntimeError instead. + */ +template +[[nodiscard]] inline T JsonToInteger(const nlohmann::json& value) +{ + static_assert(std::is_integral_v && !std::is_same_v); + if(value.is_number_unsigned()) + { + const auto number = value.get(); + if(number <= static_cast(std::numeric_limits::max())) + { + return static_cast(number); + } + } + else if(value.is_number_integer()) + { + const auto number = value.get(); + if constexpr(std::is_signed_v) + { + if(number >= static_cast(std::numeric_limits::min()) && + number <= static_cast(std::numeric_limits::max())) + { + return static_cast(number); + } + } + else if(number >= 0 && static_cast(number) <= std::numeric_limits::max()) + { + return static_cast(number); + } + } + throw RuntimeError(StrCat("JSON value [", value.dump(), "] is not an integer of type ", + BT::demangle(typeid(T)))); +} + } // namespace BT //------------------------------------------------ diff --git a/include/behaviortree_cpp/loggers/groot2_protocol.h b/include/behaviortree_cpp/loggers/groot2_protocol.h index 3ccc7b20d..cb7c178b3 100644 --- a/include/behaviortree_cpp/loggers/groot2_protocol.h +++ b/include/behaviortree_cpp/loggers/groot2_protocol.h @@ -1,6 +1,7 @@ #pragma once #include "behaviortree_cpp/basic_types.h" +#include "behaviortree_cpp/json_export.h" #ifdef BTCPP_SYSTEM_JSON #include #else @@ -256,10 +257,10 @@ inline void to_json(nlohmann::json& js, const Hook& bp) inline void from_json(const nlohmann::json& js, Hook& bp) { js.at("enabled").get_to(bp.enabled); - js.at("uid").get_to(bp.node_uid); + bp.node_uid = JsonToInteger(js.at("uid")); js.at("once").get_to(bp.remove_when_done); - bp.mode = static_cast(js.at("mode").get()); - bp.position = static_cast(js.at("position").get()); + bp.mode = static_cast(JsonToInteger(js.at("mode"))); + bp.position = static_cast(JsonToInteger(js.at("position"))); const std::string desired_value = js.at("desired_status").get(); bp.desired_status = convertFromString(desired_value); diff --git a/src/basic_types.cpp b/src/basic_types.cpp index e4c1554eb..6b346fd39 100644 --- a/src/basic_types.cpp +++ b/src/basic_types.cpp @@ -309,7 +309,18 @@ std::vector convertFromString>(StringView str) if(StartWith(str, "json:")) { str.remove_prefix(5); - return nlohmann::json::parse(str).get>(); + const auto json = nlohmann::json::parse(str); + if(!json.is_array()) + { + throw RuntimeError(StrCat("Can't convert string [", str, "] to std::vector")); + } + std::vector output; + output.reserve(json.size()); + for(const auto& element : json) + { + output.push_back(JsonToInteger(element)); + } + return output; } auto parts = splitString(str, ';'); std::vector output; diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index 9c47bd331..8cd35abf9 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -14,6 +14,7 @@ #include "tinyxml2.h" +#include "behaviortree_cpp/json_export.h" #include "behaviortree_cpp/utils/shared_library.h" #include "behaviortree_cpp/utils/wildcards.hpp" #include "behaviortree_cpp/xml_parsing.h" @@ -569,7 +570,7 @@ void BehaviorTreeFactory::loadSubstitutionRuleFromJSON(const std::string& json_t if(test_config.contains("async_delay")) { config.async_delay = - std::chrono::milliseconds(test_config["async_delay"].get()); + std::chrono::milliseconds(JsonToInteger(test_config["async_delay"])); } if(test_config.contains("post_script")) { diff --git a/src/json_export.cpp b/src/json_export.cpp index c9372045b..d9bb4e290 100644 --- a/src/json_export.cpp +++ b/src/json_export.cpp @@ -116,8 +116,14 @@ JsonExporter::ExpectedEntry JsonExporter::fromJson(const nlohmann::json& source) } if(first_element.is_number_integer()) { - return Entry{ BT::Any(source.get>()), - BT::TypeInfo::Create>() }; + // the type comes from the first element only: check every element + std::vector values; + values.reserve(source.size()); + for(const auto& element : source) + { + values.push_back(JsonToInteger(element)); + } + return Entry{ BT::Any(values), BT::TypeInfo::Create>() }; } if(first_element.is_number_float()) { diff --git a/src/loggers/groot2_publisher.cpp b/src/loggers/groot2_publisher.cpp index af740e756..7572856f3 100644 --- a/src/loggers/groot2_publisher.cpp +++ b/src/loggers/groot2_publisher.cpp @@ -352,8 +352,9 @@ void Groot2Publisher::serverLoop() } auto InsertHook = [this](nlohmann::json const& json) { - uint16_t const node_uid = json.at("uid").get(); - Position const pos = static_cast(json.at("position").get()); + uint16_t const node_uid = JsonToInteger(json.at("uid")); + int const position = JsonToInteger(json.at("position")); + Position const pos = static_cast(position); if(auto hook = getHook(pos, node_uid)) { @@ -403,9 +404,9 @@ void Groot2Publisher::serverLoop() } auto json = nlohmann::json::parse(requestMsg[1].to_string()); - const uint16_t node_uid = json.at("uid").get(); + const uint16_t node_uid = JsonToInteger(json.at("uid")); const std::string status_str = json.at("desired_status").get(); - auto position = static_cast(json.at("position").get()); + auto position = static_cast(JsonToInteger(json.at("position"))); const bool remove = json.at("remove_when_done").get(); NodeStatus desired_status = NodeStatus::SKIPPED; @@ -444,8 +445,8 @@ void Groot2Publisher::serverLoop() } auto json = nlohmann::json::parse(requestMsg[1].to_string()); - const uint16_t node_uid = json.at("uid").get(); - auto position = static_cast(json.at("position").get()); + const uint16_t node_uid = JsonToInteger(json.at("uid")); + auto position = static_cast(JsonToInteger(json.at("position"))); if(!removeHook(position, node_uid)) { diff --git a/tests/gtest_groot2_publisher_integration.cpp b/tests/gtest_groot2_publisher_integration.cpp index 41b1c4625..165c99e17 100644 --- a/tests/gtest_groot2_publisher_integration.cpp +++ b/tests/gtest_groot2_publisher_integration.cpp @@ -161,6 +161,40 @@ TEST(Groot2PublisherIntegration, PostHookRunsAfterNodeAndCanBeRemoved) EXPECT_EQ(tick_count, 2); } +TEST(Groot2PublisherIntegration, HookRequestRejectsNonIntegerUid) +{ + BT::BehaviorTreeFactory factory; + factory.registerSimpleAction("CountAction", + [](BT::TreeNode&) { return BT::NodeStatus::SUCCESS; }); + auto tree = factory.createTreeFromText(kHookTreeXml); + auto [publisher, port] = Groot2Test::makePublisher(tree); + Groot2Test::Client client(port); + const auto uid = tree.rootNode()->UID(); + + // get() used to wrap uid + 65536 and truncate uid + 0.5 to the uid + // of the node, and to cast 1e300 out of range (undefined behavior). + for(const nlohmann::json bad_uid : + { nlohmann::json(uid + 65536), nlohmann::json(uid + 0.5), nlohmann::json(1e300) }) + { + auto hook = Groot2Test::makeHook(uid, BT::Monitor::Hook::Mode::REPLACE, + BT::Monitor::Hook::Position::PRE); + hook["uid"] = bad_uid; + auto reply = client.rawRequest(BT::Monitor::RequestType::HOOK_INSERT, hook.dump()); + ASSERT_EQ(reply.size(), 2); + EXPECT_EQ(reply[0].to_string(), "error") << bad_uid; + + const auto remove = nlohmann::json{ { "uid", bad_uid }, { "position", 0 } }; + reply = client.rawRequest(BT::Monitor::RequestType::HOOK_REMOVE, remove.dump()); + ASSERT_EQ(reply.size(), 2); + EXPECT_EQ(reply[0].to_string(), "error") << bad_uid; + } + + auto dump_reply = client.request(BT::Monitor::RequestType::HOOKS_DUMP); + ASSERT_EQ(dump_reply.size(), 2); + EXPECT_TRUE(nlohmann::json::parse(dump_reply[1].to_string()).empty()); + EXPECT_EQ(tree.tickExactlyOnce(), BT::NodeStatus::SUCCESS); +} + TEST(Groot2PublisherIntegration, MalformedRequestDoesNotTerminateServer) { ::testing::FLAGS_gtest_death_test_style = "threadsafe"; diff --git a/tests/gtest_json.cpp b/tests/gtest_json.cpp index 559b405e8..fb2f3743c 100644 --- a/tests/gtest_json.cpp +++ b/tests/gtest_json.cpp @@ -271,6 +271,52 @@ TEST_F(JsonTest, VectorInteger) } } +TEST_F(JsonTest, IntegerValuesAreRangeChecked) +{ + using nlohmann::json; + BT::JsonExporter& exporter = BT::JsonExporter::get(); + + ASSERT_EQ(BT::JsonToInteger(json(-7)), -7); + ASSERT_EQ(BT::JsonToInteger(json(65535)), 65535); + ASSERT_EQ(BT::JsonToInteger(json(INT64_MIN)), INT64_MIN); + ASSERT_EQ(BT::JsonToInteger(json(UINT64_MAX)), UINT64_MAX); + ASSERT_THROW(std::ignore = BT::JsonToInteger(json(65536)), BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::JsonToInteger(json(-1)), BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::JsonToInteger(json(2.5)), BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::JsonToInteger(json(1e300)), BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::JsonToInteger(json(true)), BT::RuntimeError); + + // The type of the array is taken from the first element. The other elements + // used to be truncated, wrapped, or cast out of range (undefined behavior). + for(const char* text : + { "[1, 2.5]", "[1, 1e300]", "[1, 5000000000]", "[1, -3000000000]" }) + { + ASSERT_THROW(std::ignore = exporter.fromJson(json::parse(text)), BT::RuntimeError) + << text; + auto bb = BT::Blackboard::create(); + ASSERT_THROW(BT::ImportBlackboardFromJSON( + json::parse(R"({"v":)" + std::string(text) + "}"), *bb), + BT::RuntimeError) + << text; + } + auto result = exporter.fromJson(json::parse("[1, -2, 2147483647]")); + ASSERT_TRUE(result) << result.error(); + ASSERT_EQ(result->first.cast>(), + (std::vector{ 1, -2, 2147483647 })); + + // Same for the "json:" prefix of a std::vector port + ASSERT_EQ(BT::convertFromString>("json:[3, -4]"), + (std::vector{ 3, -4 })); + ASSERT_THROW(std::ignore = BT::convertFromString>("json:[1e300]"), + BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::convertFromString>("json:[1.5]"), + BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::convertFromString>("json:[4294967297]"), + BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::convertFromString>("json:{\"a\":1}"), + BT::RuntimeError); +} + TEST_F(JsonTest, VectorSring) { BT::JsonExporter& exporter = BT::JsonExporter::get(); diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index 1883b0385..24ee04d7c 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -133,6 +133,26 @@ TEST(Substitution, ParserRejectsMissingReturnStatusAndScript) ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(invalid_json), RuntimeError); } +TEST(Substitution, ParserRejectsNonIntegerAsyncDelay) +{ + // async_delay used to be read with get(), which truncates a real + // number and casts an out-of-range one to int (undefined behavior). + for(const char* delay : { "1e300", "2.5", "5000000000" }) + { + const std::string json_text = std::string(R"( + { + "TestNodeConfigs": { + "Delayed": { "return_status": "SUCCESS", "async_delay": )") + + delay + R"( } + }, + "SubstitutionRules": { "actionA": "Delayed" } + })"; + + BehaviorTreeFactory factory; + ASSERT_THROW(factory.loadSubstitutionRuleFromJSON(json_text), RuntimeError) << delay; + } +} + TEST(Substitution, ScriptedReturnStatusFromJson) { static const char* xml_text = R"(