microsoft / Detours

Detours is a software package for monitoring and instrumenting API calls on Windows. It is distributed in source code form.
MIT License
5.18k stars 1.01k forks source link

Add IsWow64ProcessHelper function #118

Closed temghost03ajfksdf closed 4 years ago

temghost03ajfksdf commented 4 years ago

I needed to compile Detours with VS 2005, which doesn't include IsWow64Process in its Windows SDK, so added a small wrapper function around it using GetModuleHandleW and GetProcAddress.

Used the example on MSDN as a reference.

bgianfo commented 4 years ago

Thanks for the patch @zeffy! If you wouldn't mind addressing the minor comments I left, I'll be happy the merge.

bgianfo commented 4 years ago

@dtarditi Any objections?

bgianfo commented 4 years ago

@zeffy, sorry I think you also need to rebase ontop of latest master to get the new CI build to pass.

bgianfo commented 4 years ago

Looks like this should also help with the changes that were needed to get detours to build, as mentioned in #91.

bgianfo commented 4 years ago

@zeffy, I think the rebase/merge messed up, I'm seeing commits from master included in this PR. Can you re-do the rebase to just include your changes?

temghost03ajfksdf commented 4 years ago

Yeah sorry about that, not sure how that happened. I'll fix it.

bgianfo commented 4 years ago

@jaykrell, this looks good to me, anything I'm missing here that could cause issues?

jaykrell commented 4 years ago

nits: suggest: ifdef x86 around LoadLibrary/GetProcAddress; all non-x86 supported platforms have IsWow64Process, and that mitigates the next: suggest: LoadLibrary instead of GetModuleHandle, but there are subtle arguments all around.

jaykrell commented 4 years ago

Tangenentially, we should:

  1. start testing on arm64, I bought a machine retail long ago
  2. Use IsWow64Process2
  3. Come up with a naming convention for files/directories that allow shipping more than two architectures. Obvoius suggestions: foox86.dll fooamd64.dll fooarm.dll fooarm64.dll foo.dll, ..\x86\foo.dll, ..\arm\foo.dll, ..\arm64\foo.dll, ..\amd64\foo.dll

First is more like current, I think second is in use in a patched Detours but not sure.

temghost03ajfksdf commented 4 years ago

suggest: ifdef x86 around LoadLibrary/GetProcAddress; all non-x86 supported platforms have IsWow64Process, and that mitigates

I suppose _X86_ is the preferred macro to test for this?

suggest: LoadLibrary instead of GetModuleHandle, but there are subtle arguments all around.

Agreed. Similarly, I was also considering how this might be handled when users are providing their own kernel32 implementation (in a ntdll-only scenario). Is loading K32 if it isn't already loaded by Windows guaranteed to be safe? Is this something you consider in mainline Detours?

Use IsWow64Process2

Since this is only available in Windows 10 1511 and up, would still need to have a fall back to IsWow64Process. Does the additional information provided by that function add any value in this context?

jaykrell commented 4 years ago

The arm64 OS can run arm32, arm64, and x86. IsWow64Process2 embodies that a boolean does not suffice and an enum/integer is required. I know it took 20 years to fix but it seemed obviously flawed in the first place.

I acknowedge, as you indicate, that gets you back to the LoadLibrary/GetProcAddress situation. I also acknowedge, you probably have no such machine to test on. I have one somewhere. I also acknowedge, "we" have to decide what to do about it. I also acknowedge, it can be a separate PR.

So my minimal suggestion, is yes, "X86".

#ifdef _X86_

.. the code you have ..
Maybe LoadLibrary instead.

#else

use IsWow64Process directly; no LoadLibrary / GetProcAddress

#endif
temghost03ajfksdf commented 4 years ago

I've followed your suggestions, however calling it directly, even in non-X86 where it is guaranteed to exist on a system, defeats my original purpose of the PR to let Detours compile on VS 2005.

As the function does not exist in the SDK headers (and I assume kernel32.lib as well, although I can't verify this right now), even when targeting X64.

bgianfo commented 4 years ago

Thanks @zeffy !