FIXTradingCommunity / silverflash

Reference implementation of FIXP performance session layer
Apache License 2.0
24 stars 4 forks source link

Incorrect Buffer Length with SOFH #21

Open parker-berberian opened 1 year ago

parker-berberian commented 1 year ago

When sending messages with the SofhFrameEncoder, the buffer length is calculated incorrectly, resulting in 6 extra bytes of garbage on the wire.

SofhFrameEncoder.encodeFrameTrailer incorrectly adds the HEADER_LENGTH to the buffer position. messageLength includes the HEADER_LENGTH, so this sets the buffer position 6 bytes past where it should be.

The result is that a message is sent with the correct message_length, block_length, etc, but with extra 6 bytes of garbage at the end.

Expected behavior: Send a Negotiate Message with the following bytes: 00 00 00 2d eb 50 19 00 01 00 bc 0a 00 00 <16 uuid bytes> <8 timestamp bytes> 01 04 00 <4 credential bytes>

The SOFH.message_length field of 0x2b is 45 bytes, and 45 bytes are sent.

Observed behavior: Sent a Negotiate Message with the following bytes: 00 00 00 2d eb 50 19 00 01 00 bc 0a 00 00 <16 uuid bytes> <8 timestamp bytes> 01 04 00 <4 credential bytes> 00 00 00 00 00 00

Notice the SOFH.message_length field is still 0x2b == 45 bytes, but 51 bytes are sent over the TCP socket.

parker-berberian commented 1 year ago

@donmendelson Do you have any thoughts on this? If I were to submit a PR, is there a test suite I can use to verify the fix?

parker-berberian commented 1 year ago

It seems that the SofhFrameSpliterator has the same error. From the SOFH spec:

The Message_Length shall be defined to be the length in octets (i.e. bytes) of a message inclusive of the length of the Simple Open Framing Header.

https://github.com/FIXTradingCommunity/fix-simple-open-framing-header/blob/master/v1-0-DRAFT/sofh.md

We should not be adding the sofh header length to the message length field everywhere, because the header length is already included in the field.

donmendelson commented 1 year ago

@parker-berberian a PR would be welcome. I suggest a new unit test that asserts buffer length. io.fixprotocol.silverflash.util.BufferDumper may be helpful.