cpp: Fix for undefined behavior in mcap::ParseByteArray(..)

[MichaelOrlov]

Changelog

Fix for possible undefined behavior in the cpp mcap reader.

Docs

None

Description

• We discovered undefined behavior in the mcap::ParseByteArray(..) when was running Rosbag2 tests with UBSAN (Undefined Behavior Sanitizer). In particular tests from the https://github.com/ros2/rosbag2/blob/rolling/rosbag2_cpp/test/rosbag2_cpp/test_local_message_definition_source.cpp
  UBSAN was pointing ou to the std::memcpy(output->data(), data + 4, size);
• According to the https://en.cppreference.com/w/cpp/string/byte/memcpy If either dest or src is an invalid or null pointer, the behavior is undefined, even if count is zero.
  After a comprehensive analysis, we found that we have a null pointer in dest, and the count is zero in std::memcpy(..) when the message definition was not found and was recorded with empty strings.

Before	After
Undefined behavior when reading empty message definitions	No undefined behavior when reading empty message definitions

[jtbandes]

👍 seems correct, could you also add a comment to describe why the condition is necessary? It's not obvious from the code. Relatedly, I also see this in vector::data(): "If size() is ​0​, data() may or may not return a null pointer." So the comment could be something like "data() may return nullptr if output is empty, but memcpy() does not accept nullptr".

[MichaelOrlov]

@jtbandes Thanks for the review. I've added a comment with an explanation before if (size > 0) check.

[jtbandes]

=> #1241