Handle zero non-ROS specific args properly in rcl_remove_ros_arguments (#518)

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>

rcl/src/rcl/arguments.c

@@ -883,21 +883,30 @@ rcl_remove_ros_arguments(
   int * nonros_argc,
   const char ** nonros_argv[])
 {
-  RCL_CHECK_ARGUMENT_FOR_NULL(argv, RCL_RET_INVALID_ARGUMENT);
-  RCL_CHECK_ARGUMENT_FOR_NULL(nonros_argc, RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ARGUMENT_FOR_NULL(args, RCL_RET_INVALID_ARGUMENT);
   RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT);
-
+  RCL_CHECK_ARGUMENT_FOR_NULL(nonros_argc, RCL_RET_INVALID_ARGUMENT);
-  *nonros_argc = rcl_arguments_get_count_unparsed(args);
+  RCL_CHECK_ARGUMENT_FOR_NULL(nonros_argv, RCL_RET_INVALID_ARGUMENT);
-  *nonros_argv = NULL;
+  if (NULL != *nonros_argv) {
-
+    RCL_SET_ERROR_MSG("Output nonros_argv pointer is not null. May leak memory.");
-  if (*nonros_argc <= 0) {
     return RCL_RET_INVALID_ARGUMENT;
   }
 
+  *nonros_argc = rcl_arguments_get_count_unparsed(args);
+  if (*nonros_argc < 0) {
+    RCL_SET_ERROR_MSG("Failed to get unparsed non ROS specific arguments count.");
+    return RCL_RET_INVALID_ARGUMENT;
+  } else if (*nonros_argc > 0) {
+    RCL_CHECK_ARGUMENT_FOR_NULL(argv, RCL_RET_INVALID_ARGUMENT);
+  }
+
+  *nonros_argv = NULL;
+  if (0 == *nonros_argc) {
+    return RCL_RET_OK;
+  }
+
   int * unparsed_indices = NULL;
-  rcl_ret_t ret;
+  rcl_ret_t ret = rcl_arguments_get_unparsed(args, allocator, &unparsed_indices);
-  ret = rcl_arguments_get_unparsed(args, allocator, &unparsed_indices);
 
   if (RCL_RET_OK != ret) {
     return ret;

rcl/test/rcl/test_arguments.cpp

@@ -570,6 +570,70 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_twice) {
   rcl_reset_error();
 }
 
+TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_bad_remove_ros_args) {
+  const char * argv[] = {"process_name"};
+  int argc = sizeof(argv) / sizeof(const char *);
+
+  rcl_allocator_t default_allocator = rcl_get_default_allocator();
+  rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, default_allocator, &parsed_args);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
+
+  const char ** nonros_argv = NULL;
+  int nonros_argc = 0;
+
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      NULL, &parsed_args, default_allocator, &nonros_argc, &nonros_argv));
+  rcl_reset_error();
+
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, NULL, default_allocator, &nonros_argc, &nonros_argv));
+  rcl_reset_error();
+
+  rcl_allocator_t zero_initialized_allocator =
+    (rcl_allocator_t)rcutils_get_zero_initialized_allocator();
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, &parsed_args, zero_initialized_allocator, &nonros_argc, &nonros_argv));
+  rcl_reset_error();
+
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, &parsed_args, default_allocator, NULL, &nonros_argv));
+  rcl_reset_error();
+
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, &parsed_args, default_allocator, &nonros_argc, NULL));
+  rcl_reset_error();
+
+  rcl_arguments_t zero_initialized_parsed_args = rcl_get_zero_initialized_arguments();
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, &zero_initialized_parsed_args, default_allocator, &nonros_argc, &nonros_argv));
+  rcl_reset_error();
+
+  const char * stack_allocated_nonros_argv[] = {"--foo", "--bar"};
+  const char ** initialized_nonros_argv = stack_allocated_nonros_argv;
+  int initialized_nonros_argc = sizeof(stack_allocated_nonros_argv) / sizeof(const char *);
+
+  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_remove_ros_arguments(
+      argv, &parsed_args, default_allocator, &initialized_nonros_argc, &initialized_nonros_argv));
+  rcl_reset_error();
+
+  rcl_arguments_t no_parsed_args = rcl_get_zero_initialized_arguments();
+  ret = rcl_parse_arguments(0, NULL, default_allocator, &no_parsed_args);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&no_parsed_args));
+  });
+
+  ret = rcl_remove_ros_arguments(
+    NULL, &no_parsed_args, default_allocator, &nonros_argc, &nonros_argv);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  EXPECT_TRUE(NULL == nonros_argv);
+  EXPECT_EQ(0, nonros_argc);
+}
+
 TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args) {
   const char * argv[] = {
     "process_name", "-d", "--ros-args", "-r", "__ns:=/foo/bar", "-r", "__ns:=/fiz/buz", "--",
@@ -579,12 +643,17 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args
 
   rcl_allocator_t alloc = rcl_get_default_allocator();
   rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
-  rcl_ret_t ret;
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
-  ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
 
   int nonros_argc = 0;
   const char ** nonros_argv = NULL;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    alloc.deallocate(nonros_argv, alloc.state);
+  });
 
   ret = rcl_remove_ros_arguments(
     argv,
@@ -593,7 +662,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args
     &nonros_argc,
     &nonros_argv);
 
