niklas/rcl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix memory problems in rcl. (#158)

Browse source 
* Fix memory problems in rcl.

There are actually two problems here.  In one of the problems,
we were sometimes taking an rcl_guard_condition that we did *not*
allocate, and trying to deallocate it.  This was leading to
double frees.

The second problem was forgetting to call guard_condition_fini
during node_fini.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

* Drop duplicated check for overwriting previous error.

As pointed out by Dirk in review, the RCL_SET_ERROR_MSG
macro (through the rcutils_set_error_state() function)
already checks to see if a message is being dropped and
warns about it.  Thus, we don't need to do it ourselves.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
This commit is contained in:
Chris Lalancette 2017-08-10 15:11:16 -07:00 • committed by GitHub
parent cdedde8c91
commit c1232a7288
2 changed files with 12 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

5
rcl/src/rcl/guard_condition.c
View file

@ -27,6 +27,7 @@ extern "C"
typedef struct rcl_guard_condition_impl_t typedef struct rcl_guard_condition_impl_t
{ {
rmw_guard_condition_t * rmw_handle; rmw_guard_condition_t * rmw_handle;
bool allocated_rmw_guard_condition;
rcl_guard_condition_options_t options; rcl_guard_condition_options_t options;
} rcl_guard_condition_impl_t; } rcl_guard_condition_impl_t;
@ -76,6 +77,7 @@ __rcl_guard_condition_init_from_rmw_impl(
if (rmw_guard_condition) { if (rmw_guard_condition) {
// If given, just assign (cast away const). // If given, just assign (cast away const).
guard_condition->impl->rmw_handle = (rmw_guard_condition_t *)rmw_guard_condition; guard_condition->impl->rmw_handle = (rmw_guard_condition_t *)rmw_guard_condition;
guard_condition->impl->allocated_rmw_guard_condition = false;
} else { } else {
// Otherwise create one. // Otherwise create one.
guard_condition->impl->rmw_handle = rmw_create_guard_condition(); guard_condition->impl->rmw_handle = rmw_create_guard_condition();
@ -85,6 +87,7 @@ __rcl_guard_condition_init_from_rmw_impl(
RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), *allocator); RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), *allocator);
return RCL_RET_ERROR; return RCL_RET_ERROR;
} }
guard_condition->impl->allocated_rmw_guard_condition = true;
} }
// Copy options into impl. // Copy options into impl.
guard_condition->impl->options = options; guard_condition->impl->options = options;
@ -119,7 +122,7 @@ rcl_guard_condition_fini(rcl_guard_condition_t * guard_condition)
if (guard_condition->impl) { if (guard_condition->impl) {
// assuming the allocator is valid because it is checked in rcl_guard_condition_init() // assuming the allocator is valid because it is checked in rcl_guard_condition_init()
rcl_allocator_t allocator = guard_condition->impl->options.allocator; rcl_allocator_t allocator = guard_condition->impl->options.allocator;
if (guard_condition->impl->rmw_handle) { if (guard_condition->impl->rmw_handle && guard_condition->impl->allocated_rmw_guard_condition) {
if (rmw_destroy_guard_condition(guard_condition->impl->rmw_handle) != RMW_RET_OK) { if (rmw_destroy_guard_condition(guard_condition->impl->rmw_handle) != RMW_RET_OK) {
RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator); RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator);
result = RCL_RET_ERROR; result = RCL_RET_ERROR;

10
rcl/src/rcl/node.c
View file

@ -323,11 +323,17 @@ rcl_node_fini(rcl_node_t * node)
} }
rcl_allocator_t allocator = node->impl->options.allocator; rcl_allocator_t allocator = node->impl->options.allocator;
rcl_ret_t result = RCL_RET_OK; rcl_ret_t result = RCL_RET_OK;
rmw_ret_t ret = rmw_destroy_node(node->impl->rmw_node_handle); rmw_ret_t rmw_ret = rmw_destroy_node(node->impl->rmw_node_handle);
if (ret != RMW_RET_OK) { if (rmw_ret != RMW_RET_OK) {
RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator); RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator);
result = RCL_RET_ERROR; result = RCL_RET_ERROR;
} }
rcl_ret_t rcl_ret = rcl_guard_condition_fini(node->impl->graph_guard_condition);
if (rcl_ret != RCL_RET_OK) {
RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator);
result = RCL_RET_ERROR;
}
allocator.deallocate(node->impl->graph_guard_condition, allocator.state);
// assuming that allocate and deallocate are ok since they are checked in init // assuming that allocate and deallocate are ok since they are checked in init
allocator.deallocate(node->impl, allocator.state); allocator.deallocate(node->impl, allocator.state);
node->impl = NULL; node->impl = NULL;