`Span2D`/`ReadOnlySpan2D.TryGetSpan()` has incorrect implementation for legacy framework

[Ilia-Kosenkov]

Describe the bug

I will use Span2D<T> as an example, everything described is also applicable to ReadOnlySpan2D<T> as the codebase is similar.

Method Span2D<T>.TryGetSpan(out Span<T> span) is used in several places, most notably in Span2D<T>.CopyTo(Span<T>).
When using CopyTo in my project, I encountered an ArgumentOutOfRangeException in the legacy framework (.NET 4.8).
I traced the issue down to TryGetSpan, which consistently fails in .NET 4.8 (when constructed from a 1D array) but not in .NET 5.0+ where there is a runtime support for spans.

TryGetSpan has bool return type and Try in its name, suggesting it should not fail at all but rather return false if the requested operation cannot be performed.

The problem arises here:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/d3426fad3d3decc000f42323165e00efe28c12a0/Microsoft.Toolkit.HighPerformance/Memory/Span2D%7BT%7D.cs#L1062-L1068

Here, the instance object is converted (reinterpreted) to the array type T[], and then the AsSpan method is called.
I assume this is the standard MemoryExtensions.AsSpan<T>(T[], int start, int length).
The problem is, this method accepts start and length in array elements, while this.offset, which is passed there, is actually an internal byte offset within the 1D array, see this constructor
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/d3426fad3d3decc000f42323165e00efe28c12a0/Microsoft.Toolkit.HighPerformance/Memory/Span2D%7BT%7D.cs#L252-L256

This offset is non-zero even when Span2D<T> points to the whole array. If I am not mistaken, the default value should be 8 in x64 process, corresponding to the internal offset of the data pointer within the array instance.
Now check this fragment again

Unsafe.As<T[]>(this.Instance).AsSpan((int)this.Offset, (int)Length)

if Span2D<T> points to the full array, .AsSpan tries to point to a subarray starting at Offset with length Length, which goes beyond the initial array by exactly Offset elements.
The situation should get worse if Span2D<T> is sliced and has non-default Offset value. The error should grow proportionally to the sizeof<T>, because Offset is measured in bytes and .AsSpan() accepts index of the array element. See Span2D<T> getter implementation:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/d3426fad3d3decc000f42323165e00efe28c12a0/Microsoft.Toolkit.HighPerformance/Memory/Span2D%7BT%7D.cs#L950

Steps to Reproduce

 var arr = new int[128];
 var span2D = new Span2D<int>(arr, 8, 16);
 span2D.TryGetSpan(out var span);

Expected behavior

A new Span<int> instance is created that points to the whole arr.

Actual behaviour

ArgumentOutOfRangeException gets thrown at TryGetSpan with the following message:
"Specified argument was out of the range of valid values. Parameter name: start"

Possible solutions

1. Use internal constructor of Span<T> that accepts ByReference<T>, IntPtr and int.

Here IntPtr offset is the same thing as in Span2D<T>.

2. Convert offset into element index and use .AsSpan with a proper index and length.

var initialOffset = ObjectMarshal.DangerousGetObjectDataByteOffset(
  arr,
  ref ArrayExtensions.DangerousGetReference(arr)
);
// This gives the index of the starting element within an array for T : unmanaged
// For ref types (T : class) I guess the element size is IntPtr.Size
var elemOffset = (this.Offset.ToInt64() - initialOffset.ToInt64()) / Unsafe.SizeOf<T>();
Unsafe.As<T[]>(this.Instance).AsSpan((int)elemOffset, (int)Length)

Environment

Package Version(s): 7.0.1

.NET 4.8 @ Windows 10

 OS Name:     Windows
 OS Version:  10.0.19042
 OS Platform: Windows
 RID:         win10-x64
 .NET Framework 4.8

Mono @ Ubuntu WSL2

Linux DESKTOP-PC 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Mono JIT compiler version 6.12.0.122 (tarball Mon Feb 22 17:29:18 UTC 2021)
Copyright (C) 2002-2014 Novell, Inc, Xamarin Inc and Contributors. www.mono-project.com
        TLS:           __thread
        SIGSEGV:       altstack
        Notifications: epoll
        Architecture:  amd64
        Disabled:      none
        Misc:          softdebug
        Interpreter:   yes
        LLVM:          yes(610)
        Suspend:       hybrid
        GC:            sgen (concurrent by default)

Hello Ilia-Kosenkov, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

[Kyaa-dost]

Hi, @Ilia-Kosenkov Thanks for providing detailed information on this matter and the proposed solution. @Sergio0694 does the solution seem good to you and if not what do you suggest?

[Sergio0694]

Hi @Ilia-Kosenkov, thank you for the detailed issue!
This definitely looks like an oversight and I must've missed this since there is no test on UWP for this case, which would've otherwise caught this bug. We can't use the internal ByReference<T> constructor sincee that's (1) not accessible without hacks and (2) not present on runtimes without Span<T> support, but I already have some helpers to reverse the offset back to an index, so we can just use them (in fact I'm already doing so in Memory2D<T>). Sorry for the inconvenience! 🙂

[Ilia-Kosenkov]

@Kyaa-dost, @Sergio0694,
Thank you for the update!
Obviously, you better know what type of fix should be implemented (I honestly did not check what type of helpers are available internally here, so my options were just the most obvious ones).
I should mention that 'hacks' do work in such a scenario -- it was a project with hacks that revealed this issue when tests were run against .NET 4.8.

[Sergio0694]

"I should mention that 'hacks' do work in such a scenario -- it was a project with hacks that revealed this issue when tests were run against .NET 4.8."

I'm confused as to how that can be possible, maybe I misunderstood and we're talking about different things?
.NET Framework does not have the ByReference<T> type (which was introduced in .NET Core 2.1), and relies on the "portable span" implementation, which is essentially what I'm replicating here as well, ie. carrying an object and a byte offset to the start. I'm confused as to how you could actually be using that constructor from .NET Framework, as (1) it shouldn't actually exist, and (2) the ByReference<T> is internal, so you'd need a whole other set of hacks to be able to access that (see #3367).

This is outside the scope of this issue specifically, but I'm curious now ahah
Could you elaborate on that? 🙂

EDIT: by the way, thank you for the great issue and for taking the time to write all the detailed info, appreciate it! 😊

[Ilia-Kosenkov]

@Sergio0694,

I do not claim to fully understand how it works, but I did the following: Both `Span` and `Span2D` in legacy framework rely on the `object` instance, some `IntPtr` offset and `Length`. `Span2D` has other properties to reflect its 2D nature, but the core principle is the same. So, one can carelessly take, say, instance and offset from `Span`, write it to the instance and offset of `Span2D`, and update other fields of `Span2D`, as if it was constructed from a 1D array.

This allows me to, for instance, stackalloc a regular Span, then call some extension method and get a Span2D without ever touching a raw pointer.

The reverse thing works as well. For instance, it is possible to extract a row from Span2D (row should be a continuous block of memory if I understood correctly the description written in the comments of Span2D) by simply copying object instance and carefully computing offset and length. And it does not matter whether the instance is null or not.

Actually, ObjectMarshal and ArrayExtensions here allow doing a lot of things with arrays because the offsets can now be easily computed.
Basically, I wrote a library that does hacks for .NET Standard 2.0 and uses nice ref T for .NET Standard 2.1 and ran a bunch of tests using .NET 4.8, .NET 5, .NET 6. These tests revealed problems with CopyTo in .NET 4.8, which lead me to TryGetSpan.

I tested mainly two use-cases: when Span is stackalloc-ed, so it stays in place and relies on a pointer, and when Span comes from a new-ed up array. I tried to write tests so that a full forced compacting garbage collection should move this array in memory. If asserts are OK after this in-memory movement, then it seems that transformations between spans work correctly.

I expect that it will spectacularly blow up in my face at some point, but Span is such a great API even with the legacy framework limitations.

Edit

I forgot to answer how I call that constructor. Well, I do not :-)
I sort of ref-cast Span to a struct with identical layout and grab field values and then write it to Span2D using the same technique. I think something like that is done for ArrayExtensions here.

Oh, but Span is a ref struct and it cannot be used as a generic type parameter, so Unsafe.As does not work.
But a method with a signature identical to ref MyHackyType Unsafe.As<Span<T>, MyHackyType>(ref Span<T> val) can be written manually, well, in IL, like all the Unsafe class methods are written.

[Sergio0694]

Oh I see, yeah that makes sense now 🙂

Yes, going to/from Span<T> and Span2D<T> will always work fine and be perfectly safe, regardless of what the span is wrapping (either a pointer or some object instance), both on legacy frameworks and on new ones with runtime Span<T> support. I mean, you'll have to wait for #3948 for it to actually work correcty on legacy framewors, but conceptually it does 😄
It shouldn't ever "blow up" regardless of what else you're doing, as the type conversions here are well defined and tracked.

As for the second part, with your IL method to reinterpret a span... Well that's...
Yeah that might very well explode at any point, it's incredibly dangerous, but I'm sure you already knew that ahah