Ensure rcutils_char_array_t is null-termiated after memcpy

[cottsay]

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.

[cottsay]

Preliminary CI:

• Linux
• Linux-aarch64
• Linux-rhel
• Windows

Full CI:

• (pending green preliminary CI)

[wjwwood]

I think that the reason this function does not handle anything with null terminator characters is because the function it was the analogue of, memcpy, also does not handle null terminator:

The function does not check for any terminating null character in source - it always copies exactly num bytes.
-- https://cplusplus.com/reference/cstring/memcpy/

I don't remember the details, but there are good reasons for this. If we want to have a function that does concern itself with null terminators, then I think it would be better to have a differently named function for it.

[wjwwood]

Or maybe we're already messing with the idea of memcpy by growing the destination as needed...

[cottsay]

I can follow that logic and I think it would be fine to close this PR with that in mind.

That said, do you think that it is part of the rcutils_char_array_t contract that the buffer is always null-terminated? Assuming of course that the caller isn't directly modifying the data.

[wjwwood]

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.

[cottsay]

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.

[wjwwood]

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?

[cottsay]

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.

[cottsay]

Maybe this is just a documentation problem. The other functions seem to keep the buffer termination and length value in a consistent state, but this function seems a little more dangerous and depending on the source content, things can get wonky.

Would some sort of note in the docblock to that effect be better?