From b3b0217d73a5aa5ae0b48875975ac63bd2007bef Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 1 Feb 2026 20:16:30 +0100 Subject: [PATCH 1/2] Fix #1046: prevent use-after-free when factory is destroyed before tree tick After createTree(), node manifest pointers pointed into the factory's internal map. If the factory was destroyed before ticking, these became dangling pointers causing heap-use-after-free on manifest lookups. Add Tree::remapManifestPointers() which re-points each node's NodeConfig::manifest from the factory's map to the tree's own copy, called in all three createTree* methods after copying manifests. Co-Authored-By: Claude Opus 4.5 --- include/behaviortree_cpp/bt_factory.h | 7 +++ src/bt_factory.cpp | 22 ++++++++ tests/gtest_factory.cpp | 81 +++++++++++++++++++++++++++ 3 files changed, 110 insertions(+) diff --git a/include/behaviortree_cpp/bt_factory.h b/include/behaviortree_cpp/bt_factory.h index 3471f51bf..f8aacc85e 100644 --- a/include/behaviortree_cpp/bt_factory.h +++ b/include/behaviortree_cpp/bt_factory.h @@ -192,6 +192,8 @@ class Tree } private: + friend class BehaviorTreeFactory; + std::shared_ptr wake_up_; enum TickOption @@ -203,6 +205,11 @@ class Tree NodeStatus tickRoot(TickOption opt, std::chrono::milliseconds sleep_time); + // Fix #1046: re-point each node's NodeConfig::manifest from the + // factory's map to the tree's own copy so the pointers remain + // valid after the factory is destroyed. + void remapManifestPointers(); + uint16_t uid_counter_ = 0; }; diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index a0db78827..a06ab34d1 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -350,6 +350,7 @@ Tree BehaviorTreeFactory::createTreeFromText(const std::string& text, parser.loadFromText(text); auto tree = parser.instantiateTree(blackboard); tree.manifests = this->manifests(); + tree.remapManifestPointers(); return tree; } @@ -369,6 +370,7 @@ Tree BehaviorTreeFactory::createTreeFromFile(const std::filesystem::path& file_p parser.loadFromFile(file_path); auto tree = parser.instantiateTree(blackboard); tree.manifests = this->manifests(); + tree.remapManifestPointers(); return tree; } @@ -377,6 +379,7 @@ Tree BehaviorTreeFactory::createTree(const std::string& tree_name, { auto tree = _p->parser->instantiateTree(blackboard, tree_name); tree.manifests = this->manifests(); + tree.remapManifestPointers(); return tree; } @@ -476,6 +479,25 @@ BehaviorTreeFactory::substitutionRules() const Tree::Tree() = default; +void Tree::remapManifestPointers() +{ + for(auto& subtree : subtrees) + { + for(auto& node : subtree->nodes) + { + const auto* old_manifest = node->config().manifest; + if(old_manifest) + { + auto it = manifests.find(old_manifest->registration_ID); + if(it != manifests.end()) + { + node->config().manifest = &(it->second); + } + } + } + } +} + void Tree::initialize() { wake_up_ = std::make_shared(); diff --git a/tests/gtest_factory.cpp b/tests/gtest_factory.cpp index 13455bda5..48f6eeef1 100644 --- a/tests/gtest_factory.cpp +++ b/tests/gtest_factory.cpp @@ -460,3 +460,84 @@ TEST(BehaviorTreeFactory, addMetadataToManifest) const auto& modified_manifest = factory.manifests().at("SaySomething"); EXPECT_EQ(modified_manifest.metadata, makeTestMetadata()); } + +// Action node used to reproduce issue #1046 (use-after-free on +// manifest pointer). It calls getInput() for a port name that is +// NOT in the XML, so getInputStamped falls through to the +// manifest lookup. +class ActionIssue1046 : public BT::SyncActionNode +{ +public: + ActionIssue1046(const std::string& name, const BT::NodeConfig& config) + : BT::SyncActionNode(name, config) + {} + + BT::NodeStatus tick() override + { + // "value" is declared in providedPorts() but NOT set in the + // XML, so getInput() will look it up in the manifest. + // If the manifest pointer is dangling, this is a + // use-after-free. + int value = 0; + auto res = getInput("value", value); + // We expect this to fail (no default, not in XML), but it + // must not crash. + (void)res; + return BT::NodeStatus::SUCCESS; + } + + static BT::PortsList providedPorts() + { + return { BT::InputPort("value", "a test port") }; + } +}; + +// Test for issue #1046: heap use-after-free when +// BehaviorTreeFactory is destroyed before the tree is ticked. +TEST(BehaviorTreeFactory, FactoryDestroyedBeforeTick) +{ + // clang-format off + // The XML deliberately does NOT set the "value" port so + // that getInput falls through to the manifest to read + // the port info. This triggers the dangling-pointer bug. + static const char* xml_text_issue_1046 = R"( + + + + + )"; + // clang-format on + + BT::Tree tree; + { + // Factory created in an inner scope + BT::BehaviorTreeFactory factory; + factory.registerNodeType("ActionIssue1046"); + factory.registerBehaviorTreeFromText(xml_text_issue_1046); + tree = factory.createTree("Main"); + // factory is destroyed here + } + + // Verify that every node's manifest pointer points into the + // tree's own copy, not into freed factory memory. + for(const auto& subtree : tree.subtrees) + { + for(const auto& node : subtree->nodes) + { + const auto* manifest_ptr = + static_cast(node.get())->config().manifest; + if(manifest_ptr) + { + auto it = tree.manifests.find(manifest_ptr->registration_ID); + ASSERT_NE(it, tree.manifests.end()); + EXPECT_EQ(manifest_ptr, &(it->second)); + } + } + } + + // Ticking after factory destruction should not crash. + // Before the fix, NodeConfig::manifest pointed to a manifest + // owned by the now-destroyed factory, causing use-after-free + // when getInput() looked up the port in the manifest. + EXPECT_EQ(BT::NodeStatus::SUCCESS, tree.tickWhileRunning()); +} From b2280464568587a8d16f0b79c27e03a944c0c73d Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Sun, 1 Feb 2026 20:29:14 +0100 Subject: [PATCH 2/2] Fix clang-tidy readability-implicit-bool-conversion warnings Use explicit != nullptr comparisons for pointer-to-bool conversions flagged by clang-tidy. Co-Authored-By: Claude Opus 4.5 --- src/bt_factory.cpp | 2 +- src/tree_node.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bt_factory.cpp b/src/bt_factory.cpp index a06ab34d1..6638a1d3f 100644 --- a/src/bt_factory.cpp +++ b/src/bt_factory.cpp @@ -486,7 +486,7 @@ void Tree::remapManifestPointers() for(auto& node : subtree->nodes) { const auto* old_manifest = node->config().manifest; - if(old_manifest) + if(old_manifest != nullptr) { auto it = manifests.find(old_manifest->registration_ID); if(it != manifests.end()) diff --git a/src/tree_node.cpp b/src/tree_node.cpp index d464d6af9..16171a135 100644 --- a/src/tree_node.cpp +++ b/src/tree_node.cpp @@ -481,7 +481,7 @@ AnyPtrLocked BT::TreeNode::getLockedPortContent(const std::string& key) { const auto bb_key = std::string(*remapped_key); auto result = _p->config.blackboard->getAnyLocked(bb_key); - if(!result && _p->config.manifest) + if(!result && _p->config.manifest != nullptr) { // Entry doesn't exist yet. Create it using the port's type info // from the manifest so that getLockedPortContent works even when