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..262d346639 100644 --- a/rclcpp/src/rclcpp/event_handler.cpp +++ b/rclcpp/src/rclcpp/event_handler.cpp @@ -69,6 +69,11 @@ EventHandlerBase::add_to_wait_set(rcl_wait_set_t & wait_set) bool EventHandlerBase::is_ready(const rcl_wait_set_t & wait_set) { + // Only assigned by add_to_wait_set(); unassigned, or assigned by a different wait set, + // means the index does not address this one. See ros2/rclcpp#2376. + 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..3656c91610 100644 --- a/rclcpp/test/rclcpp/test_qos_event.cpp +++ b/rclcpp/test/rclcpp/test_qos_event.cpp @@ -23,6 +23,7 @@ #include "rclcpp/rclcpp.hpp" #include "rcl/event.h" +#include "rcl/wait.h" #include "rcutils/logging.h" #include "rmw/rmw.h" #include "test_msgs/msg/empty.hpp" @@ -338,6 +339,58 @@ TEST_F(TestQosEvent, add_to_wait_set) { } } +// Regression for ros2/rclcpp#2376: with no index ever assigned, is_ready() must report +// not-ready rather than indexing wait_set.events. +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)); +} + +// Regression for ros2/rclcpp#2376: an index assigned by another wait set must not be +// dereferenced. index == size_of_events is the off-by-one the bounds check has to reject. +TEST_F(TestQosEvent, is_ready_with_index_from_another_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); + + rcl_context_t * context = + node->get_node_base_interface()->get_context()->get_rcl_context().get(); + rcl_wait_set_t sized = rcl_get_zero_initialized_wait_set(); + ASSERT_EQ( + RCL_RET_OK, + rcl_wait_set_init(&sized, 0, 0, 0, 0, 0, 1, context, rcl_get_default_allocator())); + handler.add_to_wait_set(sized); + + // The assigned index 0 is valid only for `sized`. Against a wait set with no event + // slots it equals size_of_events, which must be rejected rather than indexed. + rcl_wait_set_t empty = rcl_get_zero_initialized_wait_set(); + EXPECT_FALSE(handler.is_ready(empty)); + + EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&sized)); +} + TEST_F(TestQosEvent, test_on_new_event_callback) { if (!rclcpp::SubscriptionBase::event_type_is_supported(