Fix destroying safety (#77)

pablogs9 · web-flow · commit b88b026d668a · 2020-09-14T12:57:53.000+02:00
* Pre-regresion

* Regresion fixed

* Fix

* Renaming
diff --git a/rmw_microxrcedds_c/src/rmw_client.c b/rmw_microxrcedds_c/src/rmw_client.c
@@ -40,7 +40,7 @@ rmw_create_client(
     RMW_SET_ERROR_MSG("node handle is null");
   } else if (!type_support) {
     RMW_SET_ERROR_MSG("type support is null");
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
   } else if (!service_name || strlen(service_name) == 0) {
     RMW_SET_ERROR_MSG("service name is null or empty string");
@@ -183,7 +183,7 @@ rmw_destroy_client(
   if (!node) {
     RMW_SET_ERROR_MSG("node handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!node->data) {
@@ -192,10 +192,7 @@ rmw_destroy_client(
   } else if (!client) {
     RMW_SET_ERROR_MSG("client handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      client->implementation_identifier,                // NOLINT
-      rmw_get_implementation_identifier()) != 0)
-  {
+  } else if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {
     RMW_SET_ERROR_MSG("client handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!client->data) {
diff --git a/rmw_microxrcedds_c/src/rmw_node.c b/rmw_microxrcedds_c/src/rmw_node.c
@@ -148,7 +148,7 @@ rmw_ret_t rmw_destroy_node(rmw_node_t * node)
     return RMW_RET_ERROR;
   }
 
-  if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return RMW_RET_ERROR;
   }
diff --git a/rmw_microxrcedds_c/src/rmw_publish.c b/rmw_microxrcedds_c/src/rmw_publish.c
@@ -33,9 +33,7 @@ rmw_publish(
   } else if (!ros_message) {
     RMW_SET_ERROR_MSG("ros_message pointer is null");
     ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      publisher->implementation_identifier,
-      rmw_get_implementation_identifier()) != 0)
+  } else if (!is_uxrce_rmw_identifier_valid(publisher->implementation_identifier))
   {
     RMW_SET_ERROR_MSG("publisher handle not from this implementation");
     ret = RMW_RET_ERROR;
diff --git a/rmw_microxrcedds_c/src/rmw_publisher.c b/rmw_microxrcedds_c/src/rmw_publisher.c
@@ -67,7 +67,7 @@ rmw_create_publisher(
     RMW_SET_ERROR_MSG("node handle is null");
   } else if (!type_support) {
     RMW_SET_ERROR_MSG("type support is null");
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
   } else if (!topic_name || strlen(topic_name) == 0) {
     RMW_SET_ERROR_MSG("publisher topic is null or empty string");
@@ -288,7 +288,7 @@ rmw_destroy_publisher(
   if (!node) {
     RMW_SET_ERROR_MSG("node handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!node->data) {
@@ -297,9 +297,7 @@ rmw_destroy_publisher(
   } else if (!publisher) {
     RMW_SET_ERROR_MSG("publisher handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      publisher->implementation_identifier,                // NOLINT
-      rmw_get_implementation_identifier()) != 0)
+  } else if (!is_uxrce_rmw_identifier_valid(publisher->implementation_identifier))
   {
     RMW_SET_ERROR_MSG("publisher handle not from this implementation");
     result_ret = RMW_RET_ERROR;
diff --git a/rmw_microxrcedds_c/src/rmw_request.c b/rmw_microxrcedds_c/src/rmw_request.c
@@ -25,7 +25,7 @@ rmw_send_request(
 {
   EPROS_PRINT_TRACE();
 
-  if (strcmp(client->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;
   }
@@ -74,7 +74,7 @@ rmw_take_request(
     *taken = false;
   }
 
-  if (strcmp(service->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;
   }
diff --git a/rmw_microxrcedds_c/src/rmw_response.c b/rmw_microxrcedds_c/src/rmw_response.c
@@ -25,7 +25,7 @@ rmw_send_response(
 {
   EPROS_PRINT_TRACE();
 
-  if (strcmp(service->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;
   }
@@ -77,7 +77,7 @@ rmw_take_response(
     *taken = false;
   }
 
-  if (strcmp(client->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;
   }
diff --git a/rmw_microxrcedds_c/src/rmw_service.c b/rmw_microxrcedds_c/src/rmw_service.c
@@ -40,7 +40,7 @@ rmw_create_service(
     RMW_SET_ERROR_MSG("node handle is null");
   } else if (!type_support) {
     RMW_SET_ERROR_MSG("type support is null");
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
   } else if (!service_name || strlen(service_name) == 0) {
     RMW_SET_ERROR_MSG("service name is null or empty string");
@@ -180,7 +180,7 @@ rmw_destroy_service(
   if (!node) {
     RMW_SET_ERROR_MSG("node handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!node->data) {
@@ -189,10 +189,7 @@ rmw_destroy_service(
   } else if (!service) {
     RMW_SET_ERROR_MSG("service handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      service->implementation_identifier,                // NOLINT
-      rmw_get_implementation_identifier()) != 0)
-  {
+  } else if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {
     RMW_SET_ERROR_MSG("service handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!service->data) {
diff --git a/rmw_microxrcedds_c/src/rmw_subscription.c b/rmw_microxrcedds_c/src/rmw_subscription.c
@@ -64,7 +64,7 @@ rmw_create_subscription(
     RMW_SET_ERROR_MSG("node handle is null");
   } else if (!type_support) {
     RMW_SET_ERROR_MSG("type support is null");
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
   } else if (!topic_name || strlen(topic_name) == 0) {
     RMW_SET_ERROR_MSG("subscription topic is null or empty string");
@@ -267,7 +267,7 @@ rmw_destroy_subscription(rmw_node_t * node, rmw_subscription_t * subscription)
   if (!node) {
     RMW_SET_ERROR_MSG("node handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  } else if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!node->data) {
@@ -276,10 +276,7 @@ rmw_destroy_subscription(rmw_node_t * node, rmw_subscription_t * subscription)
   } else if (!subscription) {
     RMW_SET_ERROR_MSG("subscription handle is null");
     result_ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      subscription->implementation_identifier,                // NOLINT
-      rmw_get_implementation_identifier()) != 0)
-  {
+  } else if (!is_uxrce_rmw_identifier_valid(subscription->implementation_identifier)) {
     RMW_SET_ERROR_MSG("subscription handle not from this implementation");
     result_ret = RMW_RET_ERROR;
   } else if (!subscription->data) {
diff --git a/rmw_microxrcedds_c/src/rmw_take.c b/rmw_microxrcedds_c/src/rmw_take.c
@@ -44,7 +44,7 @@ rmw_take_with_info(
     *taken = false;
   }
 
-  if (strcmp(subscription->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(subscription->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_ERROR;
   }
@@ -97,7 +97,7 @@ rmw_take_sequence(
 
   *taken = 0;
 
-  if (strcmp(subscription->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(subscription->implementation_identifier)) {
     RMW_SET_ERROR_MSG("Wrong implementation");
     return RMW_RET_ERROR;
   }
diff --git a/rmw_microxrcedds_c/src/rmw_trigger_guard_condition.c b/rmw_microxrcedds_c/src/rmw_trigger_guard_condition.c
@@ -26,10 +26,7 @@ rmw_trigger_guard_condition(const rmw_guard_condition_t * guard_condition)
   if (!guard_condition) {
     RMW_SET_ERROR_MSG("guard condition pointer is null");
     ret = RMW_RET_ERROR;
-  } else if (strcmp(
-      guard_condition->implementation_identifier,
-      rmw_get_implementation_identifier()) != 0)
-  {
+  } else if (!is_uxrce_rmw_identifier_valid(guard_condition->implementation_identifier)) {
     RMW_SET_ERROR_MSG("guard condition handle not from this implementation");
     ret = RMW_RET_ERROR;
   } else {
diff --git a/rmw_microxrcedds_c/src/types.c b/rmw_microxrcedds_c/src/types.c
@@ -198,7 +198,7 @@ void rmw_uxrce_fini_session_memory(rmw_context_impl_t * session)
 
 void rmw_uxrce_fini_node_memory(rmw_node_t * node)
 {
-  if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return;
   }
@@ -227,7 +227,7 @@ void rmw_uxrce_fini_node_memory(rmw_node_t * node)
 
 void rmw_uxrce_fini_publisher_memory(rmw_publisher_t * publisher)
 {
-  if (strcmp(publisher->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(publisher->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return;
   }
@@ -251,7 +251,7 @@ void rmw_uxrce_fini_publisher_memory(rmw_publisher_t * publisher)
 
 void rmw_uxrce_fini_subscription_memory(rmw_subscription_t * subscriber)
 {
-  if (strcmp(subscriber->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(subscriber->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return;
   }
@@ -274,7 +274,7 @@ void rmw_uxrce_fini_subscription_memory(rmw_subscription_t * subscriber)
 
 void rmw_uxrce_fini_service_memory(rmw_service_t * service)
 {
-  if (strcmp(service->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return;
   }
@@ -296,7 +296,7 @@ void rmw_uxrce_fini_service_memory(rmw_service_t * service)
 
 void rmw_uxrce_fini_client_memory(rmw_client_t * client)
 {
-  if (strcmp(client->implementation_identifier, rmw_get_implementation_identifier()) != 0) {
+  if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {
     RMW_SET_ERROR_MSG("node handle not from this implementation");
     return;
   }
diff --git a/rmw_microxrcedds_c/src/utils.c b/rmw_microxrcedds_c/src/utils.c
@@ -369,3 +369,9 @@ bool build_datareader_profile(const char * topic_name, char profile_name[], size
   ret = (written > 0) && (written < (int)buffer_size);
   return ret;
 }
+
+bool is_uxrce_rmw_identifier_valid(const char * id)
+{
+  return id != NULL &&
+         strcmp(id, rmw_get_implementation_identifier()) == 0;
+}
diff --git a/rmw_microxrcedds_c/src/utils.h b/rmw_microxrcedds_c/src/utils.h
@@ -53,4 +53,6 @@ bool build_topic_profile(const char * topic_name, char profile_name[], size_t bu
 bool build_datawriter_profile(const char * topic_name, char profile_name[], size_t buffer_size);
 bool build_datareader_profile(const char * topic_name, char profile_name[], size_t buffer_size);
 
+
+bool is_uxrce_rmw_identifier_valid(const char * id);
 #endif  // UTILS_H_
diff --git a/rmw_microxrcedds_c/test/test_node.cpp b/rmw_microxrcedds_c/test/test_node.cpp
@@ -102,6 +102,11 @@ TEST_F(TestNode, memory_poll) {
     ret = rmw_destroy_node(nodes.at(i));
     ASSERT_EQ(ret, RMW_RET_OK);
   }
+  
+  // Destroy an already detroyed node
+  ret = rmw_destroy_node(nodes.at(0));
+  ASSERT_EQ(ret, RMW_RET_ERROR);
+
   nodes.clear();
 
 }
diff --git a/rmw_microxrcedds_c/test/test_publisher.cpp b/rmw_microxrcedds_c/test/test_publisher.cpp
@@ -263,6 +263,11 @@ TEST_F(TestPublisher, memory_poll_shared_topic) {
       ret = rmw_destroy_publisher(this->node, publisher);
       ASSERT_EQ(ret, RMW_RET_OK);
     }
+
+  // Destroy an already detroyed publisher
+  ret = rmw_destroy_publisher(this->node, publishers.at(0));
+  ASSERT_EQ(ret, RMW_RET_ERROR);
+
     publishers.clear();
   }
 }
diff --git a/rmw_microxrcedds_c/test/test_subscription.cpp b/rmw_microxrcedds_c/test/test_subscription.cpp
@@ -259,6 +259,11 @@ TEST_F(TestSubscription, memory_poll_shared_topic) {
       ret = rmw_destroy_subscription(this->node, subscription);
       ASSERT_EQ(ret, RMW_RET_OK);
     }
+
+    // Destroy an already detroyed subscriber
+    ret = rmw_destroy_subscription(this->node, subscriptions.at(0));
+    ASSERT_EQ(ret, RMW_RET_ERROR);
+
     subscriptions.clear();
   }
 }
diff --git a/rmw_microxrcedds_c/test/test_topic.cpp b/rmw_microxrcedds_c/test/test_topic.cpp
@@ -121,6 +121,11 @@ TEST_F(TestTopic, multiple_topic_creation) {
     rmw_ret_t ret = destroy_topic(created_topics.at(i));
     ASSERT_EQ(ret, RMW_RET_OK);
   }
+
+  // Destroy an already destroyed topic
+  rmw_ret_t ret = destroy_topic(created_topics.at(0));
+  ASSERT_EQ(ret, RMW_RET_ERROR);
+
   // TODO(pablogs9): Topic must be related to publisher in order to be counted
   // ASSERT_EQ(topic_count(reinterpret_cast<struct rmw_uxrce_node_t *>(node->data)), 0);
 }

Original file line number	Diff line number	Diff line change
`@@ -148,7 +148,7 @@ rmw_ret_t rmw_destroy_node(rmw_node_t * node)`
`148`	`148`	`return RMW_RET_ERROR;`
`149`	`149`	`}`
`150`	`150`
`151`		`- if (strcmp(node->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`151`	`+ if (!is_uxrce_rmw_identifier_valid(node->implementation_identifier)) {`
`152`	`152`	`RMW_SET_ERROR_MSG("node handle not from this implementation");`
`153`	`153`	`return RMW_RET_ERROR;`
`154`	`154`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ rmw_send_request(`
`25`	`25`	`{`
`26`	`26`	`EPROS_PRINT_TRACE();`
`27`	`27`
`28`		`- if (strcmp(client->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`28`	`+ if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {`
`29`	`29`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`30`	`30`	`return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;`
`31`	`31`	`}`
`@@ -74,7 +74,7 @@ rmw_take_request(`
`74`	`74`	`*taken = false;`
`75`	`75`	`}`
`76`	`76`
`77`		`- if (strcmp(service->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`77`	`+ if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {`
`78`	`78`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`79`	`79`	`return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;`
`80`	`80`	`}`
Original file line number	Diff line number	Diff line change
`@@ -25,7 +25,7 @@ rmw_send_response(`
`25`	`25`	`{`
`26`	`26`	`EPROS_PRINT_TRACE();`
`27`	`27`
`28`		`- if (strcmp(service->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`28`	`+ if (!is_uxrce_rmw_identifier_valid(service->implementation_identifier)) {`
`29`	`29`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`30`	`30`	`return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;`
`31`	`31`	`}`
`@@ -77,7 +77,7 @@ rmw_take_response(`
`77`	`77`	`*taken = false;`
`78`	`78`	`}`
`79`	`79`
`80`		`- if (strcmp(client->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`80`	`+ if (!is_uxrce_rmw_identifier_valid(client->implementation_identifier)) {`
`81`	`81`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`82`	`82`	`return RMW_RET_INCORRECT_RMW_IMPLEMENTATION;`
`83`	`83`	`}`
Original file line number	Diff line number	Diff line change
`@@ -44,7 +44,7 @@ rmw_take_with_info(`
`44`	`44`	`*taken = false;`
`45`	`45`	`}`
`46`	`46`
`47`		`- if (strcmp(subscription->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`47`	`+ if (!is_uxrce_rmw_identifier_valid(subscription->implementation_identifier)) {`
`48`	`48`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`49`	`49`	`return RMW_RET_ERROR;`
`50`	`50`	`}`
`@@ -97,7 +97,7 @@ rmw_take_sequence(`
`97`	`97`
`98`	`98`	`*taken = 0;`
`99`	`99`
`100`		`- if (strcmp(subscription->implementation_identifier, rmw_get_implementation_identifier()) != 0) {`
	`100`	`+ if (!is_uxrce_rmw_identifier_valid(subscription->implementation_identifier)) {`
`101`	`101`	`RMW_SET_ERROR_MSG("Wrong implementation");`
`102`	`102`	`return RMW_RET_ERROR;`
`103`	`103`	`}`