Skip to content

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Aug 17, 2021

Port #57479 to release/5.0

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.

image

Risk

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

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

_endInteger = ReadOnlySequence.StringToSequenceEnd(start + length);

Regression

This is not a regression.

@adamsitnik adamsitnik added this to the 5.0.x milestone Aug 17, 2021
@adamsitnik adamsitnik requested a review from ahsonkhan as a code owner August 17, 2021 10:31
@ghost
Copy link

ghost commented Aug 17, 2021

Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Port #57479 to release/5.0

Customer Impact

Customers who create ReadOnlySequence<T> out of sliced Memory owned by any MemoryManager are seeing invalid end position and length.

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

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.

image

Risk

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

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

_endInteger = ReadOnlySequence.StringToSequenceEnd(start + length);

Regression

This is not a regression.

Author: adamsitnik
Assignees: -
Labels:

area-System.Memory

Milestone: 5.0.x

@adamsitnik
Copy link
Member Author

cc @jeffhandley

@jeffhandley jeffhandley added the Servicing-consider Issue for next servicing release review label Aug 19, 2021
@stephentoub
Copy link
Member

@adamsitnik, is there a reason to only target release/5.0 and not also release/3.1?

@jeffhandley
Copy link
Member

@adamsitnik mentioned on the issue that the port to 3.1 is underway as well (but currently blocked).

@jamshedd jamshedd added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 19, 2021
@adamsitnik
Copy link
Member Author

@jeffhandley @stephentoub The missing NuGet package was added to the right feed and I was able to build corefx/release/3.1, backport and test the fix. The PR: dotnet/corefx#43102

Copy link
Member

@GrabYourPitchforks GrabYourPitchforks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backport LGTM.

@adamsitnik
Copy link
Member Author

@jeffhandley what is our next step here? the CI is green and it has been approved, but I am not authorized to merge it

@Anipik Anipik merged commit 324e5f9 into dotnet:release/5.0 Sep 13, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
@adamsitnik adamsitnik deleted the backport57472 branch March 29, 2022 17:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Memory Servicing-approved Approved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants