Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

In the function serdata_default_from_ser_iov_common, why is the calculation of pad not using % 3 (DDS_CDR_HDR_PADDING_MASK)? #2176

Open
zydzcy opened this issue Feb 14, 2025 · 1 comment

Comments

@zydzcy
Copy link

zydzcy commented Feb 14, 2025

In the function serdata_default_from_ser_iov_common, why is the calculation of pad not using % 3 (DDS_CDR_HDR_PADDING_MASK)?

@eboasson
Copy link
Contributor

I guess you mean:

  const uint32_t pad = ddsrt_fromBE2u (d->hdr.options) & 2;

and & 3 instead of & 2?

Good question. I wonder what I was thinking when I wrote or reviewed that ...

Fortunately the damage is minor.

What this means is that the data on input to the deserializer can end with one additional byte with undefined content.

If type information is available (the normal case), we check whether the type of the writer is compatible with the type of the reader ("assignability" in the XTypes spec). If the type is @final the types must be the same, and so the additional byte would simply be ignored. If the type is @appendable or @mutable or uses inheritance, we only support XCDR2, and XCDR2 is immune to garbage at the end because it contains explicit length fields in the data.

If we don't have type information, then we have to assume the type is the same if the type name is the same. Again, if it is XCDR2, we're good. If it is CDR/XCDR1 (like it would be for simple types and if the writer is a legacy DDS version, e.g., Cyclone before we added XTypes support), then with a writer of struct T{octet a;}; and a reader of struct T{octet a,b;}; it will successfully deserialize the data thanks to that additional byte of undefined contents.

It absolutely needs fixing. Thank you for reporting it!

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

No branches or pull requests

2 participants