From 9b939fdaaf9c47133a7b80e9077847e21fedc264 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sat, 24 Oct 2026 14:07:10 +0200 Subject: [PATCH] XMLParser: detect cyclic instead of overflowing the stack A tree file that includes itself, directly or through other files, made loadDocImpl() recurse until the process crashed with a stack overflow (SIGSEGV), because the include loop never checked whether a file was already being loaded. Track the normalized paths of the files in the active include chain and throw a RuntimeError naming the repeated file. The chain is a stack (pushed before, popped after each include), so a file included twice from different branches is not reported as a cycle; it is reset by the top-level loadFromFile()/loadFromText(), so a failed load leaves no stale state in the factory's parser. Co-Authored-By: Claude Opus 5.5 --- src/xml_parsing.cpp | 26 +++++++++++++++++++ tests/gtest_factory.cpp | 18 +++++++++++++ .../child/child_include_parent_cycle.xml | 7 +++++ tests/trees/parent_include_child_cycle.xml | 7 +++++ tests/trees/self_include.xml | 7 +++++ 5 files changed, 65 insertions(+) create mode 100644 tests/trees/child/child_include_parent_cycle.xml create mode 100644 tests/trees/parent_include_child_cycle.xml create mode 100644 tests/trees/self_include.xml diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index c740dd951..4c9a57b1c 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -12,6 +12,7 @@ #include "behaviortree_cpp/basic_types.h" +#include #include #include #include @@ -103,6 +104,15 @@ auto StrEqual = [](const char* str1, const char* str2) -> bool { return strcmp(str1, str2) == 0; }; +// Normalized form of an included file path, so that different spellings of +// the same file ("a.xml", "./a.xml", "dir/../a.xml") compare equal. +std::filesystem::path IncludeKey(const std::filesystem::path& absolute_path) +{ + std::error_code err; + auto canonical = std::filesystem::weakly_canonical(absolute_path, err); + return err ? absolute_path.lexically_normal() : canonical; +} + void validateModelName(const std::string& name, int line_number) { const auto line_str = std::to_string(line_number); @@ -242,6 +252,8 @@ struct XMLParser::PImpl const BehaviorTreeFactory* factory = nullptr; std::filesystem::path current_path; + // files currently being loaded through , used to detect cycles + std::vector include_stack; std::map subtree_models; int suffix_count = 0; @@ -289,6 +301,7 @@ void XMLParser::loadFromFile(const std::filesystem::path& filepath, bool add_inc doc->LoadFile(filepath.string().c_str()); _p->current_path = std::filesystem::absolute(filepath.parent_path()); + _p->include_stack = { IncludeKey(std::filesystem::absolute(filepath)) }; _p->loadDocImpl(doc, add_includes); } @@ -300,6 +313,7 @@ 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->include_stack.clear(); _p->loadDocImpl(doc, add_includes); } @@ -414,6 +428,16 @@ 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 recurse until the stack overflows. + auto include_key = IncludeKey(file_path); + if(std::find(include_stack.begin(), include_stack.end(), include_key) != + include_stack.end()) + { + throw RuntimeError("Cyclic detected: the file [", include_key.string(), + "] includes itself, directly or through other files"); + } + opened_documents.push_back(std::make_unique()); XMLDocument* next_doc = opened_documents.back().get(); @@ -421,8 +445,10 @@ void XMLParser::PImpl::loadDocImpl(XMLDocument* doc, bool add_includes) const auto previous_path = current_path; current_path = std::filesystem::absolute(file_path.parent_path()); + include_stack.push_back(std::move(include_key)); next_doc->LoadFile(file_path.string().c_str()); loadDocImpl(next_doc, add_includes); + include_stack.pop_back(); // reset current path to the previous value current_path = previous_path; diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 2c257b5b2..8056a2ba7 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; + std::string path = FilePath("trees/self_include.xml"); + ASSERT_THROW(factory.createTreeFromFile(path), RuntimeError); +} + +TEST(BehaviorTreeFactory, CreateTreeFromFileWithCyclicIncludes) +{ + BehaviorTreeFactory factory; + std::string path = FilePath("trees/parent_include_child_cycle.xml"); + ASSERT_THROW(factory.createTreeFromFile(path), RuntimeError); + + // a failed load must not leave stale state behind in the factory's parser + Tree tree = factory.createTreeFromFile(FilePath("trees/parent_include_child.xml")); + ASSERT_EQ(NodeStatus::SUCCESS, tree.tickWhileRunning()); +} + TEST(BehaviorTreeFactory, WrongTreeName) { const char* xmlA = R"( diff --git a/tests/trees/child/child_include_parent_cycle.xml b/tests/trees/child/child_include_parent_cycle.xml new file mode 100644 index 000000000..d863f832e --- /dev/null +++ b/tests/trees/child/child_include_parent_cycle.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/tests/trees/parent_include_child_cycle.xml b/tests/trees/parent_include_child_cycle.xml new file mode 100644 index 000000000..648dcaf7b --- /dev/null +++ b/tests/trees/parent_include_child_cycle.xml @@ -0,0 +1,7 @@ + + + + + + + diff --git a/tests/trees/self_include.xml b/tests/trees/self_include.xml new file mode 100644 index 000000000..22b8d07c2 --- /dev/null +++ b/tests/trees/self_include.xml @@ -0,0 +1,7 @@ + + + + + + +