Validate IntPtr and ByRef field overlap

[AaronRobinsonMSFT]

Support here could be enabled with the conservative GC but instead of permitting that, CoreCLR will block support in the type loader.

Confirmed tests pass locally on mono, CoreCLR, and with CrossGen.

/cc @davidwrighton @MichalStrehovsky @dotnet/interop-contrib

[MichalStrehovsky]

Is ByRef and IntPtr overlap a problem? We decided not to check this for ByRef-like types.

ByRef pointing to unmanaged memory or other garbage should be fine because that's what happens for Span pointing to unmanaged memory.

Or is this futureproofing for when we allow byrefs on the GC heap and we'll need to make sure these writes go through a write barriers when that happens?

[AaronRobinsonMSFT]

Or is this futureproofing for when we allow byrefs on the GC heap and we'll need to make sure these writes go through a write barriers when that happens?

I think the same thing can currently occur. For example, if a long and a Span<T> overlap, it is possible the long can be a value that resolves to an address that is on the managed heap. In this case, that would potentially cause issues because it would be finding a potentially invalid object.

@davidwrighton and I discussed this, and I was convinced it was a real issue. Feel free to push back though.

[MichalStrehovsky]

I don't have an opinion either way. But if we do this, we should probably also revisit this for byref-like types too so that we don't end up with a situation where:

ref struct Foo
{
    [Offset(0)] ref int Field1;
    [Offset(0)] IntPtr Field2
}

is disallowed but,

ref struct HiddenRef
{
    ref int Field;
}

ref struct Foo
{
    [Offset(0)] HiddenRef Field1;
    [Offset(0)] IntPtr Field2
}

is allowed, even though they're basically the same thing.

There was some conversation about it at #12842 (comment).

[MichalStrehovsky]

Or maybe your pull request already does this - I don't read C++ on weekends.

[AaronRobinsonMSFT]

I don't read C++ on weekends.

@MichalStrehovsky I get it, that's okay. However, it does handle your query above.

.class public sequential ansi sealed beforefieldinit InvalidCSharp.InnerValidByRef
    extends [System.Runtime]System.ValueType
{
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = (
        01 00 00 00
    )
    .field [0] public int32& Field
}

.class public explicit ansi sealed beforefieldinit InvalidCSharp.IntPtrOverlapWithInnerFieldType
    extends [System.Runtime]System.ValueType
{
    .custom instance void [System.Runtime]System.Runtime.CompilerServices.IsByRefLikeAttribute::.ctor() = (
        01 00 00 00
    )
    .field [0] public native int Field
    // This field's valid Type would illegally overlap this type's first field using a precise GC.
    .field [0] public valuetype InvalidCSharp.InnerValidByRef Invalid
}

[davidwrighton]

@MichalStrehovsky the problem with ByRef/ByRefLike and IntPtr overlap is that it becomes possible (without the use of unsafe) to generate a byref that points to something in the GC heap which isn't actually an object. (The GC has an invariant that ByRef pointers passed into it must either be within an object on the GC heap or not within the memory regions of the GC Heap at all.) If we've explicitly been permitting ByRefLike structure overlap with IntPtr for the purpose of doing funky things with Span<T> etc, we may need to revisit this restriction, but this is the safe approach for now.

[davidwrighton]

[MichalStrehovsky]

If we've explicitly been permitting ByRefLike structure overlap with IntPtr for the purpose of doing funky things

No, it was just decided that validating that wasn't worth the hassle at that time: #12842 (comment)

This test is going to fail on NativeAOT if we're not adding a check for that to the managed type system. I would be okay with that if it's not observable with crossgen2. The test is doing a typeof and that's an obvious type load that will let CoreCLR throw the exception no matter if the code was AOT'd. What if this was a sizeof or some other "use" of the type within the method body - will CoreCLR still get a chance to throw the exception or do we bypass it because crossgen2 already generated the necessary code and CoreCLR doesn't know what we loaded to get there? Might be difficult to write a test for that because of crossmodule restrictions within crossgen2 - the type and its use need to be in the same module.

FWIW, we've been mirroring these checks in the past (dotnet/coreclr#27054).

[lambdageek]

I don't think we should recurse into reference types when computing has_ref_fields. Otherwise mono bits LGTM

[AaronRobinsonMSFT]

I would be okay with that if it's not observable with crossgen2.

These tests pass under CrossGen2. I'm assuming that is what is meant here.

[MichalStrehovsky]

These tests pass under CrossGen2. I'm assuming that is what is meant here.

No, as I wrote above "The test is doing a typeof and that's an obvious type load that will let CoreCLR throw the exception no matter if the code was AOT'd".

I meant:

using System;
using System.Runtime.InteropServices;

Explicit ex = new Explicit
{
    Guid = Guid.NewGuid(),
};

return ex.Bytes.Length;

[StructLayout(LayoutKind.Explicit)]
ref struct Explicit
{
    [FieldOffset(0)] public Span<byte> Bytes;
    [FieldOffset(0)] public Guid Guid;
}

I just tried this:

C:\git\runtime\artifacts\bin\coreclr\windows.x64.Debug>csc /nologo /noconfig /nostdlib test.cs /r:System.Private.CoreLib.dll

C:\git\runtime\artifacts\bin\coreclr\windows.x64.Debug>corerun test.exe
Unhandled exception. System.TypeLoadException: Could not load type 'Explicit' from assembly 'test, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null' because it contains an object field at offset 0 that is incorrectly aligned or overlapped by a non-object field.
   at Program.<Main>$(String[] args)

C:\git\runtime\artifacts\bin\coreclr\windows.x64.Debug>crossgen2\crossgen2.exe test.exe -o:test.ni.exe -r:System.Private.CoreLib.dll
Emitting R2R PE file: test.ni.exe

C:\git\runtime\artifacts\bin\coreclr\windows.x64.Debug>corerun test.exe

C:\git\runtime\artifacts\bin\coreclr\windows.x64.Debug>echo %ERRORLEVEL%
985271193

So ReadyToRun currently can bypass the checks.