Fix heap-use-after-free race condition in dwb_plugins::KinematicsHandler #5707

suifengersan123 · 2025-11-21T01:49:35Z

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	(add tickets here #5706)
Primary OS tested on	ubuntu 20.04
Robotic platform tested on	N/A (detected via Asan)
Does this PR contain AI generated software?	No
Was this PR description generated by AI software?	No

Description of contribution in a few bullet points

Fixed a critical heap-use-after-free race condition in dwb_plugins::KinematicsHandler
Replaced raw pointer with std::shared_ptr for thread-safe memory management
Used std::atomic_load and std::atomic_store for atomic shared_ptr operations
Eliminated manual memory management (delete/new) to prevent dangling pointer dereferences
Bug occurred when parameter updates (Thread T0) and control loop (Thread T17) accessed kinematics simultaneously

Technical details:

Changed std::atomic<KinematicParameters*> to std::shared_ptr
Updated getKinematics(), update_kinematics(), constructor, and destructor
Reference counting ensures memory is only freed when all threads are done using it*

Description of documentation updates required from your changes

No user-facing documentation changes required (internal implementation fix)
Code comments added to clarify thread-safety mechanism
No parameter changes or API modifications

Description of how this change was tested

AddressSanitizer validation: Compiled with -fsanitize=address and verified no heap-use-after-free errors occur

For Maintainers:

Check that any new parameters added are updated in docs.nav2.org
Check that any significant change is added to the migration guide
Check that any new features OR changes to existing behaviors are reflected in the tuning guide
Check that any new functions have Doxygen added
Check that any new features have test coverage
Check that any new plugins is added to the plugins page
If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists
Should this be backported to current distributions? If so, tag with backport-*.

mergify · 2025-11-21T01:50:10Z

@suifengersan123, all pull requests must be targeted towards the main development branch.
Once merged into main, it is possible to backport to @foxy-devel, but it must be in main
to have these changes reflected into new distributions.

SteveMacenski · 2025-11-22T19:33:58Z

Can you fix this without modifying the use of atomics to the shared pointer? I don't like this solution since I imagine the atomics were designed that way for a reason. How about an atomic to a shared pointer and/or setting the pointer value to nullptr so that where the free after use is occurring can check if the pointer is nullptr before use.

Also, is this an issue still in the main branch? If not, then perhaps this has been resolved already and we should be using that solution in Foxy. If its not solved, then we should also make sure to solve this in main so that all future distributions you move to have this fix as well.

suifengersan123 · 2025-11-24T04:35:22Z

I think what you said makes perfect sense, so before using it, I will check if it is nullptr and have removed the shared pointer. And I also noticed that the main branch has the same issue since I saw that their code logic is exactly the same.Below is the new commit.> 445386f

nav2_dwb_controller/dwb_plugins/include/dwb_plugins/kinematic_parameters.hpp

SteveMacenski · 2025-11-25T17:41:26Z

@suifengersan123 two items

You have tested the locally and it fixes the issue in the final form, correct?
Can you open this same PR on main so that we can have all our CI testing run over this change (and so that it can be updated for all new distributions, probably be backported to Jazzy)? Foxy doesn't have nearly as much testing as modern Nav2 so it would give me peace-of-mind as well as future proof the change as you upgrade in the future.

With that, I can merge this + the main version.

suifengersan123 · 2025-11-26T02:38:01Z

Thank you for your patient response. I have tested it in the foxy version and it can solve this problem. Moreover, I have already submitted a PR(#5720) in the main branch.

suifengersan123 · 2025-11-26T08:07:05Z

This commit(> 0211977) is for issue #5721. This bug is not only present in the foxy branch, but the logic related to this part in the main branch is the same as that in the foxy branch. If you have time, could you please take a look and let me know if this modification is acceptable?

suifengersan123 · 2025-11-26T10:28:33Z

This commit(> c41f69b) is for issue #5725. This bug has been fixed in the main branch, but it still exists in the foxy branch. I followed the fix method from the main branch to fix the foxy branch. Could you please take a look?

suifengersan123 · 2025-11-26T11:44:37Z

This commit(> 03773a3) is for issue #5726. I'm very sorry. I didn't find the detailed logic of this part of the code in the main branch, so I don't know if there are similar issues in the main branch. However, I have already committed the fix for the foxy branch.Please check it when you have time.

SteveMacenski · 2025-11-26T19:38:00Z

Please revert all changes other than the ones related to the dwb_plugins. Open a separate PR for each issue you have a Ticket open for and link that ticket to the PR. Please keep things separated for good workflow :-)

SteveMacenski · 2025-11-26T19:38:25Z

nav2_amcl/src/amcl_node.cpp

+    if(resample_interval_ == 0){
+        RCLCPP_WARN(
+          get_logger(), "You've set resample_interval to be zero,"
+          " this isn't allowed so it will be set to default value to 1.");
+        resample_interval_ = 1;
+    }


Definite linting issues in this PR as well

SteveMacenski · 2025-11-26T19:38:29Z

nav2_costmap_2d/src/costmap_layer.cpp

 {
  Costmap2D * master = layered_costmap_->getCostmap();
+  if (!master) {
+      RCLCPP_WARN(


SteveMacenski · 2025-11-26T19:38:33Z

nav2_costmap_2d/src/costmap_2d.cpp

+    }
  }
-  return true;
+    return cells_written;


SteveMacenski · 2025-11-26T19:38:37Z

nav2_costmap_2d/src/costmap_2d.cpp

    unsigned int index = getIndex(polygon_cells[i].x, polygon_cells[i].y);
-    costmap_[index] = cost_value;
+     if (index < size_x_ * size_y_) {
+      costmap_[index] = cost_value;


SteveMacenski · 2025-11-26T19:38:47Z

nav2_dwb_controller/dwb_plugins/include/dwb_plugins/kinematic_parameters.hpp

  void initialize(const nav2_util::LifecycleNode::SharedPtr & nh, const std::string & plugin_name);

-  inline KinematicParameters getKinematics() {return *kinematics_.load();}
+   inline KinematicParameters getKinematics() {


Signed-off-by: suifengersan123 <[email protected]>

…shared pointer Signed-off-by: suifengersan123 <[email protected]>

Signed-off-by: suifengersan123 <[email protected]>

suifengersan123 · 2025-11-28T01:59:50Z

I removed the other changes, fixed the linting errors, and then made the commit. Please check if there are any other issues when you have time.

SteveMacenski linked an issue Nov 22, 2025 that may be closed by this pull request

[nav2_dwb_controller] Race Condition in KinematicsHandler Causing Heap-Use-After-Free #5706

Open

SteveMacenski reviewed Nov 24, 2025

View reviewed changes

nav2_dwb_controller/dwb_plugins/include/dwb_plugins/kinematic_parameters.hpp Outdated Show resolved Hide resolved

SteveMacenski approved these changes Nov 25, 2025

View reviewed changes

suifengersan123 mentioned this pull request Nov 26, 2025

fix bug UAF #5720

Open

8 tasks

SteveMacenski requested changes Nov 26, 2025

View reviewed changes

suifengersan123 force-pushed the foxy-devel branch from 9bf6018 to 53ca1ab Compare November 27, 2025 13:59

随风而散 and others added 9 commits November 28, 2025 09:43

fix bug

3f1e884

Signed-off-by: suifengersan123 <[email protected]>

fix bug by check if the pointer is nullptr before use

99e8073

Signed-off-by: suifengersan123 <[email protected]>

fix bug by check if the pointer is nullptr before use and remove the …

6cbd882

…shared pointer Signed-off-by: suifengersan123 <[email protected]>

throw error

061e931

Signed-off-by: suifengersan123 <[email protected]>

temporary fix bug null pointer

d383ce6

Signed-off-by: suifengersan123 <[email protected]>

fix bug FPE

74d84e7

Signed-off-by: suifengersan123 <[email protected]>

fix bug heap-buffer-overflow

70cb484

Signed-off-by: suifengersan123 <[email protected]>

removed all other changes

bc8801c

Signed-off-by: suifengersan123 <[email protected]>

fix linting problems

6cf29ac

Signed-off-by: suifengersan123 <[email protected]>

suifengersan123 force-pushed the foxy-devel branch from 6dac7d7 to 6cf29ac Compare November 28, 2025 01:43

Fix heap-use-after-free race condition in dwb_plugins::KinematicsHandler #5707

Are you sure you want to change the base?

Fix heap-use-after-free race condition in dwb_plugins::KinematicsHandler #5707

Conversation

suifengersan123 commented Nov 21, 2025

Basic Info

Description of contribution in a few bullet points

Technical details:

Description of documentation updates required from your changes

Description of how this change was tested

AddressSanitizer validation: Compiled with -fsanitize=address and verified no heap-use-after-free errors occur

For Maintainers:

Uh oh!

mergify bot commented Nov 21, 2025

Uh oh!

SteveMacenski commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suifengersan123 commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

SteveMacenski commented Nov 25, 2025

Uh oh!

suifengersan123 commented Nov 26, 2025

Uh oh!

suifengersan123 commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

suifengersan123 commented Nov 26, 2025

Uh oh!

suifengersan123 commented Nov 26, 2025

Uh oh!

SteveMacenski commented Nov 26, 2025

Uh oh!

SteveMacenski Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

SteveMacenski Nov 26, 2025

Choose a reason for hiding this comment

Uh oh!

suifengersan123 commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

SteveMacenski commented Nov 22, 2025 •

edited

Loading

suifengersan123 commented Nov 24, 2025 •

edited

Loading

suifengersan123 commented Nov 26, 2025 •

edited

Loading