-
Notifications
You must be signed in to change notification settings - Fork 5.3k
[release/5.0] Fix ReadOnlySequence created out of sliced Memory owned by MemoryManager #57562
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 |
GrabYourPitchforks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backport LGTM.
|
@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.