From 177451a0a200e3327afe9029b5cc405731f527f5 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 8 Jan 2024 21:44:36 +0000 Subject: [PATCH 1/6] Use a scope_exit to drop the keystr. This just makes sure we always clean it up. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/detail/rmw_data_types.cpp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/rmw_zenoh_cpp/src/detail/rmw_data_types.cpp b/rmw_zenoh_cpp/src/detail/rmw_data_types.cpp index 1fd9cc06..57d6b3da 100644 --- a/rmw_zenoh_cpp/src/detail/rmw_data_types.cpp +++ b/rmw_zenoh_cpp/src/detail/rmw_data_types.cpp @@ -17,6 +17,7 @@ #include #include +#include "rcpputils/scope_exit.hpp" #include "rcutils/logging_macros.h" #include "rmw_data_types.hpp" @@ -34,6 +35,10 @@ void sub_data_handler( void * data) { z_owned_str_t keystr = z_keyexpr_to_string(sample->keyexpr); + auto drop_keystr = rcpputils::make_scope_exit( + [&keystr]() { + z_drop(z_move(keystr)); + }); auto sub_data = static_cast(data); if (sub_data == nullptr) { @@ -74,8 +79,6 @@ void sub_data_handler( sub_data->condition->notify_one(); } } - - z_drop(z_move(keystr)); } @@ -93,6 +96,10 @@ void service_data_handler(const z_query_t * query, void * data) "[service_data_handler] triggered" ); z_owned_str_t keystr = z_keyexpr_to_string(z_query_keyexpr(query)); + auto drop_keystr = rcpputils::make_scope_exit( + [&keystr]() { + z_drop(z_move(keystr)); + }); rmw_service_data_t * service_data = static_cast(data); if (service_data == nullptr) { @@ -102,7 +109,6 @@ void service_data_handler(const z_query_t * query, void * data) "service for %s", z_loan(keystr) ); - z_drop(z_move(keystr)); return; } @@ -121,8 +127,6 @@ void service_data_handler(const z_query_t * query, void * data) service_data->condition->notify_one(); } } - - z_drop(z_move(keystr)); } //============================================================================== From 9d9f6f6597bf676cc7aaedeeb02fe23f63f43da4 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 8 Jan 2024 21:50:48 +0000 Subject: [PATCH 2/6] Rename find_type_support to find_message_type_support. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/rmw_zenoh.cpp | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/rmw_zenoh_cpp/src/rmw_zenoh.cpp b/rmw_zenoh_cpp/src/rmw_zenoh.cpp index 41163f41..64ac7593 100644 --- a/rmw_zenoh_cpp/src/rmw_zenoh.cpp +++ b/rmw_zenoh_cpp/src/rmw_zenoh.cpp @@ -90,7 +90,7 @@ z_owned_keyexpr_t ros_topic_name_to_zenoh_key( } //============================================================================== -const rosidl_message_type_support_t * find_type_support( +const rosidl_message_type_support_t * find_message_type_support( const rosidl_message_type_support_t * type_supports) { const rosidl_message_type_support_t * type_support = get_message_typesupport_handle( @@ -466,9 +466,9 @@ rmw_create_publisher( } // Get the RMW type support. - const rosidl_message_type_support_t * type_support = find_type_support(type_supports); + const rosidl_message_type_support_t * type_support = find_message_type_support(type_supports); if (type_support == nullptr) { - // error was already set by find_type_support + // error was already set by find_message_type_support return nullptr; } @@ -1032,9 +1032,9 @@ rmw_serialize( const rosidl_message_type_support_t * type_support, rmw_serialized_message_t * serialized_message) { - const rosidl_message_type_support_t * ts = find_type_support(type_support); + const rosidl_message_type_support_t * ts = find_message_type_support(type_support); if (ts == nullptr) { - // error was already set by find_type_support + // error was already set by find_message_type_support return RMW_RET_ERROR; } @@ -1067,9 +1067,9 @@ rmw_deserialize( const rosidl_message_type_support_t * type_support, void * ros_message) { - const rosidl_message_type_support_t * ts = find_type_support(type_support); + const rosidl_message_type_support_t * ts = find_message_type_support(type_support); if (ts == nullptr) { - // error was already set by find_type_support + // error was already set by find_message_type_support return RMW_RET_ERROR; } @@ -1147,9 +1147,9 @@ rmw_create_subscription( // return nullptr; // } - const rosidl_message_type_support_t * type_support = find_type_support(type_supports); + const rosidl_message_type_support_t * type_support = find_message_type_support(type_supports); if (type_support == nullptr) { - // error was already set by find_type_support + // error was already set by find_message_type_support return nullptr; } @@ -1759,7 +1759,7 @@ rmw_create_client( // Obtain the type support const rosidl_service_type_support_t * type_support = find_service_type_support(type_supports); if (type_support == nullptr) { - // error was already set by find_type_support + // error was already set by find_service_type_support return nullptr; } @@ -2186,7 +2186,7 @@ rmw_create_service( // Get the RMW type support. const rosidl_service_type_support_t * type_support = find_service_type_support(type_supports); if (type_support == nullptr) { - // error was already set by find_type_support + // error was already set by find_service_type_support return nullptr; } From ce6af2b52d67679705dbbb21b8216f2ae77b5b76 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 8 Jan 2024 23:05:32 +0000 Subject: [PATCH 3/6] Add error checking into ros_topic_name_to_zenoh_key Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/rmw_zenoh.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/rmw_zenoh_cpp/src/rmw_zenoh.cpp b/rmw_zenoh_cpp/src/rmw_zenoh.cpp index 64ac7593..aa796a69 100644 --- a/rmw_zenoh_cpp/src/rmw_zenoh.cpp +++ b/rmw_zenoh_cpp/src/rmw_zenoh.cpp @@ -82,6 +82,9 @@ z_owned_keyexpr_t ros_topic_name_to_zenoh_key( domain_ss << domain_id; char * stripped_topic_name = rcutils_strndup( &topic_name[start_offset], end_offset - start_offset, *allocator); + if (stripped_topic_name == nullptr) { + return z_keyexpr_null(); + } z_owned_keyexpr_t keyexpr = z_keyexpr_join( z_keyexpr(domain_ss.str().c_str()), z_keyexpr(stripped_topic_name)); allocator->deallocate(stripped_topic_name, allocator->state); From 7773aee1b9dbbad38c897527c25af04041c181b0 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Mon, 8 Jan 2024 23:15:12 +0000 Subject: [PATCH 4/6] Make sure to always free response_bytes. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/rmw_zenoh.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rmw_zenoh_cpp/src/rmw_zenoh.cpp b/rmw_zenoh_cpp/src/rmw_zenoh.cpp index aa796a69..bd11ba95 100644 --- a/rmw_zenoh_cpp/src/rmw_zenoh.cpp +++ b/rmw_zenoh_cpp/src/rmw_zenoh.cpp @@ -2474,9 +2474,12 @@ rmw_send_response( allocator->state)); if (!response_bytes) { RMW_SET_ERROR_MSG("failed allocate response message bytes"); - allocator->deallocate(response_bytes, allocator->state); return RMW_RET_ERROR; } + auto free_response_bytes = rcpputils::make_scope_exit( + [response_bytes, allocator]() { + allocator->deallocate(response_bytes, allocator->state); + }); // Object that manages the raw buffer eprosima::fastcdr::FastBuffer fastbuffer(response_bytes, max_data_length); @@ -2491,7 +2494,6 @@ rmw_send_response( ser, service_data->response_type_support_impl)) { - allocator->deallocate(response_bytes, allocator->state); return RMW_RET_ERROR; } From 805422742249132eb990e660fe1a234e85f05d95 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 9 Jan 2024 20:02:19 +0000 Subject: [PATCH 5/6] Remove unnecessary TODO comment. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/rmw_zenoh.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/rmw_zenoh_cpp/src/rmw_zenoh.cpp b/rmw_zenoh_cpp/src/rmw_zenoh.cpp index bd11ba95..f8c38509 100644 --- a/rmw_zenoh_cpp/src/rmw_zenoh.cpp +++ b/rmw_zenoh_cpp/src/rmw_zenoh.cpp @@ -2193,9 +2193,6 @@ rmw_create_service( return nullptr; } - // TODO(francocipollone): Verify if this is the right way to get the - // type support as the architecture for the service here - // is different to the one used in DDS (with fastcdr). auto service_members = static_cast(type_support->data); auto request_members = static_cast( service_members->request_members_->data); From 391cc397d8b8e1b6a296337feb00f591b9c6a571 Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Tue, 9 Jan 2024 20:27:22 +0000 Subject: [PATCH 6/6] Remember to free request_bytes. Signed-off-by: Chris Lalancette --- rmw_zenoh_cpp/src/rmw_zenoh.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rmw_zenoh_cpp/src/rmw_zenoh.cpp b/rmw_zenoh_cpp/src/rmw_zenoh.cpp index f8c38509..b5d216c2 100644 --- a/rmw_zenoh_cpp/src/rmw_zenoh.cpp +++ b/rmw_zenoh_cpp/src/rmw_zenoh.cpp @@ -1954,9 +1954,12 @@ rmw_send_request( allocator->state)); if (!request_bytes) { RMW_SET_ERROR_MSG("failed allocate request message bytes"); - allocator->deallocate(request_bytes, allocator->state); return RMW_RET_ERROR; } + auto free_request_bytes = rcpputils::make_scope_exit( + [request_bytes, allocator]() { + allocator->deallocate(request_bytes, allocator->state); + }); // Object that manages the raw buffer eprosima::fastcdr::FastBuffer fastbuffer(request_bytes, max_data_length); @@ -1971,7 +1974,6 @@ rmw_send_request( ser, client_data->request_type_support_impl)) { - allocator->deallocate(request_bytes, allocator->state); return RMW_RET_ERROR; }