diff --git a/CHANGELOG.md b/CHANGELOG.md index 9cf8f0d..0e92de3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/rmw_unix_socket_cpp/src/rmw_event.cpp b/rmw_unix_socket_cpp/src/rmw_event.cpp index a8d0d79..f37bbef 100644 --- a/rmw_unix_socket_cpp/src/rmw_event.cpp +++ b/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; } diff --git a/rmw_unix_socket_cpp/src/rmw_subscription.cpp b/rmw_unix_socket_cpp/src/rmw_subscription.cpp index dc17142..f56c19d 100644 --- a/rmw_unix_socket_cpp/src/rmw_subscription.cpp +++ b/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(subscription->data); std::lock_guard lock(sub_data->callback_mutex); diff --git a/rmw_unix_socket_cpp/test/test_rmw_event.cpp b/rmw_unix_socket_cpp/test/test_rmw_event.cpp index 1186bbb..8e21960 100644 --- a/rmw_unix_socket_cpp/test/test_rmw_event.cpp +++ b/rmw_unix_socket_cpp/test/test_rmw_event.cpp @@ -14,10 +14,12 @@ #include "test_base.hpp" +#include #include #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(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(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(); }