fix stack overflow on nested JSON in JsonToInteger and fromJson (#1274)

Author: aysha-afrah26

include/behaviortree_cpp/json_export.h

@@ -290,8 +290,11 @@ template <typename T>
 {
   static_assert(std::is_integral_v<T> && !std::is_same_v<T, bool>);
   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())
   {

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<std::string>();

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<std::vector<std::string>>()),

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));
   }
 

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"(
+<root BTCPP_format="4">
+  <BehaviorTree ID="MainTree">
+    <AlwaysSuccess/>
+  </BehaviorTree>
+</root>
+)");
+  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;

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<int>(json::parse(nested)),
+               BT::RuntimeError);
+  ASSERT_THROW(std::ignore = BT::convertFromString<std::vector<int>>("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();

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