Fix #1046: use-after-free when factory destroyed before tree (#1081)

* 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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>

Author: facontidavide

include/behaviortree_cpp/bt_factory.h

@@ -192,6 +192,8 @@ class Tree
   }
 
 private:
+  friend class BehaviorTreeFactory;
+
   std::shared_ptr<WakeUpSignal> 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;
 };
 

src/bt_factory.cpp

@@ -354,6 +354,7 @@ Tree BehaviorTreeFactory::createTreeFromText(const std::string& text,
   parser.loadFromText(text);
   auto tree = parser.instantiateTree(blackboard);
   tree.manifests = this->manifests();
+  tree.remapManifestPointers();
   return tree;
 }
 
@@ -373,6 +374,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;
 }
 
@@ -381,6 +383,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;
 }
 
@@ -480,6 +483,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 != nullptr)
+      {
+        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<WakeUpSignal>();

tests/gtest_factory.cpp

@@ -482,3 +482,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<int>("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"(
+  <root BTCPP_format="4" >
+    <BehaviorTree ID="Main">
+      <ActionIssue1046/>
+    </BehaviorTree>
+  </root> )";
+  // clang-format on
+
+  BT::Tree tree;
+  {
+    // Factory created in an inner scope
+    BT::BehaviorTreeFactory factory;
+    factory.registerNodeType<ActionIssue1046>("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<const BT::TreeNode*>(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());
+}