-  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
   ASSERT_EQ(nonros_argc, 6);
   EXPECT_STREQ(nonros_argv[0], "process_name");
   EXPECT_STREQ(nonros_argv[1], "-d");
@@ -601,11 +670,6 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args
   EXPECT_STREQ(nonros_argv[3], "--baz");
   EXPECT_STREQ(nonros_argv[4], "--");
   EXPECT_STREQ(nonros_argv[5], "arg");
-  EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
-
-  if (NULL != nonros_argv) {
-    alloc.deallocate(nonros_argv, alloc.state);
-  }
 }
 
 // \deprecated to be removed in F-Turtle
@@ -619,12 +683,51 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_deprecat
 
   rcl_allocator_t alloc = rcl_get_default_allocator();
   rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
-  rcl_ret_t ret;
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
-  ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
 
   int nonros_argc = 0;
   const char ** nonros_argv = NULL;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    alloc.deallocate(nonros_argv, alloc.state);
+  });
+
+  ret = rcl_remove_ros_arguments(
+    argv,
+    &parsed_args,
+    alloc,
+    &nonros_argc,
+    &nonros_argv);
+
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  ASSERT_EQ(nonros_argc, 5);
+  EXPECT_STREQ(nonros_argv[0], "process_name");
+  EXPECT_STREQ(nonros_argv[1], "-d");
+  EXPECT_STREQ(nonros_argv[2], "--foo=bar");
+  EXPECT_STREQ(nonros_argv[3], "--bar=baz");
+  EXPECT_STREQ(nonros_argv[4], "arg");
+}
+
+TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args_if_ros_only) {
+  const char * argv[] = {"--ros-args", "--disable-rosout-logs"};
+  int argc = sizeof(argv) / sizeof(const char *);
+
+  rcl_allocator_t alloc = rcl_get_default_allocator();
+  rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
+
+  int nonros_argc = 0;
+  const char ** nonros_argv = NULL;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    alloc.deallocate(nonros_argv, alloc.state);
+  });
 
   ret = rcl_remove_ros_arguments(
     argv,
@@ -634,28 +737,29 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_deprecat
     &nonros_argv);
 
   EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
-  ASSERT_EQ(nonros_argc, 5);
-  EXPECT_STREQ(nonros_argv[0], "process_name");
-  EXPECT_STREQ(nonros_argv[1], "-d");
-  EXPECT_STREQ(nonros_argv[2], "--foo=bar");
-  EXPECT_STREQ(nonros_argv[3], "--bar=baz");
-  EXPECT_STREQ(nonros_argv[4], "arg");
-  EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
 
-  if (NULL != nonros_argv) {
+  EXPECT_EQ(0, nonros_argc);
-    alloc.deallocate(nonros_argv, alloc.state);
+  EXPECT_TRUE(NULL == nonros_argv);
-  }
 }
 
-TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args_zero) {
+
-  const char * argv[] = {""};
+TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args_if_no_args) {
-  rcl_ret_t ret;
+  const char ** argv = NULL;
+  const int argc = 0;
 
   rcl_allocator_t alloc = rcl_get_default_allocator();
   rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
+  ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
+  });
 
   int nonros_argc = 0;
   const char ** nonros_argv = NULL;
+  OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
+    alloc.deallocate(nonros_argv, alloc.state);
+  });
 
   ret = rcl_remove_ros_arguments(
     argv,
@@ -664,22 +768,20 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args
     &nonros_argc,
     &nonros_argv);
 
-  EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string().str;
+  EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
 
-  if (NULL != nonros_argv) {
+  EXPECT_EQ(0, nonros_argc);
-    alloc.deallocate(nonros_argv, alloc.state);
+  EXPECT_TRUE(NULL == nonros_argv);
-  }
 }
 
 TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_argument_zero) {
   const char * argv[] = {"process_name", "--ros-args", "-r", "__ns:=/namespace", "random:=arg"};
   int argc = sizeof(argv) / sizeof(const char *);
-  rcl_ret_t ret;
 
   rcl_allocator_t alloc = rcl_get_default_allocator();
   rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
 
-  ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
   OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
     EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));
@@ -696,12 +798,11 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_argument_
     "--params-file", parameters_filepath.c_str()
   };
   int argc = sizeof(argv) / sizeof(const char *);
-  rcl_ret_t ret;
 
   rcl_allocator_t alloc = rcl_get_default_allocator();
   rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments();
 
-  ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
+  rcl_ret_t ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args);
   ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str;
   OSRF_TESTING_TOOLS_CPP_SCOPE_EXIT({
     EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args));