[ILVerify] Implement missing exception region errors (#4960)

* Changed branch target verification to report BranchOutOfFinally instead of BranchOutOfHandler for branching out of finally blocks.

* Added specific errors for fallthrough into/out of exception regions.

* Removed outdated PEVerify errors.

4 files changed, 227 insertions, 67 deletions

diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs
index ee9215a79..664a32cad 100644
--- a/src/ILVerify/src/ILImporter.Verify.cs
+++ b/src/ILVerify/src/ILImporter.Verify.cs
@@ -660,7 +660,7 @@ again:
             }
         }
 
-        bool IsValidBranchTarget(BasicBlock src, BasicBlock target, bool reportErrors = true)
+        bool IsValidBranchTarget(BasicBlock src, BasicBlock target, bool isFallthrough, bool reportErrors = true)
         {
             bool isValid = true;
 
@@ -672,14 +672,22 @@ again:
                     if (target.StartOffset != _exceptionRegions[target.TryIndex.Value].ILRegion.TryOffset || !IsDirectChildRegion(src, target))
                     {
                         if (reportErrors)
+                        {
+                            Debug.Assert(!isFallthrough); // This should not be reachable by fallthrough
                             VerificationError(VerifierError.BranchIntoTry);
+                        }
                         isValid = false;
                     }
                 }
                 else if (target.TryIndex == null)
                 {
                     if (reportErrors)
-                        VerificationError(VerifierError.BranchOutOfTry);
+                    {
+                        if (isFallthrough)
+                            VerificationError(VerifierError.FallthroughException);
+                        else
+                            VerificationError(VerifierError.BranchOutOfTry);
+                    }
                     isValid = false;
                 }
                 else
@@ -694,14 +702,22 @@ again:
                         if (target.StartOffset != targetRegion.TryOffset || !IsDirectChildRegion(src, target))
                         {
                             if (reportErrors)
+                            {
+                                Debug.Assert(!isFallthrough); // This should not be reachable by fallthrough
                                 VerificationError(VerifierError.BranchIntoTry);
+                            }
                             isValid = false;
                         }
                     }
                     else
                     {
                         if (reportErrors)
-                            VerificationError(VerifierError.BranchOutOfTry);
+                        {
+                            if (isFallthrough)
+                                VerificationError(VerifierError.FallthroughException);
+                            else
+                                VerificationError(VerifierError.BranchOutOfTry);
+                        }
                         isValid = false;
                     }
                 }
@@ -712,13 +728,23 @@ again:
                 if (src.FilterIndex == null)
                 {
                     if (reportErrors)
-                        VerificationError(VerifierError.BranchIntoFilter);
+                    {
+                        if (isFallthrough)
+                            VerificationError(VerifierError.FallthroughIntoFilter);
+                        else
+                            VerificationError(VerifierError.BranchIntoFilter);
+                    }
                     isValid = false;
                 }
                 else if (target.HandlerIndex == null)
                 {
                     if (reportErrors)
-                        VerificationError(VerifierError.BranchOutOfFilter);
+                    {
+                        if (isFallthrough)
+                            VerificationError(VerifierError.FallthroughException);
+                        else
+                            VerificationError(VerifierError.BranchOutOfFilter);
+                    }
                     isValid = false;
                 }
                 else
@@ -728,13 +754,23 @@ again:
                     if (srcRegion.FilterOffset <= targetRegion.FilterOffset)
                     {
                         if (reportErrors)
-                            VerificationError(VerifierError.BranchIntoFilter);
+                        {
+                            if (isFallthrough)
+                                VerificationError(VerifierError.FallthroughIntoFilter);
+                            else
+                                VerificationError(VerifierError.BranchIntoFilter);
+                        }
                         isValid = false;
                     }
                     else
                     {
                         if (reportErrors)
-                            VerificationError(VerifierError.BranchOutOfFilter);
+                        {
+                            if (isFallthrough)
+                                VerificationError(VerifierError.FallthroughException);
+                            else
+                                VerificationError(VerifierError.BranchOutOfFilter);
+                        }
                         isValid = false;
                     }
                 }
