Merge pull request #329 from ros2/hidmic/fix-empty-action-name

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

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"
@@ -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
@@ -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
+ * \param[in] action_name The name of the action whose cancel service name
- *   being returned.
+ *   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 name of the action whose result service name is
+ * \param[in] action_name The name of the action whose result service name
- *   being returned.
+ *   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 name of the action whose feedback topic name is
+ * \param[in] action_name The name of the action whose feedback topic name
- *   being returned.
+ *   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 name of the action whose status topic name is
+ * \param[in] action_name The name of the action whose status topic
- *   being returned.
+ *   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

rcl_action/src/rcl_action/names.c

@@ -19,6 +19,8 @@ extern "C"
 
 #include "rcl_action/names.h"
 
+#include <string.h>
+
 #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_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");
@@ -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");
@@ -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");
@@ -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");
@@ -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");

rcl_action/test/rcl_action/test_names.cpp

@@ -48,18 +48,21 @@ protected:
 
 TEST_P(TestActionDerivedName, validate_action_derived_getter)
 {
-  rcl_ret_t ret;
+  rcl_allocator_t default_allocator = rcl_get_default_allocator();
-  char dummy_char;
+
-  char * action_derived_name;
+  char * action_derived_name = NULL;
-  rcl_allocator_t default_allocator =
+  const char * const null_action_name = NULL;
-    rcl_get_default_allocator();
+  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 =
@@ -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,