Skip to content

Conversation

@w-utter
Copy link
Contributor

@w-utter w-utter commented Jul 21, 2025

Fragment size and the number of fragmentations are checked for integer overflow instead of casting them with as _. This will exponentiall increase the fragment size if the number of fragments cannot be fit inside a u32 (hence exponentially decreasing it) and default the fragment size to u16::MAX if it overflows.

(data_size / fragment_size) + u32::from(data_size % fragment_size != 0) has also been replaced with data_size.div_ceil(fragment_size) as it is functionally identical and has been stabilized since 1.73

@w-utter w-utter force-pushed the rpts-frag-check-overflow branch from b782965 to 57540c1 Compare July 21, 2025 13:31
@jhelovuo
Copy link
Member

jhelovuo commented Nov 3, 2025

Thank you for the PR, but I think your code is based on a misunderstanding of the limitaitons of the RTPS protocol.

The purpose of this function is to calculate a suitable fragment size and fragment count when the serialized payload is too large to fit into a single network packet.

RustDDS uses a default data_max_size_serialized of 1024 bytes. After adding RTPS submessage headers, possibly a piggybacked control submessage or two, RTPS headers, UDP/IP headers, and Ethernet framing, the result should still fit into a single Ethernet frame, which we assume to have capacity of about 1500 bytes. This is just an approximation. The actual usable maximum payload size depends on the network paths between here and all the remote Readers we are connected to.

UDP protocol can transport datagrams of up to 64k bytes in size, but that uses fragmentation at the UDP/IP level. We do not want fragmentation protocol on top of another fragmentation protocol, so RustDDS tries to avoid that.

So there is no point in trying to increase the fragment_size from 1k, unless there is a reason to believe that our network transports can deliver that.

The fragment_size and data_max_size_serialized must be less than 64k (bytes), because they must fit into u16 fields at the RTPS level, see Table 8.42 – Structure of the DataFrag Submessage (fragmentSize is ushort) or Table 8.16 - Structure of the SubmessageHeader (submessageLength is ushort). These are in the RTPS v2.5 spec document. They are just stored in RustDDS's data structures as usize, because that is what common Rust libraries use for counting bytes.

Similar reasoning applies to payload size (dataSize) , except that the limits are 4G bytes and u32.

But usize is nowadays usually 64 bits, so does not necessarily fit into u32, let alone u16, so that is why there is a danger of overflow.

The proper fix to handle the overflows would be

  • Outright reject any incoming data (from network or DDS API) that exceeds 4 Gbytes in size.
  • Ensure Writer data_max_size_serialized < 64k, e.g. by storing is as u16.
  • Then the function num_frags_and_frag_size could input payload_size: u32 and there should be no possibility of overflow.

This approach would use the type system to enforce RTPS data size limits, but it would cause some additional type conversions between usize and u32/u16 all over the code.

@jhelovuo jhelovuo closed this Nov 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants