From b4e217eeffcf148cd3de25bd05a9538923779c87 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 1 Feb 2026 20:44:44 +0100 Subject: [PATCH 1/2] Detect recursive subtree cycles at parse time (fixes #979) Use an ancestor set passed through recursivelyCreateSubtree to detect cycles, avoiding the substring-matching false positives of a prefix path check. Co-Authored-By: Claude Opus 4.5 --- src/xml_parsing.cpp | 21 ++++++----- tests/gtest_subtree.cpp | 80 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 8 deletions(-) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index a11383041..1911fe670 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #if defined(_MSVC_LANG) && !defined(__clang__) #define __bt_cplusplus (_MSC_VER == 1900 ? 201103L : _MSVC_LANG) @@ -196,7 +197,8 @@ struct XMLParser::PImpl void recursivelyCreateSubtree(const std::string& tree_ID, const std::string& tree_path, const std::string& prefix_path, Tree& output_tree, Blackboard::Ptr blackboard, - const TreeNode::Ptr& root_node); + const TreeNode::Ptr& root_node, + std::unordered_set ancestors = {}); void getPortsRecursively(const XMLElement* element, std::vector& output_ports); @@ -1000,13 +1002,16 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element, return new_node; } -void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID, - const std::string& tree_path, - const std::string& prefix_path, - Tree& output_tree, - Blackboard::Ptr blackboard, - const TreeNode::Ptr& root_node) +void BT::XMLParser::PImpl::recursivelyCreateSubtree( + const std::string& tree_ID, const std::string& tree_path, + const std::string& prefix_path, Tree& output_tree, Blackboard::Ptr blackboard, + const TreeNode::Ptr& root_node, std::unordered_set ancestors) { + if(!ancestors.insert(tree_ID).second) + { + throw RuntimeError("Recursive behavior tree cycle detected: tree '", tree_ID, + "' references itself (directly or indirectly)"); + } std::function recursiveStep; @@ -1135,7 +1140,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID, recursivelyCreateSubtree(subtree_ID, subtree_path, // name subtree_path + "/", //prefix - output_tree, new_bb, node); + output_tree, new_bb, node, ancestors); } }; diff --git a/tests/gtest_subtree.cpp b/tests/gtest_subtree.cpp index 442bfbee9..31e19c0ff 100644 --- a/tests/gtest_subtree.cpp +++ b/tests/gtest_subtree.cpp @@ -732,6 +732,86 @@ TEST(SubTree, SubtreeNameNotRegistered) ASSERT_ANY_THROW(factory.registerBehaviorTreeFromText(xml_text)); } +TEST(SubTree, RecursiveSubtree) +{ + // clang-format off + + static const char* xml_text = R"( + + + + + + + + + )"; + + // clang-format on + BehaviorTreeFactory factory; + + ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text)); +} + +TEST(SubTree, RecursiveCycle) +{ + // clang-format off + + static const char* xml_text = R"( + + + + + + + + + + + + + + + + + + + + + + + )"; + + // clang-format on + BehaviorTreeFactory factory; + + ASSERT_ANY_THROW(auto tree = factory.createTreeFromText(xml_text)); +} + +TEST(SubTree, SubstringTreeIDsAreNotRecursive) +{ + // Verify that tree IDs which are substrings of each other do NOT + // incorrectly trigger the recursive cycle detection. + // clang-format off + + static const char* xml_text = R"( + + + + + + + + + + )"; + + // clang-format on + BehaviorTreeFactory factory; + + ASSERT_NO_THROW(auto tree = factory.createTreeFromText(xml_text)); +} + // Test for Groot2 issue #56: duplicate _fullpath when multiple subtrees have the same name // https://github.com/BehaviorTree/Groot2/issues/56 // From 1d11a40c2c17718b7f178f0f0af13758533bdc9c Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 1 Feb 2026 20:49:11 +0100 Subject: [PATCH 2/2] Pass ancestors set by reference instead of by value Avoids copying the set at each recursive call. Uses insert-on-entry and erase-on-exit (DFS backtracking) so sibling subtrees sharing the same ID are handled correctly. Co-Authored-By: Claude Opus 4.5 --- src/xml_parsing.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 1911fe670..1318e7b69 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -198,7 +198,7 @@ struct XMLParser::PImpl const std::string& prefix_path, Tree& output_tree, Blackboard::Ptr blackboard, const TreeNode::Ptr& root_node, - std::unordered_set ancestors = {}); + std::unordered_set& ancestors); void getPortsRecursively(const XMLElement* element, std::vector& output_ports); @@ -705,8 +705,9 @@ Tree XMLParser::instantiateTree(const Blackboard::Ptr& root_blackboard, "root_blackboard"); } + std::unordered_set ancestors; _p->recursivelyCreateSubtree(main_tree_ID, {}, {}, output_tree, root_blackboard, - TreeNode::Ptr()); + TreeNode::Ptr(), ancestors); output_tree.initialize(); return output_tree; } @@ -1005,7 +1006,7 @@ TreeNode::Ptr XMLParser::PImpl::createNodeFromXML(const XMLElement* element, void BT::XMLParser::PImpl::recursivelyCreateSubtree( const std::string& tree_ID, const std::string& tree_path, const std::string& prefix_path, Tree& output_tree, Blackboard::Ptr blackboard, - const TreeNode::Ptr& root_node, std::unordered_set ancestors) + const TreeNode::Ptr& root_node, std::unordered_set& ancestors) { if(!ancestors.insert(tree_ID).second) { @@ -1162,6 +1163,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree( output_tree.subtrees.push_back(new_tree); recursiveStep(root_node, new_tree, prefix_path, root_element); + ancestors.erase(tree_ID); } void XMLParser::PImpl::getPortsRecursively(const XMLElement* element,