Fix #672: detect and reject deeply-nested/recursive XML to prevent stack overflow (#1091)

Adds a depth limit (256) to the recursive XML parsing functions in
VerifyXML() and recursivelyCreateSubtree(). Malformed XML with extreme
nesting now throws a clear RuntimeError instead of crashing.

Includes 11 new tests for malformed XML handling.

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

src/xml_parsing.cpp

@@ -530,9 +530,16 @@ void VerifyXML(const std::string& xml_text,
   }
 
   // function to be called recursively
-  std::function<void(const XMLElement*)> recursiveStep;
+  constexpr int kMaxNestingDepth = 256;
+  std::function<void(const XMLElement*, int)> recursiveStep;
 
-  recursiveStep = [&](const XMLElement* node) {
+  recursiveStep = [&](const XMLElement* node, int depth) {
+    if(depth > kMaxNestingDepth)
+    {
+      ThrowError(node->GetLineNum(), "Maximum XML nesting depth exceeded (limit: " +
+                                         std::to_string(kMaxNestingDepth) +
+                                         "). The XML is too deeply nested.");
+    }
     const int children_count = ChildrenCount(node);
     const std::string name = node->Name();
     const std::string ID = node->Attribute("ID") != nullptr ? node->Attribute("ID") : "";
@@ -661,14 +668,14 @@ void VerifyXML(const std::string& xml_text,
     for(auto child = node->FirstChildElement(); child != nullptr;
         child = child->NextSiblingElement())
     {
-      recursiveStep(child);
+      recursiveStep(child, depth + 1);
     }
   };
 
   for(auto bt_root = xml_root->FirstChildElement("BehaviorTree"); bt_root != nullptr;
       bt_root = bt_root->NextSiblingElement("BehaviorTree"))
   {
-    recursiveStep(bt_root);
+    recursiveStep(bt_root, 0);
   }
 }
 
@@ -1018,12 +1025,20 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(
     throw RuntimeError("Recursive behavior tree cycle detected: tree '", tree_ID,
                        "' references itself (directly or indirectly)");
   }
+  constexpr int kMaxNestingDepth = 256;
   std::function<void(const TreeNode::Ptr&, Tree::Subtree::Ptr, std::string,
-                     const XMLElement*)>
+                     const XMLElement*, int)>
       recursiveStep;
 
   recursiveStep = [&](TreeNode::Ptr parent_node, Tree::Subtree::Ptr subtree,
-                      std::string prefix, const XMLElement* element) {
+                      std::string prefix, const XMLElement* element, int depth) {
+    if(depth > kMaxNestingDepth)
+    {
+      throw RuntimeError("Maximum XML nesting depth exceeded during tree "
+                         "instantiation (limit: ",
+                         std::to_string(kMaxNestingDepth),
+                         "). The XML is too deeply nested.");
+    }
     // create the node
     auto node = createNodeFromXML(element, blackboard, parent_node, prefix, output_tree);
     subtree->nodes.push_back(node);
@@ -1034,7 +1049,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(
       for(auto child_element = element->FirstChildElement(); child_element != nullptr;
           child_element = child_element->NextSiblingElement())
       {
-        recursiveStep(node, subtree, prefix, child_element);
+        recursiveStep(node, subtree, prefix, child_element, depth + 1);
       }
     }
     else  // special case: SubTreeNode
@@ -1167,7 +1182,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(
   new_tree->tree_ID = tree_ID;
   output_tree.subtrees.push_back(new_tree);
 
-  recursiveStep(root_node, new_tree, prefix_path, root_element);
+  recursiveStep(root_node, new_tree, prefix_path, root_element, 0);
   ancestors.erase(tree_ID);
 }
 

tests/gtest_factory.cpp

@@ -563,3 +563,170 @@ TEST(BehaviorTreeFactory, FactoryDestroyedBeforeTick)
   // when getInput() looked up the port in the manifest.
   EXPECT_EQ(BT::NodeStatus::SUCCESS, tree.tickWhileRunning());
 }
