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

Proposal: Introduce DetourUpdateAllOtherThreads #233

Closed

Conversation

adams85
Copy link

@adams85 adams85 commented Apr 10, 2022

Introduce a new public function which updates all threads apart from the current one in a single operation.

Microsoft Reviewers: Open in CodeFlow

@ghost
Copy link

ghost commented Apr 10, 2022

CLA assistant check
All CLA requirements met.

@adams85 adams85 changed the title Introduce DetourUpdateAllOtherThreads Proposal: Introduce DetourUpdateAllOtherThreads Apr 10, 2022
src/detours.cpp Outdated
@@ -1524,6 +1524,24 @@ struct DetourThread
{
DetourThread * pNext;
HANDLE hThread;
BOOL fCloseThreadHandleOnDestroy;

DetourThread() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Allman braces

@@ -1524,6 +1524,24 @@ struct DetourThread
{
DetourThread * pNext;
HANDLE hThread;
BOOL fCloseThreadHandleOnDestroy;
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're giving this class ownership semantics, delete the copy constructor and assignment operator.


LONG WINAPI DetourUpdateAllOtherThreads()
{
_NtGetNextThread NtGetNextThread = (_NtGetNextThread)GetProcAddress(GetModuleHandle(TEXT("ntdll.dll")), "NtGetNextThread");
Copy link
Contributor

Choose a reason for hiding this comment

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

Try avoiding undocumented APIs if possible: Windows 8.1 and up have PssCaptureSnapshot that you can dynamically query for. Using NtGetNextThread should be fine on older platforms.

return NO_ERROR;
}

if (currentThreadId != GetThreadId(hThread)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

GetThreadId was introduced in Vista but Detours supports XP. This won't compile. See #80.

@adams85 adams85 force-pushed the proposal/update_all_other_threads branch from b13666a to 4cfe39f Compare April 10, 2022 21:35
@adams85
Copy link
Author

adams85 commented Apr 10, 2022

I've addressed the first two problems. The other two (regarding the Windows APIs used) would take more time than I can spare now...

@sonyps5201314
Copy link
Contributor

sonyps5201314 commented Apr 12, 2022

@adams85 I have already introduced the DetourUpdateAllOtherThreads API in this PR a long time ago. Your code seems to be a version that modified me a long time ago. The NtGetNextThread and GetThreadId you use do not exist in XP, so your code is compatible Question, does not apply to all systems supported by detours. And my code has been repeatedly revised and tested extensively, so it is stable.

@adams85
Copy link
Author

adams85 commented Apr 12, 2022

@sonyps5201314 My code is based on yours indeed. I just wanted to create a small and easy to follow PR which has a chance to be merged. Unfortunately I missed that this breaks XP compatibility. So I'm closing this PR in favor of yours.

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

@adams85
Thanks very much!

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