Ensure rcutils_char_array_t is null-termiated after memcpy

When copying a substring from another string, we need to explicitly
verify that the new string will contain a null terminator, which may
necessitate allocation of an additional byte.

Signed-off-by: Scott K Logan <[email protected]>

Author: cottsay

include/rcutils/types/char_array.h

@@ -208,7 +208,8 @@ rcutils_char_array_strcat(rcutils_char_array_t * char_array, const char * src);
 /// Copy memory to buffer.
 /**
  * This function is equivalent to `memcpy(char_array->buffer, src, n)` except that the buffer
- * grows as needed so a user doesn't have to worry about overflow.
+ * grows as needed so a user doesn't have to worry about overflow and a null byte is appended if
+ * necessary.
  *
  * \param[inout] char_array pointer to the instance of rcutils_char_array_t which is being resized
  * \param[in] src the memory to be copied from

src/char_array.c

@@ -194,20 +194,25 @@ rcutils_char_array_vsprintf(rcutils_char_array_t * char_array, const char * form
 rcutils_ret_t
 rcutils_char_array_memcpy(rcutils_char_array_t * char_array, const char * src, size_t n)
 {
-  rcutils_ret_t ret = rcutils_char_array_expand_as_needed(char_array, n);
+  size_t new_length = n;
+  if (n > 0 && '\0' != src[n - 1]) {
+    new_length += 1;
+  }
+  rcutils_ret_t ret = rcutils_char_array_expand_as_needed(char_array, new_length);
   if (ret != RCUTILS_RET_OK) {
     // rcutils_char_array_expand_as needed already set the error
     return ret;
   }
   memcpy(char_array->buffer, src, n);
-  char_array->buffer_length = n;
+  char_array->buffer[new_length - 1] = '\0';  // always have an ending
+  char_array->buffer_length = new_length;
   return RCUTILS_RET_OK;
 }
 
 rcutils_ret_t
 rcutils_char_array_strcpy(rcutils_char_array_t * char_array, const char * src)
 {
-  return rcutils_char_array_memcpy(char_array, src, strlen(src) + 1);
+  return rcutils_char_array_memcpy(char_array, src, strlen(src));
 }
 
 rcutils_ret_t

test/test_char_array.cpp

@@ -130,6 +130,27 @@ TEST_F(ArrayCharTest, vsprintf_fail) {
   EXPECT_EQ(RCUTILS_RET_OK, rcutils_char_array_fini(&char_array));
 }
 
+TEST_F(ArrayCharTest, memcpy) {
+  rcutils_allocator_t failing_allocator = get_failing_allocator();
+  rcutils_ret_t ret = rcutils_char_array_init(&char_array, 8, &allocator);
+  ASSERT_EQ(RCUTILS_RET_OK, ret);
+
+  EXPECT_EQ(RCUTILS_RET_OK, rcutils_char_array_memcpy(&char_array, "1234", 4));
+  EXPECT_STREQ("1234", char_array.buffer);
+  EXPECT_EQ(5lu, char_array.buffer_length);
+
+  EXPECT_EQ(RCUTILS_RET_OK, rcutils_char_array_memcpy(&char_array, "1234", 5));
+  EXPECT_STREQ("1234", char_array.buffer);
+  EXPECT_EQ(5lu, char_array.buffer_length);
+
+  char_array.allocator = failing_allocator;
+  EXPECT_EQ(RCUTILS_RET_BAD_ALLOC, rcutils_char_array_memcpy(&char_array, "123456789", 9));
+  rcutils_reset_error();
+
+  char_array.allocator = allocator;
+  EXPECT_EQ(RCUTILS_RET_OK, rcutils_char_array_fini(&char_array));
+}
+
 TEST_F(ArrayCharTest, strcpy) {
   rcutils_allocator_t failing_allocator = get_failing_allocator();
   rcutils_ret_t ret = rcutils_char_array_init(&char_array, 8, &allocator);