Skip to content

Conversation

@facontidavide
Copy link
Collaborator

@facontidavide facontidavide commented Feb 1, 2026

Summary

  • Adds Tree::remapManifestPointers() to re-point manifest pointers from the factory's internal storage to the tree's own manifests map after createTree()
  • Called automatically at the end of all three createTree variants (createTreeFromText, createTreeFromFile, createTree)
  • Prevents dangling pointer access when BehaviorTreeFactory is destroyed before Tree::tickWhileRunning() is called

Fixes #1046

Test plan

  • Added BehaviorTreeFactory.FactoryDestroyedBeforeTick test that reproduces the use-after-free scenario
  • All 309 tests pass (308 existing + 1 new)

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • Bug Fixes
    • Fixed a critical issue where behavior tree nodes could crash when accessed after the factory that created them was destroyed. The tree now maintains its own copy of manifest references, ensuring they remain valid throughout the tree's lifetime.

✏️ Tip: You can customize this high-level summary in your review settings.

…ee 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>
@coderabbitai
Copy link

coderabbitai bot commented Feb 1, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

This pull request fixes a heap-use-after-free bug (issue #1046) where BehaviorTreeFactory destruction invalidates manifest pointers held by tree nodes. The fix remaps manifest pointers to tree-owned copies after factory construction, enabling trees to survive factory destruction before ticking.

Changes

Cohort / File(s) Summary
Header Interface
include/behaviortree_cpp/bt_factory.h
Added private remapManifestPointers() method declaration and friend class declaration to enable BehaviorTreeFactory to invoke the remapping routine.
Factory Implementation
src/bt_factory.cpp
Implemented remapManifestPointers() to iterate over tree nodes and redirect manifest pointers to tree-owned manifests; integrated remapping into three factory methods (createTreeFromText, createTreeFromFile, createTree) immediately after manifest assignment.
Node Port Access
src/tree_node.cpp
Refined null-check in getLockedPortContent() for explicit manifest pointer comparison.
Test Coverage
tests/gtest_factory.cpp
Added ActionIssue1046 test node and FactoryDestroyedBeforeTick test case that validates manifest pointers remain valid after factory destruction and tree execution succeeds without crash.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A factory once left manifests behind,
Dangling pointers haunting nodes' minds.
Now trees keep copies, safe and sound,
No use-after-free bugs are found! 🌳✨

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/1046-use-after-free

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Feb 1, 2026

Codecov Report

❌ Patch coverage is 96.87500% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 65.98%. Comparing base (e0684b0) to head (b228046).
⚠️ Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
tests/gtest_factory.cpp 95.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1081      +/-   ##
==========================================
+ Coverage   65.90%   65.98%   +0.07%     
==========================================
  Files         225      225              
  Lines       12727    12759      +32     
  Branches     1186     1193       +7     
==========================================
+ Hits         8388     8419      +31     
  Misses       4289     4289              
- Partials       50       51       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Use explicit != nullptr comparisons for pointer-to-bool conversions
flagged by clang-tidy.

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@facontidavide facontidavide marked this pull request as ready for review February 1, 2026 19:42
@facontidavide facontidavide merged commit d53a8ea into master Feb 1, 2026
14 of 15 checks passed
@facontidavide facontidavide deleted the fix/1046-use-after-free branch February 1, 2026 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Heap use after free when BehaviorTreeFactory is destroyed before ticking

2 participants