[GEN][ZH] Prevent AMD/ATI driver crash on game launch by front loading the system dbghelp.dll

[xezon]

• Fixes Game crashes on launch with latest AMD Drivers + dbghelp.dll #96

This change prevents the AMD/ATI driver crash on game launch by temporarily loading and unloading the Windows system dbghelp.dll, before the driver would try to load and use the dbghelp.dll from the game install directory - if the user did not delete this file.

The crash workaround is quite a bit more complicated and intricate and adds a few new classes. On a positive note, it does get rid of the legacy dbghelp.lib link dependency.

dbghelp.dll is now loaded dynamically in WWLib code. This allows to unload it. And skip loading it if it is not even used. By default it will now also first try to load dbghelp.dll from Windows system32 directory which does not cause this crash. It appears that the AMD/ATI drivers are only incompatible with this old dbghelp.dll, likely because of insufficient error handling.

The DbgHelpLoader has a specific requirement, where it is not allowed to use new/delete, because of the timing it can be activated with MEMORYPOOL_DEBUG_STACKTRACE during game memory initialization. That is why the stl::malloc_allocator was added.

The ScopedFileRenamer was added to help safely rename and revert the dbghelp.dll on game launch.

The DbgHelpGuard was added to front load the dbghelp DLL in RAII fashion at the 2 necessary places.

• Tested in VC6
• Tested in VS2022
• Tested with MEMORYPOOL_DEBUG_STACKTRACE
• Tested without MEMORYPOOL_DEBUG_STACKTRACE
• Improve implementation with Hooks
• Improve implementation with front loading dll instead of file renaming

[helmutbuhler]

Will this still work when the game doesn't have the rights to move the dll?

I didn't look into the details of this PR, but overall this looks like overkill. Given that the retail game still works when the dll is deleted, wouldn't it make more sense to simply show an errormessage when the dll is detected and instruct the user to delete it? Maybe even just rename it automatically when the game has the rights or ask for permission from the user to do that.
I don't think we need the functionality in that dll anyway. There are better ways for callstacks during crashes. We might as well delete all the code that uses it instead of adding hacks to support something we don't need.

[xezon]

When the game cannot rename this file, then it will not work. The game would need to be launched with Admin privileges if the location requires that permission. In "Program Files" it typically does.

If we want this to work for unelevated executables, then we would need to hook ntdll's LoadLibrary and then prevent loading that way. We can add hooking for Windows with https://github.com/TsudaKageyu/minhook. Then we would not need to rename the file.

[xezon]

I will try rework the deactivation of dbghelp.dll so it does not require touching the physical file.

[xezon]

Given that the retail game still works when the dll is deleted, wouldn't it make more sense to simply show an errormessage when the dll is detected and instruct the user to delete it?

A prompt is not possible in fullscreen. It is also not necessary for NVIDIA GPU's which are used by the majority of users.

Maybe even just rename it automatically when the game has the rights or ask for permission from the user to do that.

That would mean dbghelp.dll would not work on systems that do not have dbghelp.dll in system32.

I don't think we need the functionality in that dll anyway. There are better ways for callstacks during crashes. We might as well delete all the code that uses it instead of adding hacks to support something we don't need.

Maybe. It is currently used for some debugs though.

[ViTeXFTW]

How was this tested? Do you have an AMD system?

[xezon]

Yes. AMD RX 590.

[helmutbuhler]

So, if we were to remove the dependency on dbghelp.dll, but the dll would still be there, would the AMD driver still crash?

[helmutbuhler]

If we want this to work for unelevated executables, then we would need to hook ntdll's LoadLibrary and then prevent loading that way. We can add hooking for Windows with https://github.com/TsudaKageyu/minhook. Then we would not need to rename the file.

Please don't add hooks...

[xezon]

So, if we were to remove the dependency on dbghelp.dll, but the dll would still be there, would the AMD driver still crash?

Yes. Because the driver will then load that file and try to use it.

Please don't add hooks...

Hooks are harmless if implemented correctly. GenTool works like that for 15 years :)

We either hook, or rename file on runtime. Hooking will have the advantage of requiring no permission to write files in the game install directory.

[helmutbuhler]

Hooks are harmless if implemented correctly. GenTool works like that for 15 years :)

They are still a hack. And MS explicitely recommends against using them.

So does the driver crash during DX initialization? How about we call SetDllDirectory(L"C:\\Windows\\System32") before that?

[xezon]

Good suggestion. I will try that.

[xezon]

I tested a bit yesterday with SetDllDirectory and it did not help the cause.

But there is another thing we can do that likely will work: We can front load the system dbghelp before calling into the driver. That way the driver will use the system's dbghelp instead of attempting to load it from the game directory.

[xezon]

Ok. The logic has been simplified by front loading the system dbghelp.dll before the driver attempts to load it from the game directory. This works and gets rid of the file renaming code. The process needs no elevated permissions to avoid crashing. This only works if there actually is a dbghelp.dll in Windows, which we assume to be true in modern versions of Windows.

[xezon]

This can be reviewed.

[Mauller]

In what situation does this occur? i have not come across an issue with it.

[xezon]

This happens with some AMD Driver(s).

[xezon]

Rebased.

[helmutbuhler]

Sorry, but I still don't like this change. It adds way too much complexity.

I like that you removed the link dependency, but instead of adding boilerplate to dynamically load dbghelp.dll, why not remove it entirely? Has it some hidden functionality I'm not seeing? For crashreports we can simply use memorydumps, which not only have the callstack but also the memory content (and they also work for release binaries).

Regarding the AMD driver crash: Why not simply detect the vendor and the existing file in the gamedir and show a MessageBox? We can also offer to rename the file and ask for permission if needed. We can show that Messagebox before the window is created, so fullscreen is no issue.

[xezon]

Removing dbghelp loading will not help at all, because the driver will still try to load dbghelp.dll at its own discretion (generals code does not influence it). To prevent the driver from loading the wrong one, we front load it for the driver, so the driver does not have to load it anymore.

I do not like the message box from a user experience point of view. This fix is silent and the user needs to do nothing. It will just work.

dbghelp.dll is still useful for creating callstacks at runtime.

[xezon]

Rebased on main branch.