Isn't there a possible deadlock issue with DetourUpdateThread and DetourTransactionCommit? about detours HOT 11 OPEN

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

from detours.

To be clear, I'm only suggesting it as a work around for common cases. The thorough new allocator approach is more thorough, and likely better for merging.

Yep, if another thread has the lock and is busy or suspended, it can't acquire the lock until that's free. Whether this is OK for a workaround depends on your use case.

HeapLock/HeapUnlock should always be in matched pairs, so being unlocked by another thread "shouldn't" happen. This would be extremely misbehaved code, but there's admittedly lots of misbehaved code out there - and I'm surprised that HeapUnlock doesn't require that HeapUnlock is called from the same thread as HeapLock.

I'd much rather see a thorough fix like the custom heap/alloc merged into detours - but until that's done, where I am able to assume there's no long-running heap locks and no cross-thread heap unlock, I prefer to minimize the amount of code that I'm using that hasn't been reviewed/merged by the project maintainers.

from detours.

Yes, it's possible. I met it.

from detours.

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

from detours.

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

from detours.

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.

from detours.

@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.

from detours.

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

from detours.

@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.

from detours.

As it turned out, the HeapLock approach isn't viable because of a nasty race condition (for more details, see #232 (comment) and #144 (comment)). It seems that the only fail-safe way to tackle this bug is HeapCreate and custom allocation as mentioned above.

from detours.

I started a PR which enables you to use Detours in a way that avoids deadlocks. See the PR description for details at #261. This doesn't solve the issue for any existing Detours user. You have to change your code that uses Detours in the way explained in the PR description.

from detours.