Reloaded-Project / Reloaded.Hooks

Advanced native function hooks for x86, x64. Welcome to the next level!
GNU Lesser General Public License v3.0
213 stars 33 forks source link

.net 6 Unloading from AssemblyLoadContext results in CoreCLR::ReportViolation calls. #14

Closed Senior-Moon-Moon closed 2 years ago

Senior-Moon-Moon commented 2 years ago

Using .net 6.

I have a hook to a DX11.Present function. This is done inside of an AssemblyLoadContext in order to support "unloading" / reloading the .net dll without killing the process.

Hooking works fine. The issue I am coming across is after Disabling the hook (With a time delay due to multi threading in the renderer), once the GC kicks in the hook location get's changed to the following:

(This is the WrapperPointer)

mov     rcx, 7F003C006825h
mov     rax, offset coreclr!UMEntryThunk::ReportViolation
jmp     rax

1) I assume that this is the GC flagging this memory as invalid access. 2) Is there a better way to unload without trying to do something like writing the original bytes back to the function (potentially killing any other present hooks?)

(Sample ish code of what this looks like)

// ALC Calls this using reflection create our wait signal 

public  void ALCEntry()
{ 
unloadSignal = new ManualResetEvent(false);
            unloadSignal.Reset();

//Present Hook
_presentHook = ReloadedHooks.Instance.CreateHook<PresentDelegate>(this.PresentDetour, vtable[(int)IDXGISwapChainVirtuals.SC_PRESENT].ToInt64());
_presentHook.Activate();
 _presentHook.Enable();

//Wait for unload signal
 unloadSignal.WaitOne();

_presentHook.Disable();
Thread.Sleep(200); // allow multi-threading renderer to exit our c# code.

//ALC cleans up references -- https://docs.microsoft.com/en-us/dotnet/standard/assembly/unloadability 
}

//Present a button that triggers our unload
internal IntPtr PresentDetour(IntPtr swapChain, uint syncInterval, uint presentFlags)
{
   if(ImGui::Button("Unload"))
    unloadSignal.set();

return _presentHook.OriginalFunction(swapChain, syncInterval, presentFlags);
}

Why I think this is coming from the GC:

Adjusting the Sleep timer after the disable doesn't cause a crash. I can set that to 3 minutes, the rendering of the IMGUI button goes away. no crash will happen regardless of this timer.

in the DLL that's doing the ALC stuff i have the following:

 Debugger.Launch(); // Auto attach Windbg to track CoreCLR error.
DoAlcCall(); // does all the loading and reflection & unlaoding locally to prevent GC handle stopping the unload.

Console.WriteLine("Starting GC");
 for (int i = 0; alcWeakRef.IsAlive && (i < 10); i++)
                {
                    GC.Collect();
                    GC.WaitForPendingFinalizers();
                }

I get the console log line, then the debugger will catch the exception caused by the ReportViolation above (causing me to think the GC is doing something)

Sewer56 commented 2 years ago

This is actually very strange, the snippets posted here look fine.
I wonder if the issue is potentially something completely unrelated.

Consider the following flowcharts:

Activated Hook Flow Diagram

 +-------------------+       +----------------+         
 | Original Function --------- ReverseWrapper |         
 | Start             |       +--------|-------+         
 +-------------------+                |                 
                                      |                 
                               +------|------+          
                               | C# Function |          
                               +------|------+          
                                      |                 
                                      |                 
                    +-----------------|----------------+
                    | Original Function Wrapper        |
                    |  Including x >= 6 bytes          |
                    |  long start of original function.|
                    +-----------------|----------------+
                                      |                 
                                      |                 
+-------------------+                 |                 
| Rest of Original  ------------------+                 
| Function          |                                   
+-------------------+                                   

Deactivated Hook Flow Diagram

 +-------------------+       +----------------+         
 | Original Function --------- ReverseWrapper ---+      
 | Start             |       +----------------+  |      
 +-------------------+                           |      
                               +-------------+   |      
                               | C# Function |   |      
                               +-------------+   |      
                                                 |      
                                      +----------|      
                                      |                 
                    +-----------------|----------------+
                    | Original Function Wrapper        |
                    |  Including x >= 6 bytes          |
                    |  long start of original function.|
                    +-----------------|----------------+
                                      |                 
                                      |                 
+-------------------+                 |                 
| Rest of Original  ------------------+                 
| Function          |                                   
+-------------------+  

When hooks are deactivated in Reloaded.Hooks, they don't touch the .NET runtime at all.
The library was always designed in this way to allow for unloadability in a manner that still co-operates with hook stacking.

I was curious though and I tested this with the reference https://github.com/Reloaded-Project/Reloaded-II mod at Reloaded.Imgui.Hook.

YakuzaKiwami_msX2PEfTjy

Everything unloads and loads fine, but as soon as GC hits, the process dies, yeah. It's interesting.

In any case, this is not a library issue. There are 15+ mods that support unloadability for Reloaded-II I wrote almost all of them without any issue.

I don't think this is a library issue, this might be a Dear ImGui related issue, it might be worth taking a look at all the data passed to/from Dear ImGui. I'm interested in it too, so might be worth taking a look at it sometime.

Edit: Actually, nevermind. This is probably a library bug after all; I'll probably have to have a nice walk through the code. I probably changed something in the backend that makes it indirectly rely on the .NET runtime.

Maybe related to memory allocation for the wrappers, or something of the sort.

Sewer56 commented 2 years ago

By the way, since you mentioned running .NET 6. There's a rather big issue in .NET 6 for hooking libraries (this one included).

If you plan on hooking Windows APIs in any capacity, it's best you don't use it.

An optimisation in .NET 6 makes it so that FCalls (internal calls to the native part of the .NET Runtime) don't do a managed->native context switch anymore. This means that if the native C++ part of the runtime calls a Windows API function that's hooked, your hooks will try to do a native->managed transition, when the stack frame is already in its 'managed' state/form. I.e. The runtime will basically thinks you're calling an UnmanagedCallersOnly method from managed code.

I've been pretty busy maintaining my stuff and haven't yet gotten around to making a proper investigation and report in the dotnet/runtime repo. This is most likely intended behaviour and not a bug, I don't yet know what I'm gonna do about it.

Sewer56 commented 2 years ago

Update: I don't know where or why, but the issue seems to be parts of the memory buffer used for wrappers to be overwritten on ALC collection during GC.

Sewer56 commented 2 years ago

Update: Found the issue (the only 1 I think). It's related to an optimisation I did a while back. Will take a break and most likely fix it later today.

Sewer56 commented 2 years ago

Should be fixed in latest Master. Will push an update later once I migrate CI/CD to GitHub Actions.

Sewer56 commented 2 years ago

Should be fixed. Comment or re-open this if there's still issues.

Senior-Moon-Moon commented 2 years ago

Sorry for the delayed response. The fix works great.

now to get my AssemblyLoadContext to unload properly 👍 Thanks for your help!