@@ -745,13 +781,28 @@ again:
                 if (src.HandlerIndex == null)
                 {
                     if (reportErrors)
-                        VerificationError(VerifierError.BranchIntoHandler);
+                    {
+                        if (isFallthrough)
+                            VerificationError(VerifierError.FallthroughIntoHandler);
+                        else
+                            VerificationError(VerifierError.BranchIntoHandler);
+                    }
                     isValid = false;
                 }
                 else if (target.HandlerIndex == null)
                 {
                     if (reportErrors)
-                        VerificationError(VerifierError.BranchOutOfHandler);
+                    {
+                        if (isFallthrough)
+                            VerificationError(VerifierError.FallthroughException);
+                        else
+                        {
+                            if (_exceptionRegions[src.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Finally)
+                                VerificationError(VerifierError.BranchOutOfFinally);
+                            else
+                                VerificationError(VerifierError.BranchOutOfHandler);
+                        }
+                    }
                     isValid = false;
                 }
                 else
@@ -761,13 +812,28 @@ again:
                     if (srcRegion.HandlerOffset <= targetRegion.HandlerOffset)
                     {
                         if (reportErrors)
-                            VerificationError(VerifierError.BranchIntoHandler);
+                        {
+                            if (isFallthrough)
+                                VerificationError(VerifierError.FallthroughIntoHandler);
+                            else
+                                VerificationError(VerifierError.BranchIntoHandler);
+                        }
                         isValid = false;
                     }
                     else
                     {
                         if (reportErrors)
-                            VerificationError(VerifierError.BranchOutOfHandler);
+                        {
+                            if (isFallthrough)
+                                VerificationError(VerifierError.FallthroughException);
+                            else
+                            {
+                                if (srcRegion.Kind == ILExceptionRegionKind.Finally)
+                                    VerificationError(VerifierError.BranchOutOfFinally);
+                                else
+                                    VerificationError(VerifierError.BranchOutOfHandler);
+                            }
+                        }
                         isValid = false;
                     }
                 }
@@ -1655,54 +1721,7 @@ again:
 
         void ImportFallthrough(BasicBlock next)
         {
-            if (!IsValidBranchTarget(_currentBasicBlock, next) || _currentBasicBlock.ErrorCount > 0)
-                return;
-
-            PropagateThisState(_currentBasicBlock, next);
-
-            // Propagate stack across block bounds
-            StackValue[] entryStack = next.EntryStack;
-
-            if (entryStack != null)
-            {
-                FatalCheck(entryStack.Length == _stackTop, VerifierError.PathStackDepth);
-
-                for (int i = 0; i < entryStack.Length; i++)
-                {
-                    // TODO: Do we need to allow conversions?
-                    FatalCheck(entryStack[i].Kind == _stack[i].Kind, VerifierError.PathStackUnexpected, entryStack[i], _stack[i]);
-                    
-                    if (entryStack[i].Type != _stack[i].Type)
-                    {
-                        if (!IsAssignable(_stack[i], entryStack[i]))
-                        {
-                            StackValue mergedValue;
-                            if (!TryMergeStackValues(entryStack[i], _stack[i], out mergedValue))
-                                FatalCheck(false, VerifierError.PathStackUnexpected, entryStack[i], _stack[i]);
-
-                            // If merge actually changed entry stack
-                            if (mergedValue != entryStack[i])
-                            {
-                                entryStack[i] = mergedValue;
-
-                                if (next.ErrorCount == 0 && next.State != BasicBlock.ImportState.IsPending)
-                                    next.State = BasicBlock.ImportState.Unmarked; // Make sure block is reverified
-                            }
-                        }
-                    }
-                }
-            }
-            else
-            {
-                entryStack = (_stackTop != 0) ? new StackValue[_stackTop] : s_emptyStack;
-
-                for (int i = 0; i < entryStack.Length; i++)
-                    entryStack[i] = _stack[i];
-
-                next.EntryStack = entryStack;
-            }
-
-            MarkBasicBlock(next);
+            PropagateControlFlow(next, isFallthrough: true);
         }
 
         void PropagateThisState(BasicBlock current, BasicBlock next)
