Box helpers do not handle null-valued managed pointers

[jakobbotsch]

The box helpers currently do not handle null refs inside of them. This should be fixed if we are making null-valued managed pointers legal (#69690). Repro:

object val = Unsafe.NullRef<Guid>();

Expected: NullReferenceException thrown
Actual: Fatal error. Internal CLR error. (0x80131506)

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jakobbotsch]

Note that this might properly null-ref if JIT is optimizing (since that skips the box helper).

[jkotas]

This should be fixed if we are making null-valued managed pointers legal

They are legal, but unsafe. If you are writing unsafe code, you have to know what you are doing.

I expect that there are many other places in the runtime and libraries where null managed pointers cause bad crashes.

[tannergooding]

@jkotas, I don't think this is an acceptable stance for the runtime anymore. Things like Span<T> and ref fields are going to make null refs much more prominent.

Starting with .NET 7 and C# 11, anyone can define ref struct MyStruct<T> { public ref T Value; }. Once they do, MyStruct<int> x = default; has now initialized a null ref using entirely safe and legal code and the behavior of accessing that null-ref should be well-defined.

If it is not, then I think the language team needs to be aware and needs to have specialized handling around this scenario.

[jkotas]

ok, I agree that ref fields are changing the equation on this.

cc @AaronRobinsonMSFT We may need to do an audit to make sure that null byrefs are handled gracefully throughout the runtime and libraries.

[AaronRobinsonMSFT]

Agree that dereferencing a null ref should have well-defined behavior. However, I'd argue it should only be defined for cases where it is not considered "unsafe" usage. This means that starting from object val = Unsafe.NullRef<Guid>(); doesn't qualify. I say that because boxing stubs are by definition not permissible for types that have a ref field (i.e., ByRefLike). Attempting that is illegal still and unlikely to change.

Things like Span and ref fields are going to make null refs much more prominent.

I don't see how Span<T> is involved here at all. Regardless of how Span<T> is defined now, it has identical semantic behavior as it once did. However, ref fields ...

Starting with .NET 7 and C# 11, anyone can define ref struct MyStruct { public ref T Value; }.

This is a good point and we should audit the system when user-defined ByRefLike types are involved, including the cases where we may open things up to ByRefLike types in Generics. My assumption would be the platform is safe for all C# since the only types being augmented by the first phase of this work are Span<T>, ReadOnlySpan<T>, and TypedReference. That leaves non-C# code in the runtime. The concern then is, like the boxing example, the IL Stubs that are generated at run-time.

This means we can narrow the auditing to only the IL Stub generation as the rest of the system should be well-defined.

Is this a fair assessment or am I missing some nuance?

[tannergooding]

I don't see how Span is involved here at all. Regardless of how Span is defined now, it has identical semantic behavior as it once did. However, ref fields ...

My point was that Span<T> has a ref field and so Span<T> already gets a null ref if you Span<int> span = default. Because of this, and because cases like new Span<T>(null, 1) are valid (but unsafe) to define, we're already in a world where a null byref can be easily gotten and where there is some semblance of expecting a well-defined behavior (some of this being covered by the bounds checks that otherwise exist).

An example of purely safe code that exposes the null byref is default(Span<int>).GetPinnableReference().

This means that starting from object val = Unsafe.NullRef(); doesn't qualify.

I'm not so sure on this. This is functionally no different from:

public ref struct ByRef<T>
{
    public ref T Value;
}

object val = default(ByRef<Guid>).Value;

Both should be well-defined, even if one is conceptually "unsafe". That is, not everything in Unsafe is actually "unsafe". SizeOf<T> for example is completely well-defined and safe, its just exposing something that higher level languages like C# may not support.

With ref fields, Unsafe.NullRef and Unsafe.IsNullRef become similar. Both are concepts which exist and which you typically don't want to handle in C#, but they are concepts that are possible to encounter and handle in many contexts.

[AaronRobinsonMSFT]

@tannergooding I updated the code above to be ref struct, just FYI.

This means that starting from object val = Unsafe.NullRef(); doesn't qualify.

I'm not so sure on this. This is functionally no different from:

object val = default(ByRef<Guid>).Value;

That is fair. Alright, I think I've got enough to look into this. Thanks all.