Since a snapshot of all threads is captured and then traversed - threads still have time to start a new thread before they're suspended, and the new thread won't be suspended, and can thus crash due to the hooking.
You can see an example of a thread running after all threads are suspended via:
static int gInSuspend = 0;
static DWORD WINAPI ExampleThread(LPVOID lpThreadParameter)
{
if (gInSuspend)
MessageBox(0, L"OH OH", L"OH NO", MB_OK);
CreateThread(NULL, 0, ExampleThread, 0, 0, 0);
return 0;
}
// In MH_Initialize, add:
SetThreadPriority(CreateThread(NULL, 0, ExampleThread, 0, 0, 0), THREAD_PRIORITY_HIGHEST);
// After all threads are suspended, add:
gInSuspend = 1;
// Before all threads are unsuspended, add:
gInSuspend = 0;
Now do Freeze/Unfreeze in a loop - you will see the message box pop up.
The classic solution would be to do a second traverse after all threads are suspended and see if any new threads came up (if so, suspend them and do yet another traverse, until no more threads come up).
2) Slow:
The current method gets all threads in the system and can be very slow.
On my machine, it is 2 orders of magnitude (120 times) slower than any other method. (I imagine the exact amount depends on the amount of threads in the machine total)
(While I've seen https://github.com/m417z/minhook offer an approach using NtGetNextThread - it's undocumented (may change in the future) and not meaningfully faster than PssCaptureSnapshot)
3) Unnecessary in some cases:
One nice thing from https://github.com/m417z/minhook is that the freezing can be disabled entirely, when the caller knows it's not needed in their case at all.
Caveat - for whatever reason, I wasn't able to reproduce the issue in (1) with PssCaptureSnapshot (even with extra Sleeps inserted), even though it's a snapshotting solution and thus should have the exact same issue as the original approach, unless treated with the same solution.
Freezing all threads is:
1) Unsafe:
Since a snapshot of all threads is captured and then traversed - threads still have time to start a new thread before they're suspended, and the new thread won't be suspended, and can thus crash due to the hooking.
You can see an example of a thread running after all threads are suspended via:
Now do Freeze/Unfreeze in a loop - you will see the message box pop up.
The classic solution would be to do a second traverse after all threads are suspended and see if any new threads came up (if so, suspend them and do yet another traverse, until no more threads come up).
2) Slow:
The current method gets all threads in the system and can be very slow. On my machine, it is 2 orders of magnitude (120 times) slower than any other method. (I imagine the exact amount depends on the amount of threads in the machine total)
There's a very good alternative that's documented and fast -
PssCaptureSnapshot. It's only supported in Win8.1 and up, but the code can check for the function's existence and fall back on the old slow logic for old old systems. More details: https://stackoverflow.com/questions/61870414/is-there-a-fast-way-to-list-the-threads-in-the-current-windows-process(While I've seen https://github.com/m417z/minhook offer an approach using
NtGetNextThread- it's undocumented (may change in the future) and not meaningfully faster thanPssCaptureSnapshot)3) Unnecessary in some cases:
One nice thing from https://github.com/m417z/minhook is that the freezing can be disabled entirely, when the caller knows it's not needed in their case at all.
Caveat - for whatever reason, I wasn't able to reproduce the issue in (1) with
PssCaptureSnapshot(even with extra Sleeps inserted), even though it's a snapshotting solution and thus should have the exact same issue as the original approach, unless treated with the same solution.