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.

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

Enable Detours to be used without allocating or freeing memory while threads are suspended. #261

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

PDeets
Copy link
Member

@PDeets PDeets commented Oct 21, 2022

Detours uses new and delete while threads are suspended. This can cause deadlocks if the suspended thread has locked a mutex in the heap. (See issue #70.) Additionally, when AppVerifier is enabled, VirtualFree and VirtualAlloc can be hooked by AppVerifier. These hooks do memory operations on the heap internally. The deadlocks are easier to hit when AppVerifier is enabled.

Back in 2014, I worked with https://github.com/KnicKnic on a fix for this. We also consulted with https://github.com/galenh on the API design changes. I checked in the fix to a copy of Detours in the Windows repository within Microsoft, but this wasn't propagated back to the Detours master sources. I'm now bringing a cleaned-up version of the fix through GitHub so my code in the Windows repo can switch to the open-source version of Detours.

The fix makes it so all calls to free and VirtualFree are done after all calls to ResumeThread in DetourTransactionAbort and DetourTransactionCommit. However, to fully avoid deadlocks, the user needs to also do the following:

  • Switch from DetourUpdateThread to the new DetourUpdateThreadPreallocated API.
  • Make all calls to DetourAttach, DetourAttachEx, and DetourDetach before calling DetourUpdateThreadPreallocated in a given transaction.
  • Avoid allocating or freeing any memory in user code between the first call to DetourUpdateThreadPreallocated and the call to DetourTransactionAbort or DetourTransactionCommit.

If you follow those rules, you will avoid memory allocations while threads are suspended in the process.

The DetourUpdateThreadPreallocated API is like DetourUpdateThread except the user passes in a DETOUR_THREAD_DATA struct that they allocated themselves ahead of time. The user is responsible for freeing the memory after aborting or committing the transaction. In my code, I store the DETOUR_THREAD_DATA instances in a std::vector that I resize ahead of time in order to front-load the allocation before starting the detour transaction.

Microsoft Reviewers: Open in CodeFlow

src/detours.cpp Show resolved Hide resolved
@jaykrell
Copy link
Member

jaykrell commented Nov 1, 2022

Aside: For VirtualAlloc to turn around and do a heap allocation, is imho bogus. It breaks the layering of the system.

@PDeets
Copy link
Member Author

PDeets commented Nov 1, 2022

Aside: For VirtualAlloc to turn around and do a heap allocation, is imho bogus. It breaks the layering of the system.

I get that, but it is only when you opt-in by enabling AppVerifier. AppVerifier does allocations to track usage. This supports the !avrf debugger extension which can show the history of callstacks for calls to VirtualAlloc and VirtualFree. This history can be very useful for tracking down the root cause of a bug when AppVerifier catches one.

@PDeets
Copy link
Member Author

PDeets commented Nov 1, 2022

Aside: For VirtualAlloc to turn around and do a heap allocation, is imho bogus. It breaks the layering of the system.

I get that, but it is only when you opt-in by enabling AppVerifier. AppVerifier does allocations to track usage. This supports the !avrf debugger extension which can show the history of callstacks for calls to VirtualAlloc and VirtualFree. This history can be very useful for tracking down the root cause of a bug when AppVerifier catches one.

Actually, I looked over the code to fact-check this. I think I'm wrong. The AppVerifier hooks appears to avoid allocations on the process heap. It uses a different set of lower-level allocation APIs. Regardless though, the implementation acquires locks in various places. We still need to avoid VirtualAlloc and VirtualFree calls with suspended threads while AppVerifier is enabled in order to avoid deadlocks due to the various locks that are acquired in these hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants