From 686d426dcd37365b5c766e2a8dbb571d452041a5 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Thu, 16 Jan 2020 17:23:50 -0800 Subject: [PATCH 1/5] Don't finalize parameters struct in rcl_parse_yaml_file function The function assumes that the structure has already been initialized, so it should be the callers responsibility to finalize it. Otherwise, this may lead to a double free as reported in #553. Signed-off-by: Jacob Perron --- rcl_yaml_param_parser/src/parser.c | 1 - 1 file changed, 1 deletion(-) diff --git a/rcl_yaml_param_parser/src/parser.c b/rcl_yaml_param_parser/src/parser.c index 6c5abe890..4ab6f059a 100644 --- a/rcl_yaml_param_parser/src/parser.c +++ b/rcl_yaml_param_parser/src/parser.c @@ -1705,7 +1705,6 @@ bool rcl_parse_yaml_file( if (NULL != ns_tracker.parameter_ns) { allocator.deallocate(ns_tracker.parameter_ns, allocator.state); } - rcl_yaml_node_struct_fini(params_st); return false; } From acdc970fb9be419eb4476c112cd90013182e79dc Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Thu, 16 Jan 2020 17:27:29 -0800 Subject: [PATCH 2/5] Return an error code if there's a malformed parameters file This restores behavior that was lost when the '__params:=' syntax was deprecated in #495. An extra guard was also added when finalizing the parameters struct. Signed-off-by: Jacob Perron --- rcl/src/rcl/arguments.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index cf62670bb..8ed992af5 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -610,11 +610,10 @@ rcl_parse_arguments( // Attempt to parse argument as parameter file rule args_impl->parameter_files[args_impl->num_param_files_args] = NULL; - if ( - RCL_RET_OK == _rcl_parse_param_file_rule( - argv[i], allocator, args_impl->parameter_overrides, - &args_impl->parameter_files[args_impl->num_param_files_args])) - { + ret = _rcl_parse_param_file_rule( + argv[i], allocator, args_impl->parameter_overrides, + &args_impl->parameter_files[args_impl->num_param_files_args]); + if (RCL_RET_OK == ret) { ++(args_impl->num_param_files_args); RCUTILS_LOG_WARN_NAMED(ROS_PACKAGE_NAME, "Found parameter file rule '%s'. This syntax is deprecated. Use '%s %s %s' instead.", @@ -625,6 +624,10 @@ rcl_parse_arguments( args_impl->parameter_files[args_impl->num_param_files_args - 1], args_impl->num_param_files_args); continue; + } else if (RCL_RET_ERROR == ret) { + // If we return RCL_RET_ERROR then the argument contained the prefix '__params:=', + // but the parameter file may be malformed or does not exist. + goto fail; } RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Couldn't parse arg %d (%s) as a deprecated parameter file rule. Error: %s", @@ -751,7 +754,7 @@ rcl_parse_arguments( } // Drop parameter overrides if none was found. - if (0U == args_impl->parameter_overrides->num_nodes) { + if (NULL != args_impl->parameter_overrides && 0U == args_impl->parameter_overrides->num_nodes) { rcl_yaml_node_struct_fini(args_impl->parameter_overrides); args_impl->parameter_overrides = NULL; } From 0937963718ccdb8eb09f9ce219eb1af941dc976d Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 27 Jan 2020 09:29:16 -0800 Subject: [PATCH 3/5] Refactor comment Signed-off-by: Jacob Perron --- rcl/src/rcl/arguments.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 8ed992af5..6f238ad80 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -625,8 +625,8 @@ rcl_parse_arguments( args_impl->num_param_files_args); continue; } else if (RCL_RET_ERROR == ret) { - // If we return RCL_RET_ERROR then the argument contained the prefix '__params:=', - // but the parameter file may be malformed or does not exist. + // If _rcl_parse_param_file_rule() returned RCL_RET_ERROR then the argument contained the + // '__params:=' prefix, but parsing the parameter file failed. goto fail; } RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, From 98433bca4b016e9853e9232b46d6a1e4c945aeb4 Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 27 Jan 2020 09:48:20 -0800 Subject: [PATCH 4/5] Add regression test Signed-off-by: Jacob Perron --- rcl/test/rcl/test_arguments.cpp | 20 +++++++++++++++++++ .../test_malformed_parameters.1.yaml | 5 +++++ 2 files changed, 25 insertions(+) create mode 100644 rcl/test/resources/test_arguments/test_malformed_parameters.1.yaml diff --git a/rcl/test/rcl/test_arguments.cpp b/rcl/test/rcl/test_arguments.cpp index 0f8964069..e8f4f072e 100644 --- a/rcl/test/rcl/test_arguments.cpp +++ b/rcl/test/rcl/test_arguments.cpp @@ -897,6 +897,26 @@ TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_deprecated_para EXPECT_EQ(1, *(param_value->integer_value)); } +// Regression test for https://github.com/ros2/rcl/issues/553 +// Testing behaviour that was broken in Eloquent +TEST_F( + CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_deprecated_param_argument_malformed) +{ + const std::string parameters_filepath = (test_path / "test_malformed_parameters.1.yaml").string(); + const std::string parameter_rule = "__params:=" + parameters_filepath; + const char * argv[] = { + "process_name", parameter_rule.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); + ASSERT_EQ(RCL_RET_ERROR, ret) << rcl_get_error_string().str; +} + TEST_F(CLASSNAME(TestArgumentsFixture, RMW_IMPLEMENTATION), test_param_argument_multiple) { const std::string parameters_filepath1 = (test_path / "test_parameters.1.yaml").string(); const std::string parameters_filepath2 = (test_path / "test_parameters.2.yaml").string(); diff --git a/rcl/test/resources/test_arguments/test_malformed_parameters.1.yaml b/rcl/test/resources/test_arguments/test_malformed_parameters.1.yaml new file mode 100644 index 000000000..257224a97 --- /dev/null +++ b/rcl/test/resources/test_arguments/test_malformed_parameters.1.yaml @@ -0,0 +1,5 @@ +some_node: + ros_parameters: + int_param: 1 + param_group: + string_param: foo From f308b499ea811e6366de8065fe56e730054959aa Mon Sep 17 00:00:00 2001 From: Jacob Perron Date: Mon, 27 Jan 2020 14:30:27 -0800 Subject: [PATCH 5/5] Provide detailed error message Also warn about deprecated usage, even if there is an error parsing parameters file. Signed-off-by: Jacob Perron --- rcl/src/rcl/arguments.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 6f238ad80..da51d2661 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -613,12 +613,17 @@ rcl_parse_arguments( ret = _rcl_parse_param_file_rule( argv[i], allocator, args_impl->parameter_overrides, &args_impl->parameter_files[args_impl->num_param_files_args]); - if (RCL_RET_OK == ret) { - ++(args_impl->num_param_files_args); + + // Deprecation warning regardless if there is an error parsing the file + if (RCL_RET_INVALID_PARAM_RULE != ret) { RCUTILS_LOG_WARN_NAMED(ROS_PACKAGE_NAME, "Found parameter file rule '%s'. This syntax is deprecated. Use '%s %s %s' instead.", argv[i], RCL_ROS_ARGS_FLAG, RCL_PARAM_FILE_FLAG, - args_impl->parameter_files[args_impl->num_param_files_args - 1]); + args_impl->parameter_files[args_impl->num_param_files_args]); + } + + if (RCL_RET_OK == ret) { + ++(args_impl->num_param_files_args); RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "params rule : %s\n total num param rules %d", args_impl->parameter_files[args_impl->num_param_files_args - 1], @@ -627,6 +632,12 @@ rcl_parse_arguments( } else if (RCL_RET_ERROR == ret) { // If _rcl_parse_param_file_rule() returned RCL_RET_ERROR then the argument contained the // '__params:=' prefix, but parsing the parameter file failed. + rcl_error_string_t prev_error_string = rcl_get_error_string(); + rcl_reset_error(); + RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( + "Couldn't parse params file: '%s'. Error: %s", + args_impl->parameter_files[args_impl->num_param_files_args], + prev_error_string.str); goto fail; } RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME,