[release/3.1] Fix ReadOnlySequence created out of sliced Memory owned by MemoryManager

[adamsitnik]

Port dotnet/runtime#57479 to release/3.1

The following description is a copy of the port to 5.0 description (dotnet/runtime#57562)

Customer Impact

Customers who create ReadOnlySequence<T> out of sliced Memory owned by any MemoryManager are seeing invalid end position and length. This issue was reported by a customer and the fix was provided by them as well.

Depending on the input (how many elements of the original Memory were sliced) iteration over ReadOnlySequence<T> can result in returning incomplete data or throwing ArgumentOutOfRangeException. These conditions can lead to data loss or data corruption that would be difficult to diagnose in a production application.

using System;
using System.Buffers;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

public class Program
{
    public static void Main()
    {
        MemoryManager<byte> memoryManager = new CustomMemoryManager<byte>(4);
        ReadOnlySequence<byte> ros = new ReadOnlySequence<byte>(memoryManager.Memory.Slice(1));
        Console.WriteLine(ros.Length); // prints 2 instead of 3

        foreach (var item in ros)
        {
            Console.WriteLine(item.Length); // prints 2 instead of 3
        }

        memoryManager = new CustomMemoryManager<byte>(3);
        ros = new ReadOnlySequence<byte>(memoryManager.Memory.Slice(2));
        Console.WriteLine(ros.Length); // prints -1 instead of 1

        foreach (var item in ros) // throws System.ArgumentOutOfRangeException
        {
            Console.WriteLine(item.Length); // never gets printed
        }
    }
}

public unsafe class CustomMemoryManager<T> : MemoryManager<T>
{
    private readonly T[] _buffer;

    public CustomMemoryManager(int size) => _buffer = new T[size];

    public unsafe override Span<T> GetSpan() => _buffer;

    public override unsafe MemoryHandle Pin(int elementIndex = 0)
    {
        if ((uint)elementIndex > (uint)_buffer.Length)
        {
            throw new ArgumentOutOfRangeException(nameof(elementIndex));
        }

        var handle = GCHandle.Alloc(_buffer, GCHandleType.Pinned);
        return new MemoryHandle(Unsafe.Add<T>((void*)handle.AddrOfPinnedObject(), elementIndex), handle, this);
    }

    public override void Unpin() { }

    protected override void Dispose(bool disposing) { }
}

Testing

Failing test has been added in this PR.

Risk

Low, as the change is very simple and obvious when you look at ReadOnlySequence<T> ctor that accepts ReadOnlyMemory<T> :

corefx/src/System.Memory/src/System/Buffers/ReadOnlySequence.cs

Line 165 in 646a219

_endInteger = ReadOnlySequence.ArrayToSequenceEnd(start + segment.Count);

corefx/src/System.Memory/src/System/Buffers/ReadOnlySequence.cs

Line 175 in 646a219

_endInteger = ReadOnlySequence.StringToSequenceEnd(start + length);

Regression

This is not a regression.

[jeffhandley]

When this is ready, apply the Servicing-consider label and it will then get reviewed by tactics.

[danmoseley]

We do not have a customer request for a 3.1 port of this, right? The reasoning is that it's subtle data corruption potential - we don't want to wait for a report.

[danmoseley]

The failures are all in drawing and I believe can be ignored.

[adamsitnik]

The reasoning is that it's subtle data corruption potential - we don't want to wait for a report.

Yes, exactly

[danmoseley]

approved by tactics -- @aik-jahoda can you please merge when branch is open?

[Anipik]

Milestone should be 3.1.21, the branches got closed form 3.1.20 last monday

[Anipik]

cc @aik-jahoda