the mechanism defect of bt_action_server ->on_cleanup() may lead to UAF

[GoesM]

Bug report

Required Info:

• Operating System:
  • Ubuntu22.04
• ROS2 Version:
  • humble
• Version or commit hash:
  • the latest
• DDS implementation:
  • defaulted

Steps to reproduce issue

I encounter this UAF-bug many times when using the Nav2Goal feature

Launch the navigation2 normally, as following steps:

#!/bin/bash
export ASAN_OPTIONS=halt_on_error=0:new_delete_type_mismatch=0:detect_leaks=0:log_pah=asan
source install/setup.bash
export TURTLEBOT3_MODEL=waffle
export GAZEBO_MODEL_PATH=$GAZEBO_MODEL_PATH:/opt/ros/humble/share/turtlebot3_gazebo/models
ros2 launch nav2_bringup tb3_simulation_launch.py headless:=True use_rviz:=False use_composition:=False 

Keep sending Nav2Goal in rviz2-displayer or sending goal by command ros2 action send_goal or send msm into topic /goal_pose , in which the goal_pose is random.

Finally sent Ctrl+C to shutdown navigation2, which is before stop the action-sending and even before the latest action-goal finished.

An ASAN report file was discovered in my execution environment.

Expected behavior

Actual behavior

The ASAN reporting a use-after-free bug to me, as following:

=================================================================
==151301==ERROR: AddressSanitizer: heap-use-after-free on address 0x6030006bf260 at pc 0x563e36ee1206 bp 0x7fb86af78520 sp 0x7fb86af77cd0
READ of size 16 at 0x6030006bf260 thread T17
    #0 0x563e36ee1205 in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0x4c205) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #1 0x563e36ee1759 in bcmp (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0x4c759) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #2 0x7fb87667cec2 in nav2_bt_navigator::NavigatorMuxer::stopNavigating(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x47cec2) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #3 0x7fb87666d1a6 in nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>::onCompletion(std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x46d1a6) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #4 0x7fb87667bb81 in void std::__invoke_impl<void, void (nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus), nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus>(std::__invoke_memfun_deref, void (nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus), nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >&&, nav2_behavior_tree::BtStatus&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x47bb81) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #5 0x7fb87667b6e6 in std::_Function_handler<void (std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus), std::_Bind<void (nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>::* (nav2_bt_navigator::Navigator<nav2_msgs::action::NavigateToPose>*, std::_Placeholder<1>, std::_Placeholder<2>))(std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >, nav2_behavior_tree::BtStatus)> >::_M_invoke(std::_Any_data const&, std::shared_ptr<nav2_msgs::action::NavigateToPose_Result_<std::allocator<void> > >&&, nav2_behavior_tree::BtStatus&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x47b6e6) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #6 0x7fb87667ea2f in nav2_behavior_tree::BtActionServer<nav2_msgs::action::NavigateToPose>::executeCallback() (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x47ea2f) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #7 0x7fb8766a9cef in nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::work() (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a9cef) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #8 0x7fb8766a9208 in std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>::operator()() const (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a9208) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #9 0x7fb8766a8f29 in std::enable_if<is_invocable_r_v<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter>, std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>&>, std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> >::type std::__invoke_r<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter>, std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>&>(std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a8f29) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #10 0x7fb8766a8db0 in std::_Function_handler<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> (), std::__future_base::_Task_setter<std::unique_ptr<std::__future_base::_Result<void>, std::__future_base::_Result_base::_Deleter>, std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void> >::_M_invoke(std::_Any_data const&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a8db0) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #11 0x7fb8792457f6 in std::__future_base::_State_baseV2::_M_do_set(std::function<std::unique_ptr<std::__future_base::_Result_base, std::__future_base::_Result_base::_Deleter> ()>*, bool*) (/home/*****/nav2_humble/install/nav2_behavior_tree/lib/libnav2_compute_path_to_pose_action_bt_node.so+0x307f6) (BuildId: 552cfd70a84cf279d69712ed189efea8e25d71f0)
    #12 0x7fb875899ee7 in __pthread_once_slow nptl/./nptl/pthread_once.c:116:7
    #13 0x7fb8766a6b8a in std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>::_M_run() (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a6b8a) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #14 0x7fb875cdc252  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdc252) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)
    #15 0x7fb875894ac2 in start_thread nptl/./nptl/pthread_create.c:442:8
    #16 0x7fb87592684f  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81

