From 8b593d5fb0c5d7ead5636d59f77eed55689cbc40 Mon Sep 17 00:00:00 2001 From: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Date: Thu, 20 Jan 2022 00:07:41 +0100 Subject: [PATCH] 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 --- Mono.Cecil.Cil/MethodBody.cs | 13 ++-- Test/Mono.Cecil.Tests/ILProcessorTests.cs | 85 +++++++++++++---------- 2 files changed, 57 insertions(+), 41 deletions(-) diff --git a/Mono.Cecil.Cil/MethodBody.cs b/Mono.Cecil.Cil/MethodBody.cs index 1aecb5755..c9236dba4 100644 --- a/Mono.Cecil.Cil/MethodBody.cs +++ b/Mono.Cecil.Cil/MethodBody.cs @@ -384,11 +384,16 @@ InstructionOffset ResolveInstructionOffset(InstructionOffset inputOffset, ref In // 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 @@ InstructionOffset ResolveInstructionOffset(InstructionOffset inputOffset, ref In // 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 c5854033f..c1dc13ce8 100644 --- a/Test/Mono.Cecil.Tests/ILProcessorTests.cs +++ b/Test/Mono.Cecil.Tests/ILProcessorTests.cs @@ -152,16 +152,18 @@ public void Clear () 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 @@ public void InsertAfterWithSymbolRoundtrip (RoundtripType roundtripType, bool fo 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 @@ public void RemoveWithSymbolRoundtrip (RoundtripType roundtripType, bool forceUn 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 @@ public void ReplaceWithSymbolRoundtrip (RoundtripType roundtripType, bool forceU 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 @@ static void AssertLocalScope (MethodBody methodBody, ScopeDebugInformation scope 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);