From b4f28d7470da18ec871ae1240a6c9df2db44da2c Mon Sep 17 00:00:00 2001 From: Koundinya Veluri Date: Thu, 6 Apr 2017 23:51:13 -0700 Subject: [PATCH] Call custom parameterless constructor on structs through Activator (#10778) Fixes #6843 - Disabled caching struct types that have custom parameterless constructors in `ActivatorCache` - Removed some things relevant to security, which don't apply to CoreCLR --- src/mscorlib/src/System/RtType.cs | 13 +-- src/mscorlib/src/System/RuntimeHandles.cs | 2 +- src/vm/reflectioninvocation.cpp | 64 ++---------- src/vm/runtimehandles.h | 6 +- .../ActivateStructDefCtor/ActivateStructDefCtor.il | 107 +++++++++++++++++++++ .../ActivateStructDefCtor.ilproj | 21 ++++ 6 files changed, 142 insertions(+), 71 deletions(-) create mode 100644 tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.il create mode 100644 tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.ilproj diff --git a/src/mscorlib/src/System/RtType.cs b/src/mscorlib/src/System/RtType.cs index 678e46f..370cb0d 100644 --- a/src/mscorlib/src/System/RtType.cs +++ b/src/mscorlib/src/System/RtType.cs @@ -4701,15 +4701,12 @@ namespace System internal volatile CtorDelegate m_ctor; internal readonly RuntimeMethodHandleInternal m_hCtorMethodHandle; internal readonly MethodAttributes m_ctorAttributes; - // Is a security check needed before this constructor is invoked? - internal readonly bool m_bNeedSecurityCheck; // Lazy initialization was performed internal volatile bool m_bFullyInitialized; - internal ActivatorCacheEntry(RuntimeType t, RuntimeMethodHandleInternal rmh, bool bNeedSecurityCheck) + internal ActivatorCacheEntry(RuntimeType t, RuntimeMethodHandleInternal rmh) { m_type = t; - m_bNeedSecurityCheck = bNeedSecurityCheck; m_hCtorMethodHandle = rmh; if (!m_hCtorMethodHandle.IsNullHandle()) m_ctorAttributes = RuntimeMethodHandle.GetAttributes(m_hCtorMethodHandle); @@ -4779,16 +4776,12 @@ namespace System internal Object CreateInstanceSlow(bool publicOnly, bool skipCheckThis, bool fillCache, ref StackCrawlMark stackMark) { RuntimeMethodHandleInternal runtime_ctor = default(RuntimeMethodHandleInternal); - bool bNeedSecurityCheck = true; bool bCanBeCached = false; - bool bSecurityCheckOff; if (!skipCheckThis) CreateInstanceCheckThis(); - bSecurityCheckOff = true; // CoreCLR does not use security at all. - - Object instance = RuntimeTypeHandle.CreateInstance(this, publicOnly, bSecurityCheckOff, ref bCanBeCached, ref runtime_ctor, ref bNeedSecurityCheck); + Object instance = RuntimeTypeHandle.CreateInstance(this, publicOnly, ref bCanBeCached, ref runtime_ctor); if (bCanBeCached && fillCache) { @@ -4801,7 +4794,7 @@ namespace System } // cache the ctor - ActivatorCacheEntry ace = new ActivatorCacheEntry(this, runtime_ctor, bNeedSecurityCheck); + ActivatorCacheEntry ace = new ActivatorCacheEntry(this, runtime_ctor); activatorCache.SetEntry(ace); } diff --git a/src/mscorlib/src/System/RuntimeHandles.cs b/src/mscorlib/src/System/RuntimeHandles.cs index 37db526..ab125e7 100644 --- a/src/mscorlib/src/System/RuntimeHandles.cs +++ b/src/mscorlib/src/System/RuntimeHandles.cs @@ -203,7 +203,7 @@ namespace System } [MethodImplAttribute(MethodImplOptions.InternalCall)] - internal static extern Object CreateInstance(RuntimeType type, bool publicOnly, bool noCheck, ref bool canBeCached, ref RuntimeMethodHandleInternal ctor, ref bool bNeedSecurityCheck); + internal static extern Object CreateInstance(RuntimeType type, bool publicOnly, ref bool canBeCached, ref RuntimeMethodHandleInternal ctor); [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern Object CreateCaInstance(RuntimeType type, IRuntimeMethodInfo ctor); diff --git a/src/vm/reflectioninvocation.cpp b/src/vm/reflectioninvocation.cpp index 005279c..626e872 100644 --- a/src/vm/reflectioninvocation.cpp +++ b/src/vm/reflectioninvocation.cpp @@ -470,21 +470,17 @@ FCIMPL1(Object*, RuntimeTypeHandle::Allocate, ReflectClassBaseObject* pTypeUNSAF }//Allocate FCIMPLEND -FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refThisUNSAFE, +FCIMPL4(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refThisUNSAFE, CLR_BOOL publicOnly, - CLR_BOOL securityOff, CLR_BOOL* pbCanBeCached, - MethodDesc** pConstructor, - CLR_BOOL *pbNeedSecurityCheck) { + MethodDesc** pConstructor) { CONTRACTL { FCALL_CHECK; PRECONDITION(CheckPointer(refThisUNSAFE)); PRECONDITION(CheckPointer(pbCanBeCached)); - PRECONDITION(CheckPointer(pbNeedSecurityCheck)); PRECONDITION(CheckPointer(pConstructor)); PRECONDITION(*pbCanBeCached == false); PRECONDITION(*pConstructor == NULL); - PRECONDITION(*pbNeedSecurityCheck == true); } CONTRACTL_END; @@ -508,7 +504,6 @@ FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refT HELPER_METHOD_FRAME_BEGIN_RET_2(rv, refThis); MethodTable* pVMT; - bool bNeedAccessCheck; // Get the type information associated with refThis if (thisTH.IsNull() || thisTH.IsTypeDesc()) @@ -518,8 +513,6 @@ FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refT pVMT->EnsureInstanceActive(); - bNeedAccessCheck = false; - #ifdef FEATURE_COMINTEROP // If this is __ComObject then create the underlying COM object. if (IsComObjectClass(refThis->GetType())) { @@ -577,34 +570,16 @@ FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refT COMPlusThrow(kMissingMethodException,W("Arg_NoDefCTor")); } - if (!securityOff) - { - { - // Public critical types cannot be accessed by transparent callers - bNeedAccessCheck = !pVMT->IsExternallyVisible() || Security::TypeRequiresTransparencyCheck(pVMT); - } - - if (bNeedAccessCheck) - { - RefSecContext sCtx(InvokeUtil::GetInvocationAccessCheckType()); - InvokeUtil::CanAccessClass(&sCtx, pVMT, TRUE); - } - } - - // Handle the nullable special case + // Handle the nullable special case if (Nullable::IsNullableType(thisTH)) { rv = Nullable::BoxedNullableNull(thisTH); } else rv = pVMT->Allocate(); - // Since no security checks will be performed on cached value types without default ctors, - // we cannot cache those types that require access checks. - // In fact, we don't even need to set pbNeedSecurityCheck to false here. - if (!pVMT->Collectible() && !bNeedAccessCheck) + if (!pVMT->Collectible()) { *pbCanBeCached = true; - *pbNeedSecurityCheck = false; } } else // !pVMT->HasDefaultConstructor() @@ -617,30 +592,6 @@ FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refT if (!IsMdPublic(attr) && publicOnly) COMPlusThrow(kMissingMethodException,W("Arg_NoDefCTor")); - if (!securityOff) - { - // If the type is critical or the constructor we're using is critical, we need to ensure that - // the caller is allowed to invoke it. - bool needsTransparencyCheck = Security::TypeRequiresTransparencyCheck(pVMT) || - (Security::IsMethodCritical(pMeth) && !Security::IsMethodSafeCritical(pMeth)); - - // We also need to do a check if the method or type is not public - bool needsVisibilityCheck = !IsMdPublic(attr) || !pVMT->IsExternallyVisible(); - - // If the visiblity, transparency, or legacy LinkDemands on the type or constructor dictate that - // we need to check the caller, then do that now. - bNeedAccessCheck = needsTransparencyCheck || - needsVisibilityCheck || - pMeth->RequiresLinktimeCheck(); - - if (bNeedAccessCheck) - { - // this security context will be used in cast checking as well - RefSecContext sCtx(InvokeUtil::GetInvocationAccessCheckType()); - InvokeUtil::CanAccessMethod(pMeth, pVMT, NULL, &sCtx); - } - } - // We've got the class, lets allocate it and call the constructor OBJECTREF o; bool remoting = false; @@ -663,12 +614,13 @@ FCIMPL6(Object*, RuntimeTypeHandle::CreateInstance, ReflectClassBaseObject* refT rv = o; GCPROTECT_END(); - // No need to set these if they cannot be cached - if (!remoting && !pVMT->Collectible()) + // No need to set these if they cannot be cached. In particular, if the type is a value type with a custom + // parameterless constructor, don't allow caching and have subsequent calls come back here to allocate an object and + // call the constructor. + if (!remoting && !pVMT->Collectible() && !pVMT->IsValueType()) { *pbCanBeCached = true; *pConstructor = pMeth; - *pbNeedSecurityCheck = bNeedAccessCheck; } } } diff --git a/src/vm/runtimehandles.h b/src/vm/runtimehandles.h index d9a1082..2963fbe 100644 --- a/src/vm/runtimehandles.h +++ b/src/vm/runtimehandles.h @@ -126,12 +126,10 @@ public: // Static method on RuntimeTypeHandle static FCDECL1(Object*, Allocate, ReflectClassBaseObject *refType) ; //A.CI work - static FCDECL6(Object*, CreateInstance, ReflectClassBaseObject* refThisUNSAFE, + static FCDECL4(Object*, CreateInstance, ReflectClassBaseObject* refThisUNSAFE, CLR_BOOL publicOnly, - CLR_BOOL securityOff, CLR_BOOL *pbCanBeCached, - MethodDesc** pConstructor, - CLR_BOOL *pbNeedSecurityCheck); + MethodDesc** pConstructor); static FCDECL2(Object*, CreateCaInstance, ReflectClassBaseObject* refCaType, ReflectMethodObject* pCtorUNSAFE); diff --git a/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.il b/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.il new file mode 100644 index 0000000..83bab84 --- /dev/null +++ b/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.il @@ -0,0 +1,107 @@ +.assembly extern mscorlib +{ + .publickeytoken = (B7 7A 5C 56 19 34 E0 89 ) + .ver 2:0:0:0 +} +.assembly ActivateStructDefCtor +{ +} + +// =============== CLASS MEMBERS DECLARATION =================== + +.class private abstract auto ansi sealed beforefieldinit Program + extends [System.Runtime]System.Object +{ + .class sequential ansi sealed nested private beforefieldinit Foo + extends [System.Runtime]System.ValueType + { + .field public int32 x + + .method public hidebysig specialname rtspecialname + instance void .ctor() cil managed + { + // Code size 8 (0x8) + .maxstack 8 + IL_0000: ldarg.0 + IL_0001: ldc.i4.1 + IL_0002: stfld int32 Program/Foo::x + IL_0007: ret + } // end of method Foo::.ctor + } // end of class Foo + + .field private static int32 Pass + .field private static int32 Fail + + .method public hidebysig static int32 Main() cil managed + { + .entrypoint + // Code size 89 (0x59) + .maxstack 2 + .locals init ([0] int32 testIndex, + [1] int32 i, + [2] valuetype Program/Foo foo) + IL_0000: ldc.i4.m1 + IL_0001: stloc.0 + IL_0002: ldc.i4.0 + IL_0003: stloc.1 + IL_0004: br.s IL_004f + + IL_0006: ldloc.0 + IL_0007: ldc.i4.1 + IL_0008: add + IL_0009: stloc.0 + IL_000a: call !!0 [System.Runtime]System.Activator::CreateInstance() + IL_000f: stloc.2 + IL_0010: ldloc.2 + IL_0011: ldfld int32 Program/Foo::x + IL_0016: ldc.i4.1 + IL_0017: beq.s IL_0021 + + IL_0019: ldsfld int32 Program::Fail + IL_001e: ldloc.0 + IL_001f: add + IL_0020: ret + + IL_0021: ldloc.0 + IL_0022: ldc.i4.1 + IL_0023: add + IL_0024: stloc.0 + IL_0025: ldtoken Program/Foo + IL_002a: call class [System.Runtime]System.Type [System.Runtime]System.Type::GetTypeFromHandle(valuetype [System.Runtime]System.RuntimeTypeHandle) + IL_002f: call object [System.Runtime]System.Activator::CreateInstance(class [System.Runtime]System.Type) + IL_0034: unbox.any Program/Foo + IL_0039: stloc.2 + IL_003a: ldloc.2 + IL_003b: ldfld int32 Program/Foo::x + IL_0040: ldc.i4.1 + IL_0041: beq.s IL_004b + + IL_0043: ldsfld int32 Program::Fail + IL_0048: ldloc.0 + IL_0049: add + IL_004a: ret + + IL_004b: ldloc.1 + IL_004c: ldc.i4.1 + IL_004d: add + IL_004e: stloc.1 + IL_004f: ldloc.1 + IL_0050: ldc.i4.2 + IL_0051: blt.s IL_0006 + + IL_0053: ldsfld int32 Program::Pass + IL_0058: ret + } // end of method Program::Main + + .method private hidebysig specialname rtspecialname static + void .cctor() cil managed + { + // Code size 15 (0xf) + .maxstack 8 + IL_0000: ldc.i4.s 100 + IL_0002: stsfld int32 Program::Pass + IL_0007: ldc.i4.s 101 + IL_0009: stsfld int32 Program::Fail + IL_000e: ret + } // end of method Program::.cctor +} // end of class Program diff --git a/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.ilproj b/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.ilproj new file mode 100644 index 0000000..ac335b9 --- /dev/null +++ b/tests/src/reflection/ActivateStructDefCtor/ActivateStructDefCtor.ilproj @@ -0,0 +1,21 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + {F8D9C0E2-F86F-493F-8D18-0D19ADF6E3FD} + Library + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + + + + + + + + + + + -- 2.7.4