+
+// Regression tests for issue #672: stack buffer overflow in
+// xml_parsing.cpp when parsing malformed/pathological XML.
+// In v3 a fuzz test triggered a stack-buffer-overflow via ASAN in
+// the BehaviorTree element iteration loop with recursiveStep.
+// In v4 the parser was rewritten, but the recursive validation and
+// instantiation paths can still overflow the stack with deeply nested
+// input.  The fix adds a depth limit.
+
+TEST(BehaviorTreeFactory, MalformedXML_InvalidRoot)
+{
+  // XML that is not valid XML at all
+  BehaviorTreeFactory factory;
+  EXPECT_ANY_THROW(factory.createTreeFromText("<not valid xml!!!"));
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_MissingRootElement)
+{
+  // Well-formed XML but missing <root> element
+  const char* xml = R"(
+  <something BTCPP_format="4">
+    <BehaviorTree ID="Main">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </something>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_EmptyBehaviorTree)
+{
+  // BehaviorTree element with no children
+  const char* xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="Main">
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_EmptyBehaviorTreeID)
+{
+  // BehaviorTree element with empty ID when multiple trees exist
+  const char* xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+    <BehaviorTree ID="Other">
+      <AlwaysSuccess/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_MissingBehaviorTreeID)
+{
+  // Multiple BehaviorTree elements without IDs
+  const char* xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree>
+      <AlwaysSuccess/>
+    </BehaviorTree>
+    <BehaviorTree>
+      <AlwaysFailure/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_DeeplyNestedElements)
+{
+  // Generate XML with nesting depth that exceeds the limit (256).
+  // This should throw a readable exception rather than crash with
+  // a stack overflow.
+  std::string xml = R"(<root BTCPP_format="4"><BehaviorTree ID="Main">)";
+  const int depth = 300;
+  for(int i = 0; i < depth; i++)
+  {
+    xml += "<Sequence>";
+  }
+  xml += "<AlwaysSuccess/>";
+  for(int i = 0; i < depth; i++)
+  {
+    xml += "</Sequence>";
+  }
+  xml += "</BehaviorTree></root>";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_ModerateNestingIsOK)
+{
+  // Nesting depth well within the limit should succeed.
+  std::string xml = R"(<root BTCPP_format="4"><BehaviorTree ID="Main">)";
+  const int depth = 50;
+  for(int i = 0; i < depth; i++)
+  {
+    xml += "<Sequence>";
+  }
+  xml += "<AlwaysSuccess/>";
+  for(int i = 0; i < depth; i++)
+  {
+    xml += "</Sequence>";
+  }
+  xml += "</BehaviorTree></root>";
+
+  BehaviorTreeFactory factory;
+  // Should not throw
+  EXPECT_NO_THROW(factory.createTreeFromText(xml));
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_MultipleBTChildElements)
+{
+  // BehaviorTree with more than one child element
+  const char* xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="Main">
+      <AlwaysSuccess/>
+      <AlwaysFailure/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_CompletelyEmpty)
+{
+  // Completely empty string
+  BehaviorTreeFactory factory;
+  EXPECT_ANY_THROW(factory.createTreeFromText(""));
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_EmptyRoot)
+{
+  // Root element with no children at all
+  const char* xml = R"(<root BTCPP_format="4"></root>)";
+
+  BehaviorTreeFactory factory;
+  // No BehaviorTree elements: registering succeeds but creating
+  // a tree should fail because there is nothing to instantiate.
+  factory.registerBehaviorTreeFromText(xml);
+  EXPECT_ANY_THROW(factory.createTree("MainTree"));
+}
+
+TEST(BehaviorTreeFactory, MalformedXML_UnknownNodeType)
+{
+  // Reference to a node type that is not registered
+  const char* xml = R"(
+  <root BTCPP_format="4">
+    <BehaviorTree ID="Main">
+      <NonExistentNodeType/>
+    </BehaviorTree>
+  </root>)";
+
+  BehaviorTreeFactory factory;
+  EXPECT_THROW(factory.createTreeFromText(xml), RuntimeError);
+}