fix(rmw_event): validate handles and set an error message on the failure paths

CHANGELOG.md

@@ -5,6 +5,17 @@ All notable changes to `rmw_unix_socket_cpp` are documented here.
 The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Fixed
+
+- **QoS status events are refused honestly.** `rmw_event_set_callback` returned
+  a failure with no error message behind it — the `": error not set"` half of
+  the original `EventsExecutor` crash report. `rmw_take_event`,
+  `rmw_event_set_callback`, both `*_event_init` functions and
+  `rmw_subscription_set_on_new_message_callback` were also missing the
+  `RMW_CHECK_TYPE_IDENTIFIERS_MATCH` every other entry point here already had.
+
 ## [0.5.0] - 2026-08-27
 
 The wait/wakeup release. The 200 ms `rmw_wait` poll is gone, replaced by an

rmw_unix_socket_cpp/src/rmw_event.cpp

@@ -15,6 +15,7 @@
 #include "identifier.hpp"
 #include "types.hpp"
 
+#include "rmw/check_type_identifiers_match.h"
 #include "rmw/error_handling.h"
 #include "rmw/event.h"
 #include "rmw/rmw.h"
@@ -29,6 +30,9 @@ 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);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    publisher, publisher->implementation_identifier,
+    rmw_uds::identifier, return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
   if (!rmw_event_type_is_supported(event_type)) {
     RMW_SET_ERROR_MSG("event type not supported");
     return RMW_RET_UNSUPPORTED;
@@ -48,6 +52,9 @@ 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);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    subscription, subscription->implementation_identifier,
+    rmw_uds::identifier, return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
   if (!rmw_event_type_is_supported(event_type)) {
     RMW_SET_ERROR_MSG("event type not supported");
     return RMW_RET_UNSUPPORTED;
@@ -63,21 +70,40 @@ rmw_ret_t rmw_take_event(
   void * event_info,
   bool * taken)
 {
-  (void)event_handle;
-  (void)event_info;
+  RMW_CHECK_ARGUMENT_FOR_NULL(event_handle, RMW_RET_INVALID_ARGUMENT);
   RMW_CHECK_ARGUMENT_FOR_NULL(taken, RMW_RET_INVALID_ARGUMENT);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    event_handle, event_handle->implementation_identifier,
+    rmw_uds::identifier, return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
+
+  // No event type is supported, so there is never anything to take. Leave
+  // `event_info` untouched: a taker may only write into the caller's status
+  // struct when it reports `taken`, and rcl reuses that buffer across calls.
+  (void)event_info;
   *taken = false;
   return RMW_RET_OK;
 }
 
 rmw_ret_t rmw_event_fini(rmw_event_t * event)
 {
+  // Left unvalidated on purpose. rcl_event_fini skips this call entirely when
+  // event->impl is NULL, which is exactly what rcl_*_event_init leaves behind
+  // after it frees impl on our RMW_RET_UNSUPPORTED - so a rejected init never
+  // reaches here. There is nothing to release either: rmw_event_t::data
+  // aliases the endpoint's impl struct, owned by rmw_destroy_publisher /
+  // rmw_destroy_subscription.
   (void)event;
   return RMW_RET_OK;
 }
 
 bool rmw_event_type_is_supported(rmw_event_type_t event_type)
 {
+  // No QoS status event is reported. matched, incompatible-QoS and
+  // incompatible-type would need cross-process endpoint diffing the registry
+  // generation counter does not provide, and deadline and liveliness need a
+  // periodic timer there is no thread to run (see DESIGN.md, "No background
+  // threads, and why"). Reporting false here is what makes *_event_init reject
+  // the type, which is the only refusal rclcpp handles cleanly.
   (void)event_type;
   return false;
 }
@@ -87,9 +113,19 @@ rmw_ret_t rmw_event_set_callback(
   rmw_event_callback_t callback,
   const void * user_data)
 {
-  (void)event;
+  RMW_CHECK_ARGUMENT_FOR_NULL(event, RMW_RET_INVALID_ARGUMENT);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    event, event->implementation_identifier,
+    rmw_uds::identifier, return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
   (void)callback;
   (void)user_data;
+
+  // Unreachable through rclcpp, which cannot hold an initialized event handle
+  // now that *_event_init rejects every type. Set an error message anyway:
+  // returning a failure without one is what made the original EventsExecutor
+  // crash unreadable ("failed to set the on new message callback for Event:
+  // error not set").
+  RMW_SET_ERROR_MSG("event callbacks are not supported: no event type is reported");
   return RMW_RET_UNSUPPORTED;
 }
 

rmw_unix_socket_cpp/src/rmw_subscription.cpp

@@ -700,6 +700,9 @@ rmw_ret_t rmw_subscription_set_on_new_message_callback(
   const void * user_data)
 {
   RMW_CHECK_ARGUMENT_FOR_NULL(subscription, RMW_RET_INVALID_ARGUMENT);
+  RMW_CHECK_TYPE_IDENTIFIERS_MATCH(
+    subscription, subscription->implementation_identifier,
+    rmw_uds::identifier, return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
 
   auto * sub_data = static_cast<rmw_uds::UdsSubscription *>(subscription->data);
   std::lock_guard<std::mutex> lock(sub_data->callback_mutex);

rmw_unix_socket_cpp/test/test_rmw_event.cpp

@@ -14,10 +14,12 @@
 
 #include "test_base.hpp"
 
+#include <cstdint>
 #include <cstring>
 
 #include "test_msgs/msg/basic_types.hpp"
 
+#include "rmw/error_handling.h"
 #include "rmw/event.h"
 #include "rmw/qos_profiles.h"
 #include "rosidl_typesupport_cpp/message_type_support.hpp"
@@ -54,8 +56,21 @@ class EventTest : public RmwUdsNodeTest
   {
     if (sub) { auto _r [[maybe_unused]] = rmw_destroy_subscription(node, sub); }
     if (pub) { auto _r [[maybe_unused]] = rmw_destroy_publisher(node, pub); }
+    rmw_reset_error();
     RmwUdsNodeTest::TearDown();
   }
+
+  // An event handle stamped as ours, for the entry points that validate one.
+  // *_event_init() cannot produce it: it rejects every event type, which is
+  // the behavior the tests below pin.
+  rmw_event_t our_event(rmw_event_type_t type) const
+  {
+    rmw_event_t event = rmw_get_zero_initialized_event();
+    event.implementation_identifier = uds_id();
+    event.data = sub->data;
+    event.event_type = type;
+    return event;
+  }
 };
 
 // rmw_event_type_is_supported() reports no event types as supported, so
@@ -65,22 +80,162 @@ class EventTest : public RmwUdsNodeTest
 // 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)
+//
+// Iterating the whole enum rather than naming types keeps this honest across
+// the distro matrix: rmw_event_type_t has no explicit initializers, so both
+// the ordinals and RMW_EVENT_INVALID shift as upstream adds event types.
+TEST_F(EventTest, SubscriptionEventInitRejectsEveryEventType)
+{
+  for (int i = 0; i <= RMW_EVENT_INVALID; ++i) {
+    const auto type = static_cast<rmw_event_type_t>(i);
+    ASSERT_FALSE(rmw_event_type_is_supported(type)) << "event type " << i;
+
+    rmw_event_t event = rmw_get_zero_initialized_event();
+    EXPECT_EQ(RMW_RET_UNSUPPORTED, rmw_subscription_event_init(&event, sub, type))
+      << "event type " << i;
+    // rcl logs whatever the RMW left behind, so a bare return code is a
+    // dead end for anyone debugging the rejection.
+    EXPECT_TRUE(rmw_error_is_set()) << "event type " << i;
+    rmw_reset_error();
+  }
+}
+
+TEST_F(EventTest, PublisherEventInitRejectsEveryEventType)
 {
-  ASSERT_FALSE(rmw_event_type_is_supported(RMW_EVENT_REQUESTED_QOS_INCOMPATIBLE));
+  for (int i = 0; i <= RMW_EVENT_INVALID; ++i) {
+    const auto type = static_cast<rmw_event_type_t>(i);
+    ASSERT_FALSE(rmw_event_type_is_supported(type)) << "event type " << i;
+
+    rmw_event_t event = rmw_get_zero_initialized_event();
+    EXPECT_EQ(RMW_RET_UNSUPPORTED, rmw_publisher_event_init(&event, pub, type))
+      << "event type " << i;
+    EXPECT_TRUE(rmw_error_is_set()) << "event type " << i;
+    rmw_reset_error();
+  }
+}
 
+// A rejected init leaves the handle exactly as rmw_get_zero_initialized_event()
+// made it, so a failed EventHandler construction cannot leave rcl holding a
+// handle that looks like ours.
+TEST_F(EventTest, RejectedEventInitLeavesTheHandleZeroInitialized)
+{
   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));
+  ASSERT_EQ(RMW_RET_UNSUPPORTED, rmw_subscription_event_init(&event, sub, RMW_EVENT_MESSAGE_LOST));
+  rmw_reset_error();
+
+  EXPECT_EQ(nullptr, event.implementation_identifier);
+  EXPECT_EQ(nullptr, event.data);
 }
 
