Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion rclcpp/include/rclcpp/event_handler.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include <atomic>
#include <functional>
#include <limits>
#include <memory>
#include <mutex>
#include <stdexcept>
Expand Down Expand Up @@ -247,7 +248,9 @@ class EventHandlerBase : public Waitable
std::function<void(size_t)> on_new_event_callback_{nullptr};

rcl_event_t event_handle_;
size_t wait_set_event_index_;
// Sentinel that is always out of range, so is_ready() reports "not in any wait set"
// until add_to_wait_set() assigns the real index. See ros2/rclcpp#2376.
size_t wait_set_event_index_ = std::numeric_limits<size_t>::max();
};

template<typename EventCallbackT, typename ParentHandleT>
Expand Down
7 changes: 7 additions & 0 deletions rclcpp/src/rclcpp/event_handler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,13 @@ EventHandlerBase::add_to_wait_set(rcl_wait_set_t & wait_set)
bool
EventHandlerBase::is_ready(const rcl_wait_set_t & wait_set)
{
// wait_set_event_index_ is only valid once this handler has been added to this wait
// set via add_to_wait_set(). If is_ready() is reached before that (e.g. an event
// thread racing node setup, see ros2/rclcpp#2376), the index may not refer to a slot
// in this wait set, so guard against an out-of-bounds read.
if (wait_set_event_index_ >= wait_set.size_of_events) {
return false;
}
return wait_set.events[wait_set_event_index_] == &event_handle_;
}

Expand Down
22 changes: 22 additions & 0 deletions rclcpp/test/rclcpp/test_qos_event.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,28 @@ TEST_F(TestQosEvent, add_to_wait_set) {
}
}

// Regression for ros2/rclcpp#2376: is_ready() must not index wait_set.events with an
// uninitialized/stale wait_set_event_index_ when the handler is not in the wait set
// (e.g. an event thread racing node setup). It must return false, not read out of bounds.
TEST_F(TestQosEvent, is_ready_when_not_in_wait_set) {
auto publisher = node->create_publisher<test_msgs::msg::Empty>(topic_name, 10);
auto rcl_handle = publisher->get_publisher_handle();

auto callback = [](int) {};

const rcl_publisher_event_type_t event_type =
!rclcpp::PublisherBase::event_type_is_supported(RCL_PUBLISHER_OFFERED_DEADLINE_MISSED) ?
RCL_PUBLISHER_MATCHED : RCL_PUBLISHER_OFFERED_DEADLINE_MISSED;

rclcpp::EventHandler<decltype(callback), decltype(rcl_handle)> handler(
callback, rcl_publisher_event_init, rcl_handle, event_type);

// The handler has not been added to any wait set, so its event index is not valid for
// this (empty) wait set; is_ready() must report not-ready instead of dereferencing it.
rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set();
EXPECT_FALSE(handler.is_ready(wait_set));
}

TEST_F(TestQosEvent, test_on_new_event_callback)
{
if (!rclcpp::SubscriptionBase::event_type_is_supported(
Expand Down