Freeze Unfreeze inside of EnableHookLL/EnableAllHooksLL causes crash

[mallgrab]

Certain multithreaded games have tendencies to crash if you were to create and destroy hooks.
It seems like minhook does not handle the case where SuspendThread returns "Access violation", which leads into a crash when ResumeThread then interacts with the thread.

The bug is similar to this issue from SafetyHook
cursey/safetyhook#52 (comment)

Commenting out SuspendThread and ResumeThread fixes the crashes but I don't know how well that will work when ProcessThreadIPs interacts with the context of the thread, where it expects the thread to be sleeping.

[FransBouma]

Thanks for the heads up! I had crashes appear when I started hooking a multithreaded piece of code and couldn't figure out why.

I've altered Freeze/Unfreeze to anticipate on whether a thread could be suspended. This isn't 100% failsafe but I think nothing really is.

//-------------------------------------------------------------------------
static VOID Freeze(PFROZEN_THREADS pThreads, UINT pos, UINT action)
{
    pThreads->pItems   = NULL;
    pThreads->capacity = 0;
    pThreads->size     = 0;
    EnumerateThreads(pThreads);

    if (pThreads->pItems != NULL)
    {
        for (UINT i = 0; i < pThreads->size; ++i)
        {
            HANDLE hThread = OpenThread(THREAD_ACCESS, FALSE, pThreads->pItems[i]);
            if (hThread != NULL)
            {
                DWORD result = SuspendThread(hThread);
                if(result == 0xFFFFFFFF)
                {
                    // mark thread as not suspended, so it's not resumed later on.
                    pThreads->pItems[i] = 0;
                }
                else
                {
                    ProcessThreadIPs(hThread, pos, action);
                }
                CloseHandle(hThread);
            }
        }
    }
}

//-------------------------------------------------------------------------
static VOID Unfreeze(PFROZEN_THREADS pThreads)
{
    if (pThreads->pItems != NULL)
    {
        for (UINT i = 0; i < pThreads->size; ++i)
        {
            DWORD threadId = pThreads->pItems[i];
            if(threadId > 0)
            {
                HANDLE hThread = OpenThread(THREAD_ACCESS, FALSE, threadId);
                if(hThread != NULL)
                {
                    ResumeThread(hThread);
                    CloseHandle(hThread);
                }
            }
        }

        HeapFree(g_hHeap, 0, pThreads->pItems);
    }
}

So it basically comes down to: if SuspendThread reports an error, mark the thread as not suspended so unfreeze will skip it for the resume loop.

[FransBouma]

@m417z is this project still active, as in: would you consider a PR for this change? It's not a failsafe fix but it mitigates a 100% guaranteed crash

[m417z]

Yes, looks great, go ahead, I'll merge it.

[FransBouma]

Right after you posted this, I ran into a crash again, so this isn't sufficient either. :( It mitigates it, but it's not a solution to the crashes one will run into. I'll mention this with the PR, so you can close it if you think it gives the wrong impression that it is fixed

[m417z]

Too bad, but I think that a check for whether SuspendThread succeeds is an improvement in any case.

[FransBouma]

Too bad, but I think that a check for whether SuspendThread succeeds is an improvement in any case.

yeah it definitely mitigates it a lot. Will send you a PR tomorrow.

Another area of improvement perhaps is the way EnableHookLL writes the code bytes: after this line: https://github.com/TsudaKageyu/minhook/blob/master/src/hook.c#L403 there's a race condition. A CPU can write 8 bytes atomically, so it might be better to write this out as a 64bit value in 1 go (e.g. using WriteProcessMemory or memcpy). If a thread isn't frozen it can still go wrong if the E8 byte has been written but not the operand. Of course, if all threads are frozen there's no race condition, but as there might be threads still running, that's not a given

[m417z]

Do you have a crash dump? Does the crash happen at the patched instructions?

A CPU can write 8 bytes atomically

Yes, but only if it's an aligned write. Also, it won't help if the CPU is executing one of the patched instructions, regardless of whether the write is atomic.

[FransBouma]

Do you have a crash dump? Does the crash happen at the patched instructions?

I sadly don't have a crash dump, and it only happened once yesterday in maybe 20-30 starts of my test game, so it's very hard to track down. (Unreal Engine 4.27 compiled small level with a couple of characters which starts in 2 seconds).

A CPU can write 8 bytes atomically

Yes, but only if it's an aligned write. Also, it won't help if the CPU is executing one of the patched instructions, regardless of whether the write is atomic.

That's a very good point, didn't know that! So there's no need to pursue that then :) 👍

[m417z]

it's very hard to track down

You can enable crash dumps, and then just grab a crash the next time it happens.

• Open regedit
• Go to: HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\Windows Error Reporting
• Create a key named LocalDumps
• Create a DWORD value named DumpType with value 2
• Wait for the crash
• Go to the %LocalAppData%\CrashDumps folder, you should see a dump file in there

[FransBouma]

it's very hard to track down

You can enable crash dumps, and then just grab a crash the next time it happens.

* Open regedit

* Go to: `HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows\Windows Error Reporting`

* Create a key named `LocalDumps`

* Create a DWORD value named `DumpType` with value `2`

* Wait for the crash

* Go to the `%LocalAppData%\CrashDumps` folder, you should see a dump file in there

Thanks will try that. The UE game I am using for testing does dump its own crashdump at times but these crashes apparently don't make it that far, the dump isn't written.

I've added a Sleep(150) after the Freeze in EnableHookLL now and I have had 0 crashes even after 20+ starts, but I'll add the key just in case next time it crashes :) 👍