Fix: Drop non-pseudo handles to current thread in DetourUpdateThread

[sylveon]

Fixes #78

[sylveon]

This code doesn't builds as of now, because GetThreadId requires at least Vista. Is there a better way of determining the identity of a thread that also works pre-Vista? If not, is it acceptable to drop Windows XP support, considering it's now EoL?

[poizan42]

I think NtQueryInformationThread with ThreadBasicInformation should give you the thread id in THREAD_BASIC_INFORMATION.ClientId.UniqueThread on Windows XP.

[sylveon]

Yeah that was a solution, but THREAD_BASIC_INFORMATION is undocumented (it probably doesn't matter on XP since that is not getting any changes but I'd like to know if it's OK to use undocumented functions)

[bgianfo]

Thanks for the bug report, and patch @sylveon!

Another possible way to fix this is to duplicate the thread handle, and check the duplicated handle.
So we know we never are checking a pesudo handle.

That would obviously have a larger perf impact than the current proposed change, but you have greater portability.

[sylveon]

The problem is specifically in regards to things that aren't a pseudo handle though. Currently this is blocked on the fact that Detours needs to target Windows XP (GetThreadId was added in Vista)

[bgianfo]

The problem is specifically in regards to things that aren't a pseudo handle though.

Ah, sure. Wasn't thinking straight.

There is a possible compromise, GetThreadId is defined as:

#if (_WIN32_WINNT >= 0x0502)

WINBASEAPI
DWORD
WINAPI
GetThreadId(
    _In_ HANDLE Thread
    );

#endif // _WIN32_WINNT >= 0x0502

So the SDK seems to claim it was added in Windows Server 2003 (reference).

We could add the GetThreadId(hThread) == GetCurrentThreadId() check under #if (_WIN32_WINNT >= 0x0502), so anyone who compiles detours targeting a higher version of windows would receive the fix. In the same vein, you could GetProcAddress GetThreadId at runtime and only do the additional check if it's available.

Not amazing, but as I said, a potential compromise.

[sylveon]

I'd be ok with that but this effectively requires people to edit the nmake file, which means in most (all?) cases it won't be used.

I personally fixed the issue I was encountering another way. I was using CreateToolhelp32Snapshot to enumerate threads and simply checked if th32ThreadID was not my own TID before opening a handle to it and calling DetoursUpdateThread.

We should at least document this behavior in the wiki, saying that passing a non-pseudo handle to the current thread is unsupported and will result in freezing.

While at it, also maybe document that passing a pseudo handle is a noop? I've seen many examples do that even if it's useless.

[sonyps5201314]

after official merge my PR#127, DetourUpdateAllOtherThreads will instead of the old DetourUpdateThread API, and it will exclude currrent thread automaticall. and DetourUpdateAllOtherThreads is a safer solutuon for HOOK operation. and CreateToolhelp32Snapshot mentioned above is slowly. It is not fit for extreme environment.

[sylveon]

CreateToolhelp32Snapshot is fine, I was doing this in a DllMain so any new thread would have been blocked from running by the loader lock.

[bgianfo]

We should at least document this behavior in the wiki, saying that passing a non-pseudo handle to the current thread is
unsupported and will result in freezing.

While at it, also maybe document that passing a pseudo handle is a noop? I've seen many examples do that even if it's useless

@sylveon Good suggestion, I've added documentation for both of these.
https://github.com/microsoft/Detours/wiki/DetourUpdateThread

[sonyps5201314]

CreateToolhelp32Snapshot is fine, I was doing this in a DllMain so any new thread would have been blocked from running by the loader lock.

Not use CreateToolhelp32Snapshot is a speed problem, not a function problem. This is just for efficiency reasons.
you can understand why replace CreateToolhelp32Snapshot with ZwQuerySystemInformation in the follow patch of Mhook
apriorit/mhook@f1b41f3

[bgianfo]

I propose we go forward with this change, but just have it just fall back to the previous behavior on systems without the GetThreadId() export. I want to get this into the next release, if folks running on XP need this fix they can submit an appropriate fix for XP. It probably makes sense to factor this out into a function so this logic, and potentially future XP specific logic can be contained there.

Thoughts?

[sylveon]

I can get this working this weekend

[sylveon]

I updated the code to dynamically try loading it now.

[justanotheranonymoususer]

Not use CreateToolhelp32Snapshot is a speed problem, not a function problem. This is just for efficiency reasons.
you can understand why replace CreateToolhelp32Snapshot with ZwQuerySystemInformation in the follow patch of Mhook
apriorit/mhook@f1b41f3

@sonyps5201314 you got me interested, so I checked it. See my comment here:
apriorit/mhook@f1b41f3#commitcomment-56063743

In short, I found that both methods are almost equally slow. That's unfortunate, I wish there was a quick way to get the thread list of only a single process. The kernel can get this information, see e.g. the implementation of NtSuspendProcess, but as far as I know it doesn't expose it.