[API Proposal]: [ReadOnly]Span<T>.CreateUnsafe(ref T, int) for compiler-tracked, arbitrary length spans

[Sergio0694]

Background and motivation

Follow up from the discussion started in #64750.

The general idea for this proposal is: with the changes to lifetime annotations for refs in C# 11, developers will be able to just do new [ReadOnly]Span(ref foo) to create a 1-length span from an arbitrary reference, which the compiler will be able to properly track. The issue remains though for arbitrary length spans, where currently the only alternative is MemoryMarshal.CreateSpan, which has the issue that does not track lifetime annotations of the input reference. This is not desireable, as in many cases the only "unsafe" component library authors want is being able to pass an arbitrary length, but without giving up on lifetime tracking.

To solve this, the proposal is for a new set of APIs that would act as a "safer" version of MemoryMarshal.CreateSpan.
The crucial difference is that the ref T parameter would not be scoped, so the compiler would see it escaping the API.

API Proposal

namespace System
{
    public ref struct Span<T>
    {
        public static Span<T> CreateUnsafe(ref T value, int length);
    }

    public ref struct ReadOnlySpan<T>
    {
        public static ReadOnlySpan<T> CreateUnsafe(ref T value, int length);
    }
}

These two APIs will also validate that the length is in the allowed range, just like the void* span constructors. The two APIs should essentially be equivalent on this front, the only difference being this will work on a tracked GC-ref and not a raw pointer.

Note: I'd be happy to be assigned this one in case it got past API review 🙂

API Usage

Just like with MemoryMarshal.CreateSpan, but with tracking.

Alternative Designs

A possible alternative could've been to break MemoryMarshal.CreateSpan and make the ref T argument a scoped ref T, but as discussed that could be problematic as there migth still be niche cases where dropping the tracking could be needed.

Risks

None, really. If anything else, this would push developers towards a safer version of MemoryMarshal.CreateSpan.

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

Issue Details

---

Background and motivation

Follow up from the discussion started in #64750.

The general idea for this proposal is: with the changes to lifetime annotations for refs in C# 11, developers will be able to just do new [ReadOnly]Span(ref foo) to create a 1-length span from an arbitrary reference, which the compiler will be able to properly track. The issue remains though for arbitrary length spans, where currently the only alternative is MemoryMarshal.CreateSpan, which has the issue that does not track lifetime annotations of the input reference. This is not desireable, as in many cases the only "unsafe" component library authors want is being able to pass an arbitrary length, but without giving up on lifetime tracking.

To solve this, the proposal is for a new set of APIs that would act as a "safer" version of MemoryMarshal.CreateSpan.

API Proposal

namespace System
{
    public ref struct Span<T>
    {
        public static Span<T> CreateUnsafe(ref T value, int length);
    }

    public ref struct ReadOnlySpan<T>
    {
        public static ReadOnlySpan<T> CreateUnsafe(ref T value, int length);
    }
}

API Usage

Just like with MemoryMarshal.CreateSpan, but with tracking.

Alternative Designs

A possible alternative could've been to break MemoryMarshal.CreateSpan and make the ref T argument a scoped ref T, but as discussed that could be problematic as there migth still be niche cases where dropping the tracking could be needed.

Risks

None, really. If anything else, this would push developers towards a safer version of MemoryMarshal.CreateSpan.

Author:	Sergio0694
Assignees:	-
Labels:	api-suggestion, area-System.Memory, untriaged
Milestone:	-

[GrabYourPitchforks]

@Sergio0694 - per Discord chat, do you plan on argument checking the value or length parameters? E.g., don't allow length to be negative, don't allow value to be null if length is nonzero, etc.?

[Sergio0694]

As @tannergooding also mentioned, the "unsafeness" of this API should just be from the arbitrary length, but outside of that the resulting instance should be valid and with arguments in the same range normally expected for the type. As such, I reckon we should validate that the length is in the allowed range, that is, the same exact check that the void* constructor is doing. The two APIs should essentially be equivalent on this front, the only difference being this will work on a GC-ref. I'll add this to the PR description as well.

[stephentoub]

This is not desireable, as in many cases the only "unsafe" component library authors want is being able to pass an arbitrary length, but without giving up on lifetime tracking.

I'm having trouble seeing how this adds enough value to be worth the additional API, and in particular the ensuing confusion about having two different unsafe APIs for creating a span from a ref and a length that are unsafe in slightly different ways.

Both the proposed API and MemoryMarshal.CreateSpan are inherently unsafe and dangerous. If you get the length wrong, you can start overwriting who knows what. And anyone who feels the additional API is providing meaningful security can define it themselves in one line:

static Span<T> CreateUnsafeButMaybeSlightlyLessUnsafe(ref T value, int length) => MemoryMarshal.CreateSpan(ref value, length);

Can you share concrete examples where the proposed API will be used and significantly move the needle on security to the point where it's worth said confusion?

[Sergio0694]

The proposed API will essentially make it impossible to have dangling pointers, ie. to accidentally create GC holes (as well as not allowing negative lengths). Yes being able to pass an arbitrary length is still unsafe, but much less than it was before.

As per the latest version of the "lowlevel struct improvements" (here) proposal by @jaredpar, we'd have:

// With this proposal
public ref struct Span<T>
{
    public static Span<T> CreateUnsafe(ref T value, int length);
}

// Updated BCL API in .NET 7
public static class MemoryMarshal
{
    public static Span<T> CreateSpan<T>(scoped ref T value, int length);
}

With MemoryMarshal.CreateSpan obviously cheating and not actually respecting scoped. Consider this:

// Accidental GC hole
static Span<T> Foo(int x)
{
    return MemoryMarshal.CreateSpan(ref x, 1);
}

// Accidental GC hole #2
static Span<int> Bar()
{
    Span<int> foo = stackalloc int[10];
    
    ref int value = ref foo[0];
    
    return MemoryMarshal.CreateSpan(ref value, 1);
}

// This compiles and may lead to GC holes as there's no lifetime annotations
public struct Foo
{
    public long value;
    
    public Span<byte> AsBytes()
    {
        return MemoryMarshal.CreateSpan(ref Unsafe.As<long, byte>(ref value), 4);
    }
}

All of these scenarios can be avoided with a better Span<T>.CreateUnsafe that tracked lifetime rules.

The first two cases (and all analogous ones) would just outright fail to compile. The latter would force developers to properly annotate the type/APIs so that callers wouldn't be able to accidentally escape that returned Span<byte> outside the scope of a Foo instance where that method is invoked upon.

All in all the argument is: there's still plenty of scenarios in lowlevel code where developers might have to manually create a Span<T> instance from a reference with an arbitrary length. The current API is extremely dangerous, and the current ones would make this pattern significantly safe, which for cases where you're already dealing with tricky code, is not a bad thing 🙂

EDIT: for more context, my initial proposal was to just break CreateSpan and make it non-scoped, but @tannergooding mentioned it would still make sense to keep it around for some niche scenario where that extra unsafety might be useful. I don't think it's a good idea though to just force everyone working with lowlevel code to keep going through that API once C# 11 will in fact offer the ability to have the same API, but with proper lifetime tracking (as in, with the input reference being non-scoped).

[tannergooding]

I think Sergio gave a fairly good summary.

Not all unsafe is the same level of unsafe and 100% of unsafe code is safe when used safely.

This gives developers a tool that helps make something dangerous slightly less so. While you can still be error prone or do the wrong thing with it, it helps protect against what is often a more egregious and easier to make mistake around lifetimes (and where lengths can often have relevant asserts to help ensure correctness; lifetimes cannot).

I expect there are places we would use this in the BCL to help prevent various security or lifetime bugs while still allowing high perf. We're only human and so we're bound to make mistakes, having a little bit of safety rope or equipment is always good.

[stephentoub]

I understand the feature. I disagree with the conclusion.

Of the existing uses of MemoryMarshal.Create{ReadOnly}Span in dotnet/runtime, very few of them are in a situation where there's even a possibility of a dev doing the wrong thing around ref lifetime, e.g. it's in an API that doesn't hand back a ref struct. I do not see the proposed API meaningfully moving a needle here.

// Accidental GC hole
static Span Foo(int x)
{
return MemoryMarshal.CreateSpan(ref x, 1);
}

// Accidental GC hole #2
static Span Bar()
{
Span foo = stackalloc int[10];
ref int value = ref foo[0];
return MemoryMarshal.CreateSpan(ref value, 1);
}

Those examples aren't "GC holes": they're spans referring to ints on the stack. You can cause just as much trouble providing a wrong length as you can returning them.

The current API is extremely dangerous

So is the proposed new one. Except instead of having one dangerous CreateSpan that's unsafe, now we'd have two, and two with subtly different semantics. Just adding an additional one doesn't somehow magically make the existing one go away.

If we really believe it's meaningfully better, then we should not add scoped to the existing MemoryMarshal.Create{ReadOnly}Span, essentially making some existing usage a breaking change upon upgrade to C# 11, and we should then add scoped to Unsafe.AsRef's argument, such that someone who really needs the non-lifetime-tracked behavior can get it by using Unsafe.AsRef at the call site. We should not add yet another variant of the same method.

[tannergooding]

If we really believe it's meaningfully better, then we should not add scoped to the existing MemoryMarshal.Create{ReadOnly}Span, essentially making some existing usage a breaking change upon upgrade to C# 11, and we should then add scoped to Unsafe.AsRef's argument, such that someone who really needs the non-lifetime-tracked behavior can get it by using Unsafe.AsRef at the call site. We should not add yet another variant of the same method.

Today we have Span<T> MemoryMarshal.CreateSpan(ref T value, int length) => new Span<T>(ref value, length). Due to current language rules the returned Span<T> is assumed that it cannot capture ref value and so the current API is unsafe.

With the C# 11 (or w/e version the feature ships in, assuming it does indeed ship) language rule changes here, if we make no changes it would be assumed to capture and the returned Span<T> would be tied to the lifetime of the ref value.

If we wanted to remain compatible, we can change the API to be: Span<T> MemoryMarshal.CreateSpan(scoped ref T value, int length) => new Span<T>(ref Unsafe.AsUnscoped(ref value), length);. This would preserve the current "non-capturing" semantics and uses an Unsafe API to strip the scopedness of the ref value to make this possible.

Now, if we were to take the "breaking" change to CreateSpan, then we would need some additional API that takes in the Span<T> which currently tracks the lifetime of ref value and return a new Span<T> which does not. My understanding is that we would, as such, need Span<T> Unsafe.AsUnscoped(scoped Span<T> value) and ReadOnlySpan<T> Unsafe.AsUnscoped(scoped Span<T> value).

So I believe, and hopefully @jaredpar could confirm or tell me where I'm wrong, that we need some new API in most of the scenarios.
A) Preserve the semantics around MemoryMarshal.CreateSpan keeping it so the lifetime isn't tracked and expose no new APIs. Users need to build their own Span<T> SomeClass.CreateSpan<T>(ref T value, int length) => MemoryMarshal.CreateSpan(ref value, length) so that the returned Span<T> tracks the lifetime of ref value
B) Preserve the semantics around MemoryMarshal.CreateSpan keeping it so the lifetime isn't tracked and expose some API which does track the lifetime (Span<T> Span<T>.CreateUnsafe<T>(ref T value, int length))
C) Break the semantics around MemoryMarshal.CreateSpan, breaking it so the lifetime is tracked and expose new APIs that allow removing the tracking -- I don't believe that there is a way (outside manually writing IL) for a user to do this

This proposal, and my own preference, are for B. We ultimately have to have some API on Span<T> regardless to support MemoryMarshal.CreateSpan. Today, that is an internal constructor which we would never want to expose since there is no way to do something like UnsafeCallersOnly. We could however expose a Span<T> CreateUnsafe(ref T value, int length) method which fits our existing pattern and points users towards a more correct, but still unsafe option. Another reason is that this is a feature which many lowlevel devs are passionate about and wanting, its why we already have a proposal up suggesting something be done here despite there being no feature branch or the like yet. There is obvious interest and desire for additional safety gear here, even if it only provide minimal protection.

[stephentoub]

Now, if we were to take the "breaking" change to CreateSpan, then we would need some additional API that takes in the Span which currently tracks the lifetime of ref value and return a new Span which does not. My understanding is that we would, as such, need Span Unsafe.AsUnscoped(scoped Span value) and ReadOnlySpan Unsafe.AsUnscoped(scoped Span value).

Why would it need to be on the span? If we were to leave MemoryMarshal.CreateSpan unadorned, a span returned from it is still returnable if the lifetime of the input ref isn't local... so an Unsafe.AsRef that removed the local lifetime would suffice. In other words, you wouldn't modify the scopedness of the span returned from CreateSpan, rather you'd modify the lifetime of the ref being passed in (MemoryMarshal.CreateSpan(ref Unsafe.AsRef(localLifetimeRef), length)). That is what I was suggesting we do if we feel we must do something here. We add scoped to the existing Unsafe.AsRef parameter and call it a day in combination with accepting a breaking change to the existing CreateSpan (by not changing it).

I do not think we should do option (B). I'm fine with (A). Whether (C) is the best option remains to be seen based on hands-on experience when we actually get a compiler that implements the functionality, on how painful the break actually is in practice.

[tannergooding]

I think B is the best, then A, and C would be the worst.

you wouldn't modify the scopedness of the span returned from CreateSpan, rather you'd modify the lifetime of the ref being passed in (MemoryMarshal.CreateSpan(ref Unsafe.AsRef(localLifetimeRef), length)).

Right... This would work because even through CreateSpan tracks the lifetime; ref Unsafe.AsUnscoped(ref value) would be assumed to be a different scoping from ref value and would have to assume its therefore something directly in the GC heap (like an array interior pointer) or something...

[stephentoub]

I think B is the best, then A, and C would be the worst.

Then I think we should close this.

[gfoidl]

Please don't let become CreateSpan the new timers in .NET 😉

---

When (A) is not viable, then I find it much better to annotate, etc. the existing API and fix breaking changes there*, than introducing another API for (almost) the same thing (C).

* it's unsafe, so not that widely used and users who use this API (hopefully) know what they are doing, so a breaking change here isn't that bad to also revisit what's done unsafe