Fix: Address bug where createwith does not handle all kinds of dotnet executables

[sonyps5201314]

fix can not use createwith api to start all kinds of .net exe, like https://github.com/Microsoft/Detours/files/2918112/DotNetAppTest.zip in #54

this commit revert "Improved Detours logic for detection of 32bit processes (#104)" , because there are some bugs in that commit,
for example:
1. from detours wiki text:
Is Detours compatible with Windows 95, Windows 98, or Windows ME? No. Detours is compatible only with the Windows NT family of operating systems: Windows NT, Windows XP, and Windows Server 2003, etc. Detours does not work on the Windows 9x family of operating systems because they have a primitive virtual memory system.
we known Detours compatible with Windows 2000 and later system. but IsWow64Process is only available from Windows XP SP2,
these code:

    if (!IsWow64ProcessHelper(GetCurrentProcess(), &bIs64BitOS)) {
        return FALSE;
    }

        if (!IsWow64ProcessHelper(hProcess, &bIs32BitProcess)) {
            return FALSE;
        }

are from "Improved Detours logic for detection of 32bit processes (#104)" commit, and IsWow64ProcessHelper will return FALSE when it can not find IsWow64Process function,

    LPFN_ISWOW64PROCESS pfnIsWow64Process = (LPFN_ISWOW64PROCESS)GetProcAddress(
        hKernel32, "IsWow64Process");

    if (pfnIsWow64Process == NULL) {
        DETOUR_TRACE(("GetProcAddress failed: %d\n", GetLastError()));
        return FALSE;
    }

if IsWow64ProcessHelper return FALSE, you will return FALSE too, and DetourUpdateProcessWithDll will be failed.
so these code will make Detours no longer compatible with all windows NT family.
2. "Improved Detours logic for detection of 32bit processes (#104)" commit delete these follow codes in DetourUpdateProcessWithDll :

    BOOL bHas64BitDll = FALSE;
    BOOL bHas32BitExe = FALSE;

            if (inh.OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR32_MAGIC
                && inh.FileHeader.Machine != 0) {

                bHas32BitExe = TRUE;
            }

        else {
            if (inh.OptionalHeader.Magic == IMAGE_NT_OPTIONAL_HDR64_MAGIC
                && inh.FileHeader.Machine != 0) {

                bHas64BitDll = TRUE;
            }
        }

if uncommnet

// #define DETOUR_DEBUG 1

, it will make the follow code can not be compile success:

DETOUR_TRACE(("    32BitExe=%d 32BitProcess\n", bHas32BitExe, bIs32BitProcess));

Finally, my Detour::Is64BitProcess code Provides a more perfect solution, it have test on all different kinds of windows version from Windows 2000 to Winodws 10, include x86 and x64 architecture.

[bgianfo]

@frerich since you authored #104 you should probably review these fixes as well.

[frerich]

There's a lot of stuff going on! My understanding is that there are two proposed improvements:

1. The line DETOUR_TRACE((" 32BitExe=%d 32BitProcess\n", bHas32BitExe, bIs32BitProcess)); should be removed since commit 81e6a5f removed the bHas32BitExe variable, causing builds with DETOURS_DEBUG defined to fail.

   I concur -- in fact, the DETOUR_TRACE line seems dubious since there is just one format specifier (%d) but two arguments. Did this ever do the right thing? Assuming that debug output is useful here, an alternative fix would be to adjust (instead of removing) the statement to e.g.

   DETOUR_TRACE(("    64BitOS=%d 32BitProcess=%d\n", bIs64BitOS, bIs32BitProcess));

   ...i.e. printing the value of a different (closely related) variable.

2. The function IsWow64ProcessHelper should not be used since it returns FALSE on operating systems which are supported according to the Wiki, but which do not feature the IsWow64Process function. I don't understand this argument yet: in case an operating system does not support the IsWow64Process function, isn't it safe to assume that the system does not feature a WoW64 subsystem and thus, trivially, no process can be running under WoW64? In that case, returning FALSE seems plausible to me.

   Or is the argument that the definition for bIs64BitOS is incorrect since there are supported versions of Windows which do use 64bit, but which do not support IsWow64process? I suppose that's plausible, but I'm not aware of any such Windows version - can anybody else shed some light?

[sonyps5201314]

@frerich
1.
DETOUR_TRACE((" 32BitExe=%d 32BitProcess\n", bHas32BitExe, bIs32BitProcess)); should be a clerical error, so 32BitProcess should be change to 32BitProcess=%d to use the second variable parameter bIs32BitProcess, in my commit, it have done.
2.
you can know which systems support IsWow64Process from MSDN: https://docs.microsoft.com/en-us/windows/win32/api/wow64apiset/nf-wow64apiset-iswow64process
Minimum supported client | Windows Vista, Windows XP with SP2 [desktop apps \| UWP apps]
it is authoritative.
3. if you have look the code in the leaked Windows 2000 source code, you will known that Windows 2000 is a exampled system that support Wow64 but have not IsWow64Process function,


They may be intended to enable Ia64 to support x86 emulation and other 64bit architecture like axp64, and in systems like this
Microsoft have no publish the IsWow64Process API function.
and this facts do not accord with your imagination,

I don't understand this argument yet: in case an operating system does not support the IsWow64Process function, isn't it safe to assume that the system does not feature a WoW64 subsystem and thus, trivially, no process can be running under WoW64? I
saied by you.
4.
you can test everything by install all kinds of Windows by VMware Workstation ,to verify your hypothesis , instead of being confused.

[sonyps5201314]

@bgianfo, I have rebased to only include this commit.

[frerich]

@sonyps5201314 Thanks for elaborating!

1. From what I've gathered, the DETOUR_TRACE statement in question never worked: it used to lack a format specifier for the second argument ever since the source code of Detours 4.0 was imported November 2006. I suppose that shows how often the DETOUR_DEBUG flag is set. :-) I think fixing this makes perfect sense.

2. If I understood you right, the issue with using IsWow64Process to detect 64 bit operating systems is that there is at least one system (Windows 2000 64bit?) which does support the WoW64 subsystem but which does not feature the IsWow64Process function. Thus, if Detours is compiled for 32bit, the code might incorrectly decide that the operating system is 32bit, too.

   According to the blog post Microsoft Unveils Plans for 64-Bit Windows Platform
   , the first Windows versions which featured a 64bit architecture was Windows XP. The Wikipedia page for Windows 2000 (I fully acknowledge that this may not be an authoritative source) elaborates:

   Windows was Windows XP 64-Bit Edition, released alongside the 32-bit editions of Windows XP on October 25, 2001,[35] followed by the server versions Windows Datacenter Server Limited Edition and later Windows Advanced Server Limited Edition, which were based on the pre-release Windows Server 2003 (then known as Windows .NET Server) codebase.

   My impression is that it's not Windows 2000 which is the tricky case, but it's Windows XP prior to SP2. I suspect that there may have been installations of Windows XP 64-Bit Edition which do not have SP2 installed -- and the IsWow64Process API documentation mentions that this function is only available in SP2 or later.

   Now, with all that being said, I'm not opposed to supporting those Windows XP (or maybe even Windows 2000, as you argue) installations. However, I think it could be argued that the code, as it is, is simpler than it used to be (it no longer parses PE headers) which is a reasonable improvement in exchange for dropping support for Windows XP prior to SP 2. I'm not dogmatic about this though (we don't have to support customers using such old platforms, so the status quo is good enoug hfor me).

[frerich]

For what it's worth, in case the Detours project should continue to support Windows NT, Windows XP (at least prior to SP2), and Windows Server 2003, it would probably be worthwhile to consider setting up some CI for this since I suspect that many external contributors might not take those platforms into account.

[sonyps5201314]

DETOUR_DEBUG will be set, only when Detours can not work fine, it helps users find the wrong place as quickly as possible，It is a logging system, just like in ntdll, it can be turned on when needed and when something goes wrong.

from Windows 2000 wiki page，
Microsoft planned to release a 64-bit version of Windows 2000, which would run on 64-bit Intel Itanium microprocessors, in 2000.[33][34] However, the first officially released 64-bit version of Windows was Windows XP 64-Bit Edition, released alongside the 32-bit editions of Windows XP on October 25, 2001,[35] followed by the server versions Windows Datacenter Server Limited Edition and later Windows Advanced Server Limited Edition, which were based on the pre-release Windows Server 2003 (then known as Windows .NET Server) codebase.[36][37] These editions were released in 2002, were shortly available through the OEM channel and then were superseded by the final versions of Server 2003.[37]
we can know Microsoft planned to release a 64-bit version of Windows 2000, which would run on 64-bit Intel Itanium microprocessors, in 2000，Explain the capabilities of Wow64 that Windows 2000 already has, the first picture I posted above is also a confirmation.
However, the first officially released 64-bit version of Windows was Windows XP 64-Bit Edition, released alongside the 32-bit editions of Windows XP on October 25, 2001, Explain that the first 64-bit official release version is XP, released in 2001, then this version definitely does not have the IsWow64Process API

[sonyps5201314]

Supporting these old platforms will not increase the amount of code. I don't think it is necessary to spend too much time on this issue. The core foundation of Detours is VirtualAlloc and VirtualProtect, process independent virtual address space and can modify PAGE_EXECUTE permission. As long as these conditions can be guaranteed.
My Is64BitOS and Is64BitProcess functions have served my various programs for many years, and they have also been tested in different Windows 2000 and later official release versions of Windows systems, so I believe they are stable and universal, your new appended judgment logic of 64-bit systems and 32-bit processes does have some incomplete considerations, which is also a fact, so I think we can consider using a more reliable and comprehensive solution.

[frerich]

@sonyps5201314 It would be great if you could address the question in #104 (comment) -- what does your definition of Is64BitOS return when running 32bit Windows on a 64bit CPU?

[sonyps5201314]

For what it's worth, in case the Detours project should continue to support Windows NT, Windows XP (at least prior to SP2), and Windows Server 2003, it would probably be worthwhile to consider setting up some CI for this since I suspect that many external contributors might not take those platforms into account.

If it's worthless, official people would not let you rebase your code by IsWow64ProcessHelper

[sonyps5201314]

@sonyps5201314 It would be great if you could address the question in #104 (comment) -- what does your definition of Is64BitOS return when running 32bit Windows on a 64bit CPU?

The theoretical result should be FALSE, and the actual result is also FALSE as you see below picture.

you can test by yourself by compile below code by any vc version from 6.0 as a console app.

#include <stdio.h>
#include <windows.h>

#ifndef PROCESSOR_ARCHITECTURE_AMD64
#define PROCESSOR_ARCHITECTURE_AMD64            9
#endif

#ifndef PROCESSOR_ARCHITECTURE_ARM64
#define PROCESSOR_ARCHITECTURE_ARM64            12
#endif

BOOL Is64BitOS()
{
	BOOL bRet = FALSE;
	HMODULE hModule = GetModuleHandle(TEXT("kernel32.dll"));
	if (!hModule)
	{
		return bRet;
	}
	VOID(WINAPI * _GetNativeSystemInfo)(OUT LPSYSTEM_INFO lpSystemInfo) = (void(__stdcall*)(LPSYSTEM_INFO))GetProcAddress(hModule, "GetNativeSystemInfo");
	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;
}

int main()
{
	BOOL bIs64BitOS = Is64BitOS();
	printf("bIs64BitOS=%d", bIs64BitOS);
	printf("\n");
	getchar();

	return 0;
}

In my 64 bit Windows 8.1, it is TRUE as expected.

The reason you are puzzled is because you don’t understand the function of GetNativeSystemInfo.
According to MSDN，It only return the real hardware instruction architecture, even if the current process is running under WOW64.
And, you have another doubt, Whether 64-bit CPU can install and run 32-bit system, you may use the computer late, I can tell you for sure, my first computer is Lenovo C466A, its processor is Intel Pentium T2390, it is a x64 CPU, I installed 32-bit Windows XP on it, and used it for many years until early 2014.

[frerich]

@sonyps5201314 Thanks for the explanation. Just to clarify my understanding:

• You identified that the code currently doesn't compile anymore in case DETOUR_DEBUG is defined. This is because the line DETOUR_TRACE((" 32BitExe=%d 32BitProcess\n", bHas32BitExe, bIs32BitProcess)); -- which has always been defective ever since the code got imported into Git due to the missing formatting flag for the second argument -- references the variable bHas32BitExe which got removed in commit 81e6a5f

• You propose to improve the test for whether the operating system is 32bit or 64bit by using GetNativeSystemInfo instead of IsWow64Process. As far as I can see, the GetNativeSystemInfo function is available since Windows XP whereas IsWow64Process is available only in Windows XP SP2 and later.

Is this accurate?

If so, can you clean this PR such that only those two aspects are addressed? As it is, there seem to be a lot of unrelated changes which make it a bit hard to see what's going on, e.g. in modules.cpp:872. Also, the bHas32BitExe variable is reintroduced but it doesn't seem to be used anywhere -- except for the debug output (but then, why bother including it in debug output if the value isn't used anywhere?).

[sonyps5201314]

In the official original code (including the official final commercial version 3.0 build 343), the logical judgment of bIs32BitProcess is indeed problematic, but it has now been corrected and it is perfect. It is because of defects that the corresponding formatting parameters are not given and it is just a guess of you. If this sentence DETOUR_TRACE is meaningless, it is estimated that the official has deleted it a long time ago. Only the original developer of this code knows the reason. So I think that keeping this code is a respect for the original author, and we cannot determine the original author's intentions and expressions, so we cannot decide to remove it. Detours is great. It uses such a small code to implement x86, x64, ia64, arm, arm64 hooks. There is no similar software on the market that can support such a comprehensive and stable instruction set, but Microsoft has published it in 2002. In the future, it may also support linux/unix to complete all operating systems, expand Microsoft’s influence and contribution to the world, and win more praise. It has served various commercial projects for many years. Selling it at a high price is enough to show its technical content. This is Microsoft's official high affirmation of the value of this project. Including NVIDIA is also a customer of this software. You can find it in NVIDIA's graphics driver. Therefore, when we modify these codes, we should think twice and stay in awe, so as not to make it unstable. This is related to Microsoft's reputation.
2.
as you known,
However, the first officially released 64-bit version of Windows was Windows XP 64-Bit Edition, released alongside the 32-bit editions of Windows XP on October 25, 2001,
from https://en.wikipedia.org/wiki/Windows_2000

Windows XP 64-Bit Edition
"Windows XP 64-Bit Edition" redirects here. It is not to be confused with Windows XP Professional x64 Edition.
Windows XP 64-Bit Edition was designed to run on Intel Itanium family of microprocessors in their native IA-64 mode.

Two versions of Windows XP 64-Bit Edition were released:

Windows XP 64-Bit Edition for Itanium systems, Version 2002 – Based on Windows XP codebase, was released simultaneously alongside the 32-Bit version of Windows XP on October 25, 2001.[35]
Windows XP 64-Bit Edition, Version 2003 – Based on Windows Server 2003 codebase (which added support for the Itanium 2 processor), was released on March 28, 2003.[36]

from https://en.wikipedia.org/wiki/Windows_XP_editions#Windows_XP_64-Bit_Edition
XP IA64, released in 2001, was the first 64-bit system officially released by Microsoft, and GetNativeSystemInfo was available since XP, so does it mean that it is completely fine to use GetNativeSystemInfo to determine that the system is 64-bit.

3.my #141's purpose is 'Fix can not createwith all kinds of dot net exe', and not only 'Determine whether the process is 32-bit',
and 'Determine whether the process is 32-bit' is only a part of this task. modules.cpp:872 and so on，they are the rest of this task. If not do these, can not fix all kinds of .net app, like the app build from https://github.com/Microsoft/Detours/files/2918112/DotNetAppTest.zip in #54 , you can not understand it, if you don`t known double process debug and debug in ntdll. This involves the core issues of the PE loader and the details of the .net clr runtime，but you can understand a little bit from the comments modified in my commit.

That's all, thanks for your attention.

[frerich]

I still think that it makes sense to

1. Reduce this PR to the absolute minimum required to fix the issues described in the initial comment (i.e. making Detours compile in case DETOUR_DEBUG is defined and defining bIs64BitOS in terms of GetNativeSystemInfo instead of IsWow64Helper)

2. Split the resulting diff into two PRs, one for fixing the DETOUR_DEBUG compile issue (I suppose that PR will be a oneliner) and one for adjusting the logic for identifying 64bit OS.

[sonyps5201314]

The purpose of this PR is to fix the createwith startup problem of all types of .net programs. With only one PR, other users can understand how to fix it, and what errors are fixed. And dividing into two PRs as you said is more like a fix you submitted before, which does not meet the purpose of this PR. I also explained before that in order to achieve this goal, it is not just a modification of the code in this place. Users can also open this PR to understand the details of our discussion.

[frerich]

@bgianfo Thanks for bringing this PR to my attention! I finished reviewing this code and added some questions and concerns as comments to this PR.

[bgianfo]

Thanks @frerich for taking a look.

[bgianfo]

@sonyps5201314 / @frerich can we please seperate the revert of the #104 commit and fix so the history is clean?

The current proposed PR muddles them togeather.
I would like to get this merged and included in the next release, thank you for your patience!

[frerich]

@bgianfo Thanks for bringing this up again! It appears that I understood this PR to improve two aspects of the source code, one of which being the portability of the code. PR #104 introduced the usage of the IsWow64Helper function and assumes that systems lacking this function cannot possibly run 64bit applications. However, it appears that Windows XP prior to SP2 does support 64bit application but it does not feature IsWow64Helper. Does that seem accurate?

If so, I find it hard to come up with a tested PR since I lack access to Windows XP installations which do not have SP2 installed. I could do a blind patch at best (i.e. a patch which I never tried myself), but I'd rather not do that. :-)

@sonyps5201314 quoted the Detours wiki, which pointed out that "Detours is compatible only with the Windows NT family of operating systems: Windows NT, Windows XP, and Windows Server 2003". Is this still accurate? If so, I suspect that all contributors to the project would benefit greatly from a CI setup which makes sure that all PRs are exercised (at least built) on all supported platforms. I'm afraid that otherwise, such portability issues might creep in rather sooner than later.

[sonyps5201314]

@sonyps5201314 / @frerich can we please seperate the revert of the #104 commit and fix so the history is clean?

The current proposed PR muddles them togeather.
I would like to get this merged and included in the next release, thank you for your patience!

YES，however, the first submission of this PR of mine has deleted the relevant code of @frerich because it conflicts with mine. Therefore, there should be no confusion.

[sonyps5201314]

The merge conflict has been fixed.