@@ -1731,7 +1750,7 @@ again:
             for (int i = 0; i < jmpDelta.Length; i++)
             {
                 BasicBlock target = _basicBlocks[jmpBase + jmpDelta[i]];
-                ImportFallthrough(target);
+                PropagateControlFlow(target, isFallthrough: false);
             }
 
             if (fallthrough != null)
@@ -1773,12 +1792,64 @@ again:
                     break;
             }
 
-            ImportFallthrough(target);
+            PropagateControlFlow(target, isFallthrough: false);
 
             if (fallthrough != null)
                 ImportFallthrough(fallthrough);
         }
 
+        void PropagateControlFlow(BasicBlock next, bool isFallthrough)
+        {
+            if (!IsValidBranchTarget(_currentBasicBlock, next, isFallthrough) || _currentBasicBlock.ErrorCount > 0)
+                return;
+
+            PropagateThisState(_currentBasicBlock, next);
+
+            // Propagate stack across block bounds
+            StackValue[] entryStack = next.EntryStack;
+
+            if (entryStack != null)
+            {
+                FatalCheck(entryStack.Length == _stackTop, VerifierError.PathStackDepth);
+
+                for (int i = 0; i < entryStack.Length; i++)
+                {
+                    // TODO: Do we need to allow conversions?
+                    FatalCheck(entryStack[i].Kind == _stack[i].Kind, VerifierError.PathStackUnexpected, entryStack[i], _stack[i]);
+
+                    if (entryStack[i].Type != _stack[i].Type)
+                    {
+                        if (!IsAssignable(_stack[i], entryStack[i]))
+                        {
+                            StackValue mergedValue;
+                            if (!TryMergeStackValues(entryStack[i], _stack[i], out mergedValue))
+                                FatalCheck(false, VerifierError.PathStackUnexpected, entryStack[i], _stack[i]);
+
+                            // If merge actually changed entry stack
+                            if (mergedValue != entryStack[i])
+                            {
+                                entryStack[i] = mergedValue;
+
+                                if (next.ErrorCount == 0 && next.State != BasicBlock.ImportState.IsPending)
+                                    next.State = BasicBlock.ImportState.Unmarked; // Make sure block is reverified
+                            }
+                        }
+                    }
+                }
+            }
+            else
+            {
+                entryStack = (_stackTop != 0) ? new StackValue[_stackTop] : s_emptyStack;
+
+                for (int i = 0; i < entryStack.Length; i++)
+                    entryStack[i] = _stack[i];
+
+                next.EntryStack = entryStack;
+            }
+
+            MarkBasicBlock(next);
+        }
+
         void ImportBinaryOperation(ILOpcode opcode)
         {
             var op1 = Pop();
diff --git a/src/ILVerify/src/Resources/Strings.resx b/src/ILVerify/src/Resources/Strings.resx
index 62e362410..0158facc7 100644
--- a/src/ILVerify/src/Resources/Strings.resx
+++ b/src/ILVerify/src/Resources/Strings.resx
@@ -138,6 +138,9 @@
   <data name="BranchOutOfFilter" xml:space="preserve">
     <value>Branch out of exception filter block.</value>
   </data>
+  <data name="BranchOutOfFinally" xml:space="preserve">
+    <value>Branch out of finally block.</value>
+  </data>
   <data name="BranchOutOfHandler" xml:space="preserve">
     <value>Branch out of exception handler block.</value>
   </data>
@@ -219,6 +222,15 @@
   <data name="ExpectedValClassObjRefVariable" xml:space="preserve">
     <value>Value type, ObjRef type or variable type expected.</value>
   </data>
+  <data name="FallthroughException" xml:space="preserve">
+    <value>Fallthrough the end of an exception block.</value>
+  </data>
+  <data name="FallthroughIntoFilter" xml:space="preserve">
+    <value>Fallthrough into an exception filter.</value>
+  </data>
+  <data name="FallthroughIntoHandler" xml:space="preserve">
+    <value>Fallthrough into an exception handler.</value>
+  </data>
   <data name="FieldAccess" xml:space="preserve">
     <value>Field is not visible.</value>
   </data>
diff --git a/src/ILVerify/src/VerifierError.cs b/src/ILVerify/src/VerifierError.cs
index ef92c2e38..481797d67 100644
--- a/src/ILVerify/src/VerifierError.cs
+++ b/src/ILVerify/src/VerifierError.cs
@@ -55,10 +55,9 @@ namespace ILVerify
         //E_FIL_CONT_FIL                "Nested filters."
         //E_FIL_GTEQ_CS                 "filter >= code size."
         //E_FIL_START                   "Filter starts in the middle of an instruction."
-        //E_FALLTHRU_EXCEP              "fallthru the end of an exception block."
-        //E_FALLTHRU_INTO_HND           "fallthru into an exception handler."
-        //E_FALLTHRU_INTO_FIL           "fallthru into an exception filter."
-        //E_LEAVE                       "Leave from outside a try or catch block."
+        FallthroughException,           // Fallthrough the end of an exception block.
+        FallthroughIntoHandler,         // Fallthrough into an exception handler.
+        FallthroughIntoFilter,          // Fallthrough into an exception filter.
         LeaveIntoTry,                   // Leave into try block.
         LeaveIntoHandler,               // Leave into exception handler block.
         LeaveIntoFilter,                // Leave into filter block.
@@ -75,7 +74,7 @@ namespace ILVerify
         BranchOutOfTry,                 // Branch out of try block.
         BranchOutOfHandler,             // Branch out of exception handler block.
         BranchOutOfFilter,              // Branch out of exception filter block.
-        //E_BR_OUTOF_FIN                "Branch out of finally block."
+        BranchOutOfFinally,             // Branch out of finally block.
         ReturnFromTry,                  // Return out of try block.
         ReturnFromHandler,              // Return out of exception handler block.
         ReturnFromFilter,               // Return out of exception filter block.
@@ -205,7 +204,6 @@ namespace ILVerify
         LdvirtftnOnStatic,              // ldvirtftn on static.
         CallVirtOnStatic,               // callvirt on static.
         InitLocals,                     // initlocals must be set for verifiable methods with one or more local variables.
-        //E_BR_TO_EXCEPTION             "branch/leave to the beginning of a catch/filter handler"
         CallCtor,                       // call to .ctor only allowed to initialize this pointer from within a .ctor. Try newobj.
 
         ////@GENERICSVER: new generics related error messages
diff --git a/src/ILVerify/tests/ILTests/BranchingTests.il b/src/ILVerify/tests/ILTests/BranchingTests.il
index abd4b7ba7..58e3ad561 100644
--- a/src/ILVerify/tests/ILTests/BranchingTests.il
+++ b/src/ILVerify/tests/ILTests/BranchingTests.il
@@ -636,6 +636,23 @@
         lbl_ret: ret
     }
 
+    .method static public hidebysig void Branching.OutOfFinally_Invalid_BranchOutOfFinally() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            leave.s   lbl_ret
+        }
+        finally
+        {
+            br  lbl_ret
+            endfinally
+        }
+
+        lbl_ret: ret
+    }
+
     .method static public hidebysig void Branching.FromTryIntoOtherTryStart_Invalid_BranchOutOfTry() cil managed
     {
         .maxstack 2
@@ -732,4 +749,66 @@
 
         lbl_ret: ret
     }
+
+    .method static public hidebysig void Fallthrough.OutOfTryIntoFinally_Invalid_FallthroughException.FallthroughIntoHandler() cil managed
+    {
+        .try
+        {
+            nop
+        }
+        finally
+        {
+            endfinally
+        }
+    }
+
+    .method static public hidebysig void Fallthrough.OutOfTryIntoCatch_Invalid_FallthroughException.FallthroughIntoHandler() cil managed
+    {
+        .try
+        {
+            nop
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Fallthrough.OutOfCatch_Invalid_FallthroughException() cil managed
+    {
+        .try
+        {
+            nop
+            leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Fallthrough.OutOfTryIntoFilter_Invalid_FallthroughException.FallthroughIntoFilter() cil managed
+    {
+        .try
+        {
+            nop
+        }
+        filter
+        {
+            pop
+            ldc.i4.0
+            endfilter
+        } 
+        { 
+            pop
+            leave.s lbl_ret
+        }
+
+        lbl_ret: ret
+    }
 }