From 9ce23a00116378f70bcdbd1eb4fe82141dcfa060 Mon Sep 17 00:00:00 2001 From: Aysha Afrah Ziya Date: Sat, 31 Oct 2026 12:04:54 +0530 Subject: [PATCH] fix stack overflow on nested JSON in JsonToInteger and fromJson --- include/behaviortree_cpp/json_export.h | 7 +++- src/bt_factory.cpp | 5 ++- src/json_export.cpp | 3 +- tests/groot2_test_utils.hpp | 6 +-- tests/gtest_groot2_publisher_integration.cpp | 40 ++++++++++++++++++++ tests/gtest_json.cpp | 20 ++++++++++ tests/gtest_substitution.cpp | 16 ++++++++ 7 files changed, 89 insertions(+), 8 deletions(-) diff --git a/include/behaviortree_cpp/json_export.h b/include/behaviortree_cpp/json_export.h index 129860e8f..aa5183a88 100644 --- a/include/behaviortree_cpp/json_export.h +++ b/include/behaviortree_cpp/json_export.h @@ -290,8 +290,11 @@ template { static_assert(std::is_integral_v && !std::is_same_v); const auto not_an_integer = [&value]() { - return RuntimeError(StrCat("JSON value [", value.dump(), - "] is not an integer of type ", BT::demangle(typeid(T)))); + // dump() recurses once per nesting level and the value may come from an + // untrusted JSON text: a deeply nested array would exhaust the stack here. + const std::string text = value.is_structured() ? value.type_name() : value.dump(); + return RuntimeError(StrCat("JSON value [", text, "] is not an integer of type ", + BT::demangle(typeid(T)))); }; if(value.is_number_unsigned()) { diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index 67ff3ea67..8dba3ff02 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -551,7 +551,8 @@ void BehaviorTreeFactory::loadSubstitutionRuleFromJSON(const std::string& json_t // substitution rules that map to already-registered node types. if(json.contains("TestNodeConfigs")) { - auto test_configs = json.at("TestNodeConfigs"); + // Not a copy: it would recurse once per nesting level of the JSON text + const auto& test_configs = json.at("TestNodeConfigs"); for(auto const& [name, test_config] : test_configs.items()) { auto& config = configs[name]; @@ -595,7 +596,7 @@ void BehaviorTreeFactory::loadSubstitutionRuleFromJSON(const std::string& json_t } } - auto substitutions = json.at("SubstitutionRules"); + const auto& substitutions = json.at("SubstitutionRules"); for(auto const& [node_name, test] : substitutions.items()) { auto test_name = test.get(); diff --git a/src/json_export.cpp b/src/json_export.cpp index d5b450436..29ad76474 100644 --- a/src/json_export.cpp +++ b/src/json_export.cpp @@ -108,7 +108,8 @@ JsonExporter::ExpectedEntry JsonExporter::fromJson(const nlohmann::json& source) // basic vectors if(source.is_array() && source.size() > 0 && !source.contains("__type")) { - const auto first_element = source[0]; + // Not a copy: it would recurse once per nesting level of the untrusted value + const auto& first_element = source[0]; if(first_element.is_string()) { return Entry{ BT::Any(source.get>()), diff --git a/tests/groot2_test_utils.hpp b/tests/groot2_test_utils.hpp index 10a63342f..27b9808ce 100644 --- a/tests/groot2_test_utils.hpp +++ b/tests/groot2_test_utils.hpp @@ -61,11 +61,11 @@ inline nlohmann::json makeHook(uint16_t uid, BT::Monitor::Hook::Mode mode, class Client { public: - explicit Client(unsigned port) : socket_(context_, ZMQ_REQ) + explicit Client(unsigned port, int timeout_ms = 2000) : socket_(context_, ZMQ_REQ) { socket_.set(zmq::sockopt::linger, 0); - socket_.set(zmq::sockopt::rcvtimeo, 2000); - socket_.set(zmq::sockopt::sndtimeo, 2000); + socket_.set(zmq::sockopt::rcvtimeo, timeout_ms); + socket_.set(zmq::sockopt::sndtimeo, timeout_ms); socket_.connect("tcp://127.0.0.1:" + std::to_string(port)); } diff --git a/tests/gtest_groot2_publisher_integration.cpp b/tests/gtest_groot2_publisher_integration.cpp index b6eabc3ae..a6b9525fa 100644 --- a/tests/gtest_groot2_publisher_integration.cpp +++ b/tests/gtest_groot2_publisher_integration.cpp @@ -76,6 +76,40 @@ void incompleteHookScenario() std::exit(EXIT_SUCCESS); } +void deeplyNestedHookScenario() +{ + BT::BehaviorTreeFactory factory; + auto tree = factory.createTreeFromText(R"( + + + + + +)"); + auto [publisher, port] = Groot2Test::makePublisher(tree); + // Under the sanitizers, parsing the request below takes a few seconds. + Groot2Test::Client client(port, 30000); + + // The error for a "uid" that isn't an integer used to dump() the value, which + // recurses once per nesting level and overflowed the stack of the server thread. + constexpr size_t depth = 100000; + const std::string nested = std::string(depth, '[') + std::string(depth, ']'); + auto error_reply = client.rawRequest(BT::Monitor::RequestType::HOOK_INSERT, + R"({"uid":)" + nested + R"(,"position":0})"); + if(error_reply.size() != 2 || error_reply[0].to_string() != "error") + { + std::exit(EXIT_FAILURE); + } + + // The server thread must remain usable after rejecting the nested request. + auto valid_reply = client.request(BT::Monitor::RequestType::FULLTREE); + if(valid_reply.size() != 2) + { + std::exit(EXIT_FAILURE); + } + std::exit(EXIT_SUCCESS); +} + BT::Tree createTreeWithNamedSubtrees(BT::BehaviorTreeFactory& factory, const BT::Blackboard::Ptr& main_blackboard) { @@ -207,6 +241,12 @@ TEST(Groot2PublisherIntegration, IncompleteHookRequestDoesNotTerminateServer) EXPECT_EXIT(incompleteHookScenario(), ::testing::ExitedWithCode(EXIT_SUCCESS), ".*"); } +TEST(Groot2PublisherIntegration, DeeplyNestedHookRequestDoesNotTerminateServer) +{ + ::testing::FLAGS_gtest_death_test_style = "threadsafe"; + EXPECT_EXIT(deeplyNestedHookScenario(), ::testing::ExitedWithCode(EXIT_SUCCESS), ".*"); +} + TEST(Groot2PublisherIntegration, BindToLoopbackAddress) { BT::BehaviorTreeFactory factory; diff --git a/tests/gtest_json.cpp b/tests/gtest_json.cpp index be0518888..8b6dd6917 100644 --- a/tests/gtest_json.cpp +++ b/tests/gtest_json.cpp @@ -389,6 +389,26 @@ TEST_F(JsonTest, IntegerValuesAreRangeChecked) BT::RuntimeError); } +// nlohmann::json parses and destroys a nested value iteratively, but dumping and +// copying it recurse once per nesting level, so a short text can exhaust the stack. +TEST_F(JsonTest, DeeplyNestedValueDoesNotExhaustStack) +{ + using nlohmann::json; + constexpr size_t depth = 100000; + const std::string nested = std::string(depth, '[') + std::string(depth, ']'); + + // The error message used to include value.dump() + ASSERT_THROW(std::ignore = BT::details::JsonToInteger(json::parse(nested)), + BT::RuntimeError); + ASSERT_THROW(std::ignore = BT::convertFromString>("json:" + nested), + BT::RuntimeError); + + // fromJson() used to copy the first element to pick the type of the array + auto blackboard = BT::Blackboard::create(); + BT::ImportBlackboardFromJSON(json::parse(R"({"nested":)" + nested + "}"), *blackboard); + ASSERT_FALSE(blackboard->getEntry("nested")); +} + TEST_F(JsonTest, VectorSring) { BT::JsonExporter& exporter = BT::JsonExporter::get(); diff --git a/tests/gtest_substitution.cpp b/tests/gtest_substitution.cpp index 8f028ec4e..a1bcf9dd7 100644 --- a/tests/gtest_substitution.cpp +++ b/tests/gtest_substitution.cpp @@ -610,6 +610,22 @@ TEST(Substitution, IncompatibleTypeSubstitutionThrows) } } +// loadSubstitutionRuleFromJSON() used to copy these two sections of the JSON, and +// copying a json recurses once per nesting level. +TEST(Substitution, DeeplyNestedJsonDoesNotExhaustStack) +{ + constexpr size_t depth = 100000; + const std::string nested = std::string(depth, '[') + std::string(depth, ']'); + + BehaviorTreeFactory factory; + EXPECT_THROW(factory.loadSubstitutionRuleFromJSON(R"({"TestNodeConfigs":)" + nested + + R"(,"SubstitutionRules":{}})"), + RuntimeError); + EXPECT_THROW( + factory.loadSubstitutionRuleFromJSON(R"({"SubstitutionRules":)" + nested + "}"), + std::exception); +} + // Destroying a TestNode while its async_delay timer is pending makes // ~TimerQueue() run the handler (aborted) during the PImpl deletion. The // handler used to reach the PImpl through _p, which std::unique_ptr may have