Skip to content

Avoid leaking init options in context. #198

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

Closed
wants to merge 2 commits into from
Closed

Avoid leaking init options in context. #198

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be6852e
Avoid leaking init options in context.
hidmic Jun 22, 2020
5c59e4f
Address peer review comments.
hidmic Jun 22, 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
27 changes: 19 additions & 8 deletions rmw_cyclonedds_cpp/src/rmw_node.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,17 +1127,24 @@ extern "C" rmw_ret_t rmw_init(const rmw_init_options_t * options, rmw_context_t
context->implementation_identifier = eclipse_cyclonedds_identifier;
context->impl = nullptr;

if ((ret = rmw_init_options_copy(options, &context->options)) != RMW_RET_OK) {
return ret;
ret = rmw_init_options_copy(options, &context->options);
if (RMW_RET_OK != ret) {
goto fail;
}

rmw_context_impl_t * impl = new(std::nothrow) rmw_context_impl_t();
if (nullptr == impl) {
return RMW_RET_BAD_ALLOC;
context->impl = new(std::nothrow) rmw_context_impl_t();
if (nullptr == context->impl) {
ret = RMW_RET_BAD_ALLOC;
goto fail;
}

context->impl = impl;
return RMW_RET_OK;
return ret;
fail:
if (RMW_RET_OK != rmw_init_options_fini(&context->options)) {
Copy link
Collaborator

@eboasson eboasson Jun 23, 2020

Choose a reason for hiding this comment

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

Is it safe to call fini if init failed? Just checking ...

Copy link
Member

@ivanpauno ivanpauno Jun 23, 2020

Choose a reason for hiding this comment

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

good point, I think it's not.
@hidmic maybe just returning an error in line 1132.

Copy link
Contributor Author

@hidmic hidmic Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Well, arguably it should not and both rmw_init_options_init() or rmw_init_options_copy() should guarantee output options remain uninitialized. Reality is that such guarantee is not documented in rmw API and most implementations do not really check. rmw_cyclonedds_cpp isn't the exception. Thus the extra fini, which judging by the code isn't unsafe (though again, not necessarily correct nor ideal).

Alternatively, we can establish that guarantee and change implementations to abide to it. Thoughts?

Copy link
Member

@ivanpauno ivanpauno Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Alternatively, we can establish that guarantee and change implementations to abide to it. Thoughts?

👍
If I call *_init() or *_copy() and the function fails, I expect it to cleanup all the resources it created (and not having to do it manually).
In the case of *_copy(src, dst), I also expect the function to no modify *dst if it failed to do the copy (if this cannot be guaranteed, it should be documented).

It's the more usual pattern in C code IMO.

Copy link
Contributor Author

@hidmic hidmic Jun 23, 2020

Choose a reason for hiding this comment

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

See ros2/rmw#244.

RMW_SAFE_FWRITE_TO_STDERR(
"'rmw_init_options_fini' failed while being executed due to '"
RCUTILS_STRINGIFY(__function__) "' failing.\n");
}
return ret;
}

extern "C" rmw_ret_t rmw_shutdown(rmw_context_t * context)
Expand All @@ -1161,6 +1168,10 @@ extern "C" rmw_ret_t rmw_context_fini(rmw_context_t * context)
context->implementation_identifier,
eclipse_cyclonedds_identifier,
return RMW_RET_INCORRECT_RMW_IMPLEMENTATION);
rmw_ret_t ret = rmw_init_options_fini(&context->options);
if (RMW_RET_OK != ret) {
return ret;
}
delete context->impl;
*context = rmw_get_zero_initialized_context();
return RMW_RET_OK;
Expand Down