Memory error when attempting to clean up expired nodes or callback groups. #1402
Description
This issue was introduced with #1218 and it's failure is exhibited during TestExecutors.addTemporaryNode. In Executor::wait_for_work(), weak_ptr to nodes and callback groups are cleaned up if they have expired. However, if a node has already been destructed, its associated guard_condition has already been deallocated.
Specifically the node_guard_pair->second which is a const rcl_guard_condition_t * has already been deallocated.
rclcpp/rclcpp/src/rclcpp/executor.cpp
Lines 678 to 683 in b5b8782
In the valgrind output, the second error is the same thing since this error is in the base class and this test is run for both SingleThreaded and MultiThreaded variants.
=3095829== 1 errors in context 1 of 2:
==3095829== Invalid read of size 8
==3095829== at 0x5012703: rclcpp::Executor::wait_for_work(std::chrono::duration<long, std::ratio<1l, 1000000000l> >) (executor.cpp:683)
==3095829== by 0x5013F77: rclcpp::Executor::get_next_executable(rclcpp::AnyExecutable&, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) (executor.cpp:875)
==3095829== by 0x502E237: rclcpp::executors::SingleThreadedExecutor::spin() (single_threaded_executor.cpp:35)
==3095829== by 0x22EE55: TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}::operator()() const (test_executors.cpp:157)
==3095829== by 0x298BF4: void std::__invoke_impl<void, TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}>(std::__invoke_other, TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}&&) (invoke.h:60)
==3095829== by 0x2976CA: std::__invoke_result<TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}>::type std::__invoke<TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}>(std::__invoke_result&&, (TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}&&)...) (invoke.h:95)
==3095829== by 0x294C5E: void std::thread::_Invoker<std::tuple<TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}> >::_M_invoke<0ul>(std::_Index_tuple<0ul>) (thread:244)
==3095829== by 0x291B17: std::thread::_Invoker<std::tuple<TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}> >::operator()() (thread:251)
==3095829== by 0x28E390: std::thread::_State_impl<std::thread::_Invoker<std::tuple<TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody()::{lambda()#1}> > >::_M_run() (thread:195)
==3095829== by 0x586FD83: ??? (in /usr/lib/x86_64-linux-gnu/libstdc++.so.6.0.28)
==3095829== by 0x599E608: start_thread (pthread_create.c:477)
==3095829== by 0x5ADA292: clone (clone.S:95)
==3095829== Address 0xcc83480 is 48 bytes inside a block of size 56 free'd
==3095829== at 0x483EFBF: operator delete(void*) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==3095829== by 0x5023CC6: __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::deallocate(std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >*, unsigned long) (new_allocator.h:128)
==3095829== by 0x5023186: std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > > >::deallocate(std::allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >&, std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >*, unsigned long) (alloc_traits.h:470)
==3095829== by 0x5021606: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_put_node(std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >*) (stl_tree.h:584)
==3095829== by 0x501E5BD: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_drop_node(std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >*) (stl_tree.h:651)
==3095829== by 0x501B9A8: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_erase(std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >*) (stl_tree.h:1915)
==3095829== by 0x501C08D: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::clear() (stl_tree.h:1266)
==3095829== by 0x5020B96: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_erase_aux(std::_Rb_tree_const_iterator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::_Rb_tree_const_iterator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >) (stl_tree.h:2522)
==3095829== by 0x501D771: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::erase(std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&) (stl_tree.h:2536)
==3095829== by 0x501AD57: std::map<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, rcl_guard_condition_t const*, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::erase(std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&) (stl_map.h:1068)
==3095829== by 0x501264A: rclcpp::Executor::wait_for_work(std::chrono::duration<long, std::ratio<1l, 1000000000l> >) (executor.cpp:685)
==3095829== by 0x5013F77: rclcpp::Executor::get_next_executable(rclcpp::AnyExecutable&, std::chrono::duration<long, std::ratio<1l, 1000000000l> >) (executor.cpp:881)
==3095829== Block was alloc'd at
==3095829== at 0x483DE63: operator new(unsigned long) (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==3095829== by 0x5024142: __gnu_cxx::new_allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::allocate(unsigned long, void const*) (new_allocator.h:114)
==3095829== by 0x5023733: std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > > >::allocate(std::allocator<std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >&, unsigned long) (alloc_traits.h:444)
==3095829== by 0x5022174: std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_get_node() (stl_tree.h:580)
==3095829== by 0x501F745: std::_Rb_tree_node<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >* std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_create_node<std::piecewise_construct_t const&, std::tuple<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&>, std::tuple<> >(std::piecewise_construct_t const&, std::tuple<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&>&&, std::tuple<>&&) (stl_tree.h:630)
==3095829== by 0x501D0AC: std::_Rb_tree_iterator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > std::_Rb_tree<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*>, std::_Select1st<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tuple<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&>, std::tuple<> >(std::_Rb_tree_const_iterator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> >, std::piecewise_construct_t const&, std::tuple<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&>&&, std::tuple<>&&) (stl_tree.h:2455)
==3095829== by 0x501A8FB: std::map<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, rcl_guard_condition_t const*, std::owner_less<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> >, std::allocator<std::pair<std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const, rcl_guard_condition_t const*> > >::operator[](std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> const&) (stl_map.h:499)
==3095829== by 0x500C387: rclcpp::Executor::add_callback_group_to_map(std::shared_ptr<rclcpp::CallbackGroup>, std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::map<std::weak_ptr<rclcpp::CallbackGroup>, std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface>, std::owner_less<std::weak_ptr<rclcpp::CallbackGroup> >, std::allocator<std::pair<std::weak_ptr<rclcpp::CallbackGroup> const, std::weak_ptr<rclcpp::node_interfaces::NodeBaseInterface> > > >&, bool) (executor.cpp:219)
==3095829== by 0x500CC84: rclcpp::Executor::add_node(std::shared_ptr<rclcpp::node_interfaces::NodeBaseInterface>, bool) (executor.cpp:259)
==3095829== by 0x500D9E8: rclcpp::Executor::add_node(std::shared_ptr<rclcpp::Node>, bool) (executor.cpp:321)
==3095829== by 0x22F085: TestExecutorsStable_addTemporaryNode_Test<rclcpp::executors::SingleThreadedExecutor>::TestBody() (test_executors.cpp:153)
==3095829== by 0x30802D: void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) (gtest.cc:2447)