Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix rcl arguments' API memory leaks and bugs. #778

Merged
merged 2 commits into from
Sep 1, 2020
Merged

Fix rcl arguments' API memory leaks and bugs. #778

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4757a19
Fix rcl arguments' API memory leaks.
hidmic Aug 31, 2020
f8ad263
Please uncrustify.
hidmic Sep 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 44 additions & 31 deletions rcl/src/rcl/arguments.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,30 +594,36 @@ 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
if (0 == args_impl->num_param_files_args) {
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.
Expand Down Expand Up @@ -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(
Expand All @@ -878,6 +883,7 @@ rcl_arguments_copy(
}
return ret;
}
++(args_out->impl->num_remap_rules);
}
}

Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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;
}
Expand All @@ -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");
}
Comment on lines +1827 to +1831
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I agree with this logic, or, at least, I don't understand.

It looks to me with this change that we always cleanup output_rule, but that seems weird; we don't remap_init it in this method. Shouldn't that be the responsibility of the caller?

In other words, it kind of looks to me like this method should be:

init_lexer();
if (failed_to_init_lexer)
  return fail;
parse_remap_begin_remap_rule(lexer, output_rule);
cleanup_lexer();
if (!ret) {
  if (!fini_ret)
    print_error();
  return ret;
}

And then the caller is responsible for remap_fini as appropriate. Could you explain a little more what the idea is here?

Copy link
Contributor Author

@hidmic hidmic Sep 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me with this change that we always cleanup output_rule, but that seems weird

Only when something fails.

we don't remap_init it in this method.

We do, right above. Thing is, remap_init doesn't exist.

Shouldn't that be the responsibility of the caller?

Right now it's not necessary. _rcl_remap_parse_rule takes a zero-initialized rcl_remap_t instance. On failure, it should stay zero-initialized.

It could be the case, but with the current rcl_remap_t structure and API implementation it doesn't add much value. The init/fini cycle would still be coupled to the parse API. However, if rcl_remap_parse() was capable of reusing rcl_remap_t structures (i.e. reuse past allocations), and if there was an rcl_remap_move() API to copy rules cheaply, then relocating the init/fini cycle could save up quite a few unnecessary allocation/deallocation cycles. I didn't want to go that far.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, right above. Thing is, remap_init doesn't exist.

Oh, now I see. OK, so that block of code is essentially remap_init, it just doesn't have that name.

Right now it's not necessary. _rcl_remap_parse_rule takes a zero-initialized rcl_remap_t instance. On failure, it should stay zero-initialized.

I see. So this PR is a cleanup of the allocated memory on failure further down.

I still find the way this is done weird. The rcl_remap_t is currently an "inout" parameter, but it seems to me that it should just be an "out" parameter. If we consider it more like that, then the caller can pass it in uninitialized. This method would initialize it and add to it on success, and the caller would (eventually) need to free it. On error, the parameter would still be considered "uninitialized".

The above just makes more sense to me. That being said, I don't know how much you want to refactor this here. So I'll leave it up to you whether you want to do that or just go with this code as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So this PR is a cleanup of the allocated memory on failure further down.

Exactly.

I still find the way this is done weird. The rcl_remap_t is currently an "inout" parameter, but it seems to me that it should just be an "out" parameter. If we consider it more like that, then the caller can pass it in uninitialized. This method would initialize it and add to it on success, and the caller would (eventually) need to free it. On error, the parameter would still be considered "uninitialized".

That's almost exactly how it is handled right now, except that _rcl_remap_parse_rule does not explicitly zero-initialize the given rcl_remap_t structure, it assumes it is. So in a way it is an output parameter.


return ret;
}

Expand Down Expand Up @@ -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;
}
Expand Down