0x6030006bf260 is located 0 bytes inside of 31-byte region [0x6030006bf260,0x6030006bf27f)
freed by thread T0 here:
    #0 0x563e36f8511d in operator delete(void*) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0xf011d) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #1 0x7fb876596e21 in nav2_bt_navigator::BtNavigator::~BtNavigator() (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x396e21) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #2 0x7fb875829d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16

previously allocated by thread T13 here:
    #0 0x563e36f848bd in operator new(unsigned long) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0xef8bd) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #1 0x7fb875d4bfee in std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_assign(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) (/lib/x86_64-linux-gnu/libstdc++.so.6+0x14bfee) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)

Thread T17 created by T0 here:
    #0 0x563e36f32f6c in __interceptor_pthread_create (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0x9df6c) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #1 0x7fb875cdc328 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdc328) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)
    #2 0x7fb8766a6509 in std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>::_Async_state_impl<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a6509) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #3 0x7fb8766a5e50 in std::_Sp_counted_ptr_inplace<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>, std::allocator<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void> >, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(std::allocator<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void> >, nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a5e50) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #4 0x7fb8766a365a in std::future<std::__invoke_result<std::decay<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>::type>::type> std::async<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(std::launch, nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a365a) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #5 0x7fb87668c533 in nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x48c533) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #6 0x7fb8766af3ae in void std::__invoke_impl<void, void (nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >), nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> > >(std::__invoke_memfun_deref, void (nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >), nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4af3ae) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #7 0x7fb876694973 in rclcpp_action::Server<nav2_msgs::action::NavigateToPose>::call_goal_accepted_callback(std::shared_ptr<rcl_action_goal_handle_s>, std::array<unsigned char, 16ul>, std::shared_ptr<void>) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x494973) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #8 0x7fb877a7e1b6 in rclcpp_action::ServerBase::execute_goal_request_received(std::shared_ptr<void>&) (/opt/ros/humble/lib/librclcpp_action.so+0x131b6) (BuildId: 8da0710b8af025b200f6ce73ffc85c5ed5c45a8d)

Thread T13 created by T0 here:
    #0 0x563e36f32f6c in __interceptor_pthread_create (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0x9df6c) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070)
    #1 0x7fb875cdc328 in std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) (/lib/x86_64-linux-gnu/libstdc++.so.6+0xdc328) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)
    #2 0x7fb8766a6509 in std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>::_Async_state_impl<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a6509) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #3 0x7fb8766a5e50 in std::_Sp_counted_ptr_inplace<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void>, std::allocator<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void> >, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(std::allocator<std::__future_base::_Async_state_impl<std::thread::_Invoker<std::tuple<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()> >, void> >, nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a5e50) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #4 0x7fb8766a365a in std::future<std::__invoke_result<std::decay<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>::type>::type> std::async<nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()>(std::launch, nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >)::'lambda'()&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4a365a) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #5 0x7fb87668c533 in nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::handle_accepted(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x48c533) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #6 0x7fb8766af3ae in void std::__invoke_impl<void, void (nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >), nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> > >(std::__invoke_memfun_deref, void (nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>::*&)(std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >), nav2_util::SimpleActionServer<nav2_msgs::action::NavigateToPose>*&, std::shared_ptr<rclcpp_action::ServerGoalHandle<nav2_msgs::action::NavigateToPose> >&&) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x4af3ae) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #7 0x7fb876694973 in rclcpp_action::Server<nav2_msgs::action::NavigateToPose>::call_goal_accepted_callback(std::shared_ptr<rcl_action_goal_handle_s>, std::array<unsigned char, 16ul>, std::shared_ptr<void>) (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/libbt_navigator_core.so+0x494973) (BuildId: 014f746fc22fa55b4dda12be8ead56d03af59e4e)
    #8 0x7fb877a7e1b6 in rclcpp_action::ServerBase::execute_goal_request_received(std::shared_ptr<void>&) (/opt/ros/humble/lib/librclcpp_action.so+0x131b6) (BuildId: 8da0710b8af025b200f6ce73ffc85c5ed5c45a8d)

