Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type validation holes when using Reflection to invoke a method with a void* parameter. #7430

Closed
Tracked by #44327
ghost opened this issue Feb 13, 2017 · 3 comments · Fixed by #73748
Closed
Tracked by #44327

Comments

@ghost
Copy link

ghost commented Feb 13, 2017

Consider the following test app:

namespace ConsoleApp1
{
    public unsafe class GG
    {
        public static void Main()
        {
            Test(UIntPtr.Zero);
        }

        public static void Test(object parameterToPassIn)
        {
            MyBinder binder = new MyBinder();
            MethodInfo m = typeof(GG).GetMethod("Foo");
            object[] args = { parameterToPassIn };
            m.Invoke(null, BindingFlags.Default, binder, args, null);
            if (!binder.ChangeTypeCalled)
            {
                Console.WriteLine("Failed. Binder not given chance to fix things.");
                return;
            }
            Console.WriteLine("Passed.");
            return;
        }


        public static void Foo(void*pv)
        {
        }
    }

    public sealed unsafe class MyBinder : Binder
    {
        public bool ChangeTypeCalled { get; private set; }

        public sealed override FieldInfo BindToField(BindingFlags bindingAttr, FieldInfo[] match, object value, CultureInfo culture)
        {
            throw new NotImplementedException();
        }

        public sealed override MethodBase BindToMethod(BindingFlags bindingAttr, MethodBase[] match, ref object[] args, ParameterModifier[] modifiers, CultureInfo culture, string[] names, out object state)
        {
            throw new NotImplementedException();
        }

        public sealed override object ChangeType(object value, Type type, CultureInfo culture)
        {
            ChangeTypeCalled = true;
            return Pointer.Box((void*)0, typeof(void*));
        }

        public sealed override void ReorderArgumentArray(ref object[] args, object state)
        {
            throw new NotImplementedException();
        }

        public sealed override MethodBase SelectMethod(BindingFlags bindingAttr, MethodBase[] match, Type[] types, ParameterModifier[] modifiers)
        {
            throw new NotImplementedException();
        }

        public sealed override PropertyInfo SelectProperty(BindingFlags bindingAttr, PropertyInfo[] match, Type returnType, Type[] indexes, ParameterModifier[] modifiers)
        {
            throw new NotImplementedException();
        }
    }
}

Passing a UIntPtr to a void* isn't allowed so what should happen is that Reflection should call MyBinder.ChangeType() to give it a chance to convert the UIntPtr into something that is acceptable.

Instead, Reflection throws an ArgumentException with the message "Object type cannot be converted to target type." and never calls the binder.

That is, on release builds.

On checked builds, it triggers a GC_NOTRIGGER contract violation while propagating the exception:

GC_TRIGGERS encountered in a GC_NOTRIGGER scope

                        CONTRACT in CLRException::GetThrowableFromException at "c:\dd\coreclr\src\vm\clrex.cpp" @ 714
                        GCX_COOP in UnwindAndContinueRethrowHelperInsideCatch at "c:\dd\coreclr\src\vm\excep.cpp" @ 8566
                        CONTRACT in MethodDescCallSite::CallTargetWorker at "c:\dd\coreclr\src\vm\callhelpers.cpp" @ 361
                        GCX_COOP in Assembly::ExecuteMainMethod at "c:\dd\coreclr\src\vm\assembly.cpp" @ 2468
                        CONTRACT in Assembly::ExecuteMainMethod at "c:\dd\coreclr\src\vm\assembly.cpp" @ 2452
                        GCX_COOP in CorHost2::ExecuteAssembly at "c:\dd\coreclr\src\vm\corhost.cpp" @ 651
                        CONTRACT in CorHost2::ExecuteAssembly at "c:\dd\coreclr\src\vm\corhost.cpp" @ 595


