Runtime: SequenceReader.TryCopyMultisegment has incorrect bounds check

[pranavkm]

The problematic code:
https://github.com/dotnet/corefx/blob/8292e4556eae08f497e3d8666c001b5ea75f0d3b/src/System.Memory/src/System/Buffers/SequenceReader.cs#L346-L347

It looks like it's meant to be a guard to ensure there is enough buffer to copy the remaining from the unread span, but it's inverted. When working with a multi-span sequence, if the provided buffer is larger than the remaining code, SequenceReader.TryCopyTo will always fail

```C#
class Program
{
static void Main(string[] args)
{
var segment = new BufferSegment(new[] { (byte)1, (byte)'A' });
segment.Append(new[] { (byte)'B'});
var sequence = new ReadOnlySequence(segment, 0, segment.Next, 1);

        var sequenceReader = new SequenceReader<byte>(sequence);
        sequenceReader.TryRead(out var value);
        Debug.Assert(value == (byte)1);

        var readBuffer = new byte[16];
        var result = sequenceReader.TryCopyTo(readBuffer);
        System.Console.WriteLine($"{result} {readBuffer[0]} {readBuffer[1]}");

        readBuffer = new byte[2];
        result = sequenceReader.TryCopyTo(readBuffer);
        System.Console.WriteLine($"{result} {readBuffer[0]} {readBuffer[1]}");
    }

    internal class BufferSegment<T> : ReadOnlySequenceSegment<T>
    {
        public BufferSegment(ReadOnlyMemory<T> memory)
        {
            Memory = memory;
        }

        public BufferSegment<T> Append(ReadOnlyMemory<T> memory)
        {
            var segment = new BufferSegment<T>(memory)
            {
                RunningIndex = RunningIndex + Memory.Length
            };
            Next = segment;
            return segment;
        }
    }
}

Output:

False 0 0
True 65 66
```

Environment: Microsoft.NETCore.App 3.0.0-preview5-27619-16

[pranavkm]

cc @davidfowl

[ahsonkhan]

cc @JeremyKuhne

[ahsonkhan]

It looks like it's meant to be a guard to ensure there is enough buffer to copy the remaining from the unread span, but it's inverted.

Looks like you have identified the correct problem, to me. It should be if (Remaining > destination.Length) (>= case should continue to work since that implies there is enough space).

Or simply invert the order:
if (destination.Length < Remaining)

[pranavkm]

Inverting sounds great: https://github.com/neuecc/MessagePack-CSharp/pull/430/files#diff-3dc920121fdf76ba3c2b8b5e9e153e82R441

[ahsonkhan]

Also, funnily, looks like we have a test gap here. I was wondering how such a bug wasn't caught sooner (it's precisely this branch that wasn't tested).

We should improve the code coverage overall:

Covered lines:  474
Uncovered lines:    159
Coverable lines:    633
Total lines:    1147
Line coverage:  74.8% (474 of 1147)
Covered branches:   178
Total branches: 276
Branch coverage:    64.4% (178 of 276)

Especially for the following APIs which is missing coverage!
C# public bool TryReadTo(out ReadOnlySequence<T> sequence, T delimiter, ...); public long AdvancePastAny(T value0, T value1, T value2, T value3); public bool IsNext(T next, bool advancePast = false);

[JeremyKuhne]

The behavior is actually to spec. The contract is that it _fills_ the given buffer- if it can't fill it completely it fails. I'll work on backfilling tests where we're missing them and try to be more explicit in the comments.

[ahsonkhan]

if it can't fill it completely it fails

Hmm... Do you mean the destination span has to be of the exact size? It can't be larger?

If so, that actually makes some sense, since we don't have an out int bytesWritten (or equivalent) to let the caller know how much was actually written.

[JeremyKuhne]

Hmm... Do you mean the destination span has to be of the exact size? It can't be larger?

Has to be same size or smaller.

since we don't have an out int bytesWritten (or equivalent) to let the caller know how much was actually written.

Yes, that's the reason. If you want everything up to a given size you need to check Remaining first and slice if that is what you want.

[davidfowl]

How are you supposed to use this API with the array pool?

[JeremyKuhne]

How are you supposed to use this API with the array pool?

Slice based on Remaining. This API is "give me exactly this much". It's origin was in the binary reader methods.

https://github.com/dotnet/corefx/blob/master/src/System.Memory/src/System/Buffers/SequenceReaderExtensions.Binary.cs#L39-L56

[davidfowl]

I think if our existing implementation breaks people's intuition then we need to reconsider the contract. Here's where it was originally found:

https://github.com/neuecc/MessagePack-CSharp/blob/8861abdde93a3b97180ac3b2eafa33459ad52392/src/MessagePack/MessagePackReader.cs#L775

So this code should be:

C# byte[] byteArray = ArrayPool<byte>.Shared.Rent(byteLength); ThrowInsufficientBufferUnless(this.reader.TryCopyTo(byteArray.AsSpan(0, byteLength))); string value = StringEncoding.UTF8.GetString(byteArray, 0, byteLength); ArrayPool<byte>.Shared.Return(byteArray); return value;

cc @AArnott

[JeremyKuhne]

I think if our existing implementation breaks people's intuition then we need to reconsider the contract.

The signature being the way it is we can't really change the behavior as we can't return the copied count. I'm fine with changing this to int CopyTo(Span<T> destination) if we have enough consensus on that being better.

[AArnott]

We're considering changing an internal method, right? So there aren't behavioral/API changes that are off the table here I presume--it's all possible to change.

But anyway, I agree with @JeremyKuhne's comment that I always expected the contract of the method to be that it must fill the given span. If that's breaking (internal) callers though, should we fix those callers or change the contract of the method?

We've already altered the contract of the method in https://github.com/neuecc/MessagePack-CSharp/pull/430/files and I'm concerned about where that leaves MessagePack's reliability till this issue is closed.

[JeremyKuhne]

We're considering changing an internal method, right?

No, it's public.

I'm concerned about where that leaves MessagePack's reliability till this issue is closed.

We're running late for a breaking change where I don't think we'll have enough consensus on what is "better" that what we already have come to. I'm adding more detailed comments (in PR). Closing for now, we can discuss further/reopen if needed.

[pranavkm]

@AArnott I'll send an update to MessagePack-CSharp reverting the changes I made to SequenceReader

[AArnott]

Thanks, @pranavkm.
@JeremyKuhne: the "bug" alleged in this issue is to TryCopyMultisegment which is an internal method. What public method are you thinking of?

[pranavkm]

I think this:

I'm fine with changing this to int CopyTo(Span destination) if we have enough consensus on that being better.

[davidfowl]

Reopening this issue after discussion with @dotnet/fxdc