From 7e39aa34d2b83cf8231547dfffc6874d4c1b836f Mon Sep 17 00:00:00 2001 From: Abderahmane BENALI Date: Mon, 27 Jul 2026 18:57:59 +0200 Subject: [PATCH] fix(rmw_event): reject unsupported event types in *_event_init rmw_publisher_event_init/rmw_subscription_event_init always returned RMW_RET_OK regardless of event_type, while rmw_event_type_is_supported() always returns false and rmw_event_set_callback() always returns RMW_RET_UNSUPPORTED without setting an error message. rclcpp's EventHandler constructor treats RMW_RET_UNSUPPORTED from *_event_init specially (UnsupportedEventTypeException, swallowed by SubscriptionBase::bind_event_callbacks for the default incompatible-QoS/ type callbacks every subscription gets). Since init always "succeeded", rclcpp built a live event handler for an event we don't actually support, and the failure only surfaced later, unhandled, when the EventsExecutor tried to register its callback - crashing with "failed to set the on new message callback for Event: error not set" on every subscription, under EventsExecutor specifically. Fix: check rmw_event_type_is_supported() in both *_event_init functions before reporting success, matching the RMW_SET_ERROR_MSG + UNSUPPORTED pattern already used elsewhere in this file (e.g. rmw_subscription_set_content_filter). Adds test/test_rmw_event.cpp, confirmed to fail on the old behavior and pass after the fix. Full existing suite (128 tests) still passes. Found while verifying rmw_unix_socket_cpp against ros2-benchmark-container and performance_test: EventsExecutor (default in both) crashed on every pub/sub topology; SingleThreadedExecutor/MultiThreadedExecutor were unaffected. --- rmw_unix_socket_cpp/CMakeLists.txt | 5 ++ rmw_unix_socket_cpp/src/rmw_event.cpp | 8 ++ rmw_unix_socket_cpp/test/test_rmw_event.cpp | 86 +++++++++++++++++++++ 3 files changed, 99 insertions(+) create mode 100644 rmw_unix_socket_cpp/test/test_rmw_event.cpp diff --git a/rmw_unix_socket_cpp/CMakeLists.txt b/rmw_unix_socket_cpp/CMakeLists.txt index c45a3c0..89e441f 100644 --- a/rmw_unix_socket_cpp/CMakeLists.txt +++ b/rmw_unix_socket_cpp/CMakeLists.txt @@ -211,6 +211,11 @@ if(BUILD_TESTING) target_link_libraries(test_rmw_qos ${_rmw_test_libs} ${_test_msg_deps}) target_include_directories(test_rmw_qos PRIVATE src) + # RMW API: publisher/subscription events + ament_add_gtest(test_rmw_event test/test_rmw_event.cpp) + target_link_libraries(test_rmw_event ${_rmw_test_libs} ${_test_msg_deps}) + target_include_directories(test_rmw_event PRIVATE src) + # Integration: fork()-based two-process tests pushing >4 MB payloads across # a real PID boundary through the shared-memory path. ament_add_gtest(test_rmw_cross_process test/test_rmw_cross_process.cpp) diff --git a/rmw_unix_socket_cpp/src/rmw_event.cpp b/rmw_unix_socket_cpp/src/rmw_event.cpp index fb0e012..a8d0d79 100644 --- a/rmw_unix_socket_cpp/src/rmw_event.cpp +++ b/rmw_unix_socket_cpp/src/rmw_event.cpp @@ -29,6 +29,10 @@ rmw_ret_t rmw_publisher_event_init( { RMW_CHECK_ARGUMENT_FOR_NULL(rmw_event, RMW_RET_INVALID_ARGUMENT); RMW_CHECK_ARGUMENT_FOR_NULL(publisher, RMW_RET_INVALID_ARGUMENT); + if (!rmw_event_type_is_supported(event_type)) { + RMW_SET_ERROR_MSG("event type not supported"); + return RMW_RET_UNSUPPORTED; + } // Initialize the event struct so rcl can add it to a wait set without crashing. // We don't actually generate events, but the struct must be valid. rmw_event->implementation_identifier = rmw_uds::identifier; @@ -44,6 +48,10 @@ rmw_ret_t rmw_subscription_event_init( { RMW_CHECK_ARGUMENT_FOR_NULL(rmw_event, RMW_RET_INVALID_ARGUMENT); RMW_CHECK_ARGUMENT_FOR_NULL(subscription, RMW_RET_INVALID_ARGUMENT); + if (!rmw_event_type_is_supported(event_type)) { + RMW_SET_ERROR_MSG("event type not supported"); + return RMW_RET_UNSUPPORTED; + } rmw_event->implementation_identifier = rmw_uds::identifier; rmw_event->data = subscription->data; rmw_event->event_type = event_type; diff --git a/rmw_unix_socket_cpp/test/test_rmw_event.cpp b/rmw_unix_socket_cpp/test/test_rmw_event.cpp new file mode 100644 index 0000000..1186bbb --- /dev/null +++ b/rmw_unix_socket_cpp/test/test_rmw_event.cpp @@ -0,0 +1,86 @@ +// Copyright 2026 Abderahmane BENALI +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "test_base.hpp" + +#include + +#include "test_msgs/msg/basic_types.hpp" + +#include "rmw/event.h" +#include "rmw/qos_profiles.h" +#include "rosidl_typesupport_cpp/message_type_support.hpp" + +class EventTest : public RmwUdsNodeTest +{ +protected: + rmw_publisher_t * pub = nullptr; + rmw_subscription_t * sub = nullptr; + const rosidl_message_type_support_t * ts = nullptr; + rmw_qos_profile_t qos; + + void SetUp() override + { + RmwUdsNodeTest::SetUp(); + ts = rosidl_typesupport_cpp::get_message_type_support_handle< + test_msgs::msg::BasicTypes>(); + std::memset(&qos, 0, sizeof(qos)); + qos.history = RMW_QOS_POLICY_HISTORY_KEEP_LAST; + qos.depth = 10; + qos.reliability = RMW_QOS_POLICY_RELIABILITY_RELIABLE; + qos.durability = RMW_QOS_POLICY_DURABILITY_VOLATILE; + + auto pub_opts = rmw_get_default_publisher_options(); + pub = rmw_create_publisher(node, ts, "/event_test_topic", &qos, &pub_opts); + ASSERT_NE(nullptr, pub); + + auto sub_opts = rmw_get_default_subscription_options(); + sub = rmw_create_subscription(node, ts, "/event_test_topic", &qos, &sub_opts); + ASSERT_NE(nullptr, sub); + } + + void TearDown() override + { + if (sub) { auto _r [[maybe_unused]] = rmw_destroy_subscription(node, sub); } + if (pub) { auto _r [[maybe_unused]] = rmw_destroy_publisher(node, pub); } + RmwUdsNodeTest::TearDown(); + } +}; + +// rmw_event_type_is_supported() reports no event types as supported, so +// rclcpp expects *_event_init() to fail with RMW_RET_UNSUPPORTED for those +// event types (it only swallows initialization failures reported this way, +// via UnsupportedEventTypeException). If init instead reports success, +// rclcpp constructs a live event handler for it, and the failure only +// surfaces later - unhandled - when the executor registers its callback, +// crashing with "failed to set the on new message callback for Event". +TEST_F(EventTest, SubscriptionEventInitRejectsUnsupportedEventType) +{ + ASSERT_FALSE(rmw_event_type_is_supported(RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE)); + + rmw_event_t event = rmw_get_zero_initialized_event(); + EXPECT_EQ( + RMW_RET_UNSUPPORTED, + rmw_subscription_event_init(&event, sub, RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE)); +} + +TEST_F(EventTest, PublisherEventInitRejectsUnsupportedEventType) +{ + ASSERT_FALSE(rmw_event_type_is_supported(RMW_EVENT_OFFERED_QOS_INCOMPATIBLE)); + + rmw_event_t event = rmw_get_zero_initialized_event(); + EXPECT_EQ( + RMW_RET_UNSUPPORTED, + rmw_publisher_event_init(&event, pub, RMW_EVENT_OFFERED_QOS_INCOMPATIBLE)); +}