-TEST_F(EventTest, PublisherEventInitRejectsUnsupportedEventType)
+TEST_F(EventTest, EventInitRejectsForeignEndpoints)
 {
-  ASSERT_FALSE(rmw_event_type_is_supported(RMW_EVENT_OFFERED_QOS_INCOMPATIBLE));
+  rmw_subscription_t foreign_sub = *sub;
+  foreign_sub.implementation_identifier = "rmw_bogus_cpp";
+  rmw_event_t event = rmw_get_zero_initialized_event();
+  EXPECT_EQ(
+    RMW_RET_INCORRECT_RMW_IMPLEMENTATION,
+    rmw_subscription_event_init(&event, &foreign_sub, RMW_EVENT_MESSAGE_LOST));
+  rmw_reset_error();
 
+  rmw_publisher_t foreign_pub = *pub;
+  foreign_pub.implementation_identifier = "rmw_bogus_cpp";
+  event = rmw_get_zero_initialized_event();
+  EXPECT_EQ(
+    RMW_RET_INCORRECT_RMW_IMPLEMENTATION,
+    rmw_publisher_event_init(&event, &foreign_pub, RMW_EVENT_LIVELINESS_LOST));
+  rmw_reset_error();
+}
+
+TEST_F(EventTest, EventInitRejectsNullArguments)
+{
   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));
