Skip to content

Commit

Permalink
Fix data race in EventHandlerBase (#2349)
Browse files Browse the repository at this point in the history
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 <mpasserino@irobot.com>
(cherry picked from commit 411dbe8)
  • Loading branch information
mauropasse authored and mergify[bot] committed Dec 12, 2023
1 parent dc6ac4e commit e73ffd4
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 7 deletions.
10 changes: 10 additions & 0 deletions rclcpp/include/rclcpp/event_handler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -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<void>
take_data() override
Expand Down
7 changes: 0 additions & 7 deletions rclcpp/src/rclcpp/event_handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down

0 comments on commit e73ffd4

Please sign in to comment.