From 411dbe8212dd753ee60871f8605d0a8beaae5e37 Mon Sep 17 00:00:00 2001 From: mauropasse Date: Tue, 14 Nov 2023 02:24:07 +0000 Subject: [PATCH] Fix data race in EventHandlerBase (#2349) Both the EventHandler and its associated pubs/subs share the same underlying rmw event listener. When a pub/sub is destroyed, the listener is destroyed. There is a data race when the ~EventHandlerBase wants to access the listener after it has been destroyed. The EventHandler stores a shared_ptr of its associated pub/sub. But since we were clearing the listener event callbacks on the base class destructor ~EventHandlerBase, the pub/sub was already destroyed, which means the rmw event listener was also destroyed, thus causing a segfault when trying to obtain it to clear the callbacks. Clearing the callbacks on ~EventHandler instead of ~EventHandlerBase avoids the race, since the pub/sub are still valid. Signed-off-by: Mauro Passerino --- rclcpp/include/rclcpp/event_handler.hpp | 10 ++++++++++ rclcpp/src/rclcpp/event_handler.cpp | 7 ------- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/rclcpp/include/rclcpp/event_handler.hpp b/rclcpp/include/rclcpp/event_handler.hpp index 3f41de469c..f9b75eb7cf 100644 --- a/rclcpp/include/rclcpp/event_handler.hpp +++ b/rclcpp/include/rclcpp/event_handler.hpp @@ -260,6 +260,16 @@ class EventHandler : public EventHandlerBase } } + ~EventHandler() + { + // Since the rmw event listener holds a reference to the + // "on ready" callback, we need to clear it on destruction of this class. + // This clearing is not needed for other rclcpp entities like pub/subs, since + // they do own the underlying rmw entities, which are destroyed + // on their rclcpp destructors, thus no risk of dangling pointers. + clear_on_ready_callback(); + } + /// Take data so that the callback cannot be scheduled again std::shared_ptr take_data() override diff --git a/rclcpp/src/rclcpp/event_handler.cpp b/rclcpp/src/rclcpp/event_handler.cpp index 40ae6c030d..d4b4d57b08 100644 --- a/rclcpp/src/rclcpp/event_handler.cpp +++ b/rclcpp/src/rclcpp/event_handler.cpp @@ -39,13 +39,6 @@ UnsupportedEventTypeException::UnsupportedEventTypeException( EventHandlerBase::~EventHandlerBase() { - // Since the rmw event listener holds a reference to - // this callback, we need to clear it on destruction of this class. - // This clearing is not needed for other rclcpp entities like pub/subs, since - // they do own the underlying rmw entities, which are destroyed - // on their rclcpp destructors, thus no risk of dangling pointers. - clear_on_ready_callback(); - if (rcl_event_fini(&event_handle_) != RCL_RET_OK) { RCUTILS_LOG_ERROR_NAMED( "rclcpp",