Improved 32bit detection

[frerich]

I noticed that Detours failed to inject a DLL into an IL-only .NET application. I'm pretty sure this worked in the past (right now, this is only triggered on Windows 10 machines but not on Windows 7).

These two commits resolve the issue. Instead of sticking with the approach of enumerating the modules, I went for a simpler mechanism for detecting if the running process is 32bit or 64bit. This seems to work well. See the commit logs for all the gory details.

Any feedback would be much appreciated!

[msftclas]

All CLA requirements met.

[jaykrell]

This seems like a good idea.
Enumerating address space seems bad, because 32bit cannot necessarily see much of anything in a 64bit process -- almost everything could be above 4GB.
Even if things were enumerable, the PE headers of some .dlls at least, are kinda meaningless, or hard to interpret.

Is that the problems you see? I could check.

[frerich]

@jaykrell The problem I see is that Detours incorrectly detects a 64bit process as 32bit since it merely looks at the PE headers (but without considering an optional IMAGE_COR20_HEADER header). Consequently, hooking would fail.

[jaykrell]

I think we are saying the same thing, but I wasn't clear.
Where I said .dll, really, .exe is more important.
But generally speaking, if you can enumerate the modules in a process, because of managed, you will see some seemingly random mix of x86 and amd64.

Now, if you were to ignore anything "managed" (based on the presence of the data directory), you would get much closer.

But I think you'd still be left with the problem, that a 32bit process might not be able to enumerate anything in a 64bit process -- everything might be above 4GB. It is really too bad there isn't ReadProcessMemory64, WriteProcessMemory64, VirtualAlloc64, VirtualProtect64, VirtualFree64, etc.

The way you can simulate those, roundabout, is launch a native process, and RPC to it, and have it run those functions on your behalf. But I dislike any solution involving another process.

There are sneaky ways to run native system services from a 32bit process, I've done it, but pretty unsupportable.

I'm a little confused as to the two IsWow64Process calls you are making..just because I didn't read closely,, but yeah that is a reliable simple mechanism. Except er um, for all the ARM stuff these days...that can run 3 different architectures--the boolean turned out to be insufficient. An enum is needed..

[frerich]

Yeah, even if you were to ignore any managed code, it would still be tricky to identify the target process architecture as you point out.

I agree that the approach using IsWow64Process seems a little tricky. I needed to make a little drawing to convince myself that it's accurate, too. :-) The idea is straightforward though:

First, determine if a 64bit operating system is used.

• In case the code was compiled with _WIN64 defined, I think this is trivially true. It means we are running a 64bit executable, so the OS clearly must be 64bit, too.
• In case _WIN64 is not defined (i.e. we are a 32bit executab), test if we are running under Wow64. In case we are (i.e. we are 32bit running under Wow64), it mean the OS is 64bit. In case we are not (i.e. we are 32bit, not running under Wow64), the OS must be 32bit.

Now, with this decision made (and stored in the bIs64BitOS variable), deciding whether the target process is 64bit or not should be a lot simpler:

• If it's not a 64bit OS, the target process cannot be 64bit either. I.e. it's certainly 32bit.
• If it is a 64bit OS, check if the target process runs under Wow64: if it does, it means the target process is 32bit. If it does not, it's 64bit (i.e. it runs "natively", without using Wow64).

I think this is attractive since it does not involve any file header parsing (which involves architecture-specific sizes of addresses) at all. It relies only on just one function: IsWow64Process.

[bgianfo]

Thanks for the description of the logic @frerich, would you mind encoding some of that into code comments as part of your change?

[frerich]

@bgianfo good point - I think it's evident that the logic is tricky enough to warrant a comment. Thanks for the suggestion!

I now force-pushed a new state of the branch in which comments are included which explain the two-step process for determining the bitness of the target process. What do you think?

[frerich]

Alas, the current CI builds appear to fail with

D:\a\Detours\Detours\samples\traceapi\_win32.cpp(4323): error C2065: 'GetThreadLocale': undeclared identifier
D:\a\Detours\Detours\samples\traceapi\_win32.cpp(7050): error C2065: 'SetThreadLocale': undeclared identifier

it appears this is independent of this PR though. Can anybody tell what's going on?

[bgianfo]

I think you need to rebase your changes onto latest master. That build error was fixed with the change that introduced the CI workflow (#121).

[frerich]

Thanks for the hint, @bgianfo -- that helped!

[sonyps5201314]

If official people prefer to merge my RP#127， this will no longer be a problem

[frerich]

@sonyps5201314 Your branch is really interesting, a lot of good stuff!

I had a look at how you solved the issue of identifying 32bit processes and saw that you used the same approach: first, determine if the OS is 32bit or 64bit. Then, for 64bit operating systems, test if the target process is running under Wow64 or not. I'm happy to see that we both seem to have arrived at the same solution, independently. :-)

One thing which caught my eye was that you used a fairly elaborate method to determining whether a 64bit OS is in use. I see that you defined this function:

    BOOL Is64BitOS()
    {
        BOOL bRet = FALSE;
        VOID(WINAPI * _GetNativeSystemInfo)(OUT LPSYSTEM_INFO lpSystemInfo) = (void(__stdcall *)(LPSYSTEM_INFO))GetProcAddress(GetModuleHandle(TEXT("kernel>
        if (!_GetNativeSystemInfo)
        {
            return bRet;
        }

        SYSTEM_INFO si;
        _GetNativeSystemInfo(&si);
        if (si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_AMD64 || si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM64 ||
            si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_IA64 || si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ALPHA64)
        {
            bRet = TRUE;
        }
        return bRet;
    };

I wonder -- what happens when calling this function on Windows 32bit running on a 64bit CPU. Does it actually return the architecture of the operating system, or is it rather the architecture of the CPU? I'm not totally sure whether it's possible to run 32bit Windows on a 64bit (Intel) CPU, but I believe it is.

Having second thoughts, I researched a bit more and found that the .NET libraries have a function Environment.Is64BitOperatingSystem. In the C# definition of that function, I see that it uses the same logic as what this PR uses. So maybe that's a safer approach?

[frerich]

Hmm, I tried to clean up the branch by rebasing on top of master (skipping the merge) and integrating the fix regarding IsWow64ProcessHelper. It's not immediately clear to me why Git says that there are conflicts now...'. Will do some compile testing later today.

[frerich]

I now rebased this branch on top of the current master, avoiding the merge commit and any fixups for using IsWow64ProcessHelper instead of IsWow64Process.

[dtarditi]

Looks good. Thanks!

[frerich]

Great, thanks a lot for your feedback and for integrating the patch!