Skip to content

Address issue #716 by zero initializing pointers and freeing memory #717

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

Merged
brawner merged 1 commit into from
Jul 16, 2020
Merged

Address issue #716 by zero initializing pointers and freeing memory #717

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 58 additions & 23 deletions rcl_action/src/rcl_action/action_client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,58 @@ rcl_action_get_zero_initialized_client(void)
return null_action_client;
}

rcl_action_client_impl_t
_rcl_action_get_zero_initialized_client_impl(void)
{
rcl_client_t null_client = rcl_get_zero_initialized_client();
rcl_subscription_t null_subscription = rcl_get_zero_initialized_subscription();
rcl_action_client_impl_t null_action_client = {
null_client,
null_client,
null_client,
null_subscription,
null_subscription,
rcl_action_client_get_default_options(),
NULL,
0,
0,
0,
0,
0
};
return null_action_client;
}

rcl_ret_t
_rcl_action_client_fini_impl(
rcl_action_client_t * action_client, rcl_node_t * node, rcl_allocator_t allocator)
{
if (NULL == action_client->impl) {
return RCL_RET_OK;
}
rcl_ret_t ret = RCL_RET_OK;
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) {
ret = RCL_RET_ERROR;
}
allocator.deallocate(action_client->impl->action_name, allocator.state);
allocator.deallocate(action_client->impl, allocator.state);
action_client->impl = NULL;
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized");
return ret;
}

// \internal Initializes an action client specific service client.
#define CLIENT_INIT(Type) \
char * Type ## _service_name = NULL; \
Expand Down Expand Up @@ -148,9 +200,12 @@ rcl_action_client_init(
RCL_CHECK_FOR_NULL_WITH_MSG(
action_client->impl, "allocating memory failed", return RCL_RET_BAD_ALLOC);

// Avoid uninitialized pointers should initialization fail
*action_client->impl = _rcl_action_get_zero_initialized_client_impl();
// Copy action client name and options.
action_client->impl->action_name = rcutils_strdup(action_name, allocator);
if (NULL == action_client->impl->action_name) {
RCL_SET_ERROR_MSG("failed to duplicate action name");

@fujitatomoya fujitatomoya Jul 16, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This might be overwritten by https://github.com/ros2/rcl/pull/717/files?file-filters%5B%5D=.c#diff-1f30210b7875b9b39020ed2b9c12bd78L173

@brawner brawner Jul 16, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Are there any specific circumstances you are worried about? Setting impl to get the zero_initialized values should put the object into a known state, which helps ensure cleanup occurs predictably. Otherwise if there is a double error, then it can be helpful for the message to be set twice, so that the log file has a record of the sequence of events that failed.

@fujitatomoya fujitatomoya Jul 16, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Actually no, but there's been discussion about this, #713 (comment). I do not think that this is the problem but status. I'd think it might be better to share.

ret = RCL_RET_BAD_ALLOC;
goto fail;
}
Expand All @@ -168,7 +223,7 @@ rcl_action_client_init(
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client initialized");
return ret;
fail:
fini_ret = rcl_action_client_fini(action_client, node);
fini_ret = _rcl_action_client_fini_impl(action_client, node, allocator);
if (RCL_RET_OK != fini_ret) {
RCL_SET_ERROR_MSG("failed to cleanup action client");
ret = RCL_RET_ERROR;
Expand All @@ -186,28 +241,8 @@ rcl_action_client_fini(rcl_action_client_t * action_client, rcl_node_t * node)
if (!rcl_node_is_valid_except_context(node)) {
return RCL_RET_NODE_INVALID; // error already set
}
rcl_ret_t ret = RCL_RET_OK;
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->goal_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->cancel_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_client_fini(&action_client->impl->result_client, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->feedback_subscription, node)) {
ret = RCL_RET_ERROR;
}
if (RCL_RET_OK != rcl_subscription_fini(&action_client->impl->status_subscription, node)) {
ret = RCL_RET_ERROR;
}
rcl_allocator_t * allocator = &action_client->impl->options.allocator;
allocator->deallocate(action_client->impl->action_name, allocator->state);
allocator->deallocate(action_client->impl, allocator->state);
action_client->impl = NULL;
RCUTILS_LOG_DEBUG_NAMED(ROS_PACKAGE_NAME, "Action client finalized");
return ret;
return
_rcl_action_client_fini_impl(action_client, node, action_client->impl->options.allocator);
}

rcl_action_client_options_t
Expand Down
26 changes: 20 additions & 6 deletions rcl_action/test/rcl_action/test_action_client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,19 +154,33 @@ TEST_F(TestActionClientBaseFixture, test_action_client_init_fini) {
EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str;
rcl_reset_error();

// Fail copying action_name string, this will also fail to cleanup and will return generic error
// Fail copying action name
time_bomb_state.malloc_count_until_failure = 1;
invalid_action_client_options.allocator.state = &time_bomb_state;
ret = rcl_action_client_init(
&action_client, &this->node, action_typesupport,
action_name, &invalid_action_client_options);
EXPECT_EQ(ret, RCL_RET_ERROR) << rcl_get_error_string().str;
EXPECT_EQ(ret, RCL_RET_BAD_ALLOC) << rcl_get_error_string().str;
rcl_reset_error();

// Manually cleanup to setup for next test
action_client_options.allocator.deallocate(
action_client.impl, action_client_options.allocator.state);
action_client.impl = NULL;
ret = RCL_RET_OK;
int i = 0;
do {
time_bomb_state.malloc_count_until_failure = i;
i++;
invalid_action_client_options.allocator.state = &time_bomb_state;
ret = rcl_action_client_init(
&action_client, &this->node, action_typesupport,
action_name, &invalid_action_client_options);
if (RCL_RET_OK != ret) {
EXPECT_TRUE(rcl_error_is_set());
rcl_reset_error();
} else {
EXPECT_EQ(RCL_RET_OK, rcl_action_client_fini(&action_client, &this->node)) <<
rcl_get_error_string().str;
EXPECT_FALSE(rcl_error_is_set());
}
} while (ret != RCL_RET_OK);

ret = rcl_action_client_init(
&action_client, &this->node, action_typesupport,
Expand Down