Data instances with a length of 255 bytes are wrongly written.

[Timz95]

Hello,

I've encountered a bug in the library. Messages which contain a Data instance with a size of exactly 255 bytes are written incorrectly. The cause of this is a bug in the determination of the FormatCode of the data in the Data.Write method. A message with a data section of 255 bytes is therefore written with the wrong format: namely Vbin32, but that should be Vbin8 for 255 bytes. The size of the datatype that describes the length of the data is thus written as an uint (4 bytes). That should be 1 byte.

The bug itself is fortunately easily fixable. The condition in the marked area should be: if (data.Length <= byte.MaxValue) or the equivalent if (data.Length < 256)

The existing Debug.Assert(size == written) notified me of this bug. Namely, the 'Size' property of a Data instance uses the AmqpWireformattinWrite.GetSequenceSize method, where the length of the data is correctly compared to the maximum length of a byte. This resulted in noticing the discrepancy in the size of the data and the length of the written data. See images below.

How to reproduce? Simply produce a message with a data section of exactly 255 bytes and try to place it on the event stream. Some "pseudo"-like code below for the idea.

// Example ProducerConfig with a ConfirmHandler how to check if the message is actually sent
var producer = await system.CreateProducer(new ProducerConfig
{
      Reference = streamReference,
      Stream = streamName,
      ConfirmHandler = conf =>
      {
          // Check if message is actually sent (ResponseCode = OK). No confirmation = not sent.
      }
});

var bytes = new byte[255];
var data = new Data(new ReadOnlySequence<byte>(bytes));
await producer.Send(publishingId++, new Message(data));

I've made some tests in a fork where this bug is reproduced and covered with some tests.

The branch with the tests, but without the fix: https://github.com/Timz95/rabbitmq-stream-dotnet-client/tree/fix-data-length-written-without-fix

The branch with the tests, but with the fix present: https://github.com/Timz95/rabbitmq-stream-dotnet-client/tree/fix-data-length-written

Link to test(-files): Added a test in the following file (not sure if this is the right location): https://github.com/Timz95/rabbitmq-stream-dotnet-client/blob/fix-data-length-written-without-fix/Tests/Amqp10Tests.cs

A test which actually shows in practice that a message of 255 bytes can not be sent by a producer: https://github.com/Timz95/rabbitmq-stream-dotnet-client/blob/fix-data-length-written/Tests/IntegrationTest.cs

You can checkout these branches and run the tests for comparison.

Kind regards, Tim

[joeri-jansen]

That could explain the weird issues we have in our event streams... Kudos for you!

[lukebakken]

@Timz95 thanks for all your work on this. Please open a pull request with your fixes. That makes it pretty convenient for me to check things out.

[lukebakken]

Don't sweat the PR, I can get that. I'm getting my env setup to verify these changes.

[Timz95]

Whoops, excuse me for the wait. I see that you already made the PR. Thanks.

[lukebakken]

@Timz95 - please see this comment, thanks - https://github.com/rabbitmq/rabbitmq-stream-dotnet-client/pull/161#issuecomment-1233294231

[Gsantomaggio]

closed via https://github.com/rabbitmq/rabbitmq-stream-dotnet-client/issues/161