From c1232a72887a4f117302bba61164f5c7fde1f9ae Mon Sep 17 00:00:00 2001 From: Chris Lalancette Date: Thu, 10 Aug 2017 15:11:16 -0700 Subject: [PATCH] Fix memory problems in rcl. (#158) * 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 * 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 --- rcl/src/rcl/guard_condition.c | 5 ++++- rcl/src/rcl/node.c | 10 ++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/rcl/src/rcl/guard_condition.c b/rcl/src/rcl/guard_condition.c index 381f2e0..07418ad 100644 --- a/rcl/src/rcl/guard_condition.c +++ b/rcl/src/rcl/guard_condition.c @@ -27,6 +27,7 @@ extern "C" typedef struct rcl_guard_condition_impl_t { rmw_guard_condition_t * rmw_handle; + bool allocated_rmw_guard_condition; rcl_guard_condition_options_t options; } rcl_guard_condition_impl_t; @@ -76,6 +77,7 @@ __rcl_guard_condition_init_from_rmw_impl( if (rmw_guard_condition) { // If given, just assign (cast away const). guard_condition->impl->rmw_handle = (rmw_guard_condition_t *)rmw_guard_condition; + guard_condition->impl->allocated_rmw_guard_condition = false; } else { // Otherwise create one. 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); return RCL_RET_ERROR; } + guard_condition->impl->allocated_rmw_guard_condition = true; } // Copy options into impl. guard_condition->impl->options = options; @@ -119,7 +122,7 @@ rcl_guard_condition_fini(rcl_guard_condition_t * guard_condition) if (guard_condition->impl) { // assuming the allocator is valid because it is checked in rcl_guard_condition_init() 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) { RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator); result = RCL_RET_ERROR; diff --git a/rcl/src/rcl/node.c b/rcl/src/rcl/node.c index 7088cf9..0f45a7f 100644 --- a/rcl/src/rcl/node.c +++ b/rcl/src/rcl/node.c @@ -323,11 +323,17 @@ rcl_node_fini(rcl_node_t * node) } rcl_allocator_t allocator = node->impl->options.allocator; rcl_ret_t result = RCL_RET_OK; - rmw_ret_t ret = rmw_destroy_node(node->impl->rmw_node_handle); - if (ret != RMW_RET_OK) { + rmw_ret_t rmw_ret = rmw_destroy_node(node->impl->rmw_node_handle); + if (rmw_ret != RMW_RET_OK) { RCL_SET_ERROR_MSG(rmw_get_error_string_safe(), allocator); 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 allocator.deallocate(node->impl, allocator.state); node->impl = NULL;