diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index a11383041..1318e7b69 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); @@ -703,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; } @@ -1000,13 +1003,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 +1141,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); } }; @@ -1157,6 +1163,7 @@ void BT::XMLParser::PImpl::recursivelyCreateSubtree(const std::string& tree_ID, 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, 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 //