jbevain / cecil

Cecil is a library to inspect, modify and create .NET programs and libraries.
MIT License
2.77k stars 630 forks source link

NotSupportedException in symbol write in 0.11.4 #781

Closed SteveGilham closed 3 years ago

SteveGilham commented 3 years ago

This continues on from issue #697, with the same old Mono C# compiler generated assembly, and is something which I should have caught back then as well (mea culpa).

After making the modification to the instruction offset resolution that was generating the NRE in PR #698, the upshot is a method with an OK top-level scope, but a nested scope which has the default ("end of method") InstructionOffset value at its start. This then faults when being written.

A simple test to demonstrate the behaviour is as follows

        [Test]
        public void InsertBeforeIssue697bis ()
        {
            var parameters = new ReaderParameters { SymbolReaderProvider = new MdbReaderProvider () };
            using (var module = GetResourceModule ("Issue697.dll", parameters)) {
                var pathGetterDef = module.GetTypes ()
                    .SelectMany (t => t.Methods)
                    .First (m => m.Name.Equals ("get_Defer"));

                var body = pathGetterDef.Body;
                var worker = body.GetILProcessor ();
                var initialBody = body.Instructions.ToList ();
                var head = initialBody.First ();
                var opcode = worker.Create (OpCodes.Ldc_I4_1);
                worker.InsertBefore (head, opcode);

                Assert.That (pathGetterDef.DebugInformation.Scope.Start.IsEndOfMethod, Is.False);
                foreach (var subscope in pathGetterDef.DebugInformation.Scope.Scopes)
                    Assert.That (subscope.Start.IsEndOfMethod, Is.False); // <== should fail here

                // big test -- if we can write w/o crashing when the previous asserts are removed
                var output = Path.GetTempFileName ();
                var outputdll = output + ".dll";

                var writer = new WriterParameters () {
                    SymbolWriterProvider = new MdbWriterProvider (),
                    WriteSymbols = true
                };
                using (var sink = File.Open (outputdll, FileMode.Create, FileAccess.ReadWrite)) {
                    module.Write (sink, writer);
                }

                Assert.Pass ();
            }
        }

The underlying cause is that at the start of the operation. the inner scope has a start offset of 18 and a null instruction (related to the trailing nulls at the root of the previous issue), whereas the non-null instructions sum in size to 14. Having thwarted the NRE exit, by making the previous change, we now return from the instruction resolution with an "off the end" value which is used to update the start of the scope.

That value then causes a NotSupportedException (which should probably be an InvalidDataException ) as the symbol data are being written.

A quick and very dirty fix can be made in MethodBody.UpdateLocalScope by pruning such dangling scopes

            if (scope.HasScopes) {
                foreach (var subScope in scope.Scopes)
                    UpdateLocalScope (subScope, removedInstruction, existingInstruction, ref cache);
                var dead = scope.Scopes.Where (s => s.Start.IsEndOfMethod).ToList (); // added
                dead.ForEach (d => scope.Scopes.Remove (d)); // added
            }

but I feel sure that there must be a better way to tackle this, around the point where the odd inner scope is being constructed.

SteveGilham commented 3 years ago

After a little thought, this seems to be the better fix

                    // Allow for trailing null values in the case of
                    // instructions.Size < instructions.Capacity
                    if (item == null)
-                       break;
+                       return new InstructionOffset (items [i - 1]);

                    cache.Instruction = item;