veeenu / hudhook

A videogame overlay framework written in Rust, supporting DirectX and OpenGL
MIT License
182 stars 26 forks source link

Add `UnregisterClass` calls to dummy windows #30

Closed Sunderous closed 1 year ago

Sunderous commented 1 year ago

Hey, first off kudos for this project, I went down the rabbit hole of DX hooking and using this lets me focus on the more fun parts of what I want to create.

I noticed that when I unload the DLL it crashes my game, and looking at your source there is functionality to unhook, but there's no logic implemeted for DLL Reason = Detach.

Would it make sense to do it that way? Have the hudhook macro generate a reason == DLL_DETACH block that just calls unhook for whatever DX version we're using?

It would make iterating on my code a lot faster than restarting the game every time haha.

Thanks!

Sunderous commented 1 year ago

Well I'm just a bit impatient I guess haha, after like 10 more minutes looking at the code I found your eject function:

https://github.com/veeenu/hudhook/blob/8afac9c480c6f243f1b4d119daef48cc1f49dda6/src/lib.rs#L175-L186

Putting it there for anyone else who might stumble across this. Might be worth adding that to the readme example so it's front and center, but all good on my end! Thanks again.

Sunderous commented 1 year ago

Sorry, going to re-open this as I think there's still a problem.

Right now, if I inject -> eject() it's all good. But if I try to re-inject the same process again it crashes the second time.

So there may still be something that the eject() cleanup isn't doing correctly, that makes installing the hook a second time crash >.<

veeenu commented 1 year ago

Hey, thank you for your feedback!

Injecting and then using the eject method are the correct thing to do. For an example of usage, check the practice tool; you want to invoke the function from the same thread that is doing the rendering, just to be sure.

If the process crashes again on second injection, I suggest you check where the crash is happening with a debugger like WinDbg or x64dbg. I suggest you do that with an empty render loop, i.e. something that just renders some static text, to rule out non-idempotent memory modifications you may have done to your target process (e.g. non-reverted injections in code caves).

If you can determine hudhook is itself the cause of crashing for your game, please gather and report here as much information as possible so I can look into it.

Thank you!

Sunderous commented 1 year ago

All good, I don't know if I have another DX11 application handy to see if I can replicate it with. But I'll do a little more analysis first, and close this for now since I can't prove if it's the app or hookhud yet. Thanks.

veeenu commented 1 year ago

All good, I don't know if I have another DX11 application handy to see if I can replicate it with.

I think it would be best if you used the one that you originally got the failure with -- so we can determine whether it has some special circumstances that cause hudhook to crash on reentry. Another DX11 application could behave differently.

I'm reopening the ticket as I'd rather keep track of a spurious issue, than forget about something that could be an actual issue. I'll close it eventually if I don't get further feedback in a long while, but in the meantime feel free to share your findings here!

Sunderous commented 1 year ago

So instead of wiring up a debugger, I grabbed some simple injection code and wired that up instead of using an off the shelf injector.

And doing it that way, I can't re-produce it, so I have to assume it was something the other injector was doing, all good in your code!

Sunderous commented 1 year ago

Gah, sorry but it still feels like there's something odd going on here, I might need some help diving deeper into it.

Right now I can re-inject as much as I want no problem.

But as soon as I change the DLL code, and re-compile, the next injection fails/crashes the host process 100% of the time. Doesn't matter what the change is. I can restart the app, inject the same DLL that just crashed it, and it'll work until I tweak the code again and re-compile. Which makes it impossible to iterate.

I'm fairly confident the crash is happening when it tries to setup the DX11 hook after being recompiled, the simple log trace just stops here:

image

And x64 dbg reports the last error as 0x582 = ERROR_CLASS_ALREADY_EXISTS:

image

I don't see that error show up at all during successful injections, so it seems unique to the crash.

But I'm still pretty new to reversing, so I'm not sure how to correlate what would change between compiles (other than binary size) that might impact re-installing the DX11 hook.

If you've got any suggestions where to poke at either in x64 debug or the code itself I can try and get some more details.

Thanks.

Sunderous commented 1 year ago

