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

[teo-tsirpanis]

Fixes issue dotnet/runtime#57472

Main pr dotnet/runtime#57479

Also backported to 5.0 in dotnet/runtime#57562 and 3.1 in #43102. This PR will pave the way for backporting it to the System.Memory OOB package for .NET Framework.

Customer Impact

A customer has encountered dotnet/runtime#57472 while on .NET Framework and reported it in dotnet/runtime#67295, necessiating backporting it once again. As an example provided in that issue, this memory manager:

private sealed class BasicMemoryManager : MemoryManager<byte>
{
    private readonly byte[] _buffer;
    public BasicMemoryManager(int size = 1024) => _buffer = new byte[size];

    protected override bool TryGetArray(out ArraySegment<byte> segment)
    {
        segment = new ArraySegment<byte>(_buffer);
        return true;
    }
    public override Span<byte> GetSpan() => _buffer;

    public override MemoryHandle Pin(int elementIndex = 0) => throw new NotSupportedException();
    public override void Unpin() => throw new NotSupportedException();
    protected override void Dispose(bool disposing) { }
}

will fail two of the following tests:

[Theory]
[InlineData(0, 32)]
[InlineData(0, 29)]
[InlineData(32, 32)] // fail: length 0
[InlineData(32, 29)] // fail: length -3
public void OffsetTest(int offset, int length)
{
    var mgr = new BasicMemoryManager(1024);
    var slice = mgr.Memory.Slice(offset, length);
    Assert.Equal(length, slice.Length);

    var ros = new ReadOnlySequence<byte>(slice);
    Assert.True(ros.IsSingleSegment);
    Assert.Equal(length, ros.Length);
}

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> (notice line 133 versus lines 140 and 148):

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

Lines 130 to 149 in 0d40b48

	if (MemoryMarshal.TryGetMemoryManager(memory, out MemoryManager<T> manager, out int index, out int length))
	{
	_sequenceStart = new SequencePosition(manager, ReadOnlySequence.MemoryManagerToSequenceStart(index));
	_sequenceEnd = new SequencePosition(manager, ReadOnlySequence.MemoryManagerToSequenceEnd(length));
	}
	else if (MemoryMarshal.TryGetArray(memory, out ArraySegment<T> segment))
	{
	T[] array = segment.Array;
	int start = segment.Offset;
	_sequenceStart = new SequencePosition(array, ReadOnlySequence.ArrayToSequenceStart(start));
	_sequenceEnd = new SequencePosition(array, ReadOnlySequence.ArrayToSequenceEnd(start + segment.Count));
	}
	else if (typeof(T) == typeof(char))
	{
	if (!MemoryMarshal.TryGetString(((ReadOnlyMemory<char>)(object)memory), out string text, out int start, out length))
	ThrowHelper.ThrowInvalidOperationException();
	_sequenceStart = new SequencePosition(text, ReadOnlySequence.StringToSequenceStart(start));
	_sequenceEnd = new SequencePosition(text, ReadOnlySequence.StringToSequenceEnd(start + length));
	}

Moreover, the changed code gets triggered only when creating a ReadOnlySequence from a ReadOnlyMemory backed by a MemoryManager. All other use cases (including creating from an array or from a ReadOnlyMemory backed by an array or a string) are unaffected. And the changes are identical to those in the other backports.

Regression

This is not a regression.

[ericstj]

@dotnet/area-system-memory can someone please review this and be assigned as owner to take to tactics?

[jozkee]

No backporting conflicts and test classes in 2.1 seem similar as the ones in 3.1, which already contains the fix.

[danmoseley]

cc @Pilchie - for ASP.NET..

[jozkee]

@dotnet/area-infrastructure-libraries all Linux legs are being cancelled with this message:

##[warning]An image label with the label ubuntu-16.04 does not exist.
,##[error]The remote provider was unable to process the request.
Started: Just now
Duration: 44s

Is that concerning?

[ViktorHofer]

@danmoseley @ericstj looks like the release/2.1 branch isn't buildable and testable anymore based on Helix and docker EOL changes. Personally I didn't know that we still do servicing updates for 2.1 in the open.

[jozkee]

Ping @ericstj @danmoseley

[ericstj]

Chatted offline -- we believe we can merge this once it's ready. I think this still needs package authoring. Here's a recent example of a 2.1 fix that shipped a package: 24e09fe

[teo-tsirpanis]

Done @ericstj.

[jozkee]

@teo-tsirpanis @dotnet/area-infrastructure-libraries don't we need to include System.Memory in src/packages.builds?
https://github.com/dotnet/corefx/pull/43017/files#diff-43676c2abb12b62b72c9d923c709c340a45fafccf2a7b7daf84549b0ee2b0f54R26-R29

@ericstj does this still require local validation i.e: building 2.1 locally on Linux?

[teo-tsirpanis]

Oops I forgot it, will fix it soon.

[teo-tsirpanis]

Done @jozkee.

[carlossanlop]

@jozkee @ericstj If we are taking this, it needs to be merged directly into the 2.1 internal branch, not here. @jozkee I'll give you instructions offline.

[carlossanlop]

Talked to @mmitche about this. We can take the change in the public repo, we will manually take care of any internal flows.

[ViktorHofer]

Infra changes LGTM