SUMMARY: AddressSanitizer: heap-use-after-free (/home/*****/nav2_humble/install/nav2_bt_navigator/lib/nav2_bt_navigator/bt_navigator+0x4c205) (BuildId: 34a63e084ab75f9b1885869f4001c62f73d97070) in MemcmpInterceptorCommon(void*, int (*)(void const*, void const*, unsigned long), void const*, void const*, unsigned long)
Shadow bytes around the buggy address:
  0x0c06800cfdf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800cfe00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800cfe10: fa fa fa fa fa fa 00 00 00 00 fa fa 00 00 00 03
  0x0c06800cfe20: fa fa 00 00 00 fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800cfe30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c06800cfe40: fa fa fa fa fa fa fa fa fa fa fa fa[fd]fd fd fd
  0x0c06800cfe50: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800cfe60: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c06800cfe70: fa fa fa fa fa fa fd fd fd fa fa fa fd fd fd fa
  0x0c06800cfe80: fa fa fa fa fa fa fa fa fd fd fd fa fa fa fa fa
  0x0c06800cfe90: fa fa fa fa fd fd fd fa fa fa fd fd fd fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb

Additional information

---

simple analysis:
1> During the shutdown period, a difficult to execute action thread is currently blocking and has received a shutdown signal
2> nav2 starts resource cleaning, and finally nav2_bt_navigator is released as a whole. The destructor releases all the pointers it creates, but the actionServer thread is still executing, resulting in UAF access.

[SteveMacenski]

It is not considered a valid shutdown mid-execution. You should wait between executions, then bring down the lifecycle nodes, then exit the program cleanly. With that said, if you want to improve cleanup using normal-operations signals, power to you!

server_timeout_ the spin timeout is there to handle so we can handle lack of responses. Though, it seems odd to me looking at it now that in those if statements, we don't return after the log.

You could also adjust the definition of should_cancel_goal() https://github.com/ros-planning/navigation2/blob/47374622dee01a27e5f9b8ae08f3d19a15de9b3a/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_node.hpp#L338-L356 to look at the context / rclcpp::ok() to not cancel in case of halt() in the shutdown case, thereby bypassing all of this.

[SteveMacenski]

I don't have an immediate answer, it could be many things I'd need to catch myself to see where it happened - but if we handle it the way I describe above, maybe its moot :-)

[GoesM]

I found that I have some misunderstanding before. I'm so sorry for my fault.

I've deleted the additional information before, and here's a more clear one.

Additional information

code analysis

To clearly understand why use-after-free within multi-threads, it's first to find which thread / function use and which thread free.

the thread for use
It's obvious that the pointer is used in the function BtActionServer::executeCallback()->CompletionCallback() (also named as onCompletion()) -> stopNavigating()

And the function BtActionServer::executeCallback() is called in another thread by the bt_action_server_'s member action_server_ , refer to following code:

https://github.com/ros-planning/navigation2/blob/0897da9252b4cb537b1110e88831a1e63ac157d3/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L146-L152

the thread for free

however, there's only action_server_.reset() in bt_action_server_->on_cleanup(), but no wait for action_server_'s sub-thread finished.

https://github.com/ros-planning/navigation2/blob/0897da9252b4cb537b1110e88831a1e63ac157d3/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L203-L215

Also, there's only bt_action_server_->oncleanup() in nav2_bt_navigator->on_cleanup(), but also no wait for action_server_'s sub-thread finished.

