Judge executables with API on Windows

[stevapple]

Use GetBinaryTypeW function to judge whether a file is executable on Windows.

[spevans]

This PR would need to go into the main branch first before being backported to 5.4 (assuming the swiftlang/swift#35176 PR is also backported)

[stevapple]

I see, thanks for pointing out @spevans

[compnerd]

This would break testing at the very least - .xctest is not an executable extension but is used to execute test bundles. I'm also concerned about how it treats something as being trustworthy. Do you have additional details on that?

[stevapple]

@compnerd You may check out: https://docs.microsoft.com/en-us/windows/win32/api/winsafer/nf-winsafer-saferiisexecutablefiletype

This API comes from winsafer.h, which provides a set of APIs regarding Windows Safety Policies. These policies can be managed through Secpol.msc.

[stevapple]

I think we should treat cases like xctest as exceptions here. Of course, it would be better if we can register it system-wide though some API (in the installer).

Update: Okay, I’ve found that. The list of registered executable file extensions lies in HKEY_LOCAL_MACHINE\SOFTWARE\Policies\Microsoft\Windows\safer\codeidentifiers\ExecutableTypes. However, this item will only appear if we manually enable Software Restriction Policy, so the exception way may be better.

[stevapple]

Latest update: I’ve searched through GitHub for all the related keywords, but none is used for discovering a xctest file... I think it’s totally safe to use this API, since it’s strictly aligned with system shell behavior and it works well in my environment.

The ultimate update: Managed to get identifying xctest(and many more) fixed. Now COFF executables with custom extensions will also be identified.

[stevapple]

@compnerd I've got a question now: What is this API intended for? To identify a general-meaning "executable file", or to determine whether the file can be executed through CreateProcess? If is the latter one, I think GetBinaryType is enough here.

[compnerd]

On Windows, it's largely meant for checking if the file can be executed (that is, can we just give it to CreateProcessW) as there is no executable bit on Windows. I think that using GetBinaryTypeW is a really good idea.

[stevapple]

Then I think it’s ready for merge :)

[lxbndr]

Could it be tested somehow? Maybe against TestFoundation executable itself.

[compnerd]

@lxbndr - yes, TestFileManager.test_isExecutableFile should cover that case

[compnerd]

As expected, this regresses the test suite: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=45106&view=logs&j=ec47c80d-c7cf-5f2b-7138-13cd268bea3d&t=5cfc8e20-74e6-5b9b-ff2a-9c787c40fff4

[stevapple]

@lxbndr It’s hard to test against itself since the full path of the test suite seems hard to get (CommandLine.arguments[0] is not reliable, as it can be a relative path). If is necessary, I think we should test against system executables like cmd.exe because their paths are quite the same across machines.

[spevans]

@stevapple You could try the following, it works ok on Linux so it hopefully works on Windows as well

let testFoundationBinary = try XCTUnwrap(testBundle().url(forAuxiliaryExecutable: "TestFoundation")?.path)
XCTAssertTrue(fm.isExecutableFile(atPath: testFoundationBinary)) 

It would be good if the test can be made the same on all platforms avoiding #if os(..) tests if possible. It might also be worth creating a file with a .exe extension (or modify the file that is created in the text to have that extension) and check that its not executable - assuming windows actually looks at the PE header of the file.

What does windows do for .bat files? Does it just check for the files existence?

[stevapple]

What does windows do for .bat files? Does it just check for the files existence?

For CreateProcessW, script and batch files will need to be executed through their interpreters. That is, cmd.exe for .bat, wscript.exe for .vbs, etc. To execute them, the interpreter path needs to be explicitly passed to the first parameter of CreateProcessW, where Swift will always pass NULL, so we just don’t need to consider script files now.

[spevans]

This test could still be run on Linux where it would also pass so I would suggest removing the #if os(Windows) and change the #else to #if !os(Windows) so this test is run on all platforms.

[stevapple]

This could run, but it’s meaningless. The two different tests actually reflect different behavior on POSIX and Windows systems, which I think should be parallel.

[spevans]

Ideally we want to run as many of the tests across all platforms and reduce the number of platform specific tests. In this case I would suggest adding the .exe extension to the original filename, eliminate the copy and then the 0 byte .exe file should then pass as non-executable on all platforms.

[stevapple]

I still think merging these tests would lead to confusion, and the current tests can reflect actual cases... It's not about "running as many tests across platforms". Testing with ".exe" extension makes no sense on Linux and macOS, nor does testing POSIX permissions on Windows.

[spevans]

I would simply add a comment above the file saying that it is intended to be seen as an not executable by windows even though it has a .exe extension since it lacks the appropriate header, and not executable by unix since it lacks the appropriate execute bit.

[stevapple]

Anyone to push forward this pull request?

[compnerd]

Ran a run on WIndows: https://dev.azure.com/compnerd/swift-build/_build/results?buildId=46028&view=results

[compnerd]

@swift-ci please test

[spevans]

@stevapple Please squash the commits down into one commit, thanks.