So my guess is that for some reason it's this dummy window class: https://github.com/veeenu/hudhook/blob/0ddab449018a042662cf6849e7b0403511c22304/src/hooks/dx11.rs#L302-L345

It would make sense that was getting thrown because the DUMMY class was never unregistered, but it doesn't explain why it would only happen after re-compiling. I'd expect it would happen on any injection after the first one.

Sunderous commented 1 year ago

Yep, seems to have done the trick if you ignore the traces/commented out line in my fork here: https://github.com/Sunderous/hudhook/blob/b4cad7822fd55359a46c7ba6fd63134ef6ef0eb9/src/hooks/dx11.rs#L302-L400

If I move the instance handle up to the function's scope so it's available for cleanup later, and move the dummy class name into a single spot just to be safe, then it lets me call UnregisterClassA(class_name, hinstance) after destroying the window.

This appears to have solved my issue, I can tweak the code, re-compile, and inject as much as I want.

veeenu commented 1 year ago

Oh, great catch! So, if I understood correctly, the only relevant difference is unregistering the class? This could actually be a fix for other renderers as well, that probably also aren't unregistering the dummy window class.

Sunderous commented 1 year ago

Correct. That's just how I got it to work, but as long as you have a way of getting the hInstance from the window, so you can destroy window -> unregister class with:

DestroyWindow(hWnd) UnregisterClassA(className, hInstance)

That's all that matters as far as I can tell.

I read somewhere the ordering matters, that you have to destroy first before unregistering the class. But I wasn't using the right build when I had the order reversed, or when I had some code that tries to pull the hInstance out of the window handle, so I can't say for sure it matters haha.

veeenu commented 1 year ago

Great! I've renamed the issue and will keep it open to track the necessary modification to all DX hooks. Thank you!

veeenu commented 1 year ago

Change applied to DX11 and DX12 hooks.

etra0 commented 1 year ago

Oh I'm gonna test this as well!

I noticed the same behavior, I can deinject and inject as many times I want, but as soon as I recompile it's a guaranteed crash.

I'll see if this fixed this issue!

etra0 commented 1 year ago

I did some small testing (deinjected and injected thrice) and it seems to be working better, yay!

previous behavior would simply crash at second injection after changing some bit of code.

veeenu commented 1 year ago

I've had another idea which I'm not 100% sure is going to pan out -- I'm going to try and create a swap chain straight on the desktop HWND. This could be either a disaster or a huge success, but would get rid of a lot of code which I would very much like.

veeenu commented 1 year ago

@etra0 @Sunderous turns out my idea might work after all!

Would you guys mind giving this branch a spin?

It works for me both with Dark Souls III and Elden Ring. I'm happy because I can trim away so much code 😅

etra0 commented 1 year ago

I'll give it a spin this afternoon!

etra0 commented 1 year ago

I did a quick test with for my tool for The Witcher 3 (litcher) using a branch with my changes rebased on your branch and everything seems to keep working. I tested making changes three times, and I also tested deinjecting a debug and then injecting a release version, and it works. Previously that would simply crash at the second reinjection

Sunderous commented 1 year ago

Hey, sorry I had to switch to the DX9 hook explicitly for my use case, and then tore that down to just the present hook because I was having a lot of issues trying to stabilize the repeated injection behavior (may not have been necessary, but when in doubt simplify haha).

So I probably don't have a good test case for this right now, but I'll look at the code and see if I can implement the desktop approach in my fork for dx9 and let you know how it goes.

veeenu commented 1 year ago

So I probably don't have a good test case for this right now, but I'll look at the code and see if I can implement the desktop approach in my fork for dx9 and let you know how it goes.

I applied the change to Dx9 as well, so it's all the more welcome if you can test that against Dx9 since I don't have a use case for it myself 😅

Sunderous commented 1 year ago

So I threw the new code in there for finding the function addresses with GetDesktopWindow, switched to your detour repo to make that compile again (I'm guessing detour 2 added a breaking change, or just wouldn't work with the new approach?).

And seems to inject/re-inject fine so dx9 working as far as I can tell haha.

veeenu commented 1 year ago

Thank you! 😊

New Rust nightly has a change that breaks detours-rs, I opened a PR here to fix that. Unfortunately this is blocking new releases until that itself gets a new release on crates.io.