Skip to content

Judge executables with API on Windows #2956

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 6, 2021
Merged

Judge executables with API on Windows #2956

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6735660
Judge executables with API on Windows
stevapple Dec 22, 2020
de1daf4
Remove useless codes
stevapple Dec 22, 2020
cde744f
Fix test
stevapple Dec 23, 2020
9529986
Test `isExecutableFile` against cmd.exe
stevapple Dec 23, 2020
fbf7f34
Try to test against the bundle itself
stevapple Dec 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions Sources/Foundation/FileManager+Win32.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,9 +709,10 @@ extension FileManager {
}

internal func _isExecutableFile(atPath path: String) -> Bool {
var isDirectory: ObjCBool = false
guard fileExists(atPath: path, isDirectory: &isDirectory) else { return false }
return !isDirectory.boolValue && _isReadableFile(atPath: path)
var binaryType = DWORD(0)
return path.withCString(encodedAs: UTF16.self) {
GetBinaryTypeW($0, &binaryType)
}
}

internal func _isDeletableFile(atPath path: String) -> Bool {
Expand Down
17 changes: 12 additions & 5 deletions Tests/Foundation/Tests/TestFileManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,8 +265,10 @@ class TestFileManager : XCTestCase {
func test_isExecutableFile() {
let fm = FileManager.default
let path = NSTemporaryDirectory() + "test_isExecutableFile\(NSUUID().uuidString)"
let exePath = path + ".exe"
defer {
try? fm.removeItem(atPath: path)
try? fm.removeItem(atPath: exePath)
}

do {
Expand All @@ -275,16 +277,21 @@ class TestFileManager : XCTestCase {

// test unExecutable if file has no permissions
try fm.setAttributes([.posixPermissions : NSNumber(value: Int16(0o0000))], ofItemAtPath: path)
#if os(Windows)
// Files are always executable on Windows
XCTAssertTrue(fm.isExecutableFile(atPath: path))
#else
XCTAssertFalse(fm.isExecutableFile(atPath: path))
#endif

#if os(Windows)
// test unExecutable even if file has an `exe` extension
try fm.copyItem(atPath: path, toPath: exePath)
XCTAssertFalse(fm.isExecutableFile(atPath: exePath))
#else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// test executable if file has execute permissions
try fm.setAttributes([.posixPermissions : NSNumber(value: Int16(0o0100))], ofItemAtPath: path)
XCTAssertTrue(fm.isExecutableFile(atPath: path))
#endif

// test against the test bundle itself
let testFoundationBinary = try XCTUnwrap(testBundle().path(forAuxiliaryExecutable: "TestFoundation"))
XCTAssertTrue(fm.isExecutableFile(atPath: testFoundationBinary))
} catch let e {
XCTFail("\(e)")
}
Expand Down