Harden debug scope update logic (#824)

* Harden debug scope update logic

Based on bug reports like #816 it seems there are still cases where the IL and scope offsets are out of sync in weird ways. This change modifies the logic to have no potential to cause the `IndexOutOfRangeException`. While I was not able to determine what combination could cause this, it's better this way.

The corner case comes when there's potential problem with the first/second instruction in the method body. The change in this case will potentially make the debug scopes slightly wrong by not pointing to the previous instruction (as there's none). Without having a real repro it's hard to tell what would be a better solution, this way it won't crash and the scopes still make sense.

* Fix typo

2 files changed, 57 insertions, 41 deletions

diff --git a/Mono.Cecil.Cil/MethodBody.cs b/Mono.Cecil.Cil/MethodBody.cs
index 1aecb57..c9236db 100644
--- a/Mono.Cecil.Cil/MethodBody.cs
+++ b/Mono.Cecil.Cil/MethodBody.cs
@@ -384,11 +384,16 @@ namespace Mono.Cecil.Cil {
 				// resolve by walking the instructions from start and don't cache the result.
 				int size = 0;
 				for (int i = 0; i < items.Length; i++) {
+					// The array can be larger than the actual size, in which case its padded with nulls at the end
+					// so when we reach null, treat it as an end of the IL.
+					if (items [i] == null)
+						return new InstructionOffset (i == 0 ? items [0] : items [i - 1]);
+
 					if (size == offset)
 						return new InstructionOffset (items [i]);
 
 					if (size > offset)
-						return new InstructionOffset (items [i - 1]);
+						return new InstructionOffset (i == 0 ? items [0] : items [i - 1]);
 
 					size += items [i].GetSize ();
 				}
@@ -407,15 +412,15 @@ namespace Mono.Cecil.Cil {
 					// Allow for trailing null values in the case of
 					// instructions.Size < instructions.Capacity
 					if (item == null)
-						return new InstructionOffset (items [i - 1]);
+						return new InstructionOffset (i == 0 ? items [0] : items [i - 1]);
 
 					cache.Instruction = item;
 
 					if (cache.Offset == offset)
 						return new InstructionOffset (cache.Instruction);
 
-					if (cache.Offset > offset)
-						return new InstructionOffset (items [i - 1]);
+					if (cache.Offset > offset) 
+						return new InstructionOffset (i == 0 ? items [0] : items [i - 1]);
 
 					size += item.GetSize ();
 				}
diff --git a/Test/Mono.Cecil.Tests/ILProcessorTests.cs b/Test/Mono.Cecil.Tests/ILProcessorTests.cs
index c585403..c1dc13c 100644
--- a/Test/Mono.Cecil.Tests/ILProcessorTests.cs
+++ b/Test/Mono.Cecil.Tests/ILProcessorTests.cs
@@ -152,16 +152,18 @@ namespace Mono.Cecil.Tests {
 			AssertOpCodeSequence (new OpCode[] { }, method);
 		}
 
-		[TestCase (RoundtripType.None, false, false)]
-		[TestCase (RoundtripType.Pdb, false, false)]
-		[TestCase (RoundtripType.Pdb, true, false)]
-		[TestCase (RoundtripType.Pdb, true, true)]
-		[TestCase (RoundtripType.PortablePdb, false, false)]
-		[TestCase (RoundtripType.PortablePdb, true, false)]
-		[TestCase (RoundtripType.PortablePdb, true, true)]
-		public void InsertAfterWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes)
+		[TestCase (RoundtripType.None, false, false, false)]
+		[TestCase (RoundtripType.Pdb, false, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, true)]
+		[TestCase (RoundtripType.Pdb, true, true, false)]
+		[TestCase (RoundtripType.PortablePdb, false, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, true)]
+		[TestCase (RoundtripType.PortablePdb, true, true, false)]
+		public void InsertAfterWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes, bool padIL)
 		{
-			var methodBody = CreateTestMethodWithLocalScopes ();
+			var methodBody = CreateTestMethodWithLocalScopes (padIL);
 			methodBody = RoundtripMethodBody (methodBody, roundtripType, forceUnresolved, reverseScopes);
 
 			var il = methodBody.GetILProcessor ();
@@ -176,16 +178,18 @@ namespace Mono.Cecil.Tests {
 			methodBody.Method.Module.Dispose ();
 		}
 
-		[TestCase (RoundtripType.None, false, false)]
-		[TestCase (RoundtripType.Pdb, false, false)]
-		[TestCase (RoundtripType.Pdb, true, false)]
-		[TestCase (RoundtripType.Pdb, true, true)]
-		[TestCase (RoundtripType.PortablePdb, false, false)]
-		[TestCase (RoundtripType.PortablePdb, true, false)]
-		[TestCase (RoundtripType.PortablePdb, true, true)]
-		public void RemoveWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes)
+		[TestCase (RoundtripType.None, false, false, false)]
+		[TestCase (RoundtripType.Pdb, false, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, true)]
+		[TestCase (RoundtripType.Pdb, true, true, false)]
+		[TestCase (RoundtripType.PortablePdb, false, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, true)]
+		[TestCase (RoundtripType.PortablePdb, true, true, false)]
+		public void RemoveWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes, bool padIL)
 		{
-			var methodBody = CreateTestMethodWithLocalScopes ();
+			var methodBody = CreateTestMethodWithLocalScopes (padIL);
 			methodBody = RoundtripMethodBody (methodBody, roundtripType, forceUnresolved, reverseScopes);
 
 			var il = methodBody.GetILProcessor ();
@@ -200,16 +204,18 @@ namespace Mono.Cecil.Tests {
 			methodBody.Method.Module.Dispose ();
 		}
 
-		[TestCase (RoundtripType.None, false, false)]
-		[TestCase (RoundtripType.Pdb, false, false)]
-		[TestCase (RoundtripType.Pdb, true, false)]
-		[TestCase (RoundtripType.Pdb, true, true)]
-		[TestCase (RoundtripType.PortablePdb, false, false)]
-		[TestCase (RoundtripType.PortablePdb, true, false)]
-		[TestCase (RoundtripType.PortablePdb, true, true)]
-		public void ReplaceWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes)
+		[TestCase (RoundtripType.None, false, false, false)]
+		[TestCase (RoundtripType.Pdb, false, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, true)]
+		[TestCase (RoundtripType.Pdb, true, true, false)]
+		[TestCase (RoundtripType.PortablePdb, false, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, true)]
+		[TestCase (RoundtripType.PortablePdb, true, true, false)]
+		public void ReplaceWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes, bool padIL)
 		{
-			var methodBody = CreateTestMethodWithLocalScopes ();
+			var methodBody = CreateTestMethodWithLocalScopes (padIL);
 			methodBody = RoundtripMethodBody (methodBody, roundtripType, forceUnresolved, reverseScopes);
 
 			var il = methodBody.GetILProcessor ();
@@ -224,16 +230,18 @@ namespace Mono.Cecil.Tests {
 			methodBody.Method.Module.Dispose ();
 		}
 
-		[TestCase (RoundtripType.None, false, false)]
-		[TestCase (RoundtripType.Pdb, false, false)]
-		[TestCase (RoundtripType.Pdb, true, false)]
-		[TestCase (RoundtripType.Pdb, true, true)]
-		[TestCase (RoundtripType.PortablePdb, false, false)]
-		[TestCase (RoundtripType.PortablePdb, true, false)]
-		[TestCase (RoundtripType.PortablePdb, true, true)]
-		public void EditBodyWithScopesAndSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes)
+		[TestCase (RoundtripType.None, false, false, false)]
+		[TestCase (RoundtripType.Pdb, false, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, false)]
+		[TestCase (RoundtripType.Pdb, true, false, true)]
+		[TestCase (RoundtripType.Pdb, true, true, false)]
+		[TestCase (RoundtripType.PortablePdb, false, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, false)]
+		[TestCase (RoundtripType.PortablePdb, true, false, true)]
+		[TestCase (RoundtripType.PortablePdb, true, true, false)]
+		public void EditBodyWithScopesAndSymbolRoundtrip (RoundtripType roundtripType, bool forceUnresolved, bool reverseScopes, bool padIL)
 		{
-			var methodBody = CreateTestMethodWithLocalScopes ();
+			var methodBody = CreateTestMethodWithLocalScopes (padIL);
 			methodBody = RoundtripMethodBody (methodBody, roundtripType, forceUnresolved, reverseScopes);
 
 			var il = methodBody.GetILProcessor ();
@@ -320,13 +328,16 @@ namespace Mono.Cecil.Tests {
 				Assert.IsTrue (scope.End.IsEndOfMethod);
 		}
 
-		static MethodBody CreateTestMethodWithLocalScopes ()
+		static MethodBody CreateTestMethodWithLocalScopes (bool padILWithNulls)
 		{
 			var module = ModuleDefinition.CreateModule ("TestILProcessor", ModuleKind.Dll);
 			var type = new TypeDefinition ("NS", "TestType", TypeAttributes.Public | TypeAttributes.Abstract | TypeAttributes.Sealed, module.ImportReference (typeof (object)));
 			module.Types.Add (type);
 
 			var methodBody = CreateTestMethod (OpCodes.Nop, OpCodes.Ldloc_0, OpCodes.Nop, OpCodes.Ldloc_1, OpCodes.Nop, OpCodes.Ldloc_2, OpCodes.Nop);
+			if (padILWithNulls)
+				methodBody.Instructions.Capacity += 10;
+
 			var method = methodBody.Method;
 			method.ReturnType = module.ImportReference (typeof (void));
 			type.Methods.Add (method);