Shouldn't allow randomly placing ByRef-like fields

[MichalStrehovsky]

We should probably prevent loading types that attempt to place byref-like fields at addresses that are not divisible by pointer size.

[StructLayout(LayoutKind.Explicit)]
ref struct RefStruct
{
    [FieldOffset(0)]
    public short X;
    [FieldOffset(2)]
    public Span<int> Y;
}

class Program
{
    static RefStruct GetRefStruct()
    {
        return new RefStruct { Y = new int[1] { 42 } };
    }

    static void Collect()
    {
        GC.Collect();
        GC.WaitForPendingFinalizers();
        GC.Collect();
    }

    static void Main()
    {
        for (int i = 0; i < 1000; i++)
            GC.KeepAlive(new int[8]);
        RefStruct a = GetRefStruct();
        a.X = 1234;
        for (int i = 0; i < 1000; i++)
            GC.KeepAlive(new int[8]);

        Collect();
        Console.WriteLine(a.X);
        Console.WriteLine(a.Y[0]);
    }
}

This will print 1234/0 instead of 1234/42 (that gets printed if you remove the StructLayout(Explicit)).

Found while messing around with dotnet/coreclr#25056.

[jkotas]

If somebody does this, it will lead to silent intermittent data corruptions that are always incredibly painful to debug. How hard is this to fix? I think it would be worth fixing for 3.0.

[MichalStrehovsky]

If somebody does this, it will lead to silent intermittent data corruptions that are always incredibly painful to debug. How hard is this to fix? I think it would be worth fixing for 3.0.

I just did a "file & forget" and didn't actually look at how difficult it is to fix.

I'll move it to 3.0 and have a look.

Cc @sergiy-k

[MichalStrehovsky]

I fixed part of this bug described in the initial issue, but this still has a leftover piece:

We currently don't check for people overlaying byref-like things with other things. So:

[StructLayout(LayoutKind.Explicit)]
ref struct Broken
{
    [FieldOffset(0)]
    Span<int> _foo;
    [FieldOffset(0)]
    IntPtr _bad;
}

Will load just fine, even though we would have blocked loading this if the ByReference<T> field was actually a reference type.

It seems blocking this might be a good thing. We can either block this by specifically looking for ByReference<T> fields recursively, or David suggested we could just disallow overlaying byref-like structs with anything (we don't dig into the byref-like struct to find ByReference<T> fields and instead treat the whole struct as something that cannot be unioned with anything). The latter would be a more breaking change.

One argument for not doing anything for this is that the GC doesn't actually mind if the interior pointer points at junk. So maybe we don't need to block this...

Thoughts?

Cc @davidwrighton @jkotas

[jkotas]

We currently don't check for people overlaying byref-like things with other thing

I do not have a strong opinion on this. I would be ok with doing nothing.

[MichalStrehovsky]

I do not have a strong opinion on this. I would be ok with doing nothing.

Happy to close then - this part was grey zone for me. The part we really cared about was fixed with dotnet/coreclr#25200.