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

[ethanporcaro]

Sorry if this isn't enough information, threading is not my strong suit. Please let me know.

I'll occasionally get access violation errors when using a midhook for an x64 program. This is happening when the threads are resumed.

 	ntdll.dll!NtQueryInformationThread()	Unknown
 	KernelBase.dll!GetThreadId()	Unknown
	BF2VR.dll!safetyhook::execute_while_frozen(const std::function<void __cdecl(void)> & run_fn, const std::function<void __cdecl(unsigned int,void *,_CONTEXT &)> & visit_fn) Line 139	C++
 	BF2VR.dll!safetyhook::InlineHook::e9_hook(const std::shared_ptr<safetyhook::Allocator> & allocator) Line 323	C++
 	BF2VR.dll!safetyhook::InlineHook::setup(const std::shared_ptr<safetyhook::Allocator> & allocator, unsigned char * target, unsigned char * destination) Line 191	C++
 	BF2VR.dll!safetyhook::InlineHook::create(const std::shared_ptr<safetyhook::Allocator> & allocator, void * target, void * destination) Line 147	C++
 	BF2VR.dll!safetyhook::InlineHook::create<unsigned char *,unsigned char *>(const std::shared_ptr<safetyhook::Allocator> & allocator, unsigned char * target, unsigned char * destination) Line 108	C++
 	BF2VR.dll!safetyhook::MidHook::setup(const std::shared_ptr<safetyhook::Allocator> & allocator, unsigned char * target, void(*)(safetyhook::Context64 &) destination) Line 110	C++
 	BF2VR.dll!safetyhook::MidHook::create(const std::shared_ptr<safetyhook::Allocator> & allocator, void * target, void(*)(safetyhook::Context64 &) destination) Line 56	C++
 	BF2VR.dll!safetyhook::MidHook::create(void * target, void(*)(safetyhook::Context64 &) destination) Line 48	C++
 	BF2VR.dll!safetyhook::create_mid(void * target, void(*)(safetyhook::Context64 &) destination) Line 13	C++
 	BF2VR.dll!BF2VR::BF2Service::Initialize() Line 209	C++
 	BF2VR.dll!BF2VR::MainThread(HINSTANCE__ * hModule) Line 108	C++
 	kernel32.dll!BaseThreadInitThunk()	Unknown
 	ntdll.dll!RtlUserThreadStart()	Unknown

I read up on GetThreadId and from what I understand, it shouldn't be possible to crash the program. It's showing an access violation at 0x0.

Please let me know what I need to send to help with this. Thanks.

[praydog]

Try using this PR and see if your issue goes away #51

However that is still quite strange if GetThreadId is crashing, I haven't heard of this. Would need to look at a disassembled dump of the surrounding bytes in ntdll

[praydog]

The only thing I can think of off the top of my head is that something in the TEB or PEB may be invalid for a short time, but still will need the surrounding bytes of where it crashed

[ethanporcaro]

Well that was a fun 3 hours.

Turns out GetThreadId wasn't causing the crash, it just consistently happened after a previous ResumeThread call that caused the crash.

Looking at the freezer, I see const auto suspend_count = SuspendThread(thread);. There is a check if it is -1 (error) but it just continues. I ran GetLastError() and it is returning 5 which should mean access denied. I think this is barely handled and messes up the threads when resumed.

I'll check later but I need a break.

[praydog]

One thing I kind of had an issue with with the suspender was that it just goes through the entire thread list again with the same NtGetNextThread rather than keeping a list of previously suspended handles (or thread id's and using NtOpenThread) and just iterating over those. Because any thread could be destroyed or created in the process of iterating over existing ones.

Did you try out the trapping PR?

[ethanporcaro]

Yes I did, I hit some issues and I don't quite remember exactly what it was but I'll try again.

…

On Tue, Feb 20, 2024, 7:10 PM praydog ***@***.***> wrote: One thing I kind of had an issue with with the suspender was that it just goes through the entire thread list again with the same NtGetNextThread rather than keeping a list of previously suspended handles (or thread id's and using NtOpenThread) and just iterating over those. Because any thread could be destroyed or created in the process of iterating over existing ones. Did you try out the trapping PR? — Reply to this email directly, view it on GitHub <#52 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKHIPCAIQN7CZ5VFDXJEG3DYUVJRVAVCNFSM6AAAAABBXFEZUCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJVG42DANZQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>