microsoft / clrmd

Microsoft.Diagnostics.Runtime is a set of APIs for introspecting processes and dumps.
MIT License
1.05k stars 255 forks source link

IXCLR interfaces can outlive the DAC dll, causing AV on process shutdown #25

Closed jazzdelightsme closed 9 years ago

jazzdelightsme commented 9 years ago

In the DacLibrary finalizer, we FreeLibrary the DAC. Unfortunately, sometimes some IXCLR* RCWs can live past this point, and then when the process is shutting down and the CLR is trying to call Release() on them, it AVs (because it's trying to call into an unloaded DLL).

In the past I've worked around this in my own copy of the code by simply commenting out the FreeLibrary, but I've accidentally overwritten my changes a few times when sync'ing with changes from the main clrmd repo. So I'm filing this issue to record the existence of this problem, and I'll also submit a PR that guards the FreeLibrary with a #if (which I can define in my copy of the project), like so:

#if !LEAK_ICXLR_OBJECTS
            if (_library != IntPtr.Zero)
                NativeMethods.FreeLibrary(_library);
#endif
jazzdelightsme commented 9 years ago

Problem repro in DbgShell (run under a debugger to witness the AV):

   Import-Module DbgShellTest
   New-TestApp TestManagedConsoleApp -Attach
   ~0 s
   k
   $t = ~0
   $t.ManagedThreads
   .kill
   q      # <-- AV will happen here
leculver commented 9 years ago

Before we go so far as intentionally leaking the dac, can you try this out?

        Marshal.FinalReleaseComObject(_dac);
        if (_library != IntPtr.Zero)
            NativeMethods.FreeLibrary(_library);

In DesktopRuntimeBase.EnumerateStackFrames, wrap that block with a try/finally, where the finally is:

        finally
        {
            if (task != null)
                Marshal.FinalReleaseComObject(task);

            if (stackwalk != null)
                Marshal.FinalReleaseComObject(stackwalk);
        }

I don't see this AV, so if you wouldn't mind giving that a try I'd be appreciative. If it still throws an AV with these changes can you send me a crash dump offline of the AV with this fix?

Thanks!

jazzdelightsme commented 9 years ago

I've tried to FinalRelease the _dac in the past, but I'll be happy to give it another go along with that other change.

leculver commented 9 years ago

Fixed in changeset 4fa26f39c8c057abf0e2a67d735369ce4766fa07.