Address issue 716 by zero initializing pointers and freeing memory (#717) (#820)

Signed-off-by: Stephen Brawner <brawner@gmail.com>

rcl_action/src/rcl_action/action_client.c

@@ -46,6 +46,58 @@ rcl_action_get_zero_initialized_client(void)
   return null_action_client;
 }
 
+rcl_action_client_impl_t
+_rcl_action_get_zero_initialized_client_impl(void)
+{
+  rcl_client_t null_client = rcl_get_zero_initialized_client();
+  rcl_subscription_t null_subscription = rcl_get_zero_initialized_subscription();
+  rcl_action_client_impl_t null_action_client = {
+    null_client,
+    null_client,
+    null_client,
+    null_subscription,
+    null_subscription,
+    rcl_action_client_get_default_options(),
+    NULL,
+    0,
+    0,
+    0,
+    0,
+    0
+  };
+  return null_action_client;
+}
+
+rcl_ret_t
+_rcl_action_client_fini_impl(
+  rcl_action_client_t * action_client, rcl_node_t * node, rcl_allocator_t allocator)
+{
+  if (NULL == action_client->impl) {
+    return RCL_RET_OK;
+  }
+  rcl_ret_t ret = RCL_RET_OK;
+  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) {
+    ret = RCL_RET_ERROR;
+  }
+  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) {
+    ret = RCL_RET_ERROR;
+  }
+  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) {
+    ret = RCL_RET_ERROR;
+  }
+  if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) {
+    ret = RCL_RET_ERROR;
+  }
+  if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) {
+    ret = RCL_RET_ERROR;
+  }
+  allocator.deallocate(action_client->impl->action_name, allocator.state);
+  allocator.deallocate(action_client->impl, allocator.state);
+  action_client->impl = NULL;
+  RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized");
+  return ret;
+}
+
 // \internal Initializes an action client specific service client.
 #define CLIENT_INIT(Type) \
   char * Type ## _service_name = NULL; \
@@ -148,9 +200,12 @@ rcl_action_client_init(
   RCL_CHECK_FOR_NULL_WITH_MSG(
     action_client->impl, "allocating memory failed", return RCL_RET_BAD_ALLOC);
 
+  // Avoid uninitialized pointers should initialization fail
+  *action_client->impl = _rcl_action_get_zero_initialized_client_impl();
   // Copy action client name and options.
   action_client->impl->action_name = rcutils_strdup(action_name, allocator);
   if (NULL == action_client->impl->action_name) {
+    RCL_SET_ERROR_MSG("failed to duplicate action name");
     ret = RCL_RET_BAD_ALLOC;
     goto fail;
   }
@@ -168,7 +223,7 @@ rcl_action_client_init(
   RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client initialized");
   return ret;
 fail:
-  fini_ret = rcl_action_client_fini(action_client, node);
+  fini_ret = _rcl_action_client_fini_impl(action_client, node, allocator);
   if (RCL_RET_OK != fini_ret) {
     RCL_SET_ERROR_MSG("failed to cleanup action client");
     ret = RCL_RET_ERROR;
@@ -186,28 +241,8 @@ rcl_action_client_fini(rcl_action_client_t * action_client, rcl_node_t * node)
   if (!rcl_node_is_valid_except_context(node)) {
     return RCL_RET_NODE_INVALID;  // error already set
   }
-  rcl_ret_t ret = RCL_RET_OK;
+  return
-  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) {
+    _rcl_action_client_fini_impl(action_client, node, action_client->impl->options.allocator);
-    ret = RCL_RET_ERROR;
-  }
-  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) {
-    ret = RCL_RET_ERROR;
-  }
-  if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) {
-    ret = RCL_RET_ERROR;
-  }
-  if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) {
-    ret = RCL_RET_ERROR;
-  }
-  if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) {
-    ret = RCL_RET_ERROR;
-  }
-  rcl_allocator_t * allocator = &action_client->impl->options.allocator;
-  allocator->deallocate(action_client->impl->action_name, allocator->state);
-  allocator->deallocate(action_client->impl, allocator->state);
-  action_client->impl = NULL;
-  RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized");
-  return ret;
 }
 
 rcl_action_client_options_t

rcl_action/test/rcl_action/test_action_client.cpp

@@ -154,19 +154,33 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini) {
   EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str;
   rcl_reset_error();
 
-  // Fail copying action_name string, this will also fail to cleanup and will return generic error
+  // Fail copying action name
   time_bomb_state.malloc_count_until_failure = 1;
   invalid_action_client_options.allocator.state = &time_bomb_state;
   ret = rcl_action_client_init(
     &action_client, &this->node, action_typesupport,
     action_name, &invalid_action_client_options);
-  EXPECT_EQ(ret, RCL_RET_ERROR) << rcl_get_error_string().str;
+  EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str;
   rcl_reset_error();
 
-  // Manually cleanup to setup for next test
+  ret = RCL_RET_OK;
-  action_client_options.allocator.deallocate(
+  int i = 0;
-    action_client.impl, action_client_options.allocator.state);
+  do {
-  action_client.impl = NULL;
+    time_bomb_state.malloc_count_until_failure = i;
+    i++;
+    invalid_action_client_options.allocator.state = &time_bomb_state;
+    ret = rcl_action_client_init(
+      &action_client, &this->node, action_typesupport,
+      action_name, &invalid_action_client_options);
+    if (RCL_RET_OK != ret) {
+      EXPECT_TRUE(rcl_error_is_set());
+      rcl_reset_error();
+    } else {
+      EXPECT_EQ(RCL_RET_OK, rcl_action_client_fini(&action_client, &this->node)) <<
+        rcl_get_error_string().str;
+      EXPECT_FALSE(rcl_error_is_set());
+    }
+  } while (ret != RCL_RET_OK);
 
   ret = rcl_action_client_init(
     &action_client, &this->node, action_typesupport,