[ILVerify] Fix invalid branch/fallthrough into catch/finally causing assert (#4636)

* Fixed fallthrough into filter/catch/finally causing assert.

* Fixed catch block of TryIntoFilter test not leaving correctly.

2 files changed, 90 insertions, 21 deletions

diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs
index 3b94fd25c..fd8d8e3e1 100644
--- a/src/ILVerify/src/ILImporter.Verify.cs
+++ b/src/ILVerify/src/ILImporter.Verify.cs
@@ -393,18 +393,28 @@ namespace Internal.IL
                 VerificationError(VerifierError.StackUnexpected, src, dst);
         }
 
-        void CheckIsValidBranchTarget(BasicBlock src, BasicBlock target)
+        bool IsValidBranchTarget(BasicBlock src, BasicBlock target, bool reportErrors = true)
         {
+            bool isValid = true;
+
             if (src.TryIndex != target.TryIndex)
             {
                 if (src.TryIndex == null)
                 {
                     // Branching to first instruction of try-block is valid
                     if (target.StartOffset != _exceptionRegions[(int)target.TryIndex].ILRegion.TryOffset || !IsDirectChildRegion(src, target))
-                        VerificationError(VerifierError.BranchIntoTry);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchIntoTry);
+                        isValid = false;
+                    }
                 }
                 else if (target.TryIndex == null)
-                    VerificationError(VerifierError.BranchOutOfTry);
+                {
+                    if (reportErrors)
+                        VerificationError(VerifierError.BranchOutOfTry);
+                    isValid = false;
+                }
                 else
                 {
                     ref var srcRegion = ref _exceptionRegions[(int)src.TryIndex].ILRegion;
@@ -415,46 +425,88 @@ namespace Internal.IL
                     {
                         // Only branching to first instruction of try-block is valid
                         if (target.StartOffset != targetRegion.TryOffset || !IsDirectChildRegion(src, target))
-                            VerificationError(VerifierError.BranchIntoTry);
+                        {
+                            if (reportErrors)
+                                VerificationError(VerifierError.BranchIntoTry);
+                            isValid = false;
+                        }
                     }
                     else
-                        VerificationError(VerifierError.BranchOutOfTry);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchOutOfTry);
+                        isValid = false;
+                    }
                 }
             }
 
             if (src.FilterIndex != target.FilterIndex)
             {
                 if (src.FilterIndex == null)
-                    VerificationError(VerifierError.BranchIntoFilter);
+                {
+                    if (reportErrors)
+                        VerificationError(VerifierError.BranchIntoFilter);
+                    isValid = false;
+                }
                 else if (target.HandlerIndex == null)
-                    VerificationError(VerifierError.BranchOutOfFilter);
+                {
+                    if (reportErrors)
+                        VerificationError(VerifierError.BranchOutOfFilter);
+                    isValid = false;
+                }
                 else
                 {
                     ref var srcRegion = ref _exceptionRegions[(int)src.FilterIndex].ILRegion;
                     ref var targetRegion = ref _exceptionRegions[(int)target.FilterIndex].ILRegion;
                     if (srcRegion.FilterOffset <= targetRegion.FilterOffset)
-                        VerificationError(VerifierError.BranchIntoFilter);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchIntoFilter);
+                        isValid = false;
+                    }
                     else
-                        VerificationError(VerifierError.BranchOutOfFilter);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchOutOfFilter);
+                        isValid = false;
+                    }
                 }
             }
 
             if (src.HandlerIndex != target.HandlerIndex)
             {
                 if (src.HandlerIndex == null)
-                    VerificationError(VerifierError.BranchIntoHandler);
+                {
+                    if (reportErrors)
+                        VerificationError(VerifierError.BranchIntoHandler);
+                    isValid = false;
+                }
                 else if (target.HandlerIndex == null)
-                    VerificationError(VerifierError.BranchOutOfHandler);
+                {
+                    if (reportErrors)
+                        VerificationError(VerifierError.BranchOutOfHandler);
+                    isValid = false;
+                }
                 else
                 {
                     ref var srcRegion = ref _exceptionRegions[(int)src.HandlerIndex].ILRegion;
                     ref var targetRegion = ref _exceptionRegions[(int)target.HandlerIndex].ILRegion;
                     if (srcRegion.HandlerOffset <= targetRegion.HandlerOffset)
-                        VerificationError(VerifierError.BranchIntoHandler);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchIntoHandler);
+                        isValid = false;
+                    }
                     else
-                        VerificationError(VerifierError.BranchOutOfHandler);
+                    {
+                        if (reportErrors)
+                            VerificationError(VerifierError.BranchOutOfHandler);
+                        isValid = false;
+                    }
                 }
             }
+
+            return isValid;
         }
 
         /// <summary>
@@ -1210,6 +1262,9 @@ namespace Internal.IL
 
         void ImportFallthrough(BasicBlock next)
         {
+            if (!IsValidBranchTarget(_currentBasicBlock, next))
+                return;
+
             StackValue[] entryStack = next.EntryStack;
 
             if (entryStack != null)
@@ -1262,7 +1317,6 @@ namespace Internal.IL
             for (int i = 0; i < jmpDelta.Length; i++)
             {
                 BasicBlock target = _basicBlocks[jmpBase + jmpDelta[i]];
-                CheckIsValidBranchTarget(_currentBasicBlock, target);
                 ImportFallthrough(target);
             }
 
@@ -1272,8 +1326,6 @@ namespace Internal.IL
 
         void ImportBranch(ILOpcode opcode, BasicBlock target, BasicBlock fallthrough)
         {
-            CheckIsValidBranchTarget(_currentBasicBlock, target);
-
             switch (opcode)
             {
                 case ILOpcode.br:
diff --git a/src/ILVerify/tests/ILTests/BranchingTests.il b/src/ILVerify/tests/ILTests/BranchingTests.il
index 199ffae88..abd4b7ba7 100644
--- a/src/ILVerify/tests/ILTests/BranchingTests.il
+++ b/src/ILVerify/tests/ILTests/BranchingTests.il
@@ -571,8 +571,7 @@
         lbl_ret: ret
     }
 
-    /* These tests currently yield an assert (see issue #4537), due to a non-null entry stack to a catch/finally block; uncomment once fixed
-    .method static public hidebysig void Branching.FromTryIntoCatch_Invalid_BranchOutOfTry.BranchIntoFilter() cil managed
+    .method static public hidebysig void Branching.FromTryIntoCatch_Invalid_BranchOutOfTry.BranchIntoHandler() cil managed
     {
         .maxstack 2
 
@@ -594,8 +593,27 @@
 
         lbl_ret: ret
     }
-    */
-    /*
+
+    .method static public hidebysig void Branching.FromTryIntoFilter_Invalid_BranchOutOfTry.BranchIntoFilter() cil managed
+    {
+        .try
+        {
+            br.s  lbl_filter
+        }
+        filter
+        {
+            lbl_filter: pop
+            ldc.i4.0
+            endfilter
+        } 
+        { 
+            pop
+            leave.s lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
     .method static public hidebysig void Branching.FromTryIntoFinally_Invalid_BranchOutOfTry.BranchIntoHandler() cil managed
     {
         .maxstack 2
@@ -617,7 +635,6 @@
 
         lbl_ret: ret
     }
-    */
 
     .method static public hidebysig void Branching.FromTryIntoOtherTryStart_Invalid_BranchOutOfTry() cil managed
     {