Skip to content

Commit

Permalink
Harden debug scope update logic (#824)
Browse files Browse the repository at this point in the history 
* 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
  • Loading branch information
@vitek-karas
vitek-karas authored Jan 19, 2022
1 parent a56b5bd commit 8b593d5
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 41 deletions.
13 changes: 9 additions & 4 deletions Mono.Cecil.Cil/MethodBody.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ();
}
Expand All @@ -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 ();
}
Expand Down
85 changes: 48 additions & 37 deletions Test/Mono.Cecil.Tests/ILProcessorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 ();
Expand All @@ -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 ();
Expand All @@ -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 ();
Expand All @@ -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 ();
Expand Down Expand Up @@ -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);
Expand Down

0 comments on commit 8b593d5

Please sign in to comment.