Skip to content

[foxy backport] Address issue 716 by zero initializing pointers and freeing memory (#717) #820

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
Oct 5, 2020
Merged

[foxy backport] Address issue 716 by zero initializing pointers and freeing memory (#717) #820

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");
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