Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

adams85
Copy link

@adams85 adams85 commented Apr 10, 2022 •
edited by ghost
Loading

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

Microsoft Reviewers: Open in CodeFlow

@adams85 adams85 changed the title Eliminates possible deadlock related to memory allocation on global heap Eliminate possible deadlock related to memory allocation on global heap Apr 10, 2022
@adams85 adams85 changed the title Eliminate possible deadlock related to memory allocation on global heap Fix: Eliminate possible deadlock related to memory allocation on global heap Apr 10, 2022
@sylveon
Copy link
Contributor

sylveon commented Apr 10, 2022 •
edited
Loading

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
Copy link
Contributor

sylveon commented Apr 10, 2022

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

sylveon
sylveon reviewed Apr 10, 2022
View reviewed changes
src/detours.cpp Outdated
@@ -1648,6 +1654,9 @@ LONG WINAPI DetourTransactionAbort()
s_pPendingThreads = NULL;
s_nPendingThreadId = 0;

// There is nothing we can do if this fails.
HeapUnlock(GetProcessHeap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
sylveon reviewed Apr 10, 2022
View reviewed changes
src/detours.cpp Outdated
@@ -1942,6 +1951,9 @@ typedef ULONG_PTR DETOURS_EIP_TYPE;
*pppFailedPointer = s_ppPendingError;
}

// There is nothing we can do if this fails.
HeapUnlock(GetProcessHeap());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@adams85
Copy link
Author

adams85 commented Apr 10, 2022

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

@sonyps5201314
Copy link
Contributor

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

@adams85
Copy link
Author

adams85 commented Apr 12, 2022 •
edited
Loading

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

@adams85 adams85 closed this Apr 12, 2022
@sonyps5201314
Copy link
Contributor

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

@adams85 adams85 mentioned this pull request Apr 12, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sylveon sylveon sylveon left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adams85 @sylveon @sonyps5201314