Check remaining buffer size before allocating C wstring sequences - #910
Open
florcabral wants to merge 1 commit into
Open
Check remaining buffer size before allocating C wstring sequences#910florcabral wants to merge 1 commit into
florcabral wants to merge 1 commit into
Conversation
The C introspection wstring[] deserializer passed the on-the-wire length straight to rosidl_runtime_c__U16String__Sequence__init() without the check_sequence_size() guard that ros2#827 added to the other sequence paths. A truncated sample (a huge length with no element data) therefore either failed the array allocation and let a std::runtime_error escape deserializeROSmessage() and rmw_take(), or allocated and initialised that many elements until the process was OOM-killed, in both cases before Fast-CDR reported the short buffer (CWE-400 / CWE-770). Add the guard, so the sample is rejected with NotEnoughMemoryException before anything is allocated, and a regression test covering the C and C++ introspection type supports. Signed-off-by: Florencia Cabral <florencia.a.cabral@gmail.com>
florcabral
force-pushed
the
fix/dynamic-wstring-sequence-size
branch
from
September 4, 2026 15:19
aae8432 to
3d7a82e
Compare
MiguelCompany
approved these changes
Sep 7, 2026
MiguelCompany
left a comment
Collaborator
There was a problem hiding this comment.
LGTM with green CI
Collaborator
|
@ahcorde Since this affects rmw_fastrtps_dynamic_cpp, I relaunched the CI with CI_USE_FASTRTPS_DYNAMIC set to ON: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
#827 added
check_sequence_size()to the dynamic sequence deserializers so the serialized length is checked against the remaining input buffer before anything is allocated. The C introspection wstring[] specialization,deserialize_field<std::wstring>(const rosidl_typesupport_introspection_c__MessageMember *, ...), was missed: it reads the 32-bit length and passes it straight torosidl_runtime_c__U16String__Sequence__init(), which allocates the element array and then callsrosidl_runtime_c__U16String__init()(one 2-byte allocation each) for every element before a single element is read from the wire.A participant on the same domain can send a sample whose wstring[] field is a length with no element data. Every rmw_fastrtps_dynamic_cpp subscriber using the C type support (rclpy, rclc, C nodes) then dies in one of two ways depending on the length:
__Sequence__init()fails and the resulting std::runtime_error escapesdeserializeROSmessage(), which only catches Fast-CDR exceptions and std::bad_alloc, and propagates through Fast-DDS andrmw_take(), which catch nothing;Either way the subscriber is gone before Fast-CDR reports the truncated buffer (CWE-400 / CWE-770).
This adds the same check_sequence_size() call used by the other sequence paths. A truncated sample now throws NotEnoughMemoryException, which deserializeROSmessage() already catches, so rmw_take() reports the sample as not taken and nothing is allocated.
test_deserialize_sequence_sizefeeds a hand-built XCDRv1 sample of test_msgs/msg/WStrings whose unbounded_sequence_of_wstrings claims 0x00FFFFFF elements with no element data to the C and C++ introspection type supports and checks that deserialization fails and the sequence stays empty. The length is large enough that an unfixed build allocates about 1 GB and fails the test, and small enough not to take down a CI host.Verified on rolling (Ubuntu 26.04, Fast-CDR 2.3.6): the test fails with the fix reverted and passes with it; the package test suite including linters passes; populated WStrings messages still round-trip on both type supports.
The same change is needed on jazzy, kilted and lyrical, and on humble, where the file is
rmw_fastrtps_dynamic_cpp/include/rmw_fastrtps_dynamic_cpp/TypeSupport_impl.hpp.