+    RMW_RET_INVALID_ARGUMENT,
+    rmw_subscription_event_init(nullptr, sub, RMW_EVENT_MESSAGE_LOST));
+  rmw_reset_error();
+  EXPECT_EQ(
+    RMW_RET_INVALID_ARGUMENT,
+    rmw_subscription_event_init(&event, nullptr, RMW_EVENT_MESSAGE_LOST));
+  rmw_reset_error();
+  EXPECT_EQ(
+    RMW_RET_INVALID_ARGUMENT,
+    rmw_publisher_event_init(nullptr, pub, RMW_EVENT_LIVELINESS_LOST));
+  rmw_reset_error();
+  EXPECT_EQ(
+    RMW_RET_INVALID_ARGUMENT,
+    rmw_publisher_event_init(&event, nullptr, RMW_EVENT_LIVELINESS_LOST));
+  rmw_reset_error();
+}
+
+// rmw_take_event() may only write into the caller's status struct when it
+// reports taken. rcl reuses that buffer across calls, so scribbling in it on
+// the nothing-taken path would hand the application a stale status it never
+// asked for.
+TEST_F(EventTest, TakeEventTakesNothingAndLeavesEventInfoUntouched)
+{
+  rmw_event_t event = our_event(RMW_EVENT_MESSAGE_LOST);
+
+  uint8_t info[128];
+  std::memset(info, 0xAB, sizeof(info));
+  uint8_t expected[128];
+  std::memset(expected, 0xAB, sizeof(expected));
+
+  bool taken = true;
+  EXPECT_EQ(RMW_RET_OK, rmw_take_event(&event, info, &taken));
+  EXPECT_FALSE(taken);
+  EXPECT_EQ(0, std::memcmp(info, expected, sizeof(info)));
+}
+
+TEST_F(EventTest, TakeEventRejectsBadArguments)
+{
+  rmw_event_t event = our_event(RMW_EVENT_MESSAGE_LOST);
+  uint8_t info[16] = {};
+  bool taken = false;
+
+  EXPECT_EQ(RMW_RET_INVALID_ARGUMENT, rmw_take_event(nullptr, info, &taken));
+  rmw_reset_error();
+  EXPECT_EQ(RMW_RET_INVALID_ARGUMENT, rmw_take_event(&event, info, nullptr));
+  rmw_reset_error();
+
+  rmw_event_t foreign = event;
+  foreign.implementation_identifier = "rmw_bogus_cpp";
+  EXPECT_EQ(RMW_RET_INCORRECT_RMW_IMPLEMENTATION, rmw_take_event(&foreign, info, &taken));
+  rmw_reset_error();
+}
+
+// rmw_event_fini has nothing to validate and nothing to free. rcl_event_fini
+// skips it whenever event->impl is NULL, and that is precisely what
+// rcl_*_event_init leaves behind when it frees impl on our
+// RMW_RET_UNSUPPORTED - so no handle we could reject ever arrives.
+TEST_F(EventTest, EventFiniIsANoOp)
+{
+  rmw_event_t zero = rmw_get_zero_initialized_event();
+  EXPECT_EQ(RMW_RET_OK, rmw_event_fini(&zero));
+
+  rmw_event_t ours = our_event(RMW_EVENT_MESSAGE_LOST);
+  EXPECT_EQ(RMW_RET_OK, rmw_event_fini(&ours));
+
+  EXPECT_FALSE(rmw_error_is_set());
+}
+
+// The original EventsExecutor crash surfaced as "failed to set the on new
+// message callback for Event: error not set" - the ": error not set" half
+// being this function returning a failure with no message behind it.
+TEST_F(EventTest, EventSetCallbackReportsUnsupportedWithAMessage)
+{
+  rmw_event_t event = our_event(RMW_EVENT_MESSAGE_LOST);
+  EXPECT_EQ(RMW_RET_UNSUPPORTED, rmw_event_set_callback(&event, nullptr, nullptr));
+  EXPECT_TRUE(rmw_error_is_set());
+  rmw_reset_error();
+
+  EXPECT_EQ(RMW_RET_INVALID_ARGUMENT, rmw_event_set_callback(nullptr, nullptr, nullptr));
+  rmw_reset_error();
+
+  // The identifier check the other event entry points already had.
+  rmw_event_t foreign = our_event(RMW_EVENT_MESSAGE_LOST);
+  foreign.implementation_identifier = "rmw_bogus_cpp";
+  EXPECT_EQ(
+    RMW_RET_INCORRECT_RMW_IMPLEMENTATION,
+    rmw_event_set_callback(&foreign, nullptr, nullptr));
+  rmw_reset_error();
 }