Repository navigation
Shouldn't allow randomly placing ByRef-like fields #12842
Description
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.
Activity
jkotas commentedon Jun 11, 2019
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 commentedon Jun 11, 2019
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
Prevent loading byref-like types with invalid layout (#25200)
MichalStrehovsky commentedon Jun 18, 2019
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:
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 findByReference<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 commentedon Jun 18, 2019
I do not have a strong opinion on this. I would be ok with doing nothing.
MichalStrehovsky commentedon Jun 19, 2019
Happy to close then - this part was grey zone for me. The part we really cared about was fixed with dotnet/coreclr#25200.