Messagepack-csharp: Attempted to read past the end of the stream when using LZ4ArrayCompression

[chris-eg]

Bug description

Not sure if this is an issue at the point of serialization or if the data is fine and its an issue in de-serialization but we are finding once we pass a certain size / number of records we always run into this error when we try to read back the data.

I found this in production when trying to read back some archived data that we had stored in a files and have a test now that reproduces this issue.

This only occurs when using LZ4ArrayCompression, when using LZ4Compression of no compression it doesn't happen or at least not in the sample sizes i have tried.

It appears to be size not data related as i have reproduced this using multiple objects, reordered them and moved the fields in the object around to exclude specific data and found if i use small batches its fine but when trying to write a large number of records to a single file we hit this issue.

Our use case is creating daily archive exports of data to azure storage which we can stream back when needed, 1 file per user per day to allow for easy filtering and range restores.

Repro steps

The following is a simple test that generates 1500 empty objects (in production they are a mix of null and populated fields) writes them to a stream, creates a new stream from this and then attempts to read back those records at which point it fails reading back record 945 with the error "Attempted to read past the end of the stream"

This can also be reproduced with a large complex object with 70 fields and fails at 145+ records which is the real use case but for testing went with a simple object

Code to reproduce the behavior.

[MessagePackObject(true)]
public class MessagePackTest
{
    public Guid ID { get; set; }

    public byte[] a { get; set; } 
    public int b { get; set; }
}

[Fact]
public async Task SimpleTest()
{
    var FormatterWithArrayCompression = MessagePackSerializerOptions.Standard.WithResolver(
            CompositeResolver.Create(
                NativeDecimalResolver.Instance,
                NativeGuidResolver.Instance,
                NativeDateTimeResolver.Instance,
                StandardResolver.Instance,
                ContractlessStandardResolver.Instance))
        .WithCompression(MessagePackCompression.Lz4BlockArray);

    var cancelToken = new CancellationTokenSource().Token;
    var testData = new List<MessagePackTest>();
    for (var i = 0; i < 1500; i++)
    {
        testData.Add(new MessagePackTest());
    }
    var objectsWritten = 0;
    await using var stream = new MemoryStream(500 * testData.Count); // Assume 500 bytes per obj as starting point
    foreach (var dataobj in testData)
    {
        var localStream = new MemoryStream();
        await MessagePackSerializer.SerializeAsync(localStream, dataobj, FormatterWithArrayCompression, cancelToken);
        localStream.WriteTo(stream);
        objectsWritten++;
    }

    // test the written data
    var records = 0;
    var testStream = new MemoryStream(stream.ToArray());
    var streamReader = new MessagePackStreamReader(testStream);
    try
    {
        while (await streamReader.ReadAsync(cancelToken) is ReadOnlySequence<byte> msgpack)
        {
            var cn = MessagePackSerializer.Deserialize<MessagePackTest>(msgpack, cancellationToken: cancelToken, options: FormatterWithArrayCompression);
            records++;
        }
    }
    catch (Exception e)
    {
        System.Diagnostics.Debug.WriteLine($"Failed at {records}");
        System.Diagnostics.Debug.WriteLine(e);
        throw;
    }
    streamReader.Dispose();
    Assert.Equal(objectsWritten, records);
}

Expected behavior

Should be able to read back the records that were added

Actual behavior

Consistently fails when using LZ4ArrayCompression

• Version used: v2.1.115
• Runtime: .Net Core 3.1
• Test Framework: xUnit

Additional context

Using Int or string keys doesn't make a difference.

For testing i was running on my windows laptop, in production it was running on Linux in a Container on Kubernetes both show the issue

[AArnott]

@neuecc Do you want to take a look since this is LZ4ArrayCompression related?

[taspeotis]

I am also seeing this with Lz4BlockArray but not Lz4Block compression.

[shaosss]

I looked at the issue and the problem is that when you read a chunk a memory from a stream you read private static readonly int DefaultLengthFromArrayPool = 1 + (4095 / Marshal.SizeOf<T>());
when the reader comes to the end of the chunk it checks for the remaining read data MessagePackReader.TrySkip():

if (this.reader.Remaining == 0)
{
    return false;
}

Then it checks for remaining read data to contain needed amount of bytes with reader.TryAdvance.
The thing is that for string and binary data it doesn't check for remaining read data and just tries to read it:

case MessagePackCode.Str8:
case MessagePackCode.Str16:
case MessagePackCode.Str32:
    return this.reader.TryAdvance(this.GetStringLengthInBytes());
case MessagePackCode.Bin8:
case MessagePackCode.Bin16:
case MessagePackCode.Bin32:
    return this.reader.TryAdvance(this.GetBytesLength());

It forces the exception to be thrown if you have no required read data length depending on the length type MessagePackReader.GetBytesLength():
ThrowInsufficientBufferUnless(this.reader.TryReadBigEndian(out length));
@chris-eg, in your test case this is exactly the case. The remaining read data is 4 bytes. It uses one byte to read the code and then there is only 3 bytes left which is not enough to parse an int and it throws an exception (see last mentioned code part).
As far I can see there is two options:

• catch exception on MessagePackStreamReader.TryReadNextMessage(out ReadOnlySequence<byte> completeMessage) which might be not the best solution because of try/catch and possibility of catching the same Exception type but for another reason.
• make TryGetStringLengthInBytes() and TryGetBytesLength() to return bool and not create an exception.
  The fallback to read more data is already there, so if the TryReadNextMessage will fail it will falback to TryReadMoreDataAsync.
  @neuecc , @AArnott what are your thoughts on proposed solutions?