`StructureMarshaler` doesn't guard from non-struct type arguments

[hez2010]

In StructureMarshaler<T> it only requires T to be notnull, which allows arbitrary generic instantiations including invalid ones like StructureMarshaler<object>:

runtime/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs

Line 1357 in 95aa263

internal sealed unsafe class StructureMarshaler<T> : IArrayElementMarshaler<T, StructureMarshaler<T>> where T : notnull

This can trigger a VM assertion later in dllimport.cpp:

runtime/src/coreclr/vm/dllimport.cpp

Line 4113 in 95aa263

_ASSERTE(pStructMT->IsValueType());

Repro:

using System.Reflection;
using System.Runtime.CompilerServices;

class Program
{
    static void Main()
    {
        var t = Type.GetType("System.StubHelpers.StructureMarshaler`1")!.MakeGenericType(typeof(object));
        RuntimeHelpers.RunClassConstructor(t.TypeHandle);
        var m = t.GetMethod("System.StubHelpers.IArrayElementMarshaler<T,System.StubHelpers.StructureMarshaler<T>>.ConvertToManaged", BindingFlags.NonPublic | BindingFlags.Static)!;
        RuntimeHelpers.PrepareMethod(m.MethodHandle);
    }
}

Result:

Assert failure(PID 21880 [0x00005578], Thread: 62180 [0xf2e4]): pStructMT->IsValueType()

CORECLR! StructMarshalStubs::TryGenerateStructMarshallingMethod + 0x69D (0x00007ffc`84c27c2d)
CORECLR! CEEInfo::getMethodInfoWorker + 0x4F0 (0x00007ffc`849638d0)
CORECLR! CEEInfo::getMethodInfo + 0x35B (0x00007ffc`849631ab)
CLRJIT! `Compiler::impCheckCanInline'::`2'::<lambda_1>::operator() + 0x228 (0x00007ffc`8e0b23a8)
CLRJIT! `Compiler::impCheckCanInline'::`2'::<lambda_1>::<lambda_invoker_cdecl> + 0x16 (0x00007ffc`8e0b1976)
CORECLR! CEEInfo::runWithErrorTrap + 0x2A0 (0x00007ffc`8497bef0)
CLRJIT! Compiler::eeRunWithErrorTrapImp + 0x64 (0x00007ffc`8df15744)
CLRJIT! Compiler::eeRunWithErrorTrap<`Compiler::impCheckCanInline'::`2'::Param> + 0x28 (0x00007ffc`8e0b1d88)
CLRJIT! Compiler::impCheckCanInline + 0x110 (0x00007ffc`8e0b6ef0)
CLRJIT! Compiler::impMarkInlineCandidateHelper + 0x69F (0x00007ffc`8e0c887f)
    File: D:\runtime\src\coreclr\vm\dllimport.cpp:4113
    Image: D:\runtime\artifacts\obj\coreclr\windows.x64.Debug\hosts\corerun\corerun.exe

This was found in jit-dasm-pmi as it will try instantiating a generic type with object and call RuntimeHelpers.PrepareMethod against it.

I think we need either constraint T in StructureMarshaler<T> with struct or make TryGenerateStructMarshallingMethod more resilient.

cc: @MihaZupan

[dotnet-policy-service]

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

[AaronRobinsonMSFT]

I think we need either constraint T in StructureMarshaler with struct or make TryGenerateStructMarshallingMethod more resilient.

Why was this being used at all? It is an internal type for interop purposes. If an update is needed feel free to make a PR and determine the fallout. The interop team can help review.

[hez2010]

Why was this being used at all? It is an internal type for interop purposes.

pmi is trying to instantiate all generic types with a fixed list of types for the generic arguments and run the cctor which can hit this. I think it's fine to leave it as is and instead adding a set of types to be excluded from the cctor run in pmi. But I also think calling PrepareMethod shouldn't crash the runtime.

[AaronRobinsonMSFT]

I think it's fine to leave it as is and instead adding a set of types to be excluded from the cctor run in pmi.

This is an internal type that is not designed to be used outside of the interop space where entry to it is blocked at a higher level. In fact, I'm sort of curious how PMI is accessing it since it is marked internal. I'd sincerely hope that PMI is not leveraging UnsafeAccessorType for this sort of thing. That is a big no no and something that shouldn't happen as it negatively impacts runtime development - random tests might fail outside of the normal CI because test infrastructure is relying on internal types without sign off from the respective team. You may have "broken into jail" on this one :-)

But I also think calling PrepareMethod shouldn't crash the runtime.

Agree. To be clear, I'm fine adding the appropriate constraints on this. I'm simply surprised that testing is relying on an internally marked type in SPCL without discussion from the type owning team.

[MichalPetryka]

This is an internal type that is not designed to be used outside of the interop space where entry to it is blocked at a higher level. In fact, I'm sort of curious how PMI is accessing it since it is marked internal. I'd sincerely hope that PMI is not leveraging UnsafeAccessorType for this sort of thing. That is a big no no and something that shouldn't happen as it negatively impacts runtime development - random tests might fail outside of the normal CI because test infrastructure is relying on internal types without sign off from the respective team. You may have "broken into jail" on this one :-)

PMI is basically an internal tool for JIT diffs and it doesn't really use stuff AFAIR, it just calls RuntimeHelpers.PrepareMethod on every method and type possible with reflection and just uses a fixed list of predefined types to try with generics. As such, it expects everything to compile successfully at least.

[jkotas]

constraint T in StructureMarshaler

StructureMarshaler is used in constraint bridging pattern:

runtime/src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs

Lines 1643 to 1650 in e556d5a

	if (typeof(T).IsValueType)
	{
	StructureMarshaler<T>.ConvertToUnmanaged(ref Unsafe.As<byte, T>(ref managed.GetRawData()), unmanaged, ref cleanupWorkList);
	}
	else
	{
	LayoutClassMarshaler<T>.ConvertToUnmanaged(Unsafe.As<object, T>(ref managed), unmanaged, ref cleanupWorkList);
	}

. If we were to add constraint to StructureMarshaler, it would not compile anymore.

I think the right fix is to throw an exception (or generate IL that throws exception) instead of asserting.

[AaronRobinsonMSFT]

it just calls RuntimeHelpers.PrepareMethod on every method and type possible with reflection and just uses a fixed list of predefined types to try with generics.

Ah, I recall this happening before. I see how we got here.

I think the right fix is to throw an exception (or generate IL that throws exception) instead of asserting.

+1