From 63a1d1985846ef486255087423738bde7d512b3f Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Thu, 27 Aug 2020 09:39:54 -0300 Subject: [PATCH 1/3] Add fix wait.c init/fini allocation on error Signed-off-by: Jorge Perez --- rcl/src/rcl/wait.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/rcl/src/rcl/wait.c b/rcl/src/rcl/wait.c index 79556df9f..a23c77f7b 100644 --- a/rcl/src/rcl/wait.c +++ b/rcl/src/rcl/wait.c @@ -86,13 +86,13 @@ rcl_wait_set_is_valid(const rcl_wait_set_t * wait_set) } static void -__wait_set_clean_up(rcl_wait_set_t * wait_set, rcl_allocator_t allocator) +__wait_set_clean_up(rcl_wait_set_t * wait_set) { rcl_ret_t ret = rcl_wait_set_resize(wait_set, 0, 0, 0, 0, 0, 0); (void)ret; // NO LINT assert(RCL_RET_OK == ret); // Defensive, shouldn't fail with size 0. if (wait_set->impl) { - allocator.deallocate(wait_set->impl, allocator.state); + wait_set->impl->allocator.deallocate(wait_set->impl, wait_set->impl->allocator.state); wait_set->impl = NULL; } } @@ -146,6 +146,10 @@ rcl_wait_set_init( wait_set->impl->rmw_services.service_count = 0; wait_set->impl->rmw_events.events = NULL; wait_set->impl->rmw_events.event_count = 0; + // Set context. + wait_set->impl->context = context; + // Set allocator. + wait_set->impl->allocator = allocator; size_t num_conditions = (2 * number_of_subscriptions) + @@ -159,10 +163,6 @@ rcl_wait_set_init( goto fail; } - // Set context. - wait_set->impl->context = context; - // Set allocator. - wait_set->impl->allocator = allocator; // Initialize subscription space. rcl_ret_t ret = rcl_wait_set_resize( wait_set, number_of_subscriptions, number_of_guard_conditions, number_of_timers, @@ -179,7 +179,7 @@ rcl_wait_set_init( fail_ret = RCL_RET_WAIT_SET_INVALID; } } - __wait_set_clean_up(wait_set, allocator); + __wait_set_clean_up(wait_set); return fail_ret; } @@ -195,7 +195,7 @@ rcl_wait_set_fini(rcl_wait_set_t * wait_set) RCL_SET_ERROR_MSG(rmw_get_error_string().str); result = RCL_RET_WAIT_SET_INVALID; } - __wait_set_clean_up(wait_set, wait_set->impl->allocator); + __wait_set_clean_up(wait_set); } return result; } @@ -300,6 +300,7 @@ rcl_wait_set_get_allocator(const rcl_wait_set_t * wait_set, rcl_allocator_t * al wait_set->impl->RMWStorage, sizeof(void *) * Type ## s_size, allocator.state); \ if (!wait_set->impl->RMWStorage) { \ allocator.deallocate((void *)wait_set->Type ## s, allocator.state); \ + wait_set->Type ## s = NULL; \ wait_set->size_of_ ## Type ## s = 0; \ RCL_SET_ERROR_MSG("allocating memory failed"); \ return RCL_RET_BAD_ALLOC; \ From 5f03f7dd496efec372df456f4f7b54aaad422b7e Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Thu, 27 Aug 2020 09:40:37 -0300 Subject: [PATCH 2/3] Add test for the fix Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 82244406f..0f011abdf 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -684,3 +684,32 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_get_allocator ret = rcl_wait_set_fini(&wait_set); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } + +// Test proper error handling with a fault injection test +TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_init_fail) { + rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set(); + rcl_ret_t ret = RCL_RET_OK; + rcl_allocator_t alloc = rcl_get_default_allocator(); + + RCUTILS_FAULT_INJECTION_TEST( + { + ret = rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, alloc); + if (RCL_RET_OK == ret) { + EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); + } else { + EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); + rcl_reset_error(); + } + }); + + RCUTILS_FAULT_INJECTION_TEST( + { + ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc); + if (RCL_RET_OK == ret) { + EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); + } else { + EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); + rcl_reset_error(); + } + }); +} From 66fd770d8f9d3fdc4f2b01d4f028b6dab7b4a37d Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Thu, 27 Aug 2020 16:38:14 -0300 Subject: [PATCH 3/3] Remove unit tests Signed-off-by: Jorge Perez --- rcl/test/rcl/test_wait.cpp | 29 ----------------------------- 1 file changed, 29 deletions(-) diff --git a/rcl/test/rcl/test_wait.cpp b/rcl/test/rcl/test_wait.cpp index 0f011abdf..82244406f 100644 --- a/rcl/test/rcl/test_wait.cpp +++ b/rcl/test/rcl/test_wait.cpp @@ -684,32 +684,3 @@ TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_get_allocator ret = rcl_wait_set_fini(&wait_set); EXPECT_EQ(RCL_RET_OK, ret) << rcl_get_error_string().str; } - -// Test proper error handling with a fault injection test -TEST_F(CLASSNAME(WaitSetTestFixture, RMW_IMPLEMENTATION), wait_set_test_maybe_init_fail) { - rcl_wait_set_t wait_set = rcl_get_zero_initialized_wait_set(); - rcl_ret_t ret = RCL_RET_OK; - rcl_allocator_t alloc = rcl_get_default_allocator(); - - RCUTILS_FAULT_INJECTION_TEST( - { - ret = rcl_wait_set_init(&wait_set, 1, 0, 0, 1, 1, 0, context_ptr, alloc); - if (RCL_RET_OK == ret) { - EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); - } else { - EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); - rcl_reset_error(); - } - }); - - RCUTILS_FAULT_INJECTION_TEST( - { - ret = rcl_wait_set_init(&wait_set, 1, 1, 1, 1, 1, 1, context_ptr, alloc); - if (RCL_RET_OK == ret) { - EXPECT_EQ(RCL_RET_OK, rcl_wait_set_fini(&wait_set)); - } else { - EXPECT_TRUE(RCL_RET_WAIT_SET_INVALID == ret || RCL_RET_BAD_ALLOC == ret); - rcl_reset_error(); - } - }); -}