Memory error when attempting to clean up expired nodes or callback groups.

[brawner]

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

	if (weak_group_ptr.expired() || weak_node_ptr.expired()) {
	invalid_group_ptrs.push_back(weak_group_ptr);
	auto node_guard_pair = weak_nodes_to_guard_conditions_.find(weak_node_ptr);
	weak_nodes_to_guard_conditions_.erase(weak_node_ptr);
	memory_strategy_->remove_guard_condition(node_guard_pair->second);
	}

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)

[ivanpauno]

@wjwwood can you take a look to this one?

[brawner]

I believe this was addressed by #1404