https://github.com/ros-planning/navigation2/blob/0897da9252b4cb537b1110e88831a1e63ac157d3/nav2_bt_navigator/src/bt_navigator.cpp#L175-L193

result
So that if action_server_'s sub-thread is still running after action_server_.reset() or even after bt_action_server_->on_cleanup() or even after nav2_bt_navigator->on_cleanup() or even after ~nav2_bt_navigator(), the sub-thread would access the freed pointers of nav2_bt_navigator (then UAF occurs).

experiments / POC

To make sure the executeCallback() is still working after the ~nav2_bt_navigator(), I insert some code into executeCallback() as following to extend the time-cost of sub-thread:

template<class ActionT>
void BtActionServer<ActionT>::executeCallback()
{
...
    // insert
      std::this_thread::sleep_for(std::chrono::seconds(4));
    // insert end
  on_completion_callback_(result, rc);
...
}

then launch nav2 normally

then publish a goal_pose like :

ros2 topic pub /goal_pose geometry_msgs/msg/PoseStamped " 
header:
  stamp:
    sec: 1706255138
    nanosec: 811244537
  frame_id: map
pose:
  position:
    x: 0.7312569618225098
    y: 1.8531221151351929
    z: 0.0
  orientation:
    x: 0.0
    y: 0.0
    z: -0.7723305100496971
    w: 0.635220893269715" -1

then Ctrl+C to nav2 before goal-achieved

then asan-report would occur.

It proves that ** the executeCallback() is still working after the ~nav2_bt_navigator() **

similar ticket

I believe it's quite simliar to another UAF-issue #4166, they both just do a shared-pointer reset() but no wait for the sub-thread / sub-process created by the shared-pointer, which brings in the UAF.

suggestion to fix

Is there any way to let the sub-thread / process created by action_server_ joint into the function bt_action_server_->on_cleanup() or nav2_bt_navigator->on_cleanup() ?

[SteveMacenski]

Spin thread appears to be false, so the simple action server isn't using its own executor, so I suppose the first thing to find is where that new thread is created to decide where we want to make sure its joined on the action server's reset().

reset() would destroy the object's members, including the executor and related assets

  rclcpp::CallbackGroup::SharedPtr callback_group_{nullptr};
  rclcpp::executors::SingleThreadedExecutor::SharedPtr executor_;
  std::unique_ptr<nav2_util::NodeThread> executor_thread_;

And as we've shown before, the NodeThread's destructor join()s the thread before exiting. So that all seems fine to me. Finding where that thread is the important part. I don't see that the action server has a new thread - but not that there is one from the client node https://github.com/ros-planning/navigation2/blob/0897da9252b4cb537b1110e88831a1e63ac157d3/nav2_behavior_tree/include/nav2_behavior_tree/bt_action_server_impl.hpp#L127 for the internals of the BT nodes. I leave it to your sleuthing to find where the issue thread is at to join.

[GoesM]

Conclusion :

UAF is caused by the error thrown in function SimpleActionServer::deactivate() in [nav2_uti/include/nav2_util/simple_action_server.hpp] of nav2_humble

https://github.com/ros-planning/navigation2/blob/a45b151ceb3aa9edebad8a528cd672935f0c668d/nav2_util/include/nav2_util/simple_action_server.hpp#L289-L296

if here's some error thrown here, it would step in try..catch mechanism of ros2/rclcpp, refer to code link

node_interfaces::LifecycleNodeInterface::CallbackReturn
LifecycleNode::LifecycleNodeInterfaceImpl::execute_callback(
  unsigned int cb_id, const State & previous_state) const
{
  // in case no callback was attached, we forward directly
  auto cb_success = node_interfaces::LifecycleNodeInterface::CallbackReturn::SUCCESS;


  auto it = cb_map_.find(static_cast<uint8_t>(cb_id));
  if (it != cb_map_.end()) {
    auto callback = it->second;
    try {
      cb_success = callback(State(previous_state));
    } catch (const std::exception & e) {
      RCLCPP_ERROR(
        node_logging_interface_->get_logger(),
        "Caught exception in callback for transition %d", it->first);
      RCLCPP_ERROR(
        node_logging_interface_->get_logger(),
        "Original error: %s", e.what());
      cb_success = node_interfaces::LifecycleNodeInterface::CallbackReturn::ERROR;
    }
  }
  return cb_success;
}

