From 1a840d12190d08fe79c548ad7463bf0835daa656 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 15 Oct 2019 18:20:14 -0300 Subject: [PATCH] Handle zero non-ROS specific args properly in rcl_remove_ros_arguments (#518) Signed-off-by: Michel Hidalgo --- rcl/src/rcl/arguments.c | 27 ++++-- rcl/test/rcl/test_arguments.cpp | 163 ++++++++++++++++++++++++++------ 2 files changed, 150 insertions(+), 40 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index f4806c8..a64fe35 100644 --- a/rcl/src/rcl/arguments.c +++ b/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); - - *nonros_argc = rcl_arguments_get_count_unparsed(args); - *nonros_argv = NULL; - - if (*nonros_argc <= 0) { + RCL_CHECK_ARGUMENT_FOR_NULL(nonros_argc, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(nonros_argv, RCL_RET_INVALID_ARGUMENT); + if (NULL != *nonros_argv) { + RCL_SET_ERROR_MSG("Output nonros_argv pointer is not null. May leak memory."); 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; - ret = rcl_arguments_get_unparsed(args, allocator, &unparsed_indices); + rcl_ret_t ret = rcl_arguments_get_unparsed(args, allocator, &unparsed_indices); if (RCL_RET_OK != ret) { return ret; diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index a600c78..2dc9a97 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/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; - 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)); + }); 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; - 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)); + }); 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) { - alloc.deallocate(nonros_argv, alloc.state); - } + EXPECT_EQ(0, nonros_argc); + EXPECT_TRUE(NULL == nonros_argv); } -TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args_zero) { - const char * argv[] = {""}; - rcl_ret_t ret; + +TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args_if_no_args) { + 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) { - alloc.deallocate(nonros_argv, alloc.state); - } + EXPECT_EQ(0, nonros_argc); + 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));