Skip to content

Fix Windows symlink handling in FileManager APIs #858

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 2 commits into from
Aug 16, 2024
Merged

Fix Windows symlink handling in FileManager APIs #858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6d30cbd
Fix Windows symlink handling in FileManager APIs
jmschonfeld Aug 15, 2024
950b75c
Address feedback
jmschonfeld Aug 15, 2024
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
23 changes: 17 additions & 6 deletions Sources/FoundationEssentials/FileManager/FileManager+Basics.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,14 +78,14 @@ internal struct _FileManagerImpl {
) -> Bool {
#if os(Windows)
return (try? path.withNTPathRepresentation {
let hLHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nil)
let hLHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, nil)
if hLHS == INVALID_HANDLE_VALUE {
return false
}
defer { CloseHandle(hLHS) }

return (try? other.withNTPathRepresentation {
let hRHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS, nil)
let hRHS = CreateFileW($0, GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, nil, OPEN_EXISTING, FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS, nil)
if hRHS == INVALID_HANDLE_VALUE {
return false
}
Expand Down Expand Up @@ -122,11 +122,21 @@ internal struct _FileManagerImpl {
return false
}

if fbiLHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT,
fbiRHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
let lhsIsReparsePoint = fbiLHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT
let rhsIsReparsePoint = fbiRHS.FileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT
let lhsIsDirectory = fbiLHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY
let rhsIsDirectory = fbiRHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY

guard lhsIsReparsePoint == rhsIsReparsePoint, lhsIsDirectory == rhsIsDirectory else {
// If they aren't the same "type", then they cannot be equivalent
return false
}

if lhsIsReparsePoint {
// Both are symbolic links, so they are equivalent if their destinations are equivalent
return (try? fileManager.destinationOfSymbolicLink(atPath: path) == fileManager.destinationOfSymbolicLink(atPath: other)) ?? false
} else if fbiLHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY,
fbiRHS.FileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
} else if lhsIsDirectory {
// Both are directories, so recursively compare the directories
guard let aLHSItems = try? fileManager.contentsOfDirectory(atPath: path),
let aRHSItems = try? fileManager.contentsOfDirectory(atPath: other),
aLHSItems == aRHSItems else {
Expand All @@ -153,6 +163,7 @@ internal struct _FileManagerImpl {

return true
} else {
// Both must be standard files, so binary compare the contents of the files
var liLHSSize: LARGE_INTEGER = .init()
var liRHSSize: LARGE_INTEGER = .init()
guard GetFileSizeEx(hLHS, &liLHSSize), GetFileSizeEx(hRHS, &liRHSSize), LARGE_INTEGER._equals(liLHSSize, liRHSSize) else {
Expand Down
15 changes: 12 additions & 3 deletions Sources/FoundationEssentials/FileManager/FileOperations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,7 +817,16 @@ enum _FileOperations {
guard delegate.shouldPerformOnItemAtPath(src, to: dst) else { return }

try dst.withNTPathRepresentation { pwszDestination in
if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
// Check for reparse points first because symlinks to directories are reported as both reparse points and directories, and we should copy the symlink not the contents of the linked directory
if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would a non-symlinked directory happen to satisfy this constraint as well, and is that the desired behavior? By reading at this code, I'm not sure if fileManager.createSymbolicLink would succeed since it's not a symlink. And if it fails, we'd throw instead of proceeding to copy the content of the directory. Is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah plain directories won't have this flag set, but other types of reparse points that aren't supported by destinationOfSymbolicLink would likely fail here - I think we can address that later since most of the FileManager code in general likely doesn't do well with other types of reparse points

do {
let linkDest = try fileManager.destinationOfSymbolicLink(atPath: src)
try fileManager.createSymbolicLink(atPath: dst, withDestinationPath: linkDest)
} catch {
try delegate.throwIfNecessary(error, src, dst)
return
}
} else if faAttributes.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY == FILE_ATTRIBUTE_DIRECTORY {
do {
try fileManager.createDirectory(atPath: dst, withIntermediateDirectories: true)
} catch {
Expand All @@ -826,10 +835,10 @@ enum _FileOperations {
for item in _Win32DirectoryContentsSequence(path: src, appendSlashForDirectory: true) {
try linkOrCopyFile(src.appendingPathComponent(item.fileName), dst: dst.appendingPathComponent(item.fileName), with: fileManager, delegate: delegate)
}
} else if bCopyFile || faAttributes.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT == FILE_ATTRIBUTE_REPARSE_POINT {
} else if bCopyFile {
var ExtendedParameters: COPYFILE2_EXTENDED_PARAMETERS = .init()
ExtendedParameters.dwSize = DWORD(MemoryLayout<COPYFILE2_EXTENDED_PARAMETERS>.size)
ExtendedParameters.dwCopyFlags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_COPY_SYMLINK | COPY_FILE_NO_BUFFERING | COPY_FILE_OPEN_AND_COPY_REPARSE_POINT
ExtendedParameters.dwCopyFlags = COPY_FILE_FAIL_IF_EXISTS | COPY_FILE_NO_BUFFERING

if FAILED(CopyFile2(pwszSource, pwszDestination, &ExtendedParameters)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @jakepetroules mentioned in the GitHub issue - it seems there may be an issue with CopyFile2 itself here. I was unable to figure out the issue, so instead we now just handle symlink logic separately by reading the symlink's destination and creating a new symlink at the copy destination (which is what swift-corelibs-foundation roughly did)

try delegate.throwIfNecessary(GetLastError(), src, dst)
Expand Down
22 changes: 22 additions & 0 deletions Tests/FoundationEssentialsTests/FileManager/FileManagerPlayground.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,22 @@ struct File : ExpressibleByStringLiteral, Buildable {
}
}

struct SymbolicLink : Buildable {
fileprivate let name: String
private let destination: String

init(_ name: String, destination: String) {
self.name = name
self.destination = destination
}

fileprivate func build(in path: String, using fileManager: FileManager) throws {
let linkPath = path.appendingPathComponent(name)
let destPath = path.appendingPathComponent(destination)
try fileManager.createSymbolicLink(atPath: linkPath, withDestinationPath: destPath)
}
}

struct Directory : Buildable {
fileprivate let name: String
private let attributes: [FileAttributeKey : Any]?
Expand All @@ -70,11 +86,13 @@ struct FileManagerPlayground {
enum Item : Buildable {
case file(File)
case directory(Directory)
case symbolicLink(SymbolicLink)

fileprivate func build(in path: String, using fileManager: FileManager) throws {
switch self {
case let .file(file): try file.build(in: path, using: fileManager)
case let .directory(dir): try dir.build(in: path, using: fileManager)
case let .symbolicLink(symlink): try symlink.build(in: path, using: fileManager)
}
}
}
Expand All @@ -92,6 +110,10 @@ struct FileManagerPlayground {
static func buildExpression(_ expression: Directory) -> Item {
.directory(expression)
}

static func buildExpression(_ expression: SymbolicLink) -> Item {
.symbolicLink(expression)
}
}

private let directory: Directory
Expand Down
47 changes: 46 additions & 1 deletion Tests/FoundationEssentialsTests/FileManager/FileManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,20 @@ final class FileManagerTests : XCTestCase {
File("Baz", contents: randomData())
}
}
Directory("symlinks") {
File("Foo", contents: randomData())
SymbolicLink("LinkToFoo", destination: "Foo")
}
Directory("EmptyDirectory") {}
"EmptyFile"
}.test {
XCTAssertTrue($0.contentsEqual(atPath: "dir1", andPath: "dir1_copy"))
XCTAssertFalse($0.contentsEqual(atPath: "dir1/dir2", andPath: "dir1/dir3"))
XCTAssertFalse($0.contentsEqual(atPath: "dir1", andPath: "dir1_diffdata"))
XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "symlinks/Foo"), "Symbolic link should not be equal to its destination")
XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "EmptyFile"), "Symbolic link should not be equal to an empty file")
XCTAssertFalse($0.contentsEqual(atPath: "symlinks/LinkToFoo", andPath: "EmptyDirectory"), "Symbolic link should not be equal to an empty directory")
XCTAssertFalse($0.contentsEqual(atPath: "symlinks/EmptyDirectory", andPath: "EmptyFile"), "Empty directory should not be equal to empty file")
}
}

Expand Down Expand Up @@ -253,21 +263,30 @@ final class FileManagerTests : XCTestCase {
"Baz"
}
}
Directory("symlinks") {
"Foo"
SymbolicLink("Bar", destination: "Foo")
SymbolicLink("Parent", destination: "..")
}
}.test {
XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1").sorted(), ["dir2", "dir2/Bar", "dir2/Foo", "dir3", "dir3/Baz"])
XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1/dir2").sorted(), ["Bar", "Foo"])
XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "dir1/dir3").sorted(), ["Baz"])

XCTAssertEqual(try $0.subpathsOfDirectory(atPath: "symlinks").sorted(), ["Bar", "Foo", "Parent"])

XCTAssertThrowsError(try $0.subpathsOfDirectory(atPath: "does_not_exist")) {
XCTAssertEqual(($0 as? CocoaError)?.code, .fileReadNoSuchFile)
}

let fullContents = ["dir1", "dir1/dir2", "dir1/dir2/Bar", "dir1/dir2/Foo", "dir1/dir3", "dir1/dir3/Baz"]
let fullContents = ["dir1", "dir1/dir2", "dir1/dir2/Bar", "dir1/dir2/Foo", "dir1/dir3", "dir1/dir3/Baz", "symlinks", "symlinks/Bar", "symlinks/Foo", "symlinks/Parent"]
let cwd = $0.currentDirectoryPath
XCTAssertNotEqual(cwd.last, "/")
let paths = [cwd, "\(cwd)/", "\(cwd)//", ".", "./", ".//"]
for path in paths {
XCTAssertEqual(try $0.subpathsOfDirectory(atPath: path).sorted(), fullContents)
}

}
}

