From af4251ddd84440c713062ecb6a66cd3d3583a546 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 10 Aug 2020 18:57:21 -0300 Subject: [PATCH 1/4] Add deallocate calls to free strdup allocated memory Signed-off-by: Jorge Perez --- .../rcl/rmw_implementation_identifier_check.c | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/rcl/src/rcl/rmw_implementation_identifier_check.c b/rcl/src/rcl/rmw_implementation_identifier_check.c index 9c068a67f..3e4764a98 100644 --- a/rcl/src/rcl/rmw_implementation_identifier_check.c +++ b/rcl/src/rcl/rmw_implementation_identifier_check.c @@ -59,6 +59,7 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) // If the environment variable RMW_IMPLEMENTATION is set, or // the environment variable RCL_ASSERT_RMW_ID_MATCHES is set, // check that the result of `rmw_get_implementation_identifier` matches. + rcl_ret_t fail_state = RCL_RET_OK; rcl_allocator_t allocator = rcl_get_default_allocator(); char * expected_rmw_impl = NULL; const char * expected_rmw_impl_env = NULL; @@ -69,14 +70,16 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error getting env var '" RCUTILS_STRINGIFY(RMW_IMPLEMENTATION_ENV_VAR_NAME) "': %s\n", get_env_error_str); - return RCL_RET_ERROR; + fail_state = RCL_RET_ERROR; + goto fail_cleanup; } if (strlen(expected_rmw_impl_env) > 0) { // Copy the environment variable so it doesn't get over-written by the next getenv call. expected_rmw_impl = rcutils_strdup(expected_rmw_impl_env, allocator); if (!expected_rmw_impl) { RCL_SET_ERROR_MSG("allocation failed"); - return RCL_RET_BAD_ALLOC; + fail_state = RCL_RET_BAD_ALLOC; + goto fail_cleanup; } } @@ -89,14 +92,16 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) "Error getting env var '" RCUTILS_STRINGIFY(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME) "': %s\n", get_env_error_str); - return RCL_RET_ERROR; + fail_state = RCL_RET_ERROR; + goto fail_cleanup; } if (strlen(asserted_rmw_impl_env) > 0) { // Copy the environment variable so it doesn't get over-written by the next getenv call. asserted_rmw_impl = rcutils_strdup(asserted_rmw_impl_env, allocator); if (!asserted_rmw_impl) { RCL_SET_ERROR_MSG("allocation failed"); - return RCL_RET_BAD_ALLOC; + fail_state = RCL_RET_BAD_ALLOC; + goto fail_cleanup; } } @@ -107,7 +112,8 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) "variables do not match, exiting with %d.", expected_rmw_impl, asserted_rmw_impl, RCL_RET_ERROR ); - return RCL_RET_ERROR; + fail_state = RCL_RET_ERROR; + goto fail_cleanup; } // Collapse the expected_rmw_impl and asserted_rmw_impl variables so only expected_rmw_impl needs @@ -137,7 +143,8 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) rmw_error_msg.str, RCL_RET_ERROR ); - return RCL_RET_ERROR; + fail_state = RCL_RET_ERROR; + goto fail_cleanup; } if (strcmp(actual_rmw_impl_id, expected_rmw_impl) != 0) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( @@ -146,12 +153,18 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) actual_rmw_impl_id, RCL_RET_MISMATCHED_RMW_ID ); - return RCL_RET_MISMATCHED_RMW_ID; + fail_state = RCL_RET_MISMATCHED_RMW_ID; + goto fail_cleanup; } // Free the memory now that all checking has passed. allocator.deallocate((char *)expected_rmw_impl, allocator.state); } return RCL_RET_OK; + +fail_cleanup: + allocator.deallocate((char *)expected_rmw_impl, allocator.state); + allocator.deallocate((char *)asserted_rmw_impl, allocator.state); + return fail_state; } INITIALIZER(initialize) { From 158d97395a5516a528f52605b11434fc167ab631 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Mon, 10 Aug 2020 19:52:19 -0300 Subject: [PATCH 2/4] Add variables to know if free is required Signed-off-by: Jorge Perez --- .../rcl/rmw_implementation_identifier_check.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/rcl/src/rcl/rmw_implementation_identifier_check.c b/rcl/src/rcl/rmw_implementation_identifier_check.c index 3e4764a98..d39454130 100644 --- a/rcl/src/rcl/rmw_implementation_identifier_check.c +++ b/rcl/src/rcl/rmw_implementation_identifier_check.c @@ -62,6 +62,7 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) rcl_ret_t fail_state = RCL_RET_OK; rcl_allocator_t allocator = rcl_get_default_allocator(); char * expected_rmw_impl = NULL; + bool expected_rmw_requires_free = false; const char * expected_rmw_impl_env = NULL; const char * get_env_error_str = rcutils_get_env( RMW_IMPLEMENTATION_ENV_VAR_NAME, @@ -70,21 +71,21 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( "Error getting env var '" RCUTILS_STRINGIFY(RMW_IMPLEMENTATION_ENV_VAR_NAME) "': %s\n", get_env_error_str); - fail_state = RCL_RET_ERROR; - goto fail_cleanup; + return RCL_RET_ERROR; } if (strlen(expected_rmw_impl_env) > 0) { // Copy the environment variable so it doesn't get over-written by the next getenv call. expected_rmw_impl = rcutils_strdup(expected_rmw_impl_env, allocator); if (!expected_rmw_impl) { RCL_SET_ERROR_MSG("allocation failed"); - fail_state = RCL_RET_BAD_ALLOC; - goto fail_cleanup; + return RCL_RET_BAD_ALLOC; } + expected_rmw_requires_free = true; } char * asserted_rmw_impl = NULL; const char * asserted_rmw_impl_env = NULL; + bool asserted_rmw_requires_free = false; get_env_error_str = rcutils_get_env( RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, &asserted_rmw_impl_env); if (NULL != get_env_error_str) { @@ -103,6 +104,7 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) fail_state = RCL_RET_BAD_ALLOC; goto fail_cleanup; } + asserted_rmw_requires_free = true; } // If both environment variables are set, and they do not match, print an error and exit. @@ -122,6 +124,7 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) // The strings at this point must be equal. // No need for asserted_rmw_impl anymore, free the memory. allocator.deallocate((char *)asserted_rmw_impl, allocator.state); + asserted_rmw_requires_free = false; } else { // One or none are set. // If asserted_rmw_impl has contents, move it over to expected_rmw_impl. @@ -162,8 +165,12 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) return RCL_RET_OK; fail_cleanup: - allocator.deallocate((char *)expected_rmw_impl, allocator.state); - allocator.deallocate((char *)asserted_rmw_impl, allocator.state); + if (expected_rmw_requires_free) { + allocator.deallocate((char *)expected_rmw_impl, allocator.state); + } + if (asserted_rmw_requires_free) { + allocator.deallocate((char *)asserted_rmw_impl, allocator.state); + } return fail_state; } From 9e0bed9baa665dda61dbac73bcc6ffd8bf95409b Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 11 Aug 2020 11:15:04 -0300 Subject: [PATCH 3/4] Reformat not use extra booleans Signed-off-by: Jorge Perez --- .../rcl/rmw_implementation_identifier_check.c | 43 ++++++++----------- 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/rcl/src/rcl/rmw_implementation_identifier_check.c b/rcl/src/rcl/rmw_implementation_identifier_check.c index d39454130..25d3538d2 100644 --- a/rcl/src/rcl/rmw_implementation_identifier_check.c +++ b/rcl/src/rcl/rmw_implementation_identifier_check.c @@ -59,10 +59,9 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) // If the environment variable RMW_IMPLEMENTATION is set, or // the environment variable RCL_ASSERT_RMW_ID_MATCHES is set, // check that the result of `rmw_get_implementation_identifier` matches. - rcl_ret_t fail_state = RCL_RET_OK; + rcl_ret_t ret = RCL_RET_OK; rcl_allocator_t allocator = rcl_get_default_allocator(); char * expected_rmw_impl = NULL; - bool expected_rmw_requires_free = false; const char * expected_rmw_impl_env = NULL; const char * get_env_error_str = rcutils_get_env( RMW_IMPLEMENTATION_ENV_VAR_NAME, @@ -80,12 +79,10 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) RCL_SET_ERROR_MSG("allocation failed"); return RCL_RET_BAD_ALLOC; } - expected_rmw_requires_free = true; } char * asserted_rmw_impl = NULL; const char * asserted_rmw_impl_env = NULL; - bool asserted_rmw_requires_free = false; get_env_error_str = rcutils_get_env( RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME, &asserted_rmw_impl_env); if (NULL != get_env_error_str) { @@ -93,18 +90,17 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) "Error getting env var '" RCUTILS_STRINGIFY(RCL_ASSERT_RMW_ID_MATCHES_ENV_VAR_NAME) "': %s\n", get_env_error_str); - fail_state = RCL_RET_ERROR; - goto fail_cleanup; + ret = RCL_RET_ERROR; + goto cleanup; } if (strlen(asserted_rmw_impl_env) > 0) { // Copy the environment variable so it doesn't get over-written by the next getenv call. asserted_rmw_impl = rcutils_strdup(asserted_rmw_impl_env, allocator); if (!asserted_rmw_impl) { RCL_SET_ERROR_MSG("allocation failed"); - fail_state = RCL_RET_BAD_ALLOC; - goto fail_cleanup; + ret = RCL_RET_BAD_ALLOC; + goto cleanup; } - asserted_rmw_requires_free = true; } // If both environment variables are set, and they do not match, print an error and exit. @@ -114,8 +110,8 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) "variables do not match, exiting with %d.", expected_rmw_impl, asserted_rmw_impl, RCL_RET_ERROR ); - fail_state = RCL_RET_ERROR; - goto fail_cleanup; + ret = RCL_RET_ERROR; + goto cleanup; } // Collapse the expected_rmw_impl and asserted_rmw_impl variables so only expected_rmw_impl needs @@ -124,12 +120,13 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) // The strings at this point must be equal. // No need for asserted_rmw_impl anymore, free the memory. allocator.deallocate((char *)asserted_rmw_impl, allocator.state); - asserted_rmw_requires_free = false; + asserted_rmw_impl = NULL; } else { // One or none are set. // If asserted_rmw_impl has contents, move it over to expected_rmw_impl. if (asserted_rmw_impl) { expected_rmw_impl = asserted_rmw_impl; + asserted_rmw_impl = NULL; } } @@ -146,8 +143,8 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) rmw_error_msg.str, RCL_RET_ERROR ); - fail_state = RCL_RET_ERROR; - goto fail_cleanup; + ret = RCL_RET_ERROR; + goto cleanup; } if (strcmp(actual_rmw_impl_id, expected_rmw_impl) != 0) { RCL_SET_ERROR_MSG_WITH_FORMAT_STRING( @@ -156,22 +153,20 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) actual_rmw_impl_id, RCL_RET_MISMATCHED_RMW_ID ); - fail_state = RCL_RET_MISMATCHED_RMW_ID; - goto fail_cleanup; + ret = RCL_RET_MISMATCHED_RMW_ID; + goto cleanup; } - // Free the memory now that all checking has passed. - allocator.deallocate((char *)expected_rmw_impl, allocator.state); } - return RCL_RET_OK; - -fail_cleanup: - if (expected_rmw_requires_free) { + ret = RCL_RET_OK; +// fallthrough +cleanup: + if (expected_rmw_impl) { allocator.deallocate((char *)expected_rmw_impl, allocator.state); } - if (asserted_rmw_requires_free) { + if (asserted_rmw_impl) { allocator.deallocate((char *)asserted_rmw_impl, allocator.state); } - return fail_state; + return ret; } INITIALIZER(initialize) { From 7f0ffebc9aef72a3f65db755095839a11bf80fa4 Mon Sep 17 00:00:00 2001 From: Jorge Perez Date: Tue, 11 Aug 2020 16:09:58 -0300 Subject: [PATCH 4/4] Address peer review comments Signed-off-by: Jorge Perez --- rcl/src/rcl/rmw_implementation_identifier_check.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/rcl/src/rcl/rmw_implementation_identifier_check.c b/rcl/src/rcl/rmw_implementation_identifier_check.c index 25d3538d2..cc2414418 100644 --- a/rcl/src/rcl/rmw_implementation_identifier_check.c +++ b/rcl/src/rcl/rmw_implementation_identifier_check.c @@ -119,7 +119,7 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) if (expected_rmw_impl && asserted_rmw_impl) { // The strings at this point must be equal. // No need for asserted_rmw_impl anymore, free the memory. - allocator.deallocate((char *)asserted_rmw_impl, allocator.state); + allocator.deallocate(asserted_rmw_impl, allocator.state); asserted_rmw_impl = NULL; } else { // One or none are set. @@ -160,12 +160,8 @@ rcl_ret_t rcl_rmw_implementation_identifier_check(void) ret = RCL_RET_OK; // fallthrough cleanup: - if (expected_rmw_impl) { - allocator.deallocate((char *)expected_rmw_impl, allocator.state); - } - if (asserted_rmw_impl) { - allocator.deallocate((char *)asserted_rmw_impl, allocator.state); - } + allocator.deallocate(expected_rmw_impl, allocator.state); + allocator.deallocate(asserted_rmw_impl, allocator.state); return ret; }