From 7e1ca1646afcca242f0784c4c05349785c102f33 Mon Sep 17 00:00:00 2001 From: Chaosheng Qu Date: Wed, 11 Nov 2026 16:31:10 +0800 Subject: [PATCH 1/2] Roll back a rejected XML load as a whole, not per document #1266 dropped a document that failed to load, but the rollback stopped at the document boundary. If main.xml includes sub.xml and then fails, sub.xml has already registered its trees and they stay behind, and sub.xml becomes the first document, so instantiateTree() reads main_tree_to_execute from it. Take a snapshot before the call and restore it on failure. That also puts back a tree this call redefined, whose element lives in the document about to be erased. Fixes #1282 --- src/xml_parsing.cpp | 12 ++ tests/CMakeLists.txt | 2 + tests/gtest_xml_include_rollback.cpp | 199 +++++++++++++++++++++++++++ 3 files changed, 213 insertions(+) create mode 100644 tests/gtest_xml_include_rollback.cpp diff --git a/src/xml_parsing.cpp b/src/xml_parsing.cpp index 165dacab6..55567fa4c 100644 --- a/src/xml_parsing.cpp +++ b/src/xml_parsing.cpp @@ -253,15 +253,27 @@ struct XMLParser::PImpl // it would otherwise remain the "first opened document" that // instantiateTree() reads main_tree_to_execute from, and a document that // did not parse has no root element at all. + // + // Rollback is per load call, not per document: makes one call + // register trees from several documents, and if the root document is + // rejected the included ones have already registered theirs. Restore the + // state from before the call, which also puts back a tree that this call + // redefined - its element lives in the document about to be erased. void loadLastDocImpl(bool add_includes) { const auto doc_it = std::prev(opened_documents.end()); + const auto roots_before = tree_roots; + const auto sources_before = tree_sources; + const int suffix_before = suffix_count; try { loadDocImpl(doc_it->get(), add_includes); } catch(...) { + tree_roots = roots_before; + tree_sources = sources_before; + suffix_count = suffix_before; opened_documents.erase(doc_it); throw; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 5d4e643c9..31c9aa9d0 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -61,6 +61,8 @@ set(BT_TESTS gtest_plugin_issue953.cpp gtest_blackboard_thread_safety.cpp gtest_xml_null_subtree_id.cpp + gtest_xml_include_rollback.cpp + gtest_tree_destructor.cpp script_parser_test.cpp test_helper.hpp diff --git a/tests/gtest_xml_include_rollback.cpp b/tests/gtest_xml_include_rollback.cpp new file mode 100644 index 000000000..273967261 --- /dev/null +++ b/tests/gtest_xml_include_rollback.cpp @@ -0,0 +1,199 @@ +/* Copyright (C) 2024 Davide Faconti - All Rights Reserved + * + * Test for the rollback in XMLParser::loadFromFile(): a load call that is + * rejected must not leave the trees registered by the documents it pulled in + * through . + */ + +#include "behaviortree_cpp/bt_factory.h" +#include "behaviortree_cpp/xml_parsing.h" + +#include + +#include +#include + +using namespace BT; + +namespace +{ +class TempDir +{ +public: + TempDir() + { + _path = std::filesystem::temp_directory_path() / + ("bt_xmlparser_" + std::to_string(::getpid()) + "_" + + std::to_string(reinterpret_cast(this))); + std::filesystem::create_directories(_path); + } + + ~TempDir() + { + std::error_code ec; + std::filesystem::remove_all(_path, ec); + } + + std::filesystem::path write(const std::string& name, const std::string& content) const + { + const auto path = _path / name; + std::ofstream out(path); + out << content; + return path; + } + + const std::filesystem::path& path() const + { + return _path; + } + +private: + std::filesystem::path _path; +}; + +bool contains(const std::vector& names, const std::string& name) +{ + return std::find(names.begin(), names.end(), name) != names.end(); +} +} // namespace + +// The included document is valid and registers a tree; the including document +// is rejected afterwards. Before the fix only the rejected document was +// dropped, so the tree from the include stayed registered even though the +// load call as a whole failed. +TEST(XMLParserTest, RejectedIncludeKeepsNoTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + const auto main_xml = dir.write("main.xml", R"( + + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + + EXPECT_THROW(parser.loadFromFile(main_xml), std::exception); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_FALSE(contains(registered, "FromInclude")) + << "a rejected load call must not leave the included tree registered"; + EXPECT_FALSE(contains(registered, "MainTree")); +} + +// The same for loadFromText(), which shares the rollback path. +TEST(XMLParserTest, RejectedInlineIncludesKeepNoTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + + const std::string text = R"( + + + + + )"; + + EXPECT_THROW(parser.loadFromText(text), std::exception); + EXPECT_FALSE(contains(parser.registeredBehaviorTrees(), "FromInclude")); +} + +// A load call that succeeds must still register everything, includes included. +TEST(XMLParserTest, AcceptedIncludeKeepsItsTree) +{ + TempDir dir; + dir.write("sub.xml", R"( + + + + )"); + + const auto main_xml = dir.write("main.xml", R"( + + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + parser.loadFromFile(main_xml); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_TRUE(contains(registered, "FromInclude")); + EXPECT_TRUE(contains(registered, "MainTree")); +} + +// A rejected load that redefined a tree must put the previous definition back: +// the new element lives in the document that is about to be erased, so leaving +// it in place would leave tree_roots pointing into freed memory. +TEST(XMLParserTest, RejectedRedefinitionRestoresPreviousTree) +{ + TempDir dir; + const auto good = dir.write("good.xml", R"( + + + + )"); + + const auto bad = dir.write("bad.xml", R"( + + + + )"); + + // Use the factory's own parser: createTree() reads that one's tree_roots, + // so this exercises the same state a user would hit. + BehaviorTreeFactory factory; + factory.registerBehaviorTreeFromFile(good); + + EXPECT_THROW(factory.registerBehaviorTreeFromFile(bad), std::exception); + + // The first definition must still be usable, not just present by name. + EXPECT_NO_THROW({ + auto tree = factory.createTree("Shared"); + EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS); + }); +} + +// Trees registered by an earlier, successful load must survive a later +// rejected one. +TEST(XMLParserTest, RollbackKeepsEarlierTrees) +{ + TempDir dir; + const auto good = dir.write("good.xml", R"( + + + + )"); + + const auto bad = dir.write("bad.xml", R"( + + + + )"); + + BehaviorTreeFactory factory; + XMLParser parser(factory); + parser.loadFromFile(good); + + EXPECT_THROW(parser.loadFromFile(bad), std::exception); + + const auto registered = parser.registeredBehaviorTrees(); + EXPECT_TRUE(contains(registered, "Good")); + EXPECT_FALSE(contains(registered, "Bad")); +} From 989feb8735071c918fe2a794e77322c648dbd2ab Mon Sep 17 00:00:00 2001 From: Chaosheng Qu Date: Wed, 11 Nov 2026 16:31:20 +0800 Subject: [PATCH 2/2] Don't let an exception escape Tree's destructor Tree::~Tree() calls haltTree(), and halting may throw: a TryCatch with catch_on_halt="true" ticks its catch child, and a tick is allowed to throw. An exception leaving a destructor calls std::terminate, so destroying such a tree aborted the process. Mark the destructor noexcept and swallow the exception. There is no caller left to report to. Fixes #1281 --- include/behaviortree_cpp/bt_factory.h | 2 +- src/bt_factory.cpp | 14 ++- tests/gtest_tree_destructor.cpp | 118 ++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 3 deletions(-) create mode 100644 tests/gtest_tree_destructor.cpp diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 9843f40ee..b32f1323d 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -154,7 +154,7 @@ class Tree */ [[nodiscard]] std::shared_ptr wakeUpSignal() const; - ~Tree(); + ~Tree() noexcept; /// Tick the root of the tree once, even if a node invoked /// emitWakeUpSignal() diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index e3e376c50..2f0166d9b 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -718,9 +718,19 @@ std::shared_ptr Tree::wakeUpSignal() const return wake_up_; } -Tree::~Tree() +Tree::~Tree() noexcept { - haltTree(); + // A destructor that lets an exception escape calls std::terminate, and a + // node is allowed to throw while halting - TryCatch with catch_on_halt + // ticks its catch child, and ticking may throw. Swallow it: there is no + // caller left to report to, and the tree is being destroyed anyway. + try + { + haltTree(); + } + catch(...) + { + } } NodeStatus Tree::tickExactlyOnce() diff --git a/tests/gtest_tree_destructor.cpp b/tests/gtest_tree_destructor.cpp new file mode 100644 index 000000000..d8712360d --- /dev/null +++ b/tests/gtest_tree_destructor.cpp @@ -0,0 +1,118 @@ +/* Copyright (C) 2024 Davide Faconti - All Rights Reserved + * + * Tree::~Tree() halts the tree. Halting is allowed to throw (a TryCatch with + * catch_on_halt ticks its catch child, and a tick may throw), and a destructor + * that lets an exception escape calls std::terminate. Destroying a tree whose + * halt throws must not abort the process. + */ + +#include "behaviortree_cpp/bt_factory.h" + +#include + +using namespace BT; + +namespace +{ +// Stays RUNNING, so TryCatch is still in its try block when the tree is +// destroyed - that is the condition for catch_on_halt to tick the catch child. +class RunningAction : public StatefulActionNode +{ +public: + RunningAction(const std::string& name, const NodeConfig& config) + : StatefulActionNode(name, config) + {} + + static PortsList providedPorts() + { + return {}; + } + + NodeStatus onStart() override + { + return NodeStatus::RUNNING; + } + NodeStatus onRunning() override + { + return NodeStatus::RUNNING; + } + void onHalted() override + {} +}; + +// Throws on every tick, including the one a TryCatch does while halting. +// Stateful, because a SyncActionNode that returns RUNNING is itself an error. +class ThrowingAction : public StatefulActionNode +{ +public: + ThrowingAction(const std::string& name, const NodeConfig& config) + : StatefulActionNode(name, config) + {} + + static PortsList providedPorts() + { + return {}; + } + + NodeStatus onStart() override + { + throw std::runtime_error("thrown from tick"); + } + NodeStatus onRunning() override + { + throw std::runtime_error("thrown from tick"); + } + void onHalted() override + {} +}; + +} // namespace + +// The tree reaches RUNNING, so TryCatch takes the catch_on_halt path when the +// destructor halts it; the catch child throws and the exception must not +// escape the destructor. +TEST(TreeDestructorTest, HaltThatThrowsDoesNotTerminate) +{ + BehaviorTreeFactory factory; + factory.registerNodeType("RunningAction"); + factory.registerNodeType("ThrowingAction"); + + const std::string xml_text = R"( + + + + + + + + + )"; + + // Reaching this line is the assertion: before the fix the destructor's + // exception escaped and the process called std::terminate. + EXPECT_NO_THROW({ + auto tree = factory.createTreeFromText(xml_text); + tree.tickOnce(); + }); +} + +// A plain tree without a throwing node must still be destroyed normally, and +// halting must still happen - the fix must not skip it. +TEST(TreeDestructorTest, HaltStillRuns) +{ + BehaviorTreeFactory factory; + + const std::string xml_text = R"( + + + + + + )"; + + auto tree = factory.createTreeFromText(xml_text); + EXPECT_EQ(tree.tickOnce(), NodeStatus::SUCCESS); + // Scoped, so the destructor runs here and the test can observe it. + EXPECT_NO_THROW({ auto scoped = factory.createTreeFromText(xml_text); + scoped.tickOnce(); }); +}