From 232661e319963b99d48419cc5e97514458b1a03b Mon Sep 17 00:00:00 2001 From: nkitchel Date: Wed, 22 Jul 2026 11:21:57 -0500 Subject: [PATCH] Fix IDLObjectSequence#calculateSizeBytes for variable-size elements IDLSequence#calculateSizeBytes's generic loop assumes elementSizeBytes() returns a size that is independent of the alignment context it was given, and separately computes+adds the leading alignment padding before each element via CDRBuffer.alignment(currentAlignment, elementSizeBytes). That holds for every fixed-size primitive sequence (e.g. IDLDoubleSequence, whose element size is always the constant 8, itself a valid CDR alignment boundary). It does not hold for IDLObjectSequence: its elementSizeBytes() forwards to elements[i].calculateSizeBytes(currentAlignment), and a well-behaved CDRSerializable already bakes its own leading alignment padding into that call. Reusing the generic loop on top double-counts alignment, and does so incoherently, since CDRBuffer#alignment assumes its `bytes` argument is itself a valid power-of-two CDR boundary (1/2/4/8) - never true for an arbitrary struct's total encoded size. Found while debugging why a two-element tf2_msgs.TFMessage.transforms sequence (geometry_msgs.TransformStamped[], each element containing a Header + string + Transform) computed a size tens of bytes off from what serialize() actually wrote. This matters because ROS2Publisher#writeAndPublish uses calculateSizeBytes() as the exact number of bytes to resize/copy to the outgoing DDS payload - any struct-sequence message (TFMessage, MarkerArray, DiagnosticArray, ...) published today over-copies whatever was previously in the write buffer as trailing garbage. Changes: - IDLObjectSequence#calculateSizeBytes: override with a version that doesn't double-apply alignment, verified to exactly match what serialize() writes (see the new regression test). - ROS2Publisher#writeAndPublish: defense in depth - use the buffer's actual post-serialize() position for the copied payload length instead of trusting calculateSizeBytes()'s estimate, so any future CDRSerializable whose two methods drift out of sync fails safe instead of shipping garbage/truncated bytes. - Added IDLObjectSequenceVariableSizeElementTest: IDLObjectSequenceTest's TestIDLMsg fixture is a fixed 4 bytes per element (already a valid CDR alignment boundary), so it can't exercise this bug; the new fixture's size varies with a string field, matching the shape that triggered it. --- .../cdr/idl/IDLObjectSequence.java | 35 ++++++ .../java/us/ihmc/jros2/ROS2Publisher.java | 16 ++- ...ObjectSequenceVariableSizeElementTest.java | 106 ++++++++++++++++++ 3 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 src/test/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequenceVariableSizeElementTest.java diff --git a/src/main/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequence.java b/src/main/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequence.java index 42dc4485..f007b767 100644 --- a/src/main/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequence.java +++ b/src/main/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequence.java @@ -231,6 +231,41 @@ public int elementSizeBytes(int currentAlignment, int i) return elements[i].calculateSizeBytes(currentAlignment); } + /** + * Overridden because {@link IDLSequence#calculateSizeBytes(int)}'s generic loop assumes + * {@link #elementSizeBytes(int, int)} returns a size that does not itself depend on the alignment context it + * was given - true for every fixed-size primitive sequence (e.g. {@link IDLDoubleSequence}, whose element size + * is always the constant {@code 8}), where the loop is responsible for computing and inserting the leading + * alignment padding before each element via {@code CDRBuffer.alignment(currentAlignment, elementSizeBytes)}. + *