Expand Down Expand Up @@ -345,6 +364,32 @@ final class FileManagerTests : XCTestCase {
XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("foo", "bar")])
XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [.init("foo", "bar", code: .fileWriteFileExists)])
}

try FileManagerPlayground {
"foo"
SymbolicLink("bar", destination: "foo")
}.test(captureDelegateCalls: true) {
XCTAssertTrue($0.delegateCaptures.isEmpty)
try $0.copyItem(atPath: "bar", toPath: "copy")
XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("bar", "copy")])
XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [])
let copyDestination = try $0.destinationOfSymbolicLink(atPath: "copy")
XCTAssertEqual(copyDestination.lastPathComponent, "foo", "Copied symbolic link points at \(copyDestination) instead of foo")
}

try FileManagerPlayground {
Directory("dir") {
"foo"
}
SymbolicLink("link", destination: "dir")
}.test(captureDelegateCalls: true) {
XCTAssertTrue($0.delegateCaptures.isEmpty)
try $0.copyItem(atPath: "link", toPath: "copy")
XCTAssertEqual($0.delegateCaptures.shouldCopy, [.init("link", "copy")])
XCTAssertEqual($0.delegateCaptures.shouldProceedAfterCopyError, [])
let copyDestination = try $0.destinationOfSymbolicLink(atPath: "copy")
XCTAssertEqual(copyDestination.lastPathComponent, "dir", "Copied symbolic link points at \(copyDestination) instead of foo")
}
}

func testCreateSymbolicLinkAtPath() throws {
Expand Down