CORECLR! CONTRACT_ASSERT + 0x342 (0x00007ffa`f693acf2)
CORECLR! EEContract::DoChecks + 0x3DE (0x00007ffa`f6ad1b8e)
CORECLR! CLRException::GetThrowableFromException + 0x14B (0x00007ffa`f6b164cb)
CORECLR! UnwindAndContinueRethrowHelperInsideCatch + 0xBB (0x00007ffa`f6acf6fb)
CORECLR! `RuntimeMethodHandle::InvokeMethod'::`1'::catch$11 + 0xF4 (0x00007ffa`f78c308f)
CORECLR! CallSettingFrame + 0x20 (0x00007ffa`f7611990)
CORECLR! _CxxCallCatchBlock + 0x15A (0x00007ffa`f760c16a)
NTDLL! RtlCaptureContext + 0x3C3 (0x00007ffb`442ea193)
CORECLR! RuntimeMethodHandle::InvokeMethod + 0xDEE (0x00007ffa`f72e246e)

The problems are multifold. First, when the formal parameter type is a pointer to void, the code that's supposed to catch invalid arguments and divert to the binder (https://github.com/dotnet/coreclr/blob/master/src/vm/reflectioninvocation.cpp#L298) abdicates from the duty and declares virtually any input argument as legal. So the binder never gets called and Reflection proceeds to execute the call.

However, the calling mechanism performs its own type checks so which happens at InvokeUtil::CopyArg() (https://github.com/dotnet/coreclr/blob/master/src/vm/invokeutil.cpp#L299)

    case ELEMENT_TYPE_PTR: 
    case ELEMENT_TYPE_FNPTR:
    {
        // If we got the univeral zero...Then assign it and exit.
        if (rObj == 0) {
            *(PVOID *)pArgDst = 0;
        }
        else {
            if (rObj->GetMethodTable() == MscorlibBinder::GetClassIfExist(CLASS__POINTER) && type == ELEMENT_TYPE_PTR) 
                *(PVOID *)pArgDst = GetPointerValue(rObj);
            else if (rObj->GetTypeHandle().AsMethodTable() == MscorlibBinder::GetElementType(ELEMENT_TYPE_I)) 
            {
                ARG_SLOT slot;
                CreatePrimitiveValue(oType, oType, rObj, &slot);
                *(PVOID *)pArgDst = (PVOID)slot;
            }
            else
                COMPlusThrow(kArgumentException,W("Arg_ObjObj"));
        }
        break;
    }

Firstly, there's the unguarded call to "AsMethodTable()" for the purpose of seeing if "rObj" is a System.IntPtr. If you pass in something like an int[], this triggers an !IsTypeDesc() assertion inside AsMethodTable. Since the returned "pointer" value is only compared to another constant pointer value, there's no major risk here but it is shabby.

Two, the COMPlusThrow() at this point triggers the aforementioned GC_NOTRIGGER violation while bubbling up.

sdmaclea referenced this issue in dotnet/coreclr May 8, 2018
@kouvel kouvel removed their assignment Jun 1, 2018
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the Future milestone Jan 31, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 26, 2020
@steveharter
Copy link
Member

Managed call stack:

System.ArgumentException
  HResult=0x80070057
  Message=Object type cannot be converted to target type.
  Source=mscorlib
  StackTrace:
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor)
   at System.Reflection.RuntimeMethodInfo.UnsafeInvokeInternal(Object obj, Object[] parameters, Object[] arguments)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at ConsoleApp1.GG.Test(Object parameterToPassIn) in C:\Users\sharter.NORTHAMERICA\source\repos\ConsoleApp17\Program.cs:line 19
   at ConsoleApp1.GG.Main() in Program.cs:line 11

@steveharter steveharter removed the untriaged New issue has not been triaged by the area owner label Mar 26, 2020
@steveharter steveharter modified the milestones: Future, 5.0 Mar 26, 2020
@steveharter
Copy link
Member

Moving to future based on schedule + priority.

Please add additional hits to this issue to increase priority.

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 11, 2022

Passing a UIntPtr to a void* isn't allowed so what should happen is that Reflection should call MyBinder.ChangeType() to give it a chance to convert the UIntPtr into something that is acceptable.

There is explicit operators for UIntPtr to void * and vise versa https://docs.microsoft.com/en-us/dotnet/api/system.uintptr.op_explicit?view=net-6.0#system-uintptr-op-explicit(system-void*)-system-uintptr, so I think it should just work similarly how it works for IntPtr

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 11, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

7 participants