Wow64 wrappers for all FS API calls

[malxau]

This wraps file system API calls with wrappers that disable Wow64 redirection. Prior to this change, enumeration disabled redirection, so the UI is populated with "real" file system contents, but attempting to query or manipulate those contents was still subject to redirection, creating inconsistent and unexpected results.

To test this I created a directory in \Windows\System32\test, compiled as 32 bit, and attempted to work through as many operations as I could think of.

Note there's an alternative approach of using the preprocessor to automatically remap FS APIs to use wrappers. That's not my preferred approach, but it is an option. After this change it's basically a bug to call the FS API without using these wrappers.

[craigwims]

Agree with the overall premise. Winfile already has a wrapper for many functions such as WFFindFirst. Why not extend that set of functions instead of creating another layer of wrappers.

Also, what is the purpose of WfWowInitialize? What version of Windows would this be necessary?

[schinagl]

I guess for a few functions the new layer is needed like CreateFile, for some the already existing one could be re-used, and thus making it more lean

[malxau]

The big concern I have with these changes is future code changes that don't call the wrappers, which will result in very subtle bugs that only affect specific paths. For that reason, part of me thinks this should just do something like #define GetFileAttributes WFWowGetFileAttributes in winfile.h to make doing so inexpressible.

I was hoping that following Win32 and doing just one thing with a clear pattern would make this more likely to be followed. The existing wrappers aren't used that consistently: we have one "common" function for enumeration and four callers to FindFirstFile directly; we have a WFIsDir and two call sites that call GetFileAttributes to do the same thing directly; CreateFile called from five places; DeleteFile called from three; we have an ExecProgram that calls ShellExecute but it's also called for print and ShellExecuteEx for file properties. Etc.

As for WfWowInitialize, it's needed for XP and below. That's still something my builds are targeting but I can remove it here if others think it doesn't belong. One thought I had is that this code should be under #ifndef _WIN64 or somesuch so only 32 bit builds generate API calls.

[schinagl]

Agreed. You are right with the wrappers on every function. Now I see two reasons

• We can easily introduce an #ifndef _WIN64, which I think would be nice, so that only 32bit builds are affected.
• Having PVOID pOld; and the calls to Wow64(Disable|Revert)Wow64FsRedirection everywhere bloats code and makes it unreadable.

I would keep WfWowInitialize() and related hydraulics, but have it under an #ifdef, because

• it is easier to maintain if you have one version for many plattforms
• but also separates in a way legacy coding and recent coding.

[malxau]

I don't mind using #ifdef WINVER or similar.

My 2c though is that having one piece of code is generally better than two, but we should accept two if the "old" thing becomes unduly burdensome on the 99% of people who aren't using it. With GetProcAddress, I never found the burden: the binary needs the function name either way, and needs to resolve it into a function pointer either way. The real cost is the same as the benefit, which is enabling/requiring the code to check for the presence of a function before calling it. That might be a big burden if there's two sophisticated pieces of logic following the check, but for cases like this ("call this function if it's there, otherwise do nothing") it just never seemed to make code hard to read or execute.

[craigwims]

@malxau, you mentioned "creating inconsistent and unexpected results." in the opening post. Can you provide more details on what you saw under what conditions?

[malxau]

WOW64 redirection means in normal operation, calls to \Windows\System32 are redirected to \Windows\Syswow64 when winfile is compiled as a 32 bit binary. In the current code, it disables this for enumeration, so the UI displays the "real" System32 contents in \Windows\System32 and the "real" Syswow64 contents in \Windows\Syswow64. But many operations outside of enumeration end up redirected, so if the user operates on \Windows\System32\notepad.exe winfile ends up operating on \Windows\Syswow64\notepad.exe .

For many cases, like running notepad, the user is unlikely to notice. And it's unlikely that the user would want to manipulate objects in System32. But when System32 and Syswow64 diverge, the seams become clear. You can't run ssh, because it's only in the 64 bit System32, so when redirection is applied there's nothing to redirect to. A file's size in \Windows\System32 is displayed differently in winfile compared to the file's properties, because winfile obtained it via enumeration (the 64 bit version) but is displaying properties with redirection enabled (the 32 bit version.) You can create a directory "foobar" under System32 but the creation is redirected to SysWow64 so the directory you just created doesn't exist in the UI. Etc, etc.

This PR came from a comment in #327 where it's querying file attributes to check for a directory, but this query has redirection enabled, whereas the original population had redirection disabled, so it's possible that an object doesn't exist or is a different type when queried via GetFileAttributes.

This PR attempts to ensure redirection is consistently disabled, although my biggest fear is new code changes which don't call through the wrappers. If that happens, it means that specific piece of code will issue operations against \Windows\Syswow64 when it intended to issue operations against \Windows\System32. The failure mode is impossible to predict, it just depends what the new code is adding.

Note this is all specific to 32 bit versions of Winfile. If the Winfile binary is the same architecture as the host OS, no redirection is needed or applied, so all of its calls will resolve consistently. I think that even Arm64 doesn't redirect x64 binaries, since System32 consists of CHPE binaries that are used for both.

[schinagl]

Many good arguments for one side or the other.

I guess it's not up to having 99% not use it, but on the definition of what this branch shall support:

• 32bit
• 64Bit
• Win7/Win10/win11

For 32bit we currently have a malfunction, so the changes are needed.

For 64 bit I would decide in the wrapper for wow(enabled/revert) to issue the real call via an #ifndef WIN64, because we don't know what time tag is behind the real call in the OS. At least we save this.

In both cases we have the price tag of the internal wrapper, but this can be neglected. This is O(1) so forget it.

Following my own rules from above and malxaus proposal of keeping it only in his branch, the Wowinitialze should only be available in malxaus branch.

But if it eases malxaus daily work in maintaining the w2k/wxp branch I am also fine with an #ifdef over wowinitialze.

[schinagl]

One more note on future bogus not-usage of the wrappers:

We can almost not block it. It is the same as I accidentally used MAX_PATH instead of MAXPATHLEN with some changes.

Review is the key 😉

[schinagl]

What is the common sense on this one?

[craigwims]

Of the 17 calls in question (WFWow*), 10 of them have existing "wrappers" which can be modified with the Wow redirection logic and 1 has a partial match (WFSetAttr). The other 6 would have new wrappers, which mostly would fit along side the other ones.

That would be one way to "have only one level of wrappers".

[schinagl]

@malxau: Will you change something on this issue, or is it completely stuck?
Should I come up with an alternative 'single layer' wrapper?

[craigwims]

I would prefer a solution similar to what I outlined above.