Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
facontidavide merged 2 commits into from
Feb 1, 2026
+111 −1
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b3b0217
Fix #1046: prevent use-after-free when factory is destroyed before tr…
facontidavide Feb 1, 2026
b228046
Fix clang-tidy readability-implicit-bool-conversion warnings
facontidavide Feb 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions include/behaviortree_cpp/bt_factory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,8 @@ class Tree
}

private:
friend class BehaviorTreeFactory;

std::shared_ptr<WakeUpSignal> wake_up_;

enum TickOption
Expand All @@ -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;
};

Expand Down
22 changes: 22 additions & 0 deletions src/bt_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}

Expand All @@ -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;
}

Expand Down Expand Up @@ -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 != 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>();
Expand Down
2 changes: 1 addition & 1 deletion src/tree_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
81 changes: 81 additions & 0 deletions tests/gtest_factory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<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());
}
Loading