Fix: Eliminate possible deadlock related to memory allocation on global heap

[adams85]

Addresses #70 by utilizing the HeapLock/HeapUnlock APIs as discussed in the issue.

Microsoft Reviewers: Open in CodeFlow

[sylveon]

This doesn't solve the issue. Calling HeapLock while a suspended thread has acquired a heap lock by itself will deadlock. The actual resolution would be to create our own heap (using HeapCreate) and allocate everything in that, as this isn't subject to concurrency issues with any suspended thread.

[sylveon]

Ah nevermind, I didn't notice this was in DetourTransactionBegin, before any thread could be suspended. This is good then.

[sylveon]

We should really report this if it fails, because "nobody can use the global heap anymore" seems like a pretty big problem

[sylveon]

Also, since the heap is locked after s_nPendingThreadId is set in DetourTransactionBegin, we should probably unlock the heap before allowing another thread to start a transaction, so before resetting s_nPendingThreadId here

[sylveon]

Same here

[adams85]

Thanks for the feedback. I've updated the PR.

[sonyps5201314]

HeapLock/HeapUnlock is not a perfect solution, check out the related debate in this PR.

[adams85]

@sonyps5201314 So, as I understand, the main problem with the HeapLock approach is that there is a race condition: after we have locked the process heap in DetourTransactionBegin, there could be another thread which may unlock it before we have a chance to suspend that thread in DetourUpdateThread. This seems to be a bummer indeed...

It looks like then there's no other foolproof solution than using HeapCreate and custom allocation. So I'm closing this in favor of your PR. (I suggest though you clean it up a bit by squashing commits and split it into two parts: one that addresses the deadlock issue and one that introduces DetourUpdateAllOtherThreads. So it may have a better chance to get merged.)

[sonyps5201314]

@adams85
Thank you for your actions and suggestions, because the title of my PR is "Fix if the hook is not called at the start of the process, the hook may fail", and deadlock is part of the issue covered in this issue and DetourUpdateAllOtherThreads is the solution part of the way, so I don't think they are easily separable.