Fix deadlock with Windows Loader during execute_while_frozen #51

praydog · 2023-12-28T17:26:25Z

I noticed a freeze when calling safetyhook::create_inline, and I assume this is where the problem came from. Either that or it was one of my many calls to execute_while_frozen.

cursey · 2023-12-29T07:07:25Z

src/thread_freezer.cpp

+    typedef NTSTATUS (WINAPI* PFN_LdrLockLoaderLock)(ULONG Flags, ULONG *State, ULONG_PTR *Cookie);
+    typedef NTSTATUS (WINAPI* PFN_LdrUnlockLoaderLock)(ULONG Flags, ULONG_PTR Cookie);
+
+    const auto ntdll = GetModuleHandleW(L"ntdll.dll");
+
+    auto lock_loader = (PFN_LdrLockLoaderLock)GetProcAddress(ntdll, "LdrLockLoaderLock");
+    auto unlock_loader = (PFN_LdrUnlockLoaderLock)GetProcAddress(ntdll, "LdrUnlockLoaderLock");
+
+    if (lock_loader != nullptr && unlock_loader != nullptr) {


I would prefer if these were just imported normally. See

safetyhook/src/thread_freezer.cpp

Lines 14 to 19 in 521ae9f

extern "C" {

NTSTATUS

NTAPI

NtGetNextThread(HANDLE ProcessHandle, HANDLE ThreadHandle, ACCESS_MASK DesiredAccess, ULONG HandleAttributes,

ULONG Flags, PHANDLE NewThreadHandle);

}

ThirteenAG · 2024-02-07T04:26:14Z

I've started using safetyhook::execute_while_frozen, and a small number of reports claims that the game process hangs right after startup, doesn't happen on every launch as far as I can tell. I'm still trying to gather more info on this, since I can't reproduce.

Another thing I wanted to ask, when I tried to hook d3d present without execute_while_frozen, it crashed inside the hooked present right away because calling original function caused a nullptr exception. Is it not possible to fill that one first, then insert a jump to a hook, eliminating the need of execute_while_frozen?

cursey · 2024-02-07T05:33:16Z

Another thing I wanted to ask, when I tried to hook d3d present without execute_while_frozen, it crashed inside the hooked present right away because calling original function caused a nullptr exception. Is it not possible to fill that one first, then insert a jump to a hook, eliminating the need of execute_while_frozen?

InlineHook already assigns the trampoline before freezing the threads. The intent behind freezing the threads is to fix the IP of any thread that may be actively executing instructions we intend on replacing in a safe manner.

Can you explain how you were using execute_while_frozen in more detail? In general I would prefer to not even expose this function publicly since you have to be very careful in what you do while using it.

ThirteenAG · 2024-02-09T03:15:46Z

InlineHook already assigns the trampoline before freezing the threads. The intent behind freezing the threads is to fix the IP of any thread that may be actively executing instructions we intend on replacing in a safe manner.

I was sent a memory dump, and apparently it's not execute_while_frozen issue, I called safetyhook::InlineHook::stdcall instead of unsafe_stdcall inside LdrLoadDll which resulted in this hang for some people:

 	ntdll.dll!_NtWaitForAlertByThreadId@8()	Unknown	Non-user code. Symbols loaded without source information.
 	ntdll.dll!RtlAcquireSRWLockExclusive()	Unknown	Non-user code. Symbols loaded without source information.
>	GTAIV.EFLC.FusionFix.asi!mtx_do_lock(_Mtx_internal_imp_t * mtx=0x6f57f290, const _timespec64 * target=0x00000000) Line 95	C++	Non-user code. Symbols loaded.
 	GTAIV.EFLC.FusionFix.asi!_Mtx_lock(_Mtx_internal_imp_t * mtx=0x6f57f290) Line 163	C++	Non-user code. Symbols loaded.
 	[Inline Frame] GTAIV.EFLC.FusionFix.asi!std::_Mutex_base::lock() Line 56	C++	Symbols loaded.
 	[Inline Frame] GTAIV.EFLC.FusionFix.asi!std::scoped_lock<std::recursive_mutex>::{ctor}(std::recursive_mutex &) Line 507	C++	Symbols loaded.
 	[Inline Frame] GTAIV.EFLC.FusionFix.asi!safetyhook::InlineHook::stdcall(wchar_t *) Line 388	C++	Symbols loaded.
 	GTAIV.EFLC.FusionFix.asi!LdrLoadDllHook(wchar_t * PathToFile=0x00000801, unsigned long * Flags=0x0019e9c8, _UNICODE_STRING * ModuleFileName=0x0019e9d8, HINSTANCE__ * * ModuleHandle=0x0019e9cc) Line 45	C++	Symbols loaded.
 	KERNELBASE.dll!LoadLibraryExW()	Unknown	Non-user code. Symbols loaded without source information.

Can you explain how you were using execute_while_frozen in more detail? In general I would prefer to not even expose this function publicly since you have to be very careful in what you do while using it.

Yes, I've replaced a code that utilized minhook with safetyhook, to inject into d3d device vtable calls:

    static inline bool bind(HMODULE module, std::type_index type_index, uint16_t func_index, void* function, void** original = nullptr)
    {
        auto target = deviceMethods.at(module).at(type_index).at(func_index);
        if (MH_CreateHook(target, function, original) != MH_OK || MH_EnableHook(target) != MH_OK) {
            return false;
        }
        return true;
    }

    static inline void bind(HMODULE module, std::type_index type_index, uint16_t func_index, void* function, SafetyHookInline& hook)
    {
        auto target = deviceMethods.at(module).at(type_index).at(func_index);
        try
        {
            safetyhook::execute_while_frozen([&]
            {
                hook = safetyhook::create_inline(target, function);
            });
        }
        catch (const std::exception& e) {}
    }

It doesn't work without execute_while_frozen, crashes.

Links to code mentioned above:
https://github.com/ThirteenAG/FusionDxHook/blob/main/includes/fusiondxhook.h#L1093-L1104
https://github.com/ThirteenAG/GTAIV.EFLC.FusionFix/blob/master/source/dllblacklist.ixx#L28-L46

cursey · 2024-02-09T23:44:46Z

    static inline void bind(HMODULE module, std::type_index type_index, uint16_t func_index, void* function, SafetyHookInline& hook)
    {
        auto target = deviceMethods.at(module).at(type_index).at(func_index);
        try
        {
            safetyhook::execute_while_frozen([&]
            {
                hook = safetyhook::create_inline(target, function);
            });
        }
        catch (const std::exception& e) {}
    }

It doesn't work without execute_while_frozen, crashes.

I'm confused by this code. You shouldn't ever have to wrap safeythook::create_inline in execute_while_frozen. It uses execute_while_frozen internally already. Regardless, I think I've got an idea on how to improve the library while removing execute_while_frozen entirely since it seems to be a point of confusion and miss use. I'll have a PR up soon with my idea if it works out.

angelfor3v3r · 2024-02-10T00:25:04Z

execute_while_frozen is really easy to misuse. I don't think it was ever suggested to be used from the public API, and the inline hooks will already do it for you. There's even a note here, which warns against it:

safetyhook/include/safetyhook/thread_freezer.hpp

Line 20 in 3103d4b

    
           /// @note Keep the logic inside run_fn and visit_fn as simple as possible to avoid deadlocks.

Unless you absolutely know you have to use it, then you shouldn't use it.

@ThirteenAG I guess we're more curious to why exactly you need to use it here. Could you elaborate on this or provide more details? Are you sure you need to use execute_while_frozen? I understand you said it crashes, but is it possible it's something else? Any more information really helps, thanks!

ThirteenAG · 2024-02-10T02:57:46Z

You shouldn't ever have to wrap safeythook::create_inline in execute_while_frozen. It uses execute_while_frozen internally already.

Yes, but it didn't work for me with Max Payne 3 d3d hooking and resulted in a crash. It doesn't happen with execute_while_frozen.

 	00000000()	Unknown	No symbols loaded.
 	[Frames below may be incorrect and/or missing]		Annotated Frame
>	[Inline Frame] MaxPayne3.XboxRainDroplets.asi!safetyhook::InlineHook::unsafe_stdcall(IDXGISwapChain *) Line 482	C++	Symbols loaded.
 	[Inline Frame] MaxPayne3.XboxRainDroplets.asi!FusionDxHook::HookD3D10_1::__l2::<lambda_2>::()::__l11::<lambda_1>::operator()(IDXGISwapChain *) Line 640	C++	Symbols loaded.
 	MaxPayne3.XboxRainDroplets.asi!``FusionDxHook::HookD3D10_1'::`2'::<lambda_2>::operator()'::`11'::<lambda_1>::<lambda_invoker_stdcall>(IDXGISwapChain * pSwapChain, unsigned int SyncInterval=0x00000000, unsigned int Flags=0x00000000) Line 680	C++	Symbols loaded.
 	MaxPayne3.exe!00f0f222()	Unknown	Non-user code. Cannot find or open the PDB file.

Crash on this line: https://github.com/ThirteenAG/FusionDxHook/blob/main/includes/fusiondxhook.h#L640
But do note I'm using dx11 renderer in the game, not 10.1. Also worked with minhook implementation.

That's literally all I have to do to trigger the crash immediately (commented lines):

    static inline void bind(HMODULE module, std::type_index type_index, uint16_t func_index, void* function, SafetyHookInline& hook)
    {
        auto target = deviceMethods.at(module).at(type_index).at(func_index);
        try
        {
            //safetyhook::execute_while_frozen([&]
            //{
                hook = safetyhook::create_inline(target, function);
            //});
        }
        catch (const std::exception& e) {}
    }

As I've said, works inside execute_while_frozen, no problems. I'd assume due to create_inline hooking, but hook is not assigned yet and crashes?

cursey · 2024-02-10T09:14:32Z

@ThirteenAG can you try this quick test PR I threw together? #63

You'll need to remove execute_while_frozen calls from your code to test it.

ThirteenAG · 2024-02-10T09:51:43Z

@ThirteenAG can you try this quick test PR I threw together? #63

You'll need to remove execute_while_frozen calls from your code to test it.

Tested GTAIV/Max Payne 3 plugins, and dx hook tests with all uses of execute_while_frozen removed, works fine.

angelfor3v3r · 2024-02-10T17:56:57Z

@ThirteenAG can you try this quick test PR I threw together? #63
You'll need to remove execute_while_frozen calls from your code to test it.

Tested GTAIV/Max Payne 3 plugins, and dx hook tests with all uses of execute_while_frozen removed, works fine.

Thanks for the extra information, it really helps. And thanks for testing it. I think this change is better in the long term since it's technically more portable too.

cursey · 2024-05-15T06:00:17Z

Closing this since I landed thread trapping.

praydog added 3 commits December 28, 2023 09:22

Fix deadlock with Windows Loader during execute_while_frozen

3d5b242

Also unlock the loader for the visit_fn

37d2810

NULL->nullptr

ebc0d28

cursey requested changes Dec 29, 2023

View reviewed changes

praydog mentioned this pull request Jan 11, 2024

ResumeThread causes crash when unfreezing, possibly due to unhandled SuspendThread error #52

Open

cursey closed this May 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix deadlock with Windows Loader during execute_while_frozen #51

Fix deadlock with Windows Loader during execute_while_frozen #51

praydog commented Dec 28, 2023 •

edited

Loading

cursey Dec 29, 2023

ThirteenAG commented Feb 7, 2024

cursey commented Feb 7, 2024 •

edited

Loading

ThirteenAG commented Feb 9, 2024 •

edited

Loading

cursey commented Feb 9, 2024 •

edited

Loading

angelfor3v3r commented Feb 10, 2024 •

edited

Loading

ThirteenAG commented Feb 10, 2024 •

edited

Loading

cursey commented Feb 10, 2024

ThirteenAG commented Feb 10, 2024

angelfor3v3r commented Feb 10, 2024

cursey commented May 15, 2024

	extern "C" {
	NTSTATUS
	NTAPI
	NtGetNextThread(HANDLE ProcessHandle, HANDLE ThreadHandle, ACCESS_MASK DesiredAccess, ULONG HandleAttributes,
	ULONG Flags, PHANDLE NewThreadHandle);
	}

Fix deadlock with Windows Loader during execute_while_frozen #51

Fix deadlock with Windows Loader during execute_while_frozen #51

Conversation

praydog commented Dec 28, 2023 • edited Loading

cursey Dec 29, 2023

Choose a reason for hiding this comment

ThirteenAG commented Feb 7, 2024

cursey commented Feb 7, 2024 • edited Loading

ThirteenAG commented Feb 9, 2024 • edited Loading

cursey commented Feb 9, 2024 • edited Loading

angelfor3v3r commented Feb 10, 2024 • edited Loading

ThirteenAG commented Feb 10, 2024 • edited Loading

cursey commented Feb 10, 2024

ThirteenAG commented Feb 10, 2024

angelfor3v3r commented Feb 10, 2024

cursey commented May 15, 2024

praydog commented Dec 28, 2023 •

edited

Loading

cursey commented Feb 7, 2024 •

edited

Loading

ThirteenAG commented Feb 9, 2024 •

edited

Loading

cursey commented Feb 9, 2024 •

edited

Loading

angelfor3v3r commented Feb 10, 2024 •

edited

Loading

ThirteenAG commented Feb 10, 2024 •

edited

Loading