diff --git a/rclcpp/include/rclcpp/event_handler.hpp b/rclcpp/include/rclcpp/event_handler.hpp index cf9ca04121..264046199e 100644 --- a/rclcpp/include/rclcpp/event_handler.hpp +++ b/rclcpp/include/rclcpp/event_handler.hpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -247,7 +248,9 @@ class EventHandlerBase : public Waitable std::function 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::max(); }; template diff --git a/rclcpp/src/rclcpp/event_handler.cpp b/rclcpp/src/rclcpp/event_handler.cpp index 630bc26d33..f1ef7383ec 100644 --- a/rclcpp/src/rclcpp/event_handler.cpp +++ b/rclcpp/src/rclcpp/event_handler.cpp @@ -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_; } diff --git a/rclcpp/test/rclcpp/test_qos_event.cpp b/rclcpp/test/rclcpp/test_qos_event.cpp index 5cfa6b4d60..7a84d526b5 100644 --- a/rclcpp/test/rclcpp/test_qos_event.cpp +++ b/rclcpp/test/rclcpp/test_qos_event.cpp @@ -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(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 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(