[rcl] Guard against bad allocation calling rcl_arguments_copy() (ros2…

…#367)

* [rcl] Add test for copying arguments struct with no arguments

* Override allocate function in test to reveal bug

* [rcl] Only allocate arrays if there are things to copy in rcl_argument_copy()

Also guard against freeing invalid pointers if rcl_argument_copy() fails.

* Remove uncessary guard against NULL pointer

rcl/src/rcl/arguments.c

@@ -499,44 +499,52 @@ rcl_arguments_copy(
 
   // Zero so it's safe to call rcl_arguments_fini() if an error occurrs while copying.
   args_out->impl->num_remap_rules = 0;
+  args_out->impl->remap_rules = NULL;
+  args_out->impl->unparsed_args = NULL;
   args_out->impl->num_unparsed_args = 0;
+  args_out->impl->parameter_files = NULL;
   args_out->impl->num_param_files_args = 0;
 
-  // Copy unparsed args
-  args_out->impl->unparsed_args = allocator.allocate(
-    sizeof(int) * args->impl->num_unparsed_args, allocator.state);
-  if (NULL == args_out->impl->unparsed_args) {
-    if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
-      RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
+  if (args->impl->num_unparsed_args) {
+    // Copy unparsed args
+    args_out->impl->unparsed_args = allocator.allocate(
+      sizeof(int) * args->impl->num_unparsed_args, allocator.state);
+    if (NULL == args_out->impl->unparsed_args) {
+      if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
+        RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
+      }
+      return RCL_RET_BAD_ALLOC;
     }
-    return RCL_RET_BAD_ALLOC;
-  }
-  for (int i = 0; i < args->impl->num_unparsed_args; ++i) {
-    args_out->impl->unparsed_args[i] = args->impl->unparsed_args[i];
-  }
-  args_out->impl->num_unparsed_args = args->impl->num_unparsed_args;
-
-  // Copy remap rules
-  args_out->impl->remap_rules = allocator.allocate(
-    sizeof(rcl_remap_t) * args->impl->num_remap_rules, allocator.state);
-  if (NULL == args_out->impl->remap_rules) {
-    if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
-      RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
+    for (int i = 0; i < args->impl->num_unparsed_args; ++i) {
+      args_out->impl->unparsed_args[i] = args->impl->unparsed_args[i];
     }
-    return RCL_RET_BAD_ALLOC;
+    args_out->impl->num_unparsed_args = args->impl->num_unparsed_args;
   }
-  args_out->impl->num_remap_rules = args->impl->num_remap_rules;
-  for (int i = 0; i < args->impl->num_remap_rules; ++i) {
-    args_out->impl->remap_rules[i] = rcl_remap_get_zero_initialized();
-    rcl_ret_t ret = rcl_remap_copy(
-      &(args->impl->remap_rules[i]), &(args_out->impl->remap_rules[i]));
-    if (RCL_RET_OK != ret) {
+
+  if (args->impl->num_remap_rules) {
+    // Copy remap rules
+    args_out->impl->remap_rules = allocator.allocate(
+      sizeof(rcl_remap_t) * args->impl->num_remap_rules, allocator.state);
+    if (NULL == args_out->impl->remap_rules) {
       if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
         RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
       }
-      return ret;
+      return RCL_RET_BAD_ALLOC;
+    }
+    args_out->impl->num_remap_rules = args->impl->num_remap_rules;
+    for (int i = 0; i < args->impl->num_remap_rules; ++i) {
+      args_out->impl->remap_rules[i] = rcl_remap_get_zero_initialized();
+      rcl_ret_t ret = rcl_remap_copy(
+        &(args->impl->remap_rules[i]), &(args_out->impl->remap_rules[i]));
+      if (RCL_RET_OK != ret) {
+        if (RCL_RET_OK != rcl_arguments_fini(args_out)) {
+          RCL_SET_ERROR_MSG("Error while finalizing arguments due to another error");
+        }
+        return ret;
+      }
     }
   }
+
   // Copy parameter files
   if (args->impl->num_param_files_args) {
     args_out->impl->parameter_files = allocator.allocate(
@@ -558,8 +566,6 @@ rcl_arguments_copy(
         return RCL_RET_BAD_ALLOC;
       }
     }
-  } else {
-    args_out->impl->parameter_files = NULL;
   }
   return RCL_RET_OK;
 }

rcl/test/rcl/test_arguments.cpp

@@ -202,6 +202,36 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_copy) {
   EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&copied_args));
 }
 
+// Similar to the default allocator, but returns NULL when size is zero.
+// This is useful for emulating systems where `malloc(0)` return NULL.
+// TODO(jacobperron): Consider using this allocate function in other tests
+static void *
+__return_null_on_zero_allocate(size_t size, void * state)
+{
+  RCUTILS_UNUSED(state);
+  if (size == 0) {
+    return NULL;
+  }
+  return malloc(size);
+}
+
+TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_copy_no_args) {
+  rcl_allocator_t allocator = rcl_get_default_allocator();
+  allocator.allocate = __return_null_on_zero_allocate;
+  rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
+  rcl_ret_t ret = rcl_parse_arguments(0, NULL, allocator, &parsed_args);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(0, rcl_arguments_get_count_unparsed(&parsed_args));
+
+  rcl_arguments_t copied_args = rcl_get_zero_initialized_arguments();
+  ret = rcl_arguments_copy(&parsed_args, &copied_args);
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(0, rcl_arguments_get_count_unparsed(&copied_args));
+
+  EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&copied_args));
+}
+
 TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_two_namespace) {
   const char * argv[] = {"process_name", "__ns:=/foo/bar", "__ns:=/fiz/buz"};
   int argc = sizeof(argv) / sizeof(const char *);