Isn't there a possible deadlock issue with DetourUpdateThread and DetourTransactionCommit?

[AlphaModder]

I notice that both DetourUpdateThread and DetourTransactionCommit use new and delete in their code. Assuming these eventually translate to calls to GlobalAlloc and GlobalFree, isn't it possible for deadlock to occur if a thread previously suspended by DetourUpdateThread currently holds a lock on the default heap? This could be avoided by having Detours allocate its own heap with CreateHeap and using that heap for all allocations that occur during a transaction.

[sonyps5201314]

Yes, it's possible. I met it.

[fredemmott]

I'm fairly reliably hitting this, and @adams85's patch mentioned above fixes it for me (apply with --ignore-whitespace to get a readable diff) - however, in debug builds, there's another deadlock in debug builds, as DETOUR_TRACE calls printf

[sonyps5201314]

please use PR #144 to fix your rest problems like DETOUR_TRACE you said.

I'm fairly reliably hitting this, and @adams85's patch mentioned above fixes it for me (apply with --ignore-whitespace to get a readable diff) - however, in debug builds, there's another deadlock in debug builds, as DETOUR_TRACE calls printf

[fredemmott]

While that might be a more thorough fix, I'm not comfortable diverging that far from master, or familiar enough with Detours to review #144 even for my own use. The smaller patch - and sticking to release builds of Detours - is a better solution for me.

[fredemmott]

@sonyps5201314 fwiw, while I don't work on Detours or at Microsoft, it's generally best to create pull requests that address a single bug/issue at a time, keeping them as small as possible - this makes them much easier to review, and usually leads to them getting merged by the project maintainers much faster.

[fredemmott]

Patch-free workaround:

• call HeapLock(GetProcessHeap()) before DetourTransactionBegin()
• call HeapFree(GetProcessHeap()) after DetourTransactionCommit()/DetourTransactionAbort()

Edit: turns out this is suggested at https://devblogs.microsoft.com/oldnewthing/20170125-00/?p=95255

[sonyps5201314]

@fredemmott，Please read the latest reply in PR #144 to answer your doubts.

[adams85]

@fredemmott Thanks for pointing out HeapLock/HeapUnlock. This is much simpler, so I've updated my fork to use this. I'll even submit this as a PR, let's see how the maintainers like it.