[ILVerify] Implement missing newobj verifications (dotnet#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

@@ -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)

src/ILVerify/src/Resources/Strings.resx

@@ -165,6 +165,12 @@
   <data name="ConstrainedCallWithNonByRefThis" xml:space="preserve">
     <value>The 'this' argument to a constrained call must have ByRef type.</value>
   </data>
+  <data name="CtorExpected" xml:space="preserve">
+    <value>.ctor expected.</value>
+  </data>
+  <data name="CtorSig" xml:space="preserve">
+    <value>newobj on static or abstract method.</value>
+  </data>
   <data name="DelegateCtor" xml:space="preserve">
     <value>Unrecognized arguments for delegate .ctor.</value>
   </data>
@@ -219,7 +225,7 @@
   <data name="InitLocals" xml:space="preserve">
     <value>initlocals must be set for verifiable methods with one or more local variables.</value>
   </data>
-  <data name="Initonly" xml:space="preserve">
+  <data name="InitOnly" xml:space="preserve">
     <value>Cannot change initonly field outside its .ctor.</value>
   </data>
   <data name="LdftnConstructor" xml:space="preserve">
@@ -255,6 +261,9 @@
   <data name="MethodFallthrough" xml:space="preserve">
     <value>Fall through end of the method without returning.</value>
   </data>
+  <data name="NewobjAbstractClass" xml:space="preserve">
+    <value>Cannot construct an instance of abstract class.</value>
+  </data>
   <data name="PathStackDepth" xml:space="preserve">
     <value>Stack depth differs depending on path.</value>
   </data>
@@ -267,6 +276,9 @@
   <data name="ReadOnlyIllegalWrite" xml:space="preserve">
     <value>Illegal write to readonly ByRef.</value>
   </data>
+  <data name="ReadonlyUnexpectedCallee" xml:space="preserve">
+    <value>The readonly prefix may only be applied to calls to array methods returning ByRefs.</value>
+  </data>
   <data name="Rethrow" xml:space="preserve">
     <value>Rethrow from outside a catch handler.</value>
   </data>
@@ -324,6 +336,12 @@
   <data name="TailCall" xml:space="preserve">
     <value>Missing call/callvirt/calli.</value>
   </data>
+  <data name="TailCallInsideER" xml:space="preserve">
+    <value>The tail.call (or calli or callvirt) instruction cannot be used to transfer control out of a try, filter, catch, or finally block.</value>
+  </data>
+  <data name="TailRetType" xml:space="preserve">
+    <value>Tail call return type not compatible.</value>
+  </data>
   <data name="TailRetVoid" xml:space="preserve">
     <value>Void ret type expected for tail call.</value>
   </data>
@@ -366,6 +384,9 @@
   <data name="UnsatisfiedBoxOperand" xml:space="preserve">
     <value>Type operand of box instruction has unsatisfied class type parameter constraints.</value>
   </data>
+  <data name="UnsatisfiedFieldParentInst" xml:space="preserve">
+    <value>Field parent instantiation has unsatisfied class type parameter constraints.</value>
+  </data>
   <data name="UnsatisfiedMethodInst" xml:space="preserve">
     <value>Method instantiation has unsatisfied method type parameter constraints.</value>
   </data>

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."

src/ILVerify/tests/ILTests/FieldTests.il

@@ -10,21 +10,41 @@
 {
 }
 
+.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<int32>::StaticField
+        ret
+    }
+
+    .method public instance void Stfld.InitonlyFieldOutsideCtor_Invalid_InitOnly() cil managed
     {
         ldarg.0
         ldc.i4.0
         stfld   int32 FieldTestsType::InstanceInitonlyField
         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

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
+    }
+}