From d77c9b6965f3fe341154d522f106567ae8a99d28 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 14 Nov 2018 17:27:17 -0300 Subject: [PATCH 1/2] [rcl action] Makes rcl_action_get_*_name() functions check for empty action names. --- rcl_action/include/rcl_action/names.h | 20 +++++++++---------- rcl_action/src/rcl_action/names.c | 24 ++++++++++++++++++++++- rcl_action/test/rcl_action/test_names.cpp | 16 +++++++++------ 3 files changed, 43 insertions(+), 17 deletions(-) diff --git a/rcl_action/include/rcl_action/names.h b/rcl_action/include/rcl_action/names.h index 411fc61..97f3836 100644 --- a/rcl_action/include/rcl_action/names.h +++ b/rcl_action/include/rcl_action/names.h @@ -41,8 +41,8 @@ extern "C" * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The name of the action whose goal service name is - * being returned. + * \param[in] action_name The non-empty name of the action whose goal + * service name is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] goal_service_name Either an allocated string with the action * goal service name, or `NULL` if the function failed to allocate memory @@ -73,8 +73,8 @@ rcl_action_get_goal_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The name of the action whose cancel service name is - * being returned. + * \param[in] action_name The non-empty name of the action whose cancel + * service name is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] cancel_service_name Either an allocated string with the action * cancel service name, or `NULL` if the function failed to allocate memory @@ -105,8 +105,8 @@ rcl_action_get_cancel_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The name of the action whose result service name is - * being returned. + * \param[in] action_name The non-empty name of the action whose result service + * name is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] result_service_name Either an allocated string with the action * result service name, or `NULL` if the function failed to allocate memory @@ -137,8 +137,8 @@ rcl_action_get_result_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The name of the action whose feedback topic name is - * being returned. + * \param[in] action_name The non-empty name of the action whose feedback + * topic name is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] result_service_name Either an allocated string with the action * feedback topic name, or `NULL` if the function failed to allocate memory @@ -169,8 +169,8 @@ rcl_action_get_feedback_topic_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The name of the action whose status topic name is - * being returned. + * \param[in] action_name The non-empty name of the action whose status topic + * name is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] result_service_name Either an allocated string with the action * status topic name, or `NULL` if the function failed to allocate memory diff --git a/rcl_action/src/rcl_action/names.c b/rcl_action/src/rcl_action/names.c index dbf4003..3b0cd02 100644 --- a/rcl_action/src/rcl_action/names.c +++ b/rcl_action/src/rcl_action/names.c @@ -19,6 +19,8 @@ extern "C" #include "rcl_action/names.h" +#include + #include "rcl/error_handling.h" #include "rcutils/format_string.h" @@ -30,6 +32,10 @@ rcl_action_get_goal_service_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + if (0 == strlen(action_name)) { + RCL_SET_ERROR_MSG("invalid empty action name"); + return RCL_RET_INVALID_ARGUMENT; + } RCL_CHECK_ARGUMENT_FOR_NULL(goal_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *goal_service_name) { RCL_SET_ERROR_MSG("writing action goal service name may leak memory"); @@ -51,6 +57,11 @@ rcl_action_get_cancel_service_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + if (0 == strlen(action_name)) { + RCL_SET_ERROR_MSG("invalid empty action name"); + return RCL_RET_INVALID_ARGUMENT; + } RCL_CHECK_ARGUMENT_FOR_NULL(cancel_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *cancel_service_name) { RCL_SET_ERROR_MSG("writing action cancel service name may leak memory"); @@ -72,12 +83,15 @@ rcl_action_get_result_service_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + if (0 == strlen(action_name)) { + RCL_SET_ERROR_MSG("invalid empty action name"); + return RCL_RET_INVALID_ARGUMENT; + } RCL_CHECK_ARGUMENT_FOR_NULL(result_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *result_service_name) { RCL_SET_ERROR_MSG("writing action result service name may leak memory"); return RCL_RET_INVALID_ARGUMENT; } - *result_service_name = rcutils_format_string(allocator, "%s/_action/get_result", action_name); if (NULL == *result_service_name) { RCL_SET_ERROR_MSG("failed to allocate memory for action result service name"); @@ -94,6 +108,10 @@ rcl_action_get_feedback_topic_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + if (0 == strlen(action_name)) { + RCL_SET_ERROR_MSG("invalid empty action name"); + return RCL_RET_INVALID_ARGUMENT; + } RCL_CHECK_ARGUMENT_FOR_NULL(feedback_topic_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *feedback_topic_name) { RCL_SET_ERROR_MSG("writing action feedback topic name may leak memory"); @@ -115,6 +133,10 @@ rcl_action_get_status_topic_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); + if (0 == strlen(action_name)) { + RCL_SET_ERROR_MSG("invalid empty action name"); + return RCL_RET_INVALID_ARGUMENT; + } RCL_CHECK_ARGUMENT_FOR_NULL(status_topic_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *status_topic_name) { RCL_SET_ERROR_MSG("writing action status topic name may leak memory"); diff --git a/rcl_action/test/rcl_action/test_names.cpp b/rcl_action/test/rcl_action/test_names.cpp index da9e3fa..de303c0 100644 --- a/rcl_action/test/rcl_action/test_names.cpp +++ b/rcl_action/test/rcl_action/test_names.cpp @@ -48,14 +48,17 @@ protected: TEST_P(TestActionDerivedName, validate_action_derived_getter) { - rcl_ret_t ret; - char dummy_char; - char * action_derived_name; - rcl_allocator_t default_allocator = - rcl_get_default_allocator(); + rcl_allocator_t default_allocator = rcl_get_default_allocator(); + + char * action_derived_name = NULL; + const char * const null_action_name = NULL; + rcl_ret_t ret = test_subject.get_action_derived_name( + null_action_name, default_allocator, + &action_derived_name); + EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); action_derived_name = NULL; - const char * const invalid_action_name = NULL; + const char * const invalid_action_name = ""; ret = test_subject.get_action_derived_name( invalid_action_name, default_allocator, &action_derived_name); @@ -76,6 +79,7 @@ TEST_P(TestActionDerivedName, validate_action_derived_getter) invalid_ptr_to_action_derived_name); EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); + char dummy_char = '\0'; action_derived_name = &dummy_char; ret = test_subject.get_action_derived_name( test_subject.action_name, default_allocator, From 276aed1dffbb375fa5bb8cde52ddc28fe6e7bc56 Mon Sep 17 00:00:00 2001 From: Michel Hidalgo Date: Wed, 14 Nov 2018 19:33:21 -0300 Subject: [PATCH 2/2] [rcl action] Addresses peer review comments (#329) --- rcl_action/include/rcl_action/names.h | 58 ++++++++++++++++------- rcl_action/src/rcl_action/names.c | 11 ++--- rcl_action/test/rcl_action/test_names.cpp | 2 +- 3 files changed, 48 insertions(+), 23 deletions(-) diff --git a/rcl_action/include/rcl_action/names.h b/rcl_action/include/rcl_action/names.h index 97f3836..4fb6786 100644 --- a/rcl_action/include/rcl_action/names.h +++ b/rcl_action/include/rcl_action/names.h @@ -21,6 +21,7 @@ extern "C" #endif #include "rcl_action/visibility_control.h" +#include "rcl_action/types.h" #include "rcl/allocator.h" #include "rcl/macros.h" @@ -41,14 +42,19 @@ extern "C" * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The non-empty name of the action whose goal - * service name is being returned. + * \param[in] action_name The name of the action whose goal service name is + * being returned. * \param[in] allocator A valid allocator to be used. * \param[out] goal_service_name Either an allocated string with the action * goal service name, or `NULL` if the function failed to allocate memory * for it. Must refer to a `NULL` pointer upon call. * \return `RCL_RET_OK` if the action goal service name was returned, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ACTION_NAME_INVALID` if the action name is not valid + * (i.e. empty), or + * \return `RCL_RET_INVALID_ARGUMENT` if the action name is `NULL`, or + * \return `RCL_RET_INVALID_ARGUMENT` if the allocator is invalid, or + * \return `RCL_RET_INVALID_ARGUMENT` if the goal service name pointer is + * `NULL` or points to a non-`NULL` pointer, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed. */ RCL_ACTION_PUBLIC @@ -73,14 +79,19 @@ rcl_action_get_goal_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The non-empty name of the action whose cancel - * service name is being returned. + * \param[in] action_name The name of the action whose cancel service name + * is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] cancel_service_name Either an allocated string with the action * cancel service name, or `NULL` if the function failed to allocate memory * for it. Must refer to a `NULL` pointer upon call. * \return `RCL_RET_OK` if the action cancel service name was returned, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ACTION_NAME_INVALID` if the action name is not valid + * (i.e. empty), or + * \return `RCL_RET_INVALID_ARGUMENT` if the action name is `NULL`, or + * \return `RCL_RET_INVALID_ARGUMENT` if the allocator is invalid, or + * \return `RCL_RET_INVALID_ARGUMENT` if the cancel service name is `NULL` or + * points to a non-`NULL` pointer, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed. */ RCL_ACTION_PUBLIC @@ -105,14 +116,19 @@ rcl_action_get_cancel_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The non-empty name of the action whose result service - * name is being returned. + * \param[in] action_name The name of the action whose result service name + * is being returned. * \param[in] allocator A valid allocator to be used. * \param[out] result_service_name Either an allocated string with the action * result service name, or `NULL` if the function failed to allocate memory * for it. Must refer to a `NULL` pointer upon call. * \return `RCL_RET_OK` if the action result service name was returned, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ACTION_NAME_INVALID` if the action name is not valid + * (i.e. empty), or + * \return `RCL_RET_INVALID_ARGUMENT` if the action name is `NULL`, or + * \return `RCL_RET_INVALID_ARGUMENT` if the allocator is invalid, or + * \return `RCL_RET_INVALID_ARGUMENT` if the result service name pointer is + * `NULL` or points to a non-`NULL` pointer, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed. */ RCL_ACTION_PUBLIC @@ -137,14 +153,19 @@ rcl_action_get_result_service_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The non-empty name of the action whose feedback - * topic name is being returned. + * \param[in] action_name The name of the action whose feedback topic name + * is being returned. * \param[in] allocator A valid allocator to be used. - * \param[out] result_service_name Either an allocated string with the action + * \param[out] feedback_topic_name Either an allocated string with the action * feedback topic name, or `NULL` if the function failed to allocate memory * for it. Must refer to a `NULL` pointer upon call. * \return `RCL_RET_OK` if the action feedback topic name was returned, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ACTION_NAME_INVALID` if the action name is not valid + * (i.e. empty), or + * \return `RCL_RET_INVALID_ARGUMENT` if the action name is `NULL`, or + * \return `RCL_RET_INVALID_ARGUMENT` if the allocator is invalid, or + * \return `RCL_RET_INVALID_ARGUMENT` if the feedback topic name pointer is + * `NULL` or points to a non-`NULL` pointer, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed. */ RCL_ACTION_PUBLIC @@ -169,14 +190,19 @@ rcl_action_get_feedback_topic_name( * Uses Atomics | No * Lock-Free | Yes * - * \param[in] action_name The non-empty name of the action whose status topic + * \param[in] action_name The name of the action whose status topic * name is being returned. * \param[in] allocator A valid allocator to be used. - * \param[out] result_service_name Either an allocated string with the action + * \param[out] status_topic_name Either an allocated string with the action * status topic name, or `NULL` if the function failed to allocate memory * for it. Must refer to a `NULL` pointer upon call. * \return `RCL_RET_OK` if the action status topic name was returned, or - * \return `RCL_RET_INVALID_ARGUMENT` if any arguments are invalid, or + * \return `RCL_RET_ACTION_NAME_INVALID` if the action name is not valid + * (i.e. empty), or + * \return `RCL_RET_INVALID_ARGUMENT` if the action name is `NULL`, or + * \return `RCL_RET_INVALID_ARGUMENT` if the allocator is invalid, or + * \return `RCL_RET_INVALID_ARGUMENT` if the status topic name pointer is + * `NULL` or points to a non-`NULL` pointer, or * \return `RCL_RET_BAD_ALLOC` if allocating memory failed. */ RCL_ACTION_PUBLIC diff --git a/rcl_action/src/rcl_action/names.c b/rcl_action/src/rcl_action/names.c index 3b0cd02..123b695 100644 --- a/rcl_action/src/rcl_action/names.c +++ b/rcl_action/src/rcl_action/names.c @@ -34,7 +34,7 @@ rcl_action_get_goal_service_name( RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); if (0 == strlen(action_name)) { RCL_SET_ERROR_MSG("invalid empty action name"); - return RCL_RET_INVALID_ARGUMENT; + return RCL_RET_ACTION_NAME_INVALID; } RCL_CHECK_ARGUMENT_FOR_NULL(goal_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *goal_service_name) { @@ -57,10 +57,9 @@ rcl_action_get_cancel_service_name( { RCL_CHECK_ALLOCATOR_WITH_MSG(&allocator, "allocator is invalid", return RCL_RET_INVALID_ARGUMENT); RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); - RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); if (0 == strlen(action_name)) { RCL_SET_ERROR_MSG("invalid empty action name"); - return RCL_RET_INVALID_ARGUMENT; + return RCL_RET_ACTION_NAME_INVALID; } RCL_CHECK_ARGUMENT_FOR_NULL(cancel_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *cancel_service_name) { @@ -85,7 +84,7 @@ rcl_action_get_result_service_name( RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); if (0 == strlen(action_name)) { RCL_SET_ERROR_MSG("invalid empty action name"); - return RCL_RET_INVALID_ARGUMENT; + return RCL_RET_ACTION_NAME_INVALID; } RCL_CHECK_ARGUMENT_FOR_NULL(result_service_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *result_service_name) { @@ -110,7 +109,7 @@ rcl_action_get_feedback_topic_name( RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); if (0 == strlen(action_name)) { RCL_SET_ERROR_MSG("invalid empty action name"); - return RCL_RET_INVALID_ARGUMENT; + return RCL_RET_ACTION_NAME_INVALID; } RCL_CHECK_ARGUMENT_FOR_NULL(feedback_topic_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *feedback_topic_name) { @@ -135,7 +134,7 @@ rcl_action_get_status_topic_name( RCL_CHECK_ARGUMENT_FOR_NULL(action_name, RCL_RET_INVALID_ARGUMENT); if (0 == strlen(action_name)) { RCL_SET_ERROR_MSG("invalid empty action name"); - return RCL_RET_INVALID_ARGUMENT; + return RCL_RET_ACTION_NAME_INVALID; } RCL_CHECK_ARGUMENT_FOR_NULL(status_topic_name, RCL_RET_INVALID_ARGUMENT); if (NULL != *status_topic_name) { diff --git a/rcl_action/test/rcl_action/test_names.cpp b/rcl_action/test/rcl_action/test_names.cpp index de303c0..d24ef4a 100644 --- a/rcl_action/test/rcl_action/test_names.cpp +++ b/rcl_action/test/rcl_action/test_names.cpp @@ -62,7 +62,7 @@ TEST_P(TestActionDerivedName, validate_action_derived_getter) ret = test_subject.get_action_derived_name( invalid_action_name, default_allocator, &action_derived_name); - EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret); + EXPECT_EQ(RCL_RET_ACTION_NAME_INVALID, ret); action_derived_name = NULL; rcl_allocator_t invalid_allocator =