Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes rcl_action_get_*_name() functions check for empty action names. #329

Merged
merged 2 commits into from
Nov 16, 2018
Merged

Makes rcl_action_get_*_name() functions check for empty action names. #329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d77c9b6
[rcl action] Makes rcl_action_get_*_name() functions check for empty …
hidmic Nov 14, 2018
276aed1
[rcl action] Addresses peer review comments (#329)
hidmic Nov 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 41 additions & 15 deletions rcl_action/include/rcl_action/names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -48,7 +49,12 @@ extern "C"
* 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
Expand All @@ -73,14 +79,19 @@ 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 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
Expand All @@ -105,14 +116,19 @@ 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 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
Expand All @@ -137,14 +153,19 @@ 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 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
Expand All @@ -169,14 +190,19 @@ 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 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
Expand Down
23 changes: 22 additions & 1 deletion rcl_action/src/rcl_action/names.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ extern "C"

#include "rcl_action/names.h"

#include <string.h>

#include "rcl/error_handling.h"
#include "rcutils/format_string.h"

Expand All @@ -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_ACTION_NAME_INVALID;
}
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");
Expand All @@ -51,6 +57,10 @@ 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);
if (0 == strlen(action_name)) {
RCL_SET_ERROR_MSG("invalid empty action name");
return RCL_RET_ACTION_NAME_INVALID;
}
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");
Expand All @@ -72,12 +82,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_ACTION_NAME_INVALID;
}
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");
Expand All @@ -94,6 +107,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_ACTION_NAME_INVALID;
}
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");
Expand All @@ -115,6 +132,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_ACTION_NAME_INVALID;
}
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");
Expand Down
18 changes: 11 additions & 7 deletions rcl_action/test/rcl_action/test_names.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,21 @@ class TestActionDerivedName

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);
EXPECT_EQ(RCL_RET_INVALID_ARGUMENT, ret);
EXPECT_EQ(RCL_RET_ACTION_NAME_INVALID, ret);

action_derived_name = NULL;
rcl_allocator_t invalid_allocator =
Expand All @@ -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,
Expand Down