From 4757a1929e290ecb8aa95ed8a0d5b6b63f078cfb Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Mon, 31 Aug 2020 19:13:49 -0300 Subject: [PATCH 1/2] Fix rcl arguments' API memory leaks. Signed-off-by: Michel Hidalgo --- rcl/src/rcl/arguments.c | 75 ++++++++++++++++++++++++----------------- 1 file changed, 44 insertions(+), 31 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 74c5cc7cb..7ee57bdca 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -594,17 +594,20 @@ rcl_parse_arguments( } // Shrink remap_rules array to match number of successfully parsed rules - if (args_impl->num_remap_rules > 0) { - args_impl->remap_rules = rcutils_reallocf( - args_impl->remap_rules, sizeof(rcl_remap_t) * args_impl->num_remap_rules, &allocator); - if (NULL == args_impl->remap_rules) { - ret = RCL_RET_BAD_ALLOC; - goto fail; - } - } else { + if (0 == args_impl->num_remap_rules) { // No remap rules allocator.deallocate(args_impl->remap_rules, allocator.state); args_impl->remap_rules = NULL; + } else if (args_impl->num_remap_rules < argc) { + rcl_remap_t * new_remap_rules = allocator.reallocate( + args_impl->remap_rules, + sizeof(rcl_remap_t) * args_impl->num_remap_rules, + &allocator); + if (NULL == new_remap_rules) { + ret = RCL_RET_BAD_ALLOC; + goto fail; + } + args_impl->remap_rules = new_remap_rules; } // Shrink Parameter files @@ -612,12 +615,15 @@ rcl_parse_arguments( allocator.deallocate(args_impl->parameter_files, allocator.state); args_impl->parameter_files = NULL; } else if (args_impl->num_param_files_args < argc) { - args_impl->parameter_files = rcutils_reallocf( - args_impl->parameter_files, sizeof(char *) * args_impl->num_param_files_args, &allocator); - if (NULL == args_impl->parameter_files) { + char ** new_parameter_files = allocator.reallocate( + args_impl->parameter_files, + sizeof(char *) * args_impl->num_param_files_args, + &allocator); + if (NULL == new_parameter_files) { ret = RCL_RET_BAD_ALLOC; goto fail; } + args_impl->parameter_files = new_parameter_files; } // Drop parameter overrides if none was found. @@ -867,7 +873,6 @@ rcl_arguments_copy( } 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_get_zero_initialized_remap(); ret = rcl_remap_copy( @@ -878,6 +883,7 @@ rcl_arguments_copy( } return ret; } + ++(args_out->impl->num_remap_rules); } } @@ -897,7 +903,6 @@ rcl_arguments_copy( } return RCL_RET_BAD_ALLOC; } - args_out->impl->num_param_files_args = args->impl->num_param_files_args; for (int i = 0; i < args->impl->num_param_files_args; ++i) { args_out->impl->parameter_files[i] = rcutils_strdup(args->impl->parameter_files[i], allocator); @@ -907,6 +912,7 @@ rcl_arguments_copy( } return RCL_RET_BAD_ALLOC; } + ++(args_out->impl->num_param_files_args); } } char * enclave_copy = rcutils_strdup(args->impl->enclave, allocator); @@ -1787,9 +1793,8 @@ _rcl_parse_remap_rule( RCL_CHECK_ARGUMENT_FOR_NULL(arg, RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(output_rule, RCL_RET_INVALID_ARGUMENT); - rcl_ret_t ret; - - output_rule->impl = allocator.allocate(sizeof(rcl_remap_impl_t), allocator.state); + output_rule->impl = + allocator.allocate(sizeof(rcl_remap_impl_t), allocator.state); if (NULL == output_rule->impl) { return RCL_RET_BAD_ALLOC; } @@ -1800,25 +1805,31 @@ _rcl_parse_remap_rule( output_rule->impl->replacement = NULL; rcl_lexer_lookahead2_t lex_lookahead = rcl_get_zero_initialized_lexer_lookahead2(); + rcl_ret_t ret = rcl_lexer_lookahead2_init(&lex_lookahead, arg, allocator); - ret = rcl_lexer_lookahead2_init(&lex_lookahead, arg, allocator); - if (RCL_RET_OK != ret) { - return ret; - } - - ret = _rcl_parse_remap_begin_remap_rule(&lex_lookahead, output_rule); + if (RCL_RET_OK == ret) { + ret = _rcl_parse_remap_begin_remap_rule(&lex_lookahead, output_rule); - if (RCL_RET_OK != ret) { - // cleanup stuff, but return the original error code - if (RCL_RET_OK != rcl_remap_fini(output_rule)) { - RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to fini remap rule after error occurred"); - } - if (RCL_RET_OK != rcl_lexer_lookahead2_fini(&lex_lookahead)) { - RCUTILS_LOG_ERROR_NAMED(ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred"); + rcl_ret_t fini_ret = rcl_lexer_lookahead2_fini(&lex_lookahead); + if (RCL_RET_OK != ret) { + if (RCL_RET_OK != fini_ret) { + RCUTILS_LOG_ERROR_NAMED( + ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred"); + } + } else { + if (RCL_RET_OK == fini_ret) { + return RCL_RET_OK; + } + ret = fini_ret; } - } else { - ret = rcl_lexer_lookahead2_fini(&lex_lookahead); } + + // cleanup output rule but keep first error return code + if (RCL_RET_OK != rcl_remap_fini(output_rule)) { + RCUTILS_LOG_ERROR_NAMED( + ROS_PACKAGE_NAME, "Failed to fini remap rule after error occurred"); + } + return ret; } @@ -1915,6 +1926,8 @@ _rcl_parse_param_file( return RCL_RET_BAD_ALLOC; } if (!rcl_parse_yaml_file(*param_file, params)) { + allocator.deallocate(*param_file, allocator.state); + *param_file = NULL; // Error message already set. return RCL_RET_ERROR; } From f8ad2636957128ec9bdcc6aacfcef41bfb0c5168 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Tue, 1 Sep 2020 10:56:50 -0300 Subject: [PATCH 2/2] Please uncrustify. Signed-off-by: Michel Hidalgo --- rcl/src/rcl/arguments.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index 7ee57bdca..58a74f1e5 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1814,7 +1814,7 @@ _rcl_parse_remap_rule( if (RCL_RET_OK != ret) { if (RCL_RET_OK != fini_ret) { RCUTILS_LOG_ERROR_NAMED( - ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred"); + ROS_PACKAGE_NAME, "Failed to fini lookahead2 after error occurred"); } } else { if (RCL_RET_OK == fini_ret) {