From 25c5ff539cb4a14cac911964fddd6d2408fa263e Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 5 Apr 2018 16:18:25 -0500 Subject: [PATCH 1/3] Force rcl_arguments_t to be zero initialized. * Adds a check for rcl_arguments_t->impl to be NULL before use. * Updates tests to account for zero initializationt. --- rcl/src/rcl/arguments.c | 6 ++++++ rcl/test/rcl/arg_macros.hpp | 1 + rcl/test/rcl/test_arguments.cpp | 27 ++++++++++++++++++--------- 3 files changed, 25 insertions(+), 9 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index b40ee46bb..1e3d8b1e1 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -221,12 +221,18 @@ rcl_parse_arguments( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "invalid allocator", return RCL_RET_INVALID_ARGUMENT); if (argc < 0) { + RCL_SET_ERROR_MSG("Argument count cannot be negative", allocator); return RCL_RET_INVALID_ARGUMENT; } else if (argc > 0) { RCL_CHECK_ARGUMENT_FOR_NULL(argv, RCL_RET_INVALID_ARGUMENT, allocator); } RCL_CHECK_ARGUMENT_FOR_NULL(args_output, RCL_RET_INVALID_ARGUMENT, allocator); + if (args_output->impl != NULL) { + RCL_SET_ERROR_MSG("Parse output is not zero-initialized", allocator); + return RCL_RET_INVALID_ARGUMENT; + } + rcl_ret_t ret; rcl_ret_t fail_ret; diff --git a/rcl/test/rcl/arg_macros.hpp b/rcl/test/rcl/arg_macros.hpp index 0da491628..200098101 100644 --- a/rcl/test/rcl/arg_macros.hpp +++ b/rcl/test/rcl/arg_macros.hpp @@ -61,6 +61,7 @@ destroy_args(int argc, char ** args) #define SCOPE_ARGS(local_arguments, ...) \ { \ + local_arguments = rcl_get_zero_initialized_arguments(); \ const char * local_argv[] = {__VA_ARGS__}; \ unsigned int local_argc = (sizeof(local_argv) / sizeof(const char *)); \ rcl_ret_t ret = rcl_parse_arguments( \ diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index 7a13a61d6..0b78c42fe 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/rcl/test/rcl/test_arguments.cpp @@ -72,7 +72,7 @@ bool is_valid_arg(const char * arg) { const char * argv[] = {arg}; - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret = rcl_parse_arguments(1, argv, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); bool is_valid = 0 == rcl_arguments_get_count_unparsed(&parsed_args); @@ -113,7 +113,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), check_valid_vs_inval } TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_no_args) { - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret = rcl_parse_arguments(0, NULL, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); EXPECT_EQ(0, rcl_arguments_get_count_unparsed(&parsed_args)); @@ -122,7 +122,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_no_args) { TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_null_args) { int argc = 1; - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret = rcl_parse_arguments(argc, NULL, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret) << rcl_get_error_string_safe(); rcl_reset_error(); @@ -139,7 +139,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_null_args_outpu TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_one_remap) { const char * argv[] = {"process_name", "/foo/bar:=/fiz/buz"}; int argc = sizeof(argv) / sizeof(const char *); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret; ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); @@ -150,7 +150,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_one_remap) { TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_mix_valid_invalid_rules) { const char * argv[] = {"process_name", "/foo/bar:=", "bar:=/fiz/buz", "}bar:=fiz"}; int argc = sizeof(argv) / sizeof(const char *); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret; ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); @@ -161,7 +161,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_mix_valid_inval 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 *); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret; ret = rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); @@ -169,13 +169,22 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_two_namespace) EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args)); } +TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_uninitialized_parsed_args) { + const char * argv[] = {"process_name"}; + int argc = sizeof(argv) / sizeof(const char *); + rcl_arguments_t parsed_args; + ASSERT_EQ(RCL_RET_INVALID_ARGUMENT, + rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); + rcl_reset_error(); +} + TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_null) { EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_arguments_fini(NULL)); rcl_reset_error(); } TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_impl_null) { - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); parsed_args.impl = NULL; EXPECT_EQ(RCL_RET_ERROR, rcl_arguments_fini(&parsed_args)); rcl_reset_error(); @@ -184,7 +193,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_impl_null) TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_twice) { const char * argv[] = {"process_name"}; int argc = sizeof(argv) / sizeof(const char *); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); ASSERT_EQ(RCL_RET_OK, rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); EXPECT_EQ(RCL_RET_OK, rcl_arguments_fini(&parsed_args)); EXPECT_EQ(RCL_RET_ERROR, rcl_arguments_fini(&parsed_args)); @@ -197,7 +206,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_remove_ros_args int argc = sizeof(argv) / sizeof(const char *); rcl_allocator_t alloc = rcl_get_default_allocator(); - rcl_arguments_t parsed_args; + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); rcl_ret_t ret; ret = rcl_parse_arguments(argc, argv, alloc, &parsed_args); ASSERT_EQ(RCL_RET_OK, ret) << rcl_get_error_string_safe(); From af6c9c1784a6a1d83d743fa59ebe2fd44844a706 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Thu, 5 Apr 2018 16:34:02 -0500 Subject: [PATCH 2/3] Fix tests and add documentation. --- rcl/include/rcl/arguments.h | 3 +++ rcl/test/rcl/test_arguments.cpp | 17 +++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/rcl/include/rcl/arguments.h b/rcl/include/rcl/arguments.h index b30188737..96c239b7b 100644 --- a/rcl/include/rcl/arguments.h +++ b/rcl/include/rcl/arguments.h @@ -44,6 +44,8 @@ rcl_get_zero_initialized_arguments(void); /** * If an argument does not appear to be a valid ROS argument then it is skipped * and parsing continues with the next argument in `argv`. + * + * \sa rcl_get_zero_initialized_arguments() * \sa rcl_arguments_get_count_unparsed() * \sa rcl_arguments_get_unparsed() * @@ -67,6 +69,7 @@ rcl_get_zero_initialized_arguments(void); * \param[in] argv The values of the arguments. * \param[in] allocator A valid allocator. * \param[out] args_output A structure that will contain the result of parsing. + * Must be zero initialized before use. * \return `RCL_RET_OK` if the arguments were parsed successfully, or * \return `RCL_RET_INVALID_ARGUMENT` if any function arguments are invalid, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed, or diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index 0b78c42fe..b65411c53 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/rcl/test/rcl/test_arguments.cpp @@ -173,11 +173,28 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_uninitialized_p const char * argv[] = {"process_name"}; int argc = sizeof(argv) / sizeof(const char *); rcl_arguments_t parsed_args; + int not_null = 1; + parsed_args.impl = reinterpret_cast(¬_null); ASSERT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); rcl_reset_error(); } +TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_double_parse) { + const char * argv[] = {"process_name", "__ns:=/foo/bar", "__ns:=/fiz/buz"}; + int argc = sizeof(argv) / sizeof(const char *); + rcl_arguments_t parsed_args = rcl_get_zero_initialized_arguments(); + ASSERT_EQ(RCL_RET_OK, + rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); + ASSERT_EQ(RCL_RET_INVALID_ARGUMENT, + rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); + rcl_reset_error(); +} + + + + + TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_null) { EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_arguments_fini(NULL)); rcl_reset_error(); From 9a750fb578751d9df69f4cf52efb84383aace557 Mon Sep 17 00:00:00 2001 From: Michael Carroll Date: Fri, 6 Apr 2018 09:46:42 -0500 Subject: [PATCH 3/3] Fix style issues. --- rcl/test/rcl/test_arguments.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index b65411c53..b4986d69c 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/rcl/test/rcl/test_arguments.cpp @@ -174,7 +174,7 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_uninitialized_p int argc = sizeof(argv) / sizeof(const char *); rcl_arguments_t parsed_args; int not_null = 1; - parsed_args.impl = reinterpret_cast(¬_null); + parsed_args.impl = reinterpret_cast(¬_null); ASSERT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_parse_arguments(argc, argv, rcl_get_default_allocator(), &parsed_args)); rcl_reset_error(); @@ -192,9 +192,6 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_double_parse) { } - - - TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_fini_null) { EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, rcl_arguments_fini(NULL)); rcl_reset_error();