Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

System.IndexOutOfRangeException resolving local scopes in Cecil 0.11.4 #816

Closed
SteveGilham opened this issue Dec 3, 2021 · 6 comments
Closed

Comments

@SteveGilham
Copy link
Contributor

Copying the relevant part of AltCover issue 135

Unhandled Exception: System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Mono.Cecil.Cil.InstructionCollection.ResolveInstructionOffset(InstructionOffset inputOffset, InstructionOffsetCache& cache)
   at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScope(ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, InstructionOffsetCache& cache)
   at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScope(ScopeDebugInformation scope, Instruction removedInstruction, Instruction existingInstruction, InstructionOffsetCache& cache)
   at Mono.Cecil.Cil.InstructionCollection.UpdateLocalScopes(Instruction removedInstruction, Instruction existingInstruction)
   at Mono.Collections.Generic.Collection`1.Insert(Int32 index, T item)

from a call to ILProcessor.InsertBefore on code built for .net Framework 4.8 on Windows.

Given the context, I don't have the relevant IL or a repro case, alas, but it's still worth having this on the record.

The stack trace is similar to those issues in #697 and #781 that appeared following the revision of that part of the codebase at the 0.11.3 release.

@jbevain
Copy link
Owner

jbevain commented Jan 13, 2022

@vitek-karas that seems related to previous changes. Any clue?

@vitek-karas
Copy link
Contributor

Not really - That said I can see couple of places in the ResolveInstructionOffset which could lead to the exception as there are not enough checks. For example if there was a method with no IL instruction but the items array is non-empty (with nulls), that could cause an exception like this.

vitek-karas added a commit to vitek-karas/cecil that referenced this issue Jan 13, 2022
Based on bug reports like jbevain#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.
@SteveGilham
Copy link
Contributor Author

While my previous issue reports in this area were caused by an assembly generated by an old version of the Mono C# compiler, the code base behind this one is modern C#, with a current Roslyn compiler : while I have not seen the code or the assembly in question, symbol names indicated that async local functions that are also closures -- giving 2 generated classes for the price of one small piece of source -- abound.

My guess would be that the offending method lies somewhere in the copious amounts of generated code, but I have not gone looking. It sufficed, for purposes of code instrumentation, to perform a one-pass scope resolution of my own for the method ahead of doing code injection.

jbevain pushed a commit that referenced this issue Jan 19, 2022
* 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
@jbevain
Copy link
Owner

jbevain commented Jan 19, 2022

@SteveGilham could you try with Cecil's master to validate the fix in #824?

@SteveGilham
Copy link
Contributor Author

I'm afraid I don't have a repro case; all the information I have is a stack trace from an issue against my AltCover project, which was enough to let me write a work-around.

@jbevain
Copy link
Owner

jbevain commented Oct 4, 2022

Closing this then, we should be good with the latest fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants