diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index c740dd951..06cff76f4 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -234,7 +234,8 @@ struct XMLParser::PImpl void getPortsRecursively(const XMLElement* element, std::vector& output_ports); - void loadDocImpl(XMLDocument* doc, bool add_includes); + void loadDocImpl(XMLDocument* doc, bool add_includes, + std::unordered_set& ancestors); std::list > opened_documents; std::map tree_roots; @@ -290,7 +291,10 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc _p->current_path = std::filesystem::absolute(filepath.parent_path()); - _p->loadDocImpl(doc, add_includes); + std::unordered_set ancestors = { + std::filesystem::weakly_canonical(filepath).string() + }; + _p->loadDocImpl(doc, add_includes, ancestors); } void XMLParser::loadFromText(const std::string& xml_text, bool add_includes) @@ -300,7 +304,8 @@ void XMLParser::loadFromText(const std::string& xml_text, bool add_includes) XMLDocument* doc = _p->opened_documents.back().get(); doc->Parse(xml_text.c_str(), xml_text.size()); - _p->loadDocImpl(doc, add_includes); + std::unordered_set ancestors; + _p->loadDocImpl(doc, add_includes, ancestors); } std::vector XMLParser::registeredBehaviorTrees() const @@ -333,7 +338,8 @@ void BT::XMLParser::PImpl::loadSubtreeModel(const XMLElement* xml_root) } } -void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) +void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes, + std::unordered_set& ancestors) { if(doc->Error()) { @@ -414,6 +420,15 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) file_path = current_path / file_path; } + // A file that includes itself, directly or through other files, would + // otherwise be loaded again and again until the stack overflows. + const std::string include_key = std::filesystem::weakly_canonical(file_path).string(); + if(!ancestors.insert(include_key).second) + { + throw RuntimeError("Recursive detected: file '", file_path.string(), + "' includes itself (directly or indirectly)"); + } + opened_documents.push_back(std::make_unique()); XMLDocument* next_doc = opened_documents.back().get(); @@ -422,10 +437,11 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) current_path = std::filesystem::absolute(file_path.parent_path()); next_doc->LoadFile(file_path.string().c_str()); - loadDocImpl(next_doc, add_includes); + loadDocImpl(next_doc, add_includes, ancestors); // reset current path to the previous value current_path = previous_path; + ancestors.erase(include_key); } // Collect the names of all nodes registered with the behavior tree factory diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 2c257b5b2..18fc09114 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -361,6 +361,24 @@ TEST( ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning()); } +TEST(BehaviorTreeFactory, CreateTreeFromFileWhichIncludesItself) +{ + BehaviorTreeFactory factory; + + // must throw instead of recursing until the stack overflows + std::string path = FilePath("trees/include_self.xml"); + EXPECT_THROW(auto tree = factory.createTreeFromFile(path), RuntimeError); +} + +TEST(BehaviorTreeFactory, CreateTreeFromFileWithIncludeCycle) +{ + BehaviorTreeFactory factory; + + // include_cycle_a.xml includes child/include_cycle_b.xml, which includes it back + std::string path = FilePath("trees/include_cycle_a.xml"); + EXPECT_THROW(auto tree = factory.createTreeFromFile(path), RuntimeError); +} + TEST(BehaviorTreeFactory, WrongTreeName) { const char* xmlA = R"( diff --git a/tests/trees/child/include_cycle_b.xml b/tests/trees/child/include_cycle_b.xml new file mode 100644 index 000000000..5f9063cfc --- /dev/null +++ b/tests/trees/child/include_cycle_b.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/tests/trees/include_cycle_a.xml b/tests/trees/include_cycle_a.xml new file mode 100644 index 000000000..cda774a65 --- /dev/null +++ b/tests/trees/include_cycle_a.xml @@ -0,0 +1,10 @@ + + + + + + + + + + diff --git a/tests/trees/include_self.xml b/tests/trees/include_self.xml new file mode 100644 index 000000000..9a27d58a8 --- /dev/null +++ b/tests/trees/include_self.xml @@ -0,0 +1,7 @@ + + + + + + +