From 1e036bd8450b5f967ea3f2f1dee83f988180d343 Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Fri, 31 Jan 2020 11:41:04 +0900 Subject: [PATCH 1/8] `rcl_arguments_fini` free external_log_config_file (#469) Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index a8242f0b3..f24c33683 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1117,6 +1117,11 @@ rcl_arguments_fini( args->impl->parameter_files = NULL; } + if (NULL != args->impl->external_log_config_file) { + args->impl->allocator.deallocate(args->impl->external_log_config_file, args->impl->allocator.state); + args->impl->external_log_config_file = NULL; + } + args->impl->allocator.deallocate(args->impl, args->impl->allocator.state); args->impl = NULL; return ret; From 84ae7d2192088045188d61ca064a7936326f28de Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Fri, 31 Jan 2020 12:11:58 +0900 Subject: [PATCH 2/8] Fix SEGV in last commit, rcl_arguments_copy init failure. Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 1 + 1 file changed, 1 insertion(+) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index f24c33683..f4a399c42 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -984,6 +984,7 @@ rcl_arguments_copy( args_out->impl->parameter_overrides = NULL; args_out->impl->parameter_files = NULL; args_out->impl->num_param_files_args = 0; + args_out->impl->external_log_config_file = NULL; if (args->impl->num_unparsed_args) { // Copy unparsed args From a24777cc365b12927d751bf9c4823ff2d3ea3bcf Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Fri, 31 Jan 2020 14:14:53 +0900 Subject: [PATCH 3/8] Fix memory leak in `parse_key` in parser.c Signed-off-by: y-okumura-isp --- rcl_yaml_param_parser/src/parser.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rcl_yaml_param_parser/src/parser.c b/rcl_yaml_param_parser/src/parser.c index c67df24b7..d0b7195d2 100644 --- a/rcl_yaml_param_parser/src/parser.c +++ b/rcl_yaml_param_parser/src/parser.c @@ -1347,7 +1347,9 @@ static rcutils_ret_t parse_key( break; } - ret = rem_name_from_ns(ns_tracker, NS_TYPE_NODE, allocator); + allocator.deallocate(node_name_ns, allocator.state); + + ret = rem_name_from_ns(ns_tracker, NS_TYPE_NODE, allocator); if (RCUTILS_RET_OK != ret) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Internal error adding node namespace at line %d", line_num); From 31ea8ac42b0f20a1191b9ebb12809bdb9935bdb5 Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Mon, 3 Feb 2020 13:43:29 +0900 Subject: [PATCH 4/8] Remove tab indent Signed-off-by: y-okumura-isp --- rcl_yaml_param_parser/src/parser.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rcl_yaml_param_parser/src/parser.c b/rcl_yaml_param_parser/src/parser.c index d0b7195d2..c3ee6188f 100644 --- a/rcl_yaml_param_parser/src/parser.c +++ b/rcl_yaml_param_parser/src/parser.c @@ -1347,7 +1347,7 @@ static rcutils_ret_t parse_key( break; } - allocator.deallocate(node_name_ns, allocator.state); + allocator.deallocate(node_name_ns, allocator.state); ret = rem_name_from_ns(ns_tracker, NS_TYPE_NODE, allocator); if (RCUTILS_RET_OK != ret) { From 1f9a2e9b5fd582a679a8c19038099908db4de987 Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Mon, 3 Feb 2020 15:00:43 +0900 Subject: [PATCH 5/8] Extract allocation and initialization of arguments_impl (#469) Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 82 +++++++++++++++++++++++++---------------- 1 file changed, 50 insertions(+), 32 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index f4a399c42..d0f6792d9 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -299,6 +299,16 @@ _atob( const char * str, bool * val); +/// Allocate and zero initialize arguments impl and. +/** + * \param[out] args target arguments to set impl + * \param[in] allocator an allocator to use + * \return RCL_RET_OK if a valid rule was parsed, or + * \return RCL_RET_BAD_ALLOC if an allocation failed + */ +rcl_ret_t +_rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t * allocator); + rcl_ret_t rcl_parse_arguments( int argc, @@ -323,26 +333,15 @@ rcl_parse_arguments( rcl_ret_t ret; rcl_ret_t fail_ret; - args_output->impl = allocator.allocate(sizeof(rcl_arguments_impl_t), allocator.state); - if (NULL == args_output->impl) { - return RCL_RET_BAD_ALLOC; + ret = _rcl_allocate_initialized_arguments_impl(args_output, &allocator); + if (RCL_RET_OK != ret) { + return ret; } rcl_arguments_impl_t * args_impl = args_output->impl; - args_impl->num_remap_rules = 0; - args_impl->remap_rules = NULL; args_impl->log_level = -1; - args_impl->external_log_config_file = NULL; - args_impl->unparsed_args = NULL; - args_impl->num_unparsed_args = 0; - args_impl->unparsed_ros_args = NULL; - args_impl->num_unparsed_ros_args = 0; - args_impl->parameter_overrides = NULL; - args_impl->parameter_files = NULL; - args_impl->num_param_files_args = 0; args_impl->log_stdout_disabled = false; args_impl->log_rosout_disabled = false; args_impl->log_ext_lib_disabled = false; - args_impl->allocator = allocator; if (argc == 0) { // there are no arguments to parse @@ -967,24 +966,12 @@ rcl_arguments_copy( rcl_allocator_t allocator = args->impl->allocator; - args_out->impl = allocator.allocate(sizeof(rcl_arguments_impl_t), allocator.state); - if (NULL == args_out->impl) { - return RCL_RET_BAD_ALLOC; - } - - args_out->impl->allocator = allocator; - - // Zero so it's safe to call rcl_arguments_fini() if an error occurrs while copying. - args_out->impl->num_remap_rules = 0; - args_out->impl->remap_rules = NULL; - args_out->impl->unparsed_args = NULL; - args_out->impl->num_unparsed_args = 0; - args_out->impl->unparsed_ros_args = NULL; - args_out->impl->num_unparsed_ros_args = 0; - args_out->impl->parameter_overrides = NULL; - args_out->impl->parameter_files = NULL; - args_out->impl->num_param_files_args = 0; - args_out->impl->external_log_config_file = NULL; + do { + rcl_ret_t ret = _rcl_allocate_initialized_arguments_impl(args_out, &allocator); + if (RCL_RET_OK != ret) { + return ret; + } + } while(false); if (args->impl->num_unparsed_args) { // Copy unparsed args @@ -2100,6 +2087,37 @@ _atob( return RCL_RET_ERROR; } +rcl_ret_t +_rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t * allocator) +{ + args->impl = allocator->allocate(sizeof(rcl_arguments_impl_t), allocator->state); + if (NULL == args->impl) { + return RCL_RET_BAD_ALLOC; + } + + // TODO(y-okumura-isp): log_level and log_*disabled are initialized in rcl_parse_arguments() + // but not in rcl_arguments_copy(). + // Check they are only forgotten or there is some reason. + rcl_arguments_impl_t * args_impl = args->impl; + args_impl->num_remap_rules = 0; + args_impl->remap_rules = NULL; + // args_impl->log_level = -1; + args_impl->external_log_config_file = NULL; + args_impl->unparsed_args = NULL; + args_impl->num_unparsed_args = 0; + args_impl->unparsed_ros_args = NULL; + args_impl->num_unparsed_ros_args = 0; + args_impl->parameter_overrides = NULL; + args_impl->parameter_files = NULL; + args_impl->num_param_files_args = 0; + // args_impl->log_stdout_disabled = false; + // args_impl->log_rosout_disabled = false; + // args_impl->log_ext_lib_disabled = false; + args_impl->allocator = *allocator; + + return RCL_RET_OK; +} + #ifdef __cplusplus } #endif From 5ceb13dadb7351ccb60f9b670e007747ef3996a0 Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Thu, 13 Feb 2020 11:51:29 +0900 Subject: [PATCH 6/8] Use same default values at rcl_parse_arguments & rcl_arguments_copy Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index d0f6792d9..ad7443f2a 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -338,10 +338,6 @@ rcl_parse_arguments( return ret; } rcl_arguments_impl_t * args_impl = args_output->impl; - args_impl->log_level = -1; - args_impl->log_stdout_disabled = false; - args_impl->log_rosout_disabled = false; - args_impl->log_ext_lib_disabled = false; if (argc == 0) { // there are no arguments to parse @@ -2095,13 +2091,10 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t return RCL_RET_BAD_ALLOC; } - // TODO(y-okumura-isp): log_level and log_*disabled are initialized in rcl_parse_arguments() - // but not in rcl_arguments_copy(). - // Check they are only forgotten or there is some reason. rcl_arguments_impl_t * args_impl = args->impl; args_impl->num_remap_rules = 0; args_impl->remap_rules = NULL; - // args_impl->log_level = -1; + args_impl->log_level = -1; args_impl->external_log_config_file = NULL; args_impl->unparsed_args = NULL; args_impl->num_unparsed_args = 0; @@ -2110,9 +2103,9 @@ _rcl_allocate_initialized_arguments_impl(rcl_arguments_t * args, rcl_allocator_t args_impl->parameter_overrides = NULL; args_impl->parameter_files = NULL; args_impl->num_param_files_args = 0; - // args_impl->log_stdout_disabled = false; - // args_impl->log_rosout_disabled = false; - // args_impl->log_ext_lib_disabled = false; + args_impl->log_stdout_disabled = false; + args_impl->log_rosout_disabled = false; + args_impl->log_ext_lib_disabled = false; args_impl->allocator = *allocator; return RCL_RET_OK; From 42239d6a00055a39a9792f530878ac2bcb9f9af2 Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Thu, 13 Feb 2020 12:12:52 +0900 Subject: [PATCH 7/8] Fix lint error Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index ad7443f2a..e97c6717d 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -1102,7 +1102,8 @@ rcl_arguments_fini( } if (NULL != args->impl->external_log_config_file) { - args->impl->allocator.deallocate(args->impl->external_log_config_file, args->impl->allocator.state); + args->impl->allocator.deallocate(args->impl->external_log_config_file, + args->impl->allocator.state); args->impl->external_log_config_file = NULL; } From 09a692c4101928afd6796d4b85566a47030a1c6a Mon Sep 17 00:00:00 2001 From: y-okumura-isp Date: Fri, 6 Mar 2020 11:50:41 +0900 Subject: [PATCH 8/8] Fix potential leak and improve readability Signed-off-by: y-okumura-isp --- rcl/src/rcl/arguments.c | 16 +++++++--------- rcl_yaml_param_parser/src/parser.c | 5 ++--- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/rcl/src/rcl/arguments.c b/rcl/src/rcl/arguments.c index e97c6717d..15fd3d796 100644 --- a/rcl/src/rcl/arguments.c +++ b/rcl/src/rcl/arguments.c @@ -962,12 +962,10 @@ rcl_arguments_copy( rcl_allocator_t allocator = args->impl->allocator; - do { - rcl_ret_t ret = _rcl_allocate_initialized_arguments_impl(args_out, &allocator); - if (RCL_RET_OK != ret) { - return ret; - } - } while(false); + rcl_ret_t ret = _rcl_allocate_initialized_arguments_impl(args_out, &allocator); + if (RCL_RET_OK != ret) { + return ret; + } if (args->impl->num_unparsed_args) { // Copy unparsed args @@ -1014,7 +1012,7 @@ rcl_arguments_copy( 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(); - rcl_ret_t ret = rcl_remap_copy( + ret = rcl_remap_copy( &(args->impl->remap_rules[i]), &(args_out->impl->remap_rules[i])); if (RCL_RET_OK != ret) { if (RCL_RET_OK != rcl_arguments_fini(args_out)) { @@ -1102,8 +1100,8 @@ rcl_arguments_fini( } if (NULL != args->impl->external_log_config_file) { - args->impl->allocator.deallocate(args->impl->external_log_config_file, - args->impl->allocator.state); + args->impl->allocator.deallocate( + args->impl->external_log_config_file, args->impl->allocator.state); args->impl->external_log_config_file = NULL; } diff --git a/rcl_yaml_param_parser/src/parser.c b/rcl_yaml_param_parser/src/parser.c index c3ee6188f..6ca41b8bf 100644 --- a/rcl_yaml_param_parser/src/parser.c +++ b/rcl_yaml_param_parser/src/parser.c @@ -1343,13 +1343,12 @@ static rcutils_ret_t parse_key( } ret = find_node(node_name_ns, params_st, node_idx); + allocator.deallocate(node_name_ns, allocator.state); if (RCUTILS_RET_OK != ret) { break; } - allocator.deallocate(node_name_ns, allocator.state); - - ret = rem_name_from_ns(ns_tracker, NS_TYPE_NODE, allocator); + ret = rem_name_from_ns(ns_tracker, NS_TYPE_NODE, allocator); if (RCUTILS_RET_OK != ret) { RCUTILS_SET_ERROR_MSG_WITH_FORMAT_STRING( "Internal error adding node namespace at line %d", line_num);