[ILVerify] Implement verification of jump target offsets (#5101)

* Implemented bad jump verification error.

* Added test cases for bad jump verification.

4 files changed, 170 insertions, 9 deletions

diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs
index c7d156d16..1551ce34f 100644
--- a/src/ILVerify/src/ILImporter.Verify.cs
+++ b/src/ILVerify/src/ILImporter.Verify.cs
@@ -60,6 +60,8 @@ namespace Internal.IL
         bool _modifiesThisPtr;
         bool _trackObjCtorState;
 
+        bool[] _validTargetOffsets;
+
         int? _delegateCreateStart;
 
         class ExceptionRegion
@@ -211,7 +213,7 @@ namespace Internal.IL
 
             FindBasicBlocks();
             FindEnclosingExceptionRegions();
-            FindThisPtrModification();
+            InitialPass();
             ImportBasicBlocks();
         }
 
@@ -278,28 +280,37 @@ namespace Internal.IL
             }
         }
 
-        private void FindThisPtrModification()
+        /// <summary>
+        /// Checks whether the metod's il modifies the this pointer and builds up the
+        /// array of valid target offsets.
+        /// </summary>
+        private void InitialPass()
         {
             _modifiesThisPtr = false;
+            _validTargetOffsets = new bool[_ilBytes.Length];
 
-            if (_thisType == null)
-                return; // Early exit: no this pointer in this method
+            bool previousWasPrefix = false;
 
             _currentOffset = 0;
 
             while (_currentOffset < _ilBytes.Length)
             {
+                if (!previousWasPrefix) // The instruction following a prefix is not a valid branch target.
+                    _validTargetOffsets[_currentOffset] = true;
+
                 ILOpcode opCode = (ILOpcode)ReadILByte();
 
+                previousWasPrefix = false;
 again:
                 switch (opCode)
                 {
+                    // Check this pointer modification
                     case ILOpcode.starg_s:
                     case ILOpcode.ldarga_s:
                         if (ReadILByte() == 0)
                         {
                             _modifiesThisPtr = true;
-                            return;
+                            break;
                         }
                         break;
                     case ILOpcode.starg:
@@ -307,16 +318,31 @@ again:
                         if (ReadILUInt16() == 0)
                         {
                             _modifiesThisPtr = true;
-                            return;
+                            break;
                         }
                         break;
+
+                    // Keep track of prefixes
+                    case ILOpcode.unaligned:
+                        SkipIL(1);
+                        previousWasPrefix = true;
+                        break;
+                    case ILOpcode.constrained:
+                        previousWasPrefix = true;
+                        SkipIL(4);
+                        break;
+                    case ILOpcode.tail:
+                    case ILOpcode.volatile_:
+                    case ILOpcode.readonly_:
+                        previousWasPrefix = true;
+                        continue;
+
                     // Skip all other Opcodes
                     case ILOpcode.ldarg_s:
                     case ILOpcode.ldloc_s:
                     case ILOpcode.ldloca_s:
                     case ILOpcode.stloc_s:
                     case ILOpcode.ldc_i4_s:
-                    case ILOpcode.unaligned:
                     case ILOpcode.br_s:
                     case ILOpcode.leave_s:
                     case ILOpcode.brfalse_s:
@@ -371,7 +397,6 @@ again:
                     case ILOpcode.ldftn:
                     case ILOpcode.ldvirtftn:
                     case ILOpcode.initobj:
-                    case ILOpcode.constrained:
                     case ILOpcode.sizeof_:
                     case ILOpcode.br:
                     case ILOpcode.leave:
@@ -550,6 +575,12 @@ again:
 
         private void CheckIsValidLeaveTarget(BasicBlock src, BasicBlock target)
         {
+            if (!_validTargetOffsets[target.StartOffset])
+            {
+                VerificationError(VerifierError.BadJumpTarget);
+                return;
+            }
+
             // If the source is within filter, target shall be within the same
             if (src.FilterIndex.HasValue && src.FilterIndex != target.FilterIndex)
             {
@@ -662,6 +693,13 @@ again:
 
         bool IsValidBranchTarget(BasicBlock src, BasicBlock target, bool isFallthrough, bool reportErrors = true)
         {
+            if (!_validTargetOffsets[target.StartOffset])
+            {
+                if (reportErrors)
+                    VerificationError(VerifierError.BadJumpTarget);
+                return false;
+            }
+
             bool isValid = true;
 
             if (src.TryIndex != target.TryIndex)
diff --git a/src/ILVerify/src/Resources/Strings.resx b/src/ILVerify/src/Resources/Strings.resx
index 0158facc7..f0fbc52b0 100644
--- a/src/ILVerify/src/Resources/Strings.resx
+++ b/src/ILVerify/src/Resources/Strings.resx
@@ -123,6 +123,9 @@
   <data name="BadBranch" xml:space="preserve">
     <value>Branch out of the method.</value>
   </data>
+  <data name="BadJumpTarget" xml:space="preserve">
+    <value>Branch / Leave into the middle of an instruction.</value>
+  </data>
   <data name="BoxByRef" xml:space="preserve">
     <value>Cannot box byref.</value>
   </data>
diff --git a/src/ILVerify/src/VerifierError.cs b/src/ILVerify/src/VerifierError.cs
index 26a78870d..fc152029b 100644
--- a/src/ILVerify/src/VerifierError.cs
+++ b/src/ILVerify/src/VerifierError.cs
@@ -73,7 +73,7 @@ namespace ILVerify
         ReturnFromTry,                  // Return out of try block.
         ReturnFromHandler,              // Return out of exception handler block.
         ReturnFromFilter,               // Return out of exception filter block.
-        //E_BAD_JMP_TARGET              "jmp / exception into the middle of an instruction."
+        BadJumpTarget,                  // Branch / Leave into the middle of an instruction.
         PathStackUnexpected,            // Non-compatible types on stack depending on path.
         PathStackDepth,                 // Stack depth differs depending on path.
         //E_THIS_UNINIT_EXCEP           "Uninitialized this on entering a try block."
diff --git a/src/ILVerify/tests/ILTests/BranchingTests.il b/src/ILVerify/tests/ILTests/BranchingTests.il
index 58e3ad561..4188baa6a 100644
--- a/src/ILVerify/tests/ILTests/BranchingTests.il
+++ b/src/ILVerify/tests/ILTests/BranchingTests.il
@@ -47,6 +47,14 @@
 .class public auto ansi beforefieldinit BranchingTestsType
        extends [System.Runtime]System.Object
 {
+    // Volatile field for testing
+    .field private static int32 modreq([System.Runtime]System.Runtime.CompilerServices.IsVolatile) volatileField
+
+    .method static public hidebysig void StaticMethod() cil managed
+    {
+        ret
+    }
+
     .method static public hidebysig void Branching.NullConditional_Valid() cil managed
     {
         //object o = null;
@@ -811,4 +819,116 @@
 
         lbl_ret: ret
     }
+
+    .method static public hidebysig void Branch.BeforeReadonlyInstruction_Valid(object[] objectArray) cil managed
+    {
+        // objectArray[0];
+
+        ldarg.0
+        ldc.i4.0
+        br          BeforeInstr
+
+    BeforeInstr:
+        readonly.
+        ldelema     [System.Runtime]System.Object
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoReadonlyInstruction_Invalid_BadJumpTarget(object[] objectArray) cil managed
+    {
+        // objectArray[0];
+
+        ldarg.0
+        ldc.i4.0
+        br          MidInstr
+
+        readonly.
+    MidInstr:
+        ldelema     [System.Runtime]System.Object
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoConstrainedInstruction_Invalid_BadJumpTarget<T>(!!T arg) cil managed
+    {
+        // arg.ToString();
+        ldarga.s    arg
+        br          MidInstr
+
+        constrained. !!T
+    MidInstr:
+        callvirt    instance string [System.Runtime]System.Object::ToString()
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.AfterConstrainedInstruction_Valid<T>(!!T arg) cil managed
+    {
+        // arg.ToString();
+        ldarga.s    arg
+        br          AfterInstr
+
+        constrained. !!T
+        callvirt    instance string [System.Runtime]System.Object::ToString()
+    AfterInstr:
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoVolatileInstruction_Invalid_BadJumpTarget() cil managed
+    {
+        // volatileField = 0;
+
+        ldc.i4.0
+        br          MidInstr
+
+        volatile.
+    MidInstr:
+        stsfld    int32 modreq([System.Runtime]System.Runtime.CompilerServices.IsVolatile) BranchingTestsType::volatileField
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoUnalignedInstruction_Invalid_BadJumpTarget() cil managed 
+    {
+        .locals init (
+            int32&
+        )
+
+        ldloc.0
+        br      MidInstr
+
+        unaligned. 4
+    MidInstr:
+        ldind.i4
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoUnalignedVolatileInstruction_Invalid_BadJumpTarget() cil managed 
+    {
+        .locals init (
+            int32&
+        )
+
+        ldloc.0
+        br      MidInstr
+
+        unaligned. 4
+        volatile.
+    MidInstr:
+        ldind.i4
+        pop
+        ret
+    }
+
+    .method static public hidebysig void Branch.IntoTailInstruction_Invalid_BadJumpTarget() cil managed 
+    {
+        br      MidInstr
+
+        tail.
+    MidInstr:
+        call    void BranchingTestsType::StaticMethod()
+        ret
+    }
 }