Following that, it interrupts the process's original deactivate() function, causing the process to exit without completing the task of waiting for the thread to exit, resulting in a UAF.

[GoesM]

SO, it seems that we should solve error-thrown here, if we want to avoid UAF.

error thrown 1 : the lack of nullptr-check bring in bad function call error catched

in nav2-main stack , here's a nullptr-check for completion_callback_ function-pointer, which is used to avoid nullptr-accessed

https://github.com/ros-planning/navigation2/blob/09ba08b7f45004df92f77fa683ea33d8afa2acce/nav2_util/include/nav2_util/simple_action_server.hpp#L317

However, here's the lack of it in nav2-humble version:

https://github.com/ros-planning/navigation2/blob/a45b151ceb3aa9edebad8a528cd672935f0c668d/nav2_util/include/nav2_util/simple_action_server.hpp#L293

BUT, according to the code of bt_navigator, this pointer is always NULL, so that NullPtr-accessed would be catched as "bad function call".

debug info for error thrown 1

I insert some log here for debug, as following:

 while (execution_future_.wait_for(milliseconds(100)) != std::future_status::ready) {
      info_msg("Waiting for async process to finish.");
      if (steady_clock::now() - start_time >= server_timeout_) {
           // goes insert
              warn_msg("terminate_all() started. ");
           // insert end
        terminate_all();
          // goes insert
            warn_msg("terminate_all() finished. ");
            warn_msg("completion_callback_ start ");
          // insert end
      if(completion_callback_) completion_callback_();
          // goes insert
            warn_msg("completion_callback_ finished.");
            warn_msg("before throw error. ");
          // insert end
        throw std::runtime_error("Action callback is still running and missed deadline to stop");
      }
    debug_msg("Deactivation completed.");
  }

then, the log of nav2_bt_navigator shows:

[INFO] [1710427380.141387057] [rclcpp]: signal_handler(signum=2)
[INFO] [1710427380.141517732] [bt_navigator]: Running Nav2 LifecycleNode rcl preshutdown (bt_navigator)
[INFO] [1710427380.141572676] [bt_navigator]: Deactivating
[WARN] [1710427380.141646003] [bt_navigator]: [navigate_to_pose] [ActionServer] Requested to deactivate server but goal is still executing. Should check if action server is running before deactivating.
[ERROR] [1710427380.171597874] [bt_navigator_navigate_to_pose_rclcpp_node]: Failed to get result for follow_path in node halt!
[INFO] [1710427380.241779538] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427380.341846408] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427380.442045833] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427380.542353693] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427380.642514433] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[WARN] [1710427380.642575567] [bt_navigator]: [navigate_to_pose] [ActionServer] terminate_all() started. 
[WARN] [1710427380.642596713] [bt_navigator]: [navigate_to_pose] [ActionServer] Aborting handle.
[WARN] [1710427381.642985458] [bt_navigator]: [navigate_to_pose] [ActionServer] terminate_all() finished. 
[WARN] [1710427381.642994046] [bt_navigator]: [navigate_to_pose] [ActionServer] completion_callback_ start 
[ERROR] [1710427381.643055223] [bt_navigator]: Caught exception in callback for transition 14
[ERROR] [1710427381.643063858] [bt_navigator]: Original error: bad_function_call
[WARN] [1710427381.643086517] [bt_navigator]: Error occurred while doing error handling.
[FATAL] [1710427381.643113948] [bt_navigator]: Lifecycle node bt_navigator does not have error state implemented
[INFO] [1710427381.643134721] [bt_navigator]: Destroying bond (bt_navigator) to lifecycle manager.

As we can see, the log is interrupted after completion_callback_ start and before completion_callback_ finished, which proves that the interruption is caused by the nullptr-accessed here.

error thrown 2 : the proactive "throw error" leads to error catched

To avoid NullPtr-accessed, I fixed as how it's fixed in nav2-main:

 while (execution_future_.wait_for(milliseconds(100)) != std::future_status::ready) {
      info_msg("Waiting for async process to finish.");
      if (steady_clock::now() - start_time >= server_timeout_) {
           // goes insert
              warn_msg("terminate_all() started. ");
           // insert end
        terminate_all();
          // goes insert
            warn_msg("terminate_all() finished. ");
            warn_msg("completion_callback_ start ");
          // insert end
     if(completion_callback_) completion_callback_();
          // goes insert
            warn_msg("completion_callback_ finished.");
            warn_msg("before throw error. ");
          // insert end
        throw std::runtime_error("Action callback is still running and missed deadline to stop");
      }
    debug_msg("Deactivation completed.");
  }

BUT, the UAF still occurs. That because the code throw an runtime_error here, also catched by rclcpp:

https://github.com/ros-planning/navigation2/blob/a45b151ceb3aa9edebad8a528cd672935f0c668d/nav2_util/include/nav2_util/simple_action_server.hpp#L294

debug info for error thrown 2

we can see the log is as following:

[INFO] [1710427133.548628103] [rclcpp]: signal_handler(signum=2)
[INFO] [1710427133.548761756] [bt_navigator]: Running Nav2 LifecycleNode rcl preshutdown (bt_navigator)
[INFO] [1710427133.548870713] [bt_navigator]: Deactivating
[WARN] [1710427133.548921523] [bt_navigator]: [navigate_to_pose] [ActionServer] Requested to deactivate server but goal is still executing. Should check if action server is running before deactivating.
[ERROR] [1710427133.572469539] [bt_navigator_navigate_to_pose_rclcpp_node]: Failed to get result for follow_path in node halt!
[INFO] [1710427133.649079516] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427133.749291894] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427133.849653475] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427133.949895585] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[INFO] [1710427134.050078388] [bt_navigator]: [navigate_to_pose] [ActionServer] Waiting for async process to finish.
[WARN] [1710427134.050137301] [bt_navigator]: [navigate_to_pose] [ActionServer] terminate_all() started. 
[WARN] [1710427134.050147568] [bt_navigator]: [navigate_to_pose] [ActionServer] Aborting handle.
[WARN] [1710427135.050402705] [bt_navigator]: [navigate_to_pose] [ActionServer] terminate_all() finished. 
[WARN] [1710427135.050417258] [bt_navigator]: [navigate_to_pose] [ActionServer] before throw error. 
[ERROR] [1710427135.050471087] [bt_navigator]: Caught exception in callback for transition 14
[ERROR] [1710427135.050495290] [bt_navigator]: Original error: Action callback is still running and missed deadline to stop
[WARN] [1710427135.050514797] [bt_navigator]: Error occurred while doing error handling.
[FATAL] [1710427135.050531012] [bt_navigator]: Lifecycle node bt_navigator does not have error state implemented
[INFO] [1710427135.050548761] [bt_navigator]: Destroying bond (bt_navigator) to lifecycle manager.

it shows that the runtime_error is catched by rclcpp, and no final debug_msg Deactivation completed. occurs, which proves that the processes is also interrupted by try-catch of rclcpp

[GoesM]

suggestion to fix ?

1> add nullptr-check in nav2_humble

2> just warn but no throw error ?

I'm not very clear about the details of why there's a proactive "throw error" approach and why there's a “try-catch” mechanism in rclcpp, but I believe they are set for mitigating the harm caused by errors.

However, if such methods end up introducing more harmful UAF-type bugs, personally, I think it might be better for us not to proactively "throw errors" but just warn_msg() it for the user ?

Since, if user wrongly shutdown the navigation when some tasks are still on-work , it should be better to show a warning for his own operation than bring a more harmful security-issue like UAF ? I think....

[SteveMacenski]

Please submit a PR! I think a strongly worded error message would be sufficient as well