Skip to content

Ensure rcutils_char_array_t is null-termiated after memcpy #493

New issue

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

#

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

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Ensure rcutils_char_array_t is null-termiated after memcpy #493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion include/rcutils/types/char_array.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
11 changes: 8 additions & 3 deletions src/char_array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love the fact that the new length will depend on the content being copied, and it also prevents you from intentionally creating a character array without a null terminating character (maybe you're tracking the length separately and build a string out of intermediate string fragments or something idk). I suppose it's fine because you can use the rcutils_uint8_array_t for that, but it still feels weird.

Copy link
Member Author

@cottsay cottsay Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd question whether non-terminated strings is something this type is intended to support at all. The _strcat and _strncat functions expect the character at index buffer_length - 1 to be a terminator and overwrite it with the beginning of the source string. Also, the _resize function writes a null terminator to the buffer when the string is shortened. Oh, actually it only does that if it doesn't own the buffer?

The argument against changing the allocated size based on content is valid. I'd say we should balance that against the risk of an inconsistent state we live with today.

}
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line is my main complaint with this change, consider this additional test code:

  {
    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_strcpy(&char_array, "hello world"));
    EXPECT_STREQ("hello world", char_array.buffer);
    EXPECT_EQ(12lu, char_array.buffer_length);

    EXPECT_EQ(RCUTILS_RET_OK, rcutils_char_array_memcpy(&char_array, "HELLO", strlen("HELLO")));
    EXPECT_STREQ("HELLO world", char_array.buffer);
    EXPECT_EQ(12lu, char_array.buffer_length);
  }

Personally I would expect the result (with memcpy) to be "HELLO world", but it's not:

[ RUN      ] ArrayCharTest.memcpy
/workspaces/rcutils/test/test_char_array.cpp:162: Failure
Expected equality of these values:
  "HELLO world"
  char_array.buffer
    Which is: "HELLO"

/workspaces/rcutils/test/test_char_array.cpp:163: Failure
Expected equality of these values:
  12lu
    Which is: 12
  char_array.buffer_length
    Which is: 6

[  FAILED  ] ArrayCharTest.memcpy (0 ms)

Maybe this could be addressed with only setting the null-terminator when the array was expanded?

Copy link
Member Author

@cottsay cottsay Mar 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this PR, the buffer would be in the state you're expecting it to be, but the buffer_length would be set to 5. The way I see it, the function is disregarding the "extra" space available in the buffer at that point, and only considers the string's "length" to be 5 characters long. Also, if I subsequently performed a rcutils_char_array_strcat() it would pick up writing to the string at index 5, not after the null terminator.

So unless the memory we're copying in has a single null character right at the end, we'll be in an inconsistent state.

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
Expand Down
21 changes: 21 additions & 0 deletions test/test_char_array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down