[release/5.0] Fix ReadOnlySequence created out of sliced Memory owned by MemoryManager#57562
Conversation
|
Tagging subscribers to this area: @GrabYourPitchforks, @dotnet/area-system-memory Issue DetailsPort #57479 to release/5.0 Customer ImpactCustomers who create Depending on the input (how many elements of the original 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) { }
}TestingFailing test has been added in this PR. RiskLow, as the change is very simple and obvious when you look at RegressionThis is not a regression.
|
|
cc @jeffhandley |
|
@adamsitnik, is there a reason to only target release/5.0 and not also release/3.1? |
|
@adamsitnik mentioned on the issue that the port to 3.1 is underway as well (but currently blocked). |
|
@jeffhandley @stephentoub The missing NuGet package was added to the right feed and I was able to build |
|
@jeffhandley what is our next step here? the CI is green and it has been approved, but I am not authorized to merge it |

Port #57479 to release/5.0
Customer Impact
Customers who create
ReadOnlySequence<T>out of slicedMemoryowned by anyMemoryManagerare 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
Memorywere sliced) iteration overReadOnlySequence<T>can result in returning incomplete data or throwingArgumentOutOfRangeException. These conditions can lead to data loss or data corruption that would be difficult to diagnose in a production application.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 acceptsReadOnlyMemory<T>:runtime/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs
Line 162 in 54b37c9
runtime/src/libraries/System.Memory/src/System/Buffers/ReadOnlySequence.cs
Line 172 in 54b37c9
Regression
This is not a regression.