Reflection on enums creates instances of open generic types

[jkotas]

From https://codeblog.jonskeet.uk/2017/04/26/surprise-creating-an-instance-of-an-open-generic-type/

using System;
using System.Reflection;
 
class Program
{
    static void Main(string[] args)
    {
        object x = GetWeirdValue();
        // This line prints True
        Console.WriteLine(x.GetType().GetTypeInfo().IsGenericTypeDefinition);
    }
 
    static object GetWeirdValue() =>
        typeof(Generic<>.GenericEnum).GetTypeInfo()
            .GetDeclaredField("Foo")
            .GetValue(null);
 
    class Generic<T>
    {
        public enum GenericEnum
        {
            Foo = 0
        }
    }
}

[jkotas]

cc @AtsushiKan

[mattwarren]

Would this issue ever be 'up-for-grabs'?

If so I'd be happy to attempt it, I've recently looked at the reflection code for dotnet/runtime#7321.

[jkotas]

@mattwarren It's yours. Thanks!

[mattwarren]

Thanks, I just want to check, is fix is to make 'GetValue(null)' through an exception or something else.

[jkotas]

I think it may be better to return value of the underlying type.

[gkhanna79]

@jkotas Is this for 2.0?

[jkotas]

I do not think that this is must have for 2.0.

[ghost]

That same article sites two other ways to do this so if this loophole really needs to be closed, it needs to be done at the lower level than the specific apis - i.e. at the point where the runtime tries to instantiate the value with an open type as its umm.. "pMethodTable" - substituting the raw type might have other ripple effects.

I'm not sure if this loophole needs to be closed, though. We don't need to support it in CoreRT or other DotNet flavors.

[mattwarren]

I've been digging into this a bit more, to re-cap there are 3 scenarios:

Scenario One (from Jon Skeet's Blog)

class Program
{
    static void Main(string[] args)
    {
        object x = typeof(Generic<>.GenericEnum).GetTypeInfo()
            .GetDeclaredField("Foo")
            .GetValue(null);
        Console.WriteLine(x.GetType().GetTypeInfo().IsGenericTypeDefinition); // True!!!
    }

    class Generic<T> { public enum GenericEnum { Foo = 0 } }
}

Scenario Two (from Kirill Osenkov's blog)

class Program
{
    static void Main()
    {
        var open = Enum.ToObject(typeof(C<>.E), 0);
        Console.WriteLine(open.GetType().GetTypeInfo().IsGenericTypeDefinition); // True!!!
    }
}

class C<T> { public enum E { } }

Scenario Three (also from Kirill Osenkov's blog)

class Program
{
    static void Main()
    {
        Action<C<int>.E> a = M;
        var open = a.Method.GetGenericMethodDefinition().GetParameters()[0].DefaultValue;
        Console.WriteLine(open.GetType().GetTypeInfo().IsGenericTypeDefinition); // True!!!
    }

    static void M<T>(C<T>.E e = 0) { }
}

class C<T> { public enum E { } }

[mattwarren]

Scenarios One and Three end up in ReflectionInvocation::CreateEnum(..), Scenario Two however goes to ReflectionEnum::InternalBoxEnum(..)

So as a possible fix I made this change in ReflectionInvocation::CreateEnum(..)

     REFLECTCLASSBASEREF refType = (REFLECTCLASSBASEREF)ObjectToOBJECTREF(pTypeUNSAFE);

     TypeHandle typeHandle = refType->GetType();
     OBJECTREF obj = NULL;
     HELPER_METHOD_FRAME_BEGIN_RET_1(refType);
+    if (typeHandle.ContainsGenericVariables())
+    {      
+        COMPlusThrow(kInvalidOperationException, W("Acc_CreateGeneric")); //TODO is this the right error message?
+    }
     MethodTable *pEnumMT = typeHandle.AsMethodTable();
     obj = pEnumMT->Box(ArgSlotEndianessFixup ((ARG_SLOT*)&value,
                                              pEnumMT->GetNumInstanceFieldBytes()));

and this one in ReflectionEnum::InternalBoxEnum(..)

     FCALL_CONTRACT;

     VALIDATEOBJECT(target);
     OBJECTREF ret = NULL;

     MethodTable* pMT = target->GetType().AsMethodTable();
     HELPER_METHOD_FRAME_BEGIN_RET_0();

+    if (typeHandle.ContainsGenericVariables())
+    {
+        COMPlusThrow(kInvalidOperationException, W("Acc_CreateGeneric")); //TODO is this the right error message?
+    }

     ret = pMT->Box(ArgSlotEndianessFixup((ARG_SLOT*)&value, pMT->GetNumInstanceFieldBytes()));

     HELPER_METHOD_FRAME_END();

Both changes are in reflectioninvocation.cpp, which is useful because it limits the changes to 'reflection' scenarios. The single, common place where one fix could be done is in MethodTable::Box(void* data), but that seems to be used by code-paths that aren't related to reflection.

Am I on the right track?

I'm not sure if this loophole needs to be closed, though. We don't need to support it in CoreRT or other DotNet flavors.

Does this need fixing at all?

[ghost]

Well the existing behavior won't be easy to support in CoreRT so I suppose it's worth trying to close this loophole now - with the exception first, and keep "return the underlying int" as a Plan B if that turns out to break more than expected.

How expensive is TypeHandle.ContainsGenericVariables()? If it's super cheap, I'd say fix it in the common place (MethodTable::Box).

[mattwarren]

Sorry, I forgot too add that the reason I looked at throwing an InvalidOperationException was because I found that if you change Scenario One to instead fetch value__ (the magic compiler field)

object x = typeof(Generic<>.GenericEnum).GetTypeInfo()
            .GetDeclaredField("value__") // the 'value__' field is emitted by the compiler
            .GetValue(null);

Then InvalidOperationException is thrown with the message:

Late bound operations cannot be performed on fields with types for which Type.ContainsGenericParameters is true.

Because it hits this check in RtFieldInfo::InternalGetValue(..), in comparision if you access the 'Foo' field it goes via MdFieldInfo::GetValue(..) which doesn't have the check

[lauren-van-sloun]

Is this issue resolved? Can it be closed?

[GrabYourPitchforks]

Still repros on latest 5.0 master. @mattwarren do you still want to take this or should one of us tackle it?

[mattwarren]

@GrabYourPitchforks realistically I'm not going to get round to doing this, so feel to assign it to someone else

[steveharter]

This still repros on 7.0.

FWIW if the test shown in the description is modified to close the generic, the test returns false as expected.