+ * That assumption does not hold here: {@link #elementSizeBytes(int, int)} forwards directly to + * {@code elements[i].calculateSizeBytes(currentAlignment)}, and a well-behaved {@link CDRSerializable} (e.g. any + * jros2-generated message class) already accounts for its own leading alignment padding relative to the given + * {@code currentAlignment} as part of that call - see e.g. {@code std_msgs.Header#calculateSizeBytes}. Reusing + * the base loop on top of that double-counts alignment: it re-derives a padding amount from the *total* size of + * the (already-aligned) element and adds it again, which is not only redundant but incoherent, since + * {@link CDRBuffer#alignment(int, int)} assumes its {@code bytes} argument is itself a valid power-of-two CDR + * alignment boundary (1, 2, 4, or 8) - never true for an arbitrary struct's total encoded size. In practice this + * could throw a two-element {@code tf2_msgs.TFMessage.transforms} sequence's computed size off by tens of + * bytes in either direction (observed both over- and under-counting depending on element sizes), which matters + * because callers such as {@code ROS2Publisher#writeAndPublish} use this value as the exact number of bytes + * copied to the wire. + */ + @Override + public int calculateSizeBytes(int currentAlignment) + { + int initialAlignment = currentAlignment; + + currentAlignment += 4 + CDRBuffer.alignment(currentAlignment, 4); // Length header + + for (int i = 0; i < size(); i++) + { + currentAlignment += elementSizeBytes(currentAlignment, i); + } + + return currentAlignment - initialAlignment; + } + @Override public void readElement(CDRBuffer buffer) { diff --git a/src/main/java/us/ihmc/jros2/ROS2Publisher.java b/src/main/java/us/ihmc/jros2/ROS2Publisher.java index 54212d20..e2c651ec 100644 --- a/src/main/java/us/ihmc/jros2/ROS2Publisher.java +++ b/src/main/java/us/ihmc/jros2/ROS2Publisher.java @@ -186,15 +186,25 @@ private void writeAndPublish(T message, boolean recordStatistics) { writeBuffer.rewind(); - payloadSizeBytes = CDRBuffer.PAYLOAD_HEADER.length + message.calculateSizeBytes(0); - if (payloadSizeBytes > writeBuffer.getBufferUnsafe().capacity()) + // calculateSizeBytes is used here only to presize the buffer before writing - it is not trusted as the + // exact final payload length below. A CDRSerializable whose calculateSizeBytes() and serialize() ever + // drift out of sync (see the IDLObjectSequence#calculateSizeBytes fix, prompted by exactly this class of + // bug for messages containing an IDLObjectSequence of variable-size structs, e.g. tf2_msgs.TFMessage's + // TransformStamped[]) would otherwise have its miscount silently resized/copied as-is, shipping the wrong + // number of bytes - truncated or padded with garbage - as part of the message on the wire. + int estimatedSizeBytes = CDRBuffer.PAYLOAD_HEADER.length + message.calculateSizeBytes(0); + if (estimatedSizeBytes > writeBuffer.getBufferUnsafe().capacity()) { - writeBuffer.ensureRemainingCapacity(payloadSizeBytes); + writeBuffer.ensureRemainingCapacity(estimatedSizeBytes); } writeBuffer.writePayloadHeader(); message.serialize(writeBuffer); + // The buffer's position after serialize() is the ground truth for how many bytes were actually written, + // regardless of whether the estimate above was exact. + payloadSizeBytes = writeBuffer.getBufferUnsafe().position(); + if (payloadSizeBytes != lastPayloadSizeBytes) { topicDataWrapper.data_vector().resize(payloadSizeBytes); diff --git a/src/test/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequenceVariableSizeElementTest.java b/src/test/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequenceVariableSizeElementTest.java new file mode 100644 index 00000000..01d59283 --- /dev/null +++ b/src/test/java/us/ihmc/fastddsjava/cdr/idl/IDLObjectSequenceVariableSizeElementTest.java @@ -0,0 +1,106 @@ +package us.ihmc.fastddsjava.cdr.idl; + +import org.junit.jupiter.api.Test; +import us.ihmc.fastddsjava.cdr.CDRBuffer; +import us.ihmc.fastddsjava.cdr.CDRSerializable; + +import static org.junit.jupiter.api.Assertions.assertEquals; + +/** + * Regression test for a bug where {@link IDLSequence#calculateSizeBytes(int)} could return a value larger than + * the number of bytes {@link IDLSequence#serialize(CDRBuffer)} actually writes, for an {@link IDLObjectSequence} + * of elements whose size varies (e.g. because they contain a string field). {@link IDLObjectSequenceTest} does not + * catch this because its {@code TestIDLMsg} fixture is a fixed 4 bytes per element - already a valid CDR alignment + * boundary - so the bug this guards against never manifests there. + */ +public class IDLObjectSequenceVariableSizeElementTest +{ + /** + * A struct whose encoded size varies with the name field's length, unlike {@code TestIDLMsg}'s fixed 4 bytes. + * Mirrors the shape (an int followed by a string) that exposed the bug in a real generated message, + * {@code geometry_msgs.TransformStamped} (nested under {@code tf2_msgs.TFMessage.transforms}). + */ + static class VariableSizeMsg implements CDRSerializable + { + private int id; + private final StringBuilder name = new StringBuilder(); + + @Override + public int calculateSizeBytes(int currentAlignment) + { + int initialAlignment = currentAlignment; + currentAlignment += 4 + CDRBuffer.alignment(currentAlignment, 4); // id + currentAlignment += 4 + CDRBuffer.alignment(currentAlignment, 4) + name.length() + 1; // name + return currentAlignment - initialAlignment; + } + + @Override + public void serialize(CDRBuffer buffer) + { + buffer.writeInt(id); + buffer.writeString(name); + } + + @Override + public void deserialize(CDRBuffer buffer) + { + id = buffer.readInt(); + buffer.readString(name); + } + } + + @Test + public void testCalculateSizeBytesMatchesActualSerializedSize() + { + IDLObjectSequence sequence = new IDLObjectSequence<>(VariableSizeMsg.class); + + VariableSizeMsg first = sequence.add(); + first.id = 1; + first.name.append("pelvis"); // 6 chars -> element size is not a power of two + + VariableSizeMsg second = sequence.add(); + second.id = 2; + second.name.append("thigh"); // 5 chars + + int calculatedSizeBytes = sequence.calculateSizeBytes(0); + + CDRBuffer buffer = new CDRBuffer(); + buffer.ensureRemainingCapacity(CDRBuffer.PAYLOAD_HEADER.length + calculatedSizeBytes); + buffer.writePayloadHeader(); + sequence.serialize(buffer); + + int actualSizeBytes = buffer.getBufferUnsafe().position() - CDRBuffer.PAYLOAD_HEADER.length; + + assertEquals(actualSizeBytes, + calculatedSizeBytes, + "calculateSizeBytes() must equal the number of bytes serialize() actually writes - " + + "callers such as ROS2Publisher#writeAndPublish previously trusted it as the exact payload " + + "length written to the wire."); + } + + @Test + public void testDeserializeRoundTrip() + { + IDLObjectSequence sequence = new IDLObjectSequence<>(VariableSizeMsg.class); + sequence.add().id = 42; + sequence.get(0).name.append("longer_frame_name_example"); + sequence.add().id = 7; + sequence.get(1).name.append("x"); + + CDRBuffer buffer = new CDRBuffer(); + buffer.ensureRemainingCapacity(CDRBuffer.PAYLOAD_HEADER.length + sequence.calculateSizeBytes(0)); + buffer.writePayloadHeader(); + sequence.serialize(buffer); + buffer.rewind(); + buffer.readPayloadHeader(); + + IDLObjectSequence deserialized = new IDLObjectSequence<>(VariableSizeMsg.class); + deserialized.deserialize(buffer); + + assertEquals(2, deserialized.size()); + assertEquals(42, deserialized.get(0).id); + assertEquals("longer_frame_name_example", deserialized.get(0).name.toString()); + assertEquals(7, deserialized.get(1).id); + assertEquals("x", deserialized.get(1).name.toString()); + } +}