From c903cafd8c2715f7938ce26b009a5e3b32bb1826 Mon Sep 17 00:00:00 2001 From: Samuel Arzt Date: Thu, 16 Nov 2017 19:44:33 +0100 Subject: [PATCH] [ILVerify] Implement missing newobj verifications (#4949) * Added some missing newobj verifications. * Renamed Initonly VerifierError to InitOnly. * Implemented remaining newobj verifications. * Added missing readonly call verification. * Added missing field parent constraints check. * Added missing check for tail call in exception regions. * Added found type to Endfilter Int expected error. * Added tests for newly implemented verification rules. --- src/ILVerify/src/ILImporter.Verify.cs | 57 ++++++++++++++++------- src/ILVerify/src/Resources/Strings.resx | 23 ++++++++- src/ILVerify/src/VerifierError.cs | 16 +++---- src/ILVerify/tests/ILTests/FieldTests.il | 34 +++++++++++--- src/ILVerify/tests/ILTests/NewobjTests.il | 46 ++++++++++++++++++ src/ILVerify/tests/ILTests/PrefixTests.il | 57 +++++++++++++++++++++-- 6 files changed, 198 insertions(+), 35 deletions(-) create mode 100644 src/ILVerify/tests/ILTests/NewobjTests.il diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs index f1eedf754f1..025d2475610 100644 --- a/src/ILVerify/src/ILImporter.Verify.cs +++ b/src/ILVerify/src/ILImporter.Verify.cs @@ -1332,7 +1332,7 @@ void ImportCall(ILOpcode opcode, int token) Check(methodType != null, VerifierError.CallVirtOnStatic); Check(!methodType.IsValueType, VerifierError.CallVirtOnValueType); } - else + else if (opcode != ILOpcode.newobj) { EcmaMethod ecmaMethod = method.GetTypicalMethodDefinition() as EcmaMethod; if (ecmaMethod != null) @@ -1378,7 +1378,24 @@ void ImportCall(ILOpcode opcode, int token) if (opcode == ILOpcode.newobj) { - // TODO: + Check(method.IsConstructor, VerifierError.CtorExpected); + if (sig.IsStatic || methodType == null || method.IsAbstract) + { + VerificationError(VerifierError.CtorSig); + } + else + { + if (methodType.IsArray) + { + var arrayType = (ArrayType)methodType; + Check(!IsByRefLike(StackValue.CreateFromType(arrayType.ElementType)), VerifierError.ArrayByRef); + } + else + { + var metadataType = (MetadataType)methodType; + Check(!metadataType.IsAbstract, VerifierError.NewobjAbstractClass); + } + } } else if (methodType != null) @@ -1452,7 +1469,7 @@ void ImportCall(ILOpcode opcode, int token) if (tailCall) { - // also check the specil tailcall rule + // also check the special tailcall rule Check(!IsByRefLike(declaredThis), VerifierError.TailByRef, declaredThis); // Tail calls on constrained calls should be illegal too: @@ -1487,7 +1504,9 @@ void ImportCall(ILOpcode opcode, int token) // } else { - CheckIsAssignable(StackValue.CreateFromType(returnType), StackValue.CreateFromType(callerReturnType)); + var retStackType = StackValue.CreateFromType(returnType); + var callerRetStackType = StackValue.CreateFromType(callerReturnType); + Check(IsAssignable(retStackType, callerRetStackType), VerifierError.TailRetType, retStackType, callerRetStackType); } // for tailcall, stack must be empty @@ -1504,18 +1523,18 @@ void ImportCall(ILOpcode opcode, int token) { var returnValue = StackValue.CreateFromType(returnType); -#if false // "readonly." prefixed calls only allowed for the Address operation on arrays. // The methods supported by array types are under the control of the EE // so we can trust that only the Address operation returns a byref. - if (readonlyCall) + if (HasPendingPrefix(Prefix.ReadOnly)) { - VerifyOrReturn ((methodClassFlgs & CORINFO_FLG_ARRAY) && tiRetVal.IsByRef(), - MVER_E_READONLY_UNEXPECTED_CALLEE);//"unexpected use of readonly prefix" - vstate->readonlyPrefix = false; - tiRetVal.SetIsReadonlyByRef(); + if (method.OwningType.IsArray && sig.ReturnType.IsByRef) + returnValue.SetIsReadOnly(); + else + VerificationError(VerifierError.ReadonlyUnexpectedCallee); + + ClearPendingPrefix(Prefix.ReadOnly); } -#endif if (returnValue.Kind == StackValueKind.ByRef) returnValue.SetIsPermanentHome(); @@ -1891,7 +1910,7 @@ void ImportAddressOfField(int token, bool isStatic) instance = null; if (field.IsInitOnly) - Check(_method.IsStaticConstructor && field.OwningType == _method.OwningType, VerifierError.Initonly); + Check(_method.IsStaticConstructor && field.OwningType == _method.OwningType, VerifierError.InitOnly); } else { @@ -1913,7 +1932,7 @@ void ImportAddressOfField(int token, bool isStatic) instance = actualThis.Type; if (field.IsInitOnly) - Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.Initonly); + Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly); } Check(_method.OwningType.CanAccess(field, instance), VerifierError.FieldAccess); @@ -1938,7 +1957,7 @@ void ImportStoreField(int token, bool isStatic) instance = null; if (field.IsInitOnly) - Check(_method.IsStaticConstructor && field.OwningType == _method.OwningType, VerifierError.Initonly); + Check(_method.IsStaticConstructor && field.OwningType == _method.OwningType, VerifierError.InitOnly); } else { @@ -1959,9 +1978,12 @@ void ImportStoreField(int token, bool isStatic) instance = actualThis.Type; if (field.IsInitOnly) - Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.Initonly); + Check(_method.IsConstructor && field.OwningType == _method.OwningType && actualThis.IsThisPtr, VerifierError.InitOnly); } + // Check any constraints on the fields' class --- accessing the field might cause a class constructor to run. + Check(field.OwningType.CheckConstraints(), VerifierError.UnsatisfiedFieldParentInst); + Check(_method.OwningType.CanAccess(field, instance), VerifierError.FieldAccess); CheckIsAssignable(value, StackValue.CreateFromType(field.FieldType)); @@ -2316,7 +2338,7 @@ void ImportEndFilter() Check(_currentOffset == _exceptionRegions[_currentBasicBlock.FilterIndex.Value].ILRegion.HandlerOffset, VerifierError.Endfilter); var result = Pop(allowUninitThis: true); - Check(result.Kind == StackValueKind.Int32, VerifierError.StackUnexpected); + Check(result.Kind == StackValueKind.Int32, VerifierError.StackUnexpected, result); Check(_stackTop == 0, VerifierError.EndfilterStack); } @@ -2404,6 +2426,9 @@ void ImportTailPrefix() { CheckPendingPrefix(_pendingPrefix); _pendingPrefix |= Prefix.Tail; + + Check(!_currentBasicBlock.TryIndex.HasValue && !_currentBasicBlock.FilterIndex.HasValue && + !_currentBasicBlock.HandlerIndex.HasValue, VerifierError.TailCallInsideER); } void ImportConstrainedPrefix(int token) diff --git a/src/ILVerify/src/Resources/Strings.resx b/src/ILVerify/src/Resources/Strings.resx index 746fea21d7a..62e3624109c 100644 --- a/src/ILVerify/src/Resources/Strings.resx +++ b/src/ILVerify/src/Resources/Strings.resx @@ -165,6 +165,12 @@ The 'this' argument to a constrained call must have ByRef type. + + .ctor expected. + + + newobj on static or abstract method. + Unrecognized arguments for delegate .ctor. @@ -219,7 +225,7 @@ initlocals must be set for verifiable methods with one or more local variables. - + Cannot change initonly field outside its .ctor. @@ -255,6 +261,9 @@ Fall through end of the method without returning. + + Cannot construct an instance of abstract class. + Stack depth differs depending on path. @@ -267,6 +276,9 @@ Illegal write to readonly ByRef. + + The readonly prefix may only be applied to calls to array methods returning ByRefs. + Rethrow from outside a catch handler. @@ -324,6 +336,12 @@ Missing call/callvirt/calli. + + The tail.call (or calli or callvirt) instruction cannot be used to transfer control out of a try, filter, catch, or finally block. + + + Tail call return type not compatible. + Void ret type expected for tail call. @@ -366,6 +384,9 @@ Type operand of box instruction has unsatisfied class type parameter constraints. + + Field parent instantiation has unsatisfied class type parameter constraints. + Method instantiation has unsatisfied method type parameter constraints. diff --git a/src/ILVerify/src/VerifierError.cs b/src/ILVerify/src/VerifierError.cs index d1efe85dd49..ef92c2e380f 100644 --- a/src/ILVerify/src/VerifierError.cs +++ b/src/ILVerify/src/VerifierError.cs @@ -146,13 +146,14 @@ enum VerifierError //E_ADDR "Address of not allowed for this item." //E_ADDR_BYREF "Address of not allowed for ByRef." //E_ADDR_LITERAL "Address of not allowed for literal field." - Initonly, // Cannot change initonly field outside its .ctor. + InitOnly, // Cannot change initonly field outside its .ctor. //E_WRITE_RVA_STATIC "Cannot modify an imaged based (RVA) static" //E_THROW "Cannot throw this object." CallVirtOnValueType, // Callvirt on a value type method. //E_CALL_SIG "Call signature mismatch." //E_CALL_STATIC "Static function expected." - //E_CTOR ".ctor expected." + CtorExpected, // .ctor expected. + CtorSig, // newobj on static or abstract method. //E_CTOR_VIRT "Cannot use callvirt on .ctor." //E_CTOR_OR_SUPER "Only super::ctor or typeof(this)::ctor allowed here." //E_CTOR_MUL_INIT "Possible call to .ctor more than once." @@ -170,7 +171,7 @@ enum VerifierError TailByRef, // Cannot pass ByRef to a tail call. //E_TAIL_RET "Missing ret." TailRetVoid, // Void ret type expected for tail call. - //E_TAIL_RET_TYPE "Tail call return type not compatible." + TailRetType, // Tail call return type not compatible. TailStackEmpty, // Stack not empty after tail call. //E_METHOD_END "Method ends in the middle of an instruction." BadBranch, // Branch out of the method. @@ -228,23 +229,22 @@ enum VerifierError UnsatisfiedMethodInst, // Method instantiation has unsatisfied method type parameter constraints. UnsatisfiedMethodParentInst, // Method parent instantiation has unsatisfied class type parameter constraints. - //E_UNSATISFIED_FIELD_PARENT_INST "Field parent instantiation has unsatisfied class type parameter constraints." + UnsatisfiedFieldParentInst, // Field parent instantiation has unsatisfied class type parameter constraints. UnsatisfiedBoxOperand, // Type operand of box instruction has unsatisfied class type parameter constraints. ConstrainedCallWithNonByRefThis, // The 'this' argument to a constrained call must have ByRef type. //E_CONSTRAINED_OF_NON_VARIABLE_TYPE "The operand to a constrained prefix instruction must be a type parameter." - //E_READONLY_UNEXPECTED_CALLEE "The readonly prefix may only be applied to calls to array methods returning ByRefs." + ReadonlyUnexpectedCallee, // The readonly prefix may only be applied to calls to array methods returning ByRefs. ReadOnlyIllegalWrite, // Illegal write to readonly ByRef. //E_READONLY_IN_MKREFANY "A readonly ByRef cannot be used with mkrefany." //E_UNALIGNED_ALIGNMENT "Alignment specified for 'unaligned' prefix must be 1, 2, or 4." - //E_TAILCALL_INSIDE_EH "The tail.call (or calli or callvirt) instruction cannot be used to transfer control out of a try, filter, catch, or finally block." + TailCallInsideER, // The tail.call (or calli or callvirt) instruction cannot be used to transfer control out of a try, filter, catch, or finally block. //E_BACKWARD_BRANCH "Stack height at all points must be determinable in a single forward scan of IL." //E_CALL_TO_VTYPE_BASE "Call to base type of valuetype." - //E_NEWOBJ_OF_ABSTRACT_CLASS "Cannot construct an instance of abstract class." + NewobjAbstractClass, // Cannot construct an instance of abstract class. UnmanagedPointer, // Unmanaged pointers are not a verifiable type. LdftnNonFinalVirtual, // Cannot LDFTN a non-final virtual method for delegate creation if target object is potentially not the same type as the method class. //E_FIELD_OVERLAP "Accessing type with overlapping fields." ThisMismatch, // The 'this' parameter to the call must be the calling method's 'this' parameter. - //E_STACK_I_I4 "Expected I4 on the stack." //E_BAD_PE "Unverifiable PE Header/native stub." //E_BAD_MD "Unrecognized metadata, unable to verify IL." diff --git a/src/ILVerify/tests/ILTests/FieldTests.il b/src/ILVerify/tests/ILTests/FieldTests.il index 8780250ae93..816f6fee7fc 100644 --- a/src/ILVerify/tests/ILTests/FieldTests.il +++ b/src/ILVerify/tests/ILTests/FieldTests.il @@ -10,13 +10,33 @@ { } +.class private auto ansi beforefieldinit ConstrainedClass`1<([System.Runtime]System.Collections.IEnumerable) T> + extends [System.Runtime]System.Object +{ + .field public static int32 StaticField + + .method public hidebysig specialname rtspecialname instance void .ctor () cil managed + { + ldarg.0 + call instance void [System.Runtime]System.Object::.ctor() + ret + } +} + .class public auto ansi beforefieldinit FieldTestsType extends [System.Runtime]System.Object { .field public initonly int32 InstanceInitonlyField .field public static initonly int32 StaticInitonlyField - .method public instance void Stfld.InitonlyFieldOutsideCtor_Invalid_Initonly() cil managed + .method public instance void Stsfld.UnsatisfiedParentConstraints_Invalid_UnsatisfiedFieldParentInst() cil managed + { + ldc.i4.0 + stsfld int32 class ConstrainedClass`1::StaticField + ret + } + + .method public instance void Stfld.InitonlyFieldOutsideCtor_Invalid_InitOnly() cil managed { ldarg.0 ldc.i4.0 @@ -24,7 +44,7 @@ ret } - .method public instance void Ldflda.InitonlyFieldOutsideCtor_Invalid_Initonly() cil managed + .method public instance void Ldflda.InitonlyFieldOutsideCtor_Invalid_InitOnly() cil managed { ldarg.0 ldflda int32 FieldTestsType::InstanceInitonlyField @@ -65,7 +85,7 @@ ret } - .method public hidebysig instance void 'special.StoreInitonlyFieldOtherType..ctor_Invalid_Initonly'(class OtherType c) cil managed { ret } + .method public hidebysig instance void 'special.StoreInitonlyFieldOtherType..ctor_Invalid_InitOnly'(class OtherType c) cil managed { ret } .method public hidebysig specialname rtspecialname instance void .ctor(class OtherType c) cil managed { ldarg.0 @@ -76,7 +96,7 @@ ret } - .method public hidebysig instance void 'special.StoreInitonlyFieldOtherInstance..ctor_Invalid_Initonly'(class FieldTestsType c) cil managed { ret } + .method public hidebysig instance void 'special.StoreInitonlyFieldOtherInstance..ctor_Invalid_InitOnly'(class FieldTestsType c) cil managed { ret } .method public hidebysig specialname rtspecialname instance void .ctor(class FieldTestsType c) cil managed { ldarg.0 @@ -87,7 +107,7 @@ ret } - .method public hidebysig instance void 'special.StsfldInitonlyInCtor..ctor_Invalid_Initonly'(bool) cil managed { ret } + .method public hidebysig instance void 'special.StsfldInitonlyInCtor..ctor_Invalid_InitOnly'(bool) cil managed { ret } .method public hidebysig specialname rtspecialname instance void .ctor(bool) cil managed { ldarg.0 @@ -97,7 +117,7 @@ ret } - .method public hidebysig instance void 'special.LdsfldInitonlyInCtor..ctor_Invalid_Initonly'(char) cil managed { ret } + .method public hidebysig instance void 'special.LdsfldInitonlyInCtor..ctor_Invalid_InitOnly'(char) cil managed { ret } .method public hidebysig specialname rtspecialname instance void .ctor(char) cil managed { ldarg.0 @@ -126,7 +146,7 @@ .field public initonly int32 InstanceInitonlyField .field public static initonly int32 StaticInitonlyField - .method public hidebysig instance void 'special.LdfldStlfdInitonlyCctor..cctor_Invalid_Initonly.Initonly'() cil managed { ret } + .method public hidebysig instance void 'special.LdfldStlfdInitonlyCctor..cctor_Invalid_InitOnly.InitOnly'() cil managed { ret } .method public hidebysig specialname rtspecialname instance void .cctor() cil managed { ldsfld class OtherType OtherType::Instance diff --git a/src/ILVerify/tests/ILTests/NewobjTests.il b/src/ILVerify/tests/ILTests/NewobjTests.il new file mode 100644 index 00000000000..38d121c7229 --- /dev/null +++ b/src/ILVerify/tests/ILTests/NewobjTests.il @@ -0,0 +1,46 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +.assembly extern System.Runtime +{ +} + +.assembly NewobjTests +{ +} + +.class public auto ansi abstract beforefieldinit AbstractClass +{ + .method public hidebysig newslot abstract virtual instance void AbstractMethod() cil managed + { + } +} + +.class public auto ansi beforefieldinit NewobjTestsType + extends [System.Runtime]System.Object +{ + .method public hidebysig instance void InstanceMethod() cil managed + { + ret + } + + .method public hidebysig static void StaticMethod() cil managed + { + ret + } + + .method public hidebysig instance void Newobj.InstanceMethod_Invalid_CtorExpected() cil managed + { + newobj instance void NewobjTestsType::InstanceMethod() + pop + ret + } + + .method public hidebysig instance void Newobj.AbstractMethod_Invalid_CtorSig.CtorExpected() cil managed + { + newobj instance void AbstractClass::AbstractMethod() + pop + ret + } +} diff --git a/src/ILVerify/tests/ILTests/PrefixTests.il b/src/ILVerify/tests/ILTests/PrefixTests.il index 5ba5e354610..ec1fbe0040d 100644 --- a/src/ILVerify/tests/ILTests/PrefixTests.il +++ b/src/ILVerify/tests/ILTests/PrefixTests.il @@ -13,7 +13,12 @@ .class public auto ansi beforefieldinit PrefixTestsType extends [System.Runtime]System.Object { - .method static public hidebysig void Prefix.Readonly_Valid() cil managed + .method static public hidebysig void StaticMethod() cil managed + { + ret + } + + .method static public hidebysig void Readonly.Ldelema_Valid() cil managed { .locals init (int32[] V_0) @@ -31,7 +36,7 @@ ret } - .method static public hidebysig void Prefix.Readonly_Invalid_ReadOnly() cil managed + .method static public hidebysig void Readonly.Ldelem_Invalid_ReadOnly() cil managed { .locals init (int32[] V_0) @@ -48,7 +53,35 @@ ret } - .method static public hidebysig void Prefix.PrefixUnalignedAndVolatile_Valid() cil managed + .method static public hidebysig void Readonly.ArrayAddress_Valid() cil managed + { + //int[,] array = new int[1, 1]; + ldc.i4.1 + ldc.i4.1 + newobj instance void int32[0..., 0...]::.ctor(int32, int32) + ldc.i4.0 + ldc.i4.0 + readonly. + call instance int32& int32[0..., 0...]::Address(int32, int32) + pop + ret + } + + .method static public hidebysig void Readonly.ArrayGet_Invalid_ReadonlyUnexpectedCallee() cil managed + { + //int[,] array = new int[1, 1]; + ldc.i4.1 + ldc.i4.1 + newobj instance void int32[0..., 0...]::.ctor(int32, int32) + ldc.i4.0 + ldc.i4.0 + readonly. + call instance int32 int32[0..., 0...]::Get(int32, int32) + pop + ret + } + + .method static public hidebysig void Prefix.UnalignedAndVolatile_Valid() cil managed { .locals init (int32& V_0, int32 V_1) // ref int x; @@ -61,4 +94,22 @@ stloc.1 ret } + + .method static public hidebysig void Tail.FromTry_Invalid_TailCallInsideER() cil managed + { + .try + { + tail. + call void PrefixTestsType::StaticMethod() + + leave MethodEnd + } + finally + { + endfinally + } + + MethodEnd: + ret + } }