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.

[adams85]

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

[fredemmott]

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.

[PDeets]

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.