From 60f5faf702f1cd1086dd53bc4dd5da47f48ec76b Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Fri, 27 Oct 2017 23:20:59 +0800 Subject: [PATCH 1/2] * Fix the issues to dereference to nullptr Issue: the info in fail is always non-nullptr issue: no checking to memory allocation of rmw_client, which may leads to free nullptr in the following code: rmw_client_free(rmw_client); Issue: uninteded memory free from rmw_client->service_name: if (rmw_client->service_name != nullptr) { rmw_free(const_cast(rmw_client->service_name)); rmw_client->service_name = nullptr; } when the code goes to fail before it's actually allocated here: rmw_client->service_name = reinterpret_cast( rmw_allocate(strlen(service_name) + 1)); Signed-off-by: Ethan Gao --- rmw_fastrtps_cpp/src/rmw_client.cpp | 59 ++++++++++++++++------------- 1 file changed, 32 insertions(+), 27 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_client.cpp b/rmw_fastrtps_cpp/src/rmw_client.cpp index d2e0b5971..944ebb892 100644 --- a/rmw_fastrtps_cpp/src/rmw_client.cpp +++ b/rmw_fastrtps_cpp/src/rmw_client.cpp @@ -191,6 +191,11 @@ rmw_create_client( info->writer_guid_ = info->request_publisher_->getGuid(); rmw_client = rmw_client_allocate(); + if (!rmw_client) { + RMW_SET_ERROR_MSG("failed to allocate memory for client"); + goto fail; + } + rmw_client->implementation_identifier = eprosima_fastrtps_identifier; rmw_client->data = info; rmw_client->service_name = reinterpret_cast( @@ -204,42 +209,42 @@ rmw_create_client( return rmw_client; fail: + if (info->request_publisher_ != nullptr) { + Domain::removePublisher(info->request_publisher_); + } - if (info != nullptr) { - if (info->request_publisher_ != nullptr) { - Domain::removePublisher(info->request_publisher_); - } + if (info->response_subscriber_ != nullptr) { + Domain::removeSubscriber(info->response_subscriber_); + } - if (info->response_subscriber_ != nullptr) { - Domain::removeSubscriber(info->response_subscriber_); - } + if (info->listener_ != nullptr) { + delete info->listener_; + } - if (info->listener_ != nullptr) { - delete info->listener_; + if (impl) { + if (info->request_type_support_ != nullptr) { + _unregister_type(participant, info->request_type_support_, info->typesupport_identifier_); } - if (impl) { - if (info->request_type_support_ != nullptr) { - _unregister_type(participant, info->request_type_support_, info->typesupport_identifier_); - } - - if (info->response_type_support_ != nullptr) { - _unregister_type(participant, info->response_type_support_, info->typesupport_identifier_); - } - } else { - RCUTILS_LOG_ERROR_NAMED( - "rmw_fastrtps_cpp", - "leaking type support objects because node impl is null") + if (info->response_type_support_ != nullptr) { + _unregister_type(participant, info->response_type_support_, info->typesupport_identifier_); } - - delete info; + } else { + RCUTILS_LOG_ERROR_NAMED( + "rmw_fastrtps_cpp", + "leaking type support objects because node impl is null") } - if (rmw_client->service_name != nullptr) { - rmw_free(const_cast(rmw_client->service_name)); - rmw_client->service_name = nullptr; + delete info; + info = nullptr; + + if (nullptr != rmw_client) { + if (rmw_client->service_name != nullptr) { + rmw_free(const_cast(rmw_client->service_name)); + rmw_client->service_name = nullptr; + } + rmw_client_free(rmw_client); } - rmw_client_free(rmw_client); return nullptr; } From 4fff42124c5dc54456007251b23c04e049abb19f Mon Sep 17 00:00:00 2001 From: Ethan Gao Date: Mon, 30 Oct 2017 18:14:08 +0800 Subject: [PATCH 2/2] Restore the checking of info to avoid future earilier stepping into fail even though it's no possible to trigger it now Signed-off-by: Ethan Gao --- rmw_fastrtps_cpp/src/rmw_client.cpp | 46 +++++++++++++++-------------- 1 file changed, 24 insertions(+), 22 deletions(-) diff --git a/rmw_fastrtps_cpp/src/rmw_client.cpp b/rmw_fastrtps_cpp/src/rmw_client.cpp index 944ebb892..9b5b5aef4 100644 --- a/rmw_fastrtps_cpp/src/rmw_client.cpp +++ b/rmw_fastrtps_cpp/src/rmw_client.cpp @@ -209,34 +209,36 @@ rmw_create_client( return rmw_client; fail: - if (info->request_publisher_ != nullptr) { - Domain::removePublisher(info->request_publisher_); - } - - if (info->response_subscriber_ != nullptr) { - Domain::removeSubscriber(info->response_subscriber_); - } + if (info != nullptr) { + if (info->request_publisher_ != nullptr) { + Domain::removePublisher(info->request_publisher_); + } - if (info->listener_ != nullptr) { - delete info->listener_; - } + if (info->response_subscriber_ != nullptr) { + Domain::removeSubscriber(info->response_subscriber_); + } - if (impl) { - if (info->request_type_support_ != nullptr) { - _unregister_type(participant, info->request_type_support_, info->typesupport_identifier_); + if (info->listener_ != nullptr) { + delete info->listener_; } - if (info->response_type_support_ != nullptr) { - _unregister_type(participant, info->response_type_support_, info->typesupport_identifier_); + if (impl) { + if (info->request_type_support_ != nullptr) { + _unregister_type(participant, info->request_type_support_, info->typesupport_identifier_); + } + + if (info->response_type_support_ != nullptr) { + _unregister_type(participant, info->response_type_support_, info->typesupport_identifier_); + } + } else { + RCUTILS_LOG_ERROR_NAMED( + "rmw_fastrtps_cpp", + "leaking type support objects because node impl is null") } - } else { - RCUTILS_LOG_ERROR_NAMED( - "rmw_fastrtps_cpp", - "leaking type support objects because node impl is null") - } - delete info; - info = nullptr; + delete info; + info = nullptr; + } if (nullptr != rmw_client) { if (rmw_client->service_name != nullptr) {