[ILVerify] Fixed IsValidBranchTarget not allowing to branch to first instruction of try blocks (#4381)

* Changed theory display name and categorised theories with trait attributes.

* Fixed IsValidBranchTarget not allowing to branch to the first instruction of try blocks.

* Added additional tests for branching into/out of protected blocks.

* Removed redundant moduleName info from TestCase toString.

* Added additional test cases for branching into another try block outside of the current one.

* Added method parameters to error output in order to be able to distinguish between overloaded methods.

* Added additional test case for branching into an inner try.

* Added additional valid test case for branching to the start of a double nested try-block.

* Added additional test case for branching into try with predecessor.

* Fixed IsValidBranchTarget not checking for double nested try blocks.

* Fixed jumping to nested try start from outside of try being verified and added a test case for this scenario.

6 files changed, 531 insertions, 14 deletions

diff --git a/src/ILVerify/src/ILImporter.Verify.cs b/src/ILVerify/src/ILImporter.Verify.cs
index cc7adc402..d0f1375c0 100644
--- a/src/ILVerify/src/ILImporter.Verify.cs
+++ b/src/ILVerify/src/ILImporter.Verify.cs
@@ -372,17 +372,28 @@ namespace Internal.IL
             if (src.TryIndex != target.TryIndex)
             {
                 if (src.TryIndex == null)
-                    VerificationError(VerifierError.BranchIntoTry);
+                {
+                    // Branching to first instruction of try-block is valid
+                    if (target.StartOffset != _exceptionRegions[(int)target.TryIndex].ILRegion.TryOffset || !IsDirectChildRegion(src, target))
+                        VerificationError(VerifierError.BranchIntoTry);
+                }
                 else if (target.TryIndex == null)
                     VerificationError(VerifierError.BranchOutOfTry);
                 else
                 {
-                    if (_exceptionRegions[(int)src.TryIndex].ILRegion.TryOffset < _exceptionRegions[(int)target.TryIndex].ILRegion.TryOffset)
-                        VerificationError(VerifierError.BranchIntoTry);
+                    var srcRegion = _exceptionRegions[(int)src.TryIndex].ILRegion;
+                    var targetRegion = _exceptionRegions[(int)target.TryIndex].ILRegion;
+                    // If target is inside source region
+                    if (srcRegion.TryOffset <= targetRegion.TryOffset && 
+                        target.StartOffset < srcRegion.TryOffset + srcRegion.TryLength)
+                    {
+                        // Only branching to first instruction of try-block is valid
+                        if (target.StartOffset != targetRegion.TryOffset || !IsDirectChildRegion(src, target))
+                            VerificationError(VerifierError.BranchIntoTry);
+                    }
                     else
                         VerificationError(VerifierError.BranchOutOfTry);
                 }
-                return;
             }
 
             if (src.FilterIndex != target.FilterIndex)
@@ -393,12 +404,13 @@ namespace Internal.IL
                     VerificationError(VerifierError.BranchOutOfFilter);
                 else
                 {
-                    if (_exceptionRegions[(int)src.FilterIndex].ILRegion.FilterOffset < _exceptionRegions[(int)target.FilterIndex].ILRegion.FilterOffset)
+                    var srcRegion = _exceptionRegions[(int)src.FilterIndex].ILRegion;
+                    var targetRegion = _exceptionRegions[(int)target.FilterIndex].ILRegion;
+                    if (srcRegion.FilterOffset <= targetRegion.FilterOffset)
                         VerificationError(VerifierError.BranchIntoFilter);
                     else
                         VerificationError(VerifierError.BranchOutOfFilter);
                 }
-                return;
             }
 
             if (src.HandlerIndex != target.HandlerIndex)
@@ -409,15 +421,46 @@ namespace Internal.IL
                     VerificationError(VerifierError.BranchOutOfHandler);
                 else
                 {
-                    if (_exceptionRegions[(int)src.HandlerIndex].ILRegion.HandlerOffset < _exceptionRegions[(int)target.HandlerIndex].ILRegion.HandlerOffset)
+                    var srcRegion = _exceptionRegions[(int)src.HandlerIndex].ILRegion;
+                    var targetRegion = _exceptionRegions[(int)target.HandlerIndex].ILRegion;
+                    if (srcRegion.HandlerOffset <= targetRegion.HandlerOffset)
                         VerificationError(VerifierError.BranchIntoHandler);
                     else
                         VerificationError(VerifierError.BranchOutOfHandler);
                 }
-                return;
             }
         }
 
+        /// <summary>
+        /// Checks whether the given enclosed try block is a direct child try-region of 
+        /// the given enclosing try block.
+        /// </summary>
+        /// <param name="enclosingBlock">The block enclosing the try block given by <paramref name="enclosedBlock"/>.</param>
+        /// <param name="enclosedBlock">The block to check whether it is a direct child try-region of <paramref name="enclosingBlock"/>.</param>
+        /// <returns>True if <paramref name="enclosedBlock"/> is a direct child try region of <paramref name="enclosingBlock"/>.</returns>
+        bool IsDirectChildRegion(BasicBlock enclosingBlock, BasicBlock enclosedBlock)
+        {
+            var enclosedRegion = _exceptionRegions[(int)enclosedBlock.TryIndex].ILRegion;
+
+            // Walk from enclosed try start backwards and check each BasicBlock whether it is a try-start
+            for (int i = enclosedRegion.TryOffset - 1; i > enclosingBlock.StartOffset; --i)
+            {
+                var block = _basicBlocks[i];
+                if (block == null)
+                    continue;
+
+                if (block.TryStart && block.TryIndex != enclosingBlock.TryIndex)
+                {
+                    var blockRegion = _exceptionRegions[(int)block.TryIndex].ILRegion;
+                    // blockRegion is actually enclosing enclosedRegion
+                    if (blockRegion.TryOffset + blockRegion.TryLength > enclosedRegion.TryOffset)
+                        return false;
+                }
+            }
+
+            return true;
+        }
+
         // For now, match PEVerify type formating to make it easy to compare with baseline
         static string TypeToStringForIsAssignable(TypeDesc type)
         {
diff --git a/src/ILVerify/src/Program.cs b/src/ILVerify/src/Program.cs
index 0b2f928ca..aec3b5c18 100644
--- a/src/ILVerify/src/Program.cs
+++ b/src/ILVerify/src/Program.cs
@@ -133,6 +133,17 @@ namespace ILVerify
                     message.Append(((EcmaType)method.OwningType).Name);
                     message.Append("::");
                     message.Append(method.Name);
+                    message.Append("(");
+                    if (method.Signature._parameters != null && method.Signature._parameters.Length > 0)
+                    {
+                        foreach (TypeDesc parameter in method.Signature._parameters)
+                        {
+                            message.Append(parameter.ToString());
+                            message.Append(", ");
+                        }
+                        message.Remove(message.Length - 2, 2);
+                    }
+                    message.Append(")");
                     message.Append("]");
 
                     message.Append("[offset 0x");
diff --git a/src/ILVerify/tests/ILMethodTester.cs b/src/ILVerify/tests/ILMethodTester.cs
index 5ecd71002..6c59f146a 100644
--- a/src/ILVerify/tests/ILMethodTester.cs
+++ b/src/ILVerify/tests/ILMethodTester.cs
@@ -13,8 +13,9 @@ namespace ILVerify.Tests
 {
     public class ILMethodTester
     {
-        [Theory]
+        [Theory(DisplayName = "")]
         [MemberData(nameof(TestDataLoader.GetMethodsWithValidIL), MemberType = typeof(TestDataLoader))]
+        [Trait("", "Valid IL Tests")]
         void TestMethodsWithValidIL(ValidILTestCase validILTestCase)
         {
             ILImporter importer = ConstructILImporter(validILTestCase);
@@ -29,8 +30,9 @@ namespace ILVerify.Tests
             Assert.Equal(0, verifierErrors.Count);
         }
 
-        [Theory]
+        [Theory(DisplayName = "")]
         [MemberData(nameof(TestDataLoader.GetMethodsWithInvalidIL), MemberType = typeof(TestDataLoader))]
+        [Trait("", "Invalid IL Tests")]
         void TestMethodsWithInvalidIL(InvalidILTestCase invalidILTestCase)
         {
             ILImporter importer = ConstructILImporter(invalidILTestCase);
diff --git a/src/ILVerify/tests/ILTests/BranchingTests.il b/src/ILVerify/tests/ILTests/BranchingTests.il
index 09b3b35bf..d223bb2b8 100644
--- a/src/ILVerify/tests/ILTests/BranchingTests.il
+++ b/src/ILVerify/tests/ILTests/BranchingTests.il
@@ -119,7 +119,7 @@
         lbl_ret: ret
     }
 
-    .method static public hidebysig void Branching.IntoTry_Invalid_BranchIntoTry() cil managed
+    .method static public hidebysig void Branching.IntoTryStart_Valid() cil managed
     {
         .maxstack 2
 
@@ -144,4 +144,404 @@
 
         lbl_ret: ret
     }
+
+    .method static public hidebysig void Branching.IntoTryMiddle_Invalid_BranchIntoTry() cil managed
+    {
+        .maxstack 2
+
+        ldc.i4.s  10
+        ldc.i4.s  5
+        bne.un.s  lbl_true
+
+        nop
+        br.s      lbl_ret
+
+        .try
+        {
+            nop
+            lbl_true: nop
+
+            leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.IntoNestedTryStart_Valid() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_true
+
+            nop
+            br.s      lbl_leave
+            .try
+            {
+                lbl_true: nop
+
+                leave.s   lbl_leave
+            }
+            catch [System.Runtime]System.Object
+            {
+                pop
+                leave.s   lbl_leave
+            }
+
+            lbl_leave: leave.s lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.IntoDoubleNestedTryStart_Invalid_BranchIntoTry() cil managed
+    {
+        .try
+        {
+            nop
+            br    Inner
+
+            .try
+            {
+                nop
+
+                .try
+                {
+                    Inner:
+                    leave.s    A
+                }
+                catch [System.Runtime]System.Object 
+                {
+                    pop
+                    leave.s    A
+                }
+                A:
+
+                leave.s    B
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    B
+            }
+            B:
+
+            leave.s    C
+        }
+        catch [System.Runtime]System.Object 
+        {
+            pop
+            leave.s    C
+        }
+        C:
+
+        ret
+    }
+
+    .method static public hidebysig void Branching.IntoDoubleNestedTryStartStart_Valid() cil managed
+    {
+        .try
+        {
+            nop
+            br    Inner
+
+            .try
+            {
+                .try
+                {
+                    Inner:
+                    leave.s    A
+                }
+                catch [System.Runtime]System.Object 
+                {
+                    pop
+                    leave.s    A
+                }
+                A:
+
+                leave.s    B
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    B
+            }
+            B:
+
+            leave.s    C
+        }
+        catch [System.Runtime]System.Object 
+        {
+            pop
+            leave.s    C
+        }
+        C:
+
+        ret
+    }
+
+    .method static public hidebysig void Branching.IntoDoubleNestedTryStartFromOutside_Invalid_BranchIntoTry() cil managed
+    {
+        br    Inner
+
+        .try
+        {
+            nop
+
+            .try
+            {
+                Inner:
+                leave.s    A
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    A
+            }
+            A:
+
+            leave.s    B
+        }
+        catch [System.Runtime]System.Object 
+        {
+            pop
+            leave.s    B
+        }
+        B:
+
+        ret
+    }
+
+    .method static public hidebysig void Branching.IntoTryStartWithPredecessor_Valid() cil managed
+    {
+        .try
+        {
+            nop
+            br    Inner
+
+            .try
+            {
+                leave.s    A
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    A
+            }
+
+            .try
+            {
+                Inner:
+                leave.s    A
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    A
+            }
+
+            A:
+
+            leave.s    C
+        }
+        catch [System.Runtime]System.Object 
+        {
+            pop
+            leave.s    C
+        }
+        C:
+
+        ret
+    }
+
+    .method static public hidebysig void Branching.IntoNestedTryMiddle_Invalid_BranchIntoTry() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_true
+
+            nop
+            br.s      lbl_leave
+            .try
+            {
+                nop
+                lbl_true: nop
+
+                leave.s   lbl_leave
+            }
+            catch [System.Runtime]System.Object
+            {
+                pop
+                leave.s   lbl_leave
+            }
+
+            lbl_leave: leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.FromTryIntoCatch_Invalid_BranchOutOfTry_BranchIntoFilter() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_true
+
+            nop
+            leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            lbl_true: nop
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.FromTryIntoFinally_Invalid_BranchOutOfTry_BranchIntoHandler() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_finally
+
+            nop
+            leave.s   lbl_ret
+        }
+        finally
+        {
+            lbl_finally: nop
+            endfinally
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.FromTryIntoOtherTryStart_Invalid_BranchOutOfTry() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_try2
+
+            nop
+            leave.s   lbl_try
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_try
+        }
+
+        lbl_try: nop
+        .try
+        {
+            lbl_try2: nop
+            leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
+    .method static public hidebysig void Branching.OutOfTrySameStart_Invalid_BranchOutOfTry() cil managed
+    {
+        .try
+        {
+            .try
+            {
+                br    A
+
+                leave.s    A
+            }
+            catch [System.Runtime]System.Object 
+            {
+                pop
+                leave.s    A
+            }
+            A:
+
+            leave.s    B
+        }
+        catch [System.Runtime]System.Object 
+        {
+            pop
+            leave.s    B
+        }
+        B:
+
+        ret
+    }
+
+    .method static public hidebysig void Branching.FromTryIntoOtherTryMiddle_Invalid_BranchOutOfTry() cil managed
+    {
+        .maxstack 2
+
+        .try
+        {
+            ldc.i4.s  10
+            ldc.i4.s  5
+            bne.un.s  lbl_try2
+
+            nop
+            leave.s   lbl_try
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_try
+        }
+
+        lbl_try: nop
+        .try
+        {
+            nop
+            lbl_try2: leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
 }
diff --git a/src/ILVerify/tests/ILTests/SwitchTests.il b/src/ILVerify/tests/ILTests/SwitchTests.il
index 22fc68e3c..974af29b6 100644
--- a/src/ILVerify/tests/ILTests/SwitchTests.il
+++ b/src/ILVerify/tests/ILTests/SwitchTests.il
@@ -74,7 +74,7 @@
         lbl_ret: ret
     }
 
-    .method static public hidebysig void Switch.IntoTry_Invalid_BranchIntoTry() cil managed
+    .method static public hidebysig void Switch.IntoTryStart_Valid() cil managed
     {
         .maxstack 1
 
@@ -99,6 +99,31 @@
         lbl_ret: ret
     }
 
+    .method static public hidebysig void Switch.IntoTryMiddle_Invalid_BranchIntoTry() cil managed
+    {
+        .maxstack 1
+
+        ldc.i4.s   10
+        switch     (lbl_ret, lbl_a)
+
+        br.s       lbl_ret
+
+        .try
+        {
+            nop
+            lbl_a: br.s     lbl_leave
+
+            lbl_leave: leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            nop
+            leave.s     lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
     .method static public hidebysig void Switch.OutOfTry_Invalid_BranchOutOfTry() cil managed
     {
         .maxstack 1
@@ -159,7 +184,7 @@
         lbl_ret: ret
     }
 
-    .method static public hidebysig void Switch.NestedIntoTry_Invalid_BranchIntoTry() cil managed
+    .method static public hidebysig void Switch.NestedIntoTryStart_Valid() cil managed
     {
         .maxstack 1
 
@@ -195,6 +220,42 @@
         lbl_ret: ret
     }
 
+    .method static public hidebysig void Switch.NestedIntoTryMiddle_Invalid_BranchIntoTry() cil managed
+    {
+        .maxstack 1
+
+        .try
+        {
+            ldc.i4.s  10
+            switch    (lbl_leave, lbl_a, lbl_b)
+
+            br.s      lbl_leave
+
+            lbl_a: nop
+            br.s      lbl_leave
+
+            .try
+            {
+                nop
+                lbl_b: leave.s   lbl_leave
+            }
+            catch [System.Runtime]System.Object
+            {
+                pop
+                leave.s   lbl_leave
+            }
+
+            lbl_leave: leave.s   lbl_ret
+        }
+        catch [System.Runtime]System.Object
+        {
+            pop
+            leave.s   lbl_ret
+        }
+
+        lbl_ret: ret
+    }
+
     .method static public hidebysig void Switch.ObjectValue_Invalid_StackUnexpected() cil managed
     {
         .maxstack 1
diff --git a/src/ILVerify/tests/TestDataLoader.cs b/src/ILVerify/tests/TestDataLoader.cs
index 2a403993f..5f8af6d9a 100644
--- a/src/ILVerify/tests/TestDataLoader.cs
+++ b/src/ILVerify/tests/TestDataLoader.cs
@@ -171,7 +171,7 @@ namespace ILVerify.Tests
 
         public override string ToString()
         {
-            return $"{ModuleName} - {MethodName}";
+            return $"{MethodName}";
         }
     }