From 7826e7c80de028bc08c660c3afad5629e5f92da1 Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Fri, 11 Jul 2025 10:23:42 -0700 Subject: [PATCH] Tighten up some logic around file permissions Fix a regression in PbxCp which caused files to inappropriately gain the execute bit. `isExecutable` is conceptually a higher level operation which (per POSIX) can return true even if the file has no execute permission bits set: "If execute permission is requested, access shall be granted if execute permission is granted to at least one user by the file permission bits or by an alternate access control mechanism" No test because it's not clear what the concrete conditions are under which this occurs. That said, copying now checks against the owner permission bit specifically, as that's the actual intent of what the code is meaning to do and matches the original behavior. Additionally, fix some missing permission information in getFileInfo for the PseudoFS implementation and increase test coverage a bit. Avoids it coming back to bite later. rdar://154663442 --- Sources/SWBUtil/FSProxy.swift | 56 ++++++++++++++------------- Sources/SWBUtil/PbxCp.swift | 3 +- Tests/SWBUtilTests/FSProxyTests.swift | 24 +++++++++++- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/Sources/SWBUtil/FSProxy.swift b/Sources/SWBUtil/FSProxy.swift index f3d53e2c..ddd3c5a9 100644 --- a/Sources/SWBUtil/FSProxy.swift +++ b/Sources/SWBUtil/FSProxy.swift @@ -160,7 +160,11 @@ public protocol FSProxy: AnyObject, Sendable { // FIXME: Need to document behavior w.r.t. error handling. func isDirectory(_ path: Path) -> Bool - /// Checks whether the given path has the execute bit (which on Windows is determined by the file extension). + /// Checks whether the given path is executable. + /// + /// On Windows, this is determined by the file extension (based on `SHGetFileInfo`), while on Unix it's determined by `access`, which means a file may be deemed executable even if no execute bit is set in the POSIX permissions. + /// + /// - seealso: [_stat, _stat32, _stat64, _stati64, _stat32i64, _stat64i32, _wstat, _wstat32, _wstat64, _wstati64, _wstat32i64, _wstat64i32](https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions) func isExecutable(_ path: Path) throws -> Bool /// Checks whether the given path is a symlink, also returning whether the linked file exists. @@ -309,6 +313,10 @@ public extension FSProxy { func getFileSize(_ path: Path) throws -> ByteCount { try ByteCount(Int64(getFileInfo(path).size)) } + + func getFilePermissions(_ path: Path) throws -> FilePermissions { + try FilePermissions(rawValue: CModeT(getFilePermissions(path))) + } } fileprivate extension FSProxy { @@ -853,7 +861,7 @@ public class PseudoFS: FSProxy, @unchecked Sendable { } // Write the symlink. - directory.contents[path.basename] = Node(.symlink(target), permissions: 0o644, timestamp: getTimestamp(), inode: nextInode()) + directory.contents[path.basename] = Node(.symlink(target), permissions: 0o755, timestamp: getTimestamp(), inode: nextInode()) parent.timestamp = getTimestamp() } @@ -1237,35 +1245,29 @@ public class PseudoFS: FSProxy, @unchecked Sendable { public func getFileInfo(_ path: Path) throws -> FileInfo { return try queue.blocking_sync { guard let node = getNode(path) else { throw POSIXError(ENOENT) } + + let type: FileAttributeType + let size: Int switch node.contents { case .file(let contents): - let info: [FileAttributeKey: any Sendable] = [ - .modificationDate : Date(timeIntervalSince1970: TimeInterval(node.timestamp)), - .type: FileAttributeType.typeRegular, - .size: contents.bytes.count, - .posixPermissions: 0, - .systemNumber: node.device, - .systemFileNumber: node.inode] - return createFileInfo(info) + type = .typeRegular + size = contents.bytes.count case .directory(let dir): - let info: [FileAttributeKey: any Sendable] = [ - .modificationDate: Date(timeIntervalSince1970: TimeInterval(node.timestamp)), - .type: FileAttributeType.typeDirectory, - .size: dir.contents.count, - .posixPermissions: 0, - .systemNumber: node.device, - .systemFileNumber: node.inode] - return createFileInfo(info) - case .symlink(_): - let info: [FileAttributeKey: any Sendable] = [ - .modificationDate: Date(timeIntervalSince1970: TimeInterval(node.timestamp)), - .type: FileAttributeType.typeSymbolicLink, - .size: 0, - .posixPermissions: 0, - .systemNumber: node.device, - .systemFileNumber: node.inode] - return createFileInfo(info) + type = .typeDirectory + size = dir.contents.count + case .symlink(let destination): + type = .typeSymbolicLink + size = destination.str.utf8.count } + + let info: [FileAttributeKey: any Sendable] = [ + .modificationDate : Date(timeIntervalSince1970: TimeInterval(node.timestamp)), + .type: type, + .size: size, + .posixPermissions: node.permissions, + .systemNumber: node.device, + .systemFileNumber: node.inode] + return createFileInfo(info) } } diff --git a/Sources/SWBUtil/PbxCp.swift b/Sources/SWBUtil/PbxCp.swift index f94fc7b7..a8dcd536 100644 --- a/Sources/SWBUtil/PbxCp.swift +++ b/Sources/SWBUtil/PbxCp.swift @@ -298,8 +298,9 @@ fileprivate func copyRegular(_ srcPath: Path, _ srcParentPath: Path, _ dstPath: func _copyFile(_ srcPath: Path, _ dstPath: Path) throws { do { + let existingPermissions: FilePermissions = try localFS.getFilePermissions(srcPath) var permissions: FilePermissions = [.ownerRead, .ownerWrite, .groupRead, .groupWrite, .otherRead, .otherWrite] - if try localFS.isExecutable(srcPath) { + if existingPermissions.contains(.ownerExecute) { permissions.insert([.ownerExecute, .groupExecute, .otherExecute]) } let dstFd = try FileDescriptor.open(FilePath(dstPath.str), .writeOnly, options: [.create, .truncate], permissions: permissions) diff --git a/Tests/SWBUtilTests/FSProxyTests.swift b/Tests/SWBUtilTests/FSProxyTests.swift index 7a1aa90a..0389b852 100644 --- a/Tests/SWBUtilTests/FSProxyTests.swift +++ b/Tests/SWBUtilTests/FSProxyTests.swift @@ -350,11 +350,21 @@ import SWBTestSupport // Test setting file permissions. let execPath = tmpDir.join("script.sh") try localFS.write(execPath, contents: []) + #expect(try localFS.getFilePermissions(execPath) == 0o644) + #expect(try localFS.getFileInfo(execPath).permissions == 0o644) #expect(try !localFS.isExecutable(execPath)) - try localFS.setFilePermissions(execPath, permissions: 0o755) #expect(try localFS.getFilePermissions(execPath) == 0o755) + #expect(try localFS.getFileInfo(execPath).permissions == 0o755) #expect(try localFS.isExecutable(execPath)) + + let linkPath = tmpDir.join("script") + try localFS.symlink(linkPath, target: Path("script.sh")) + #expect(try localFS.getFilePermissions(linkPath) == 0o755) + #expect(try localFS.getFileInfo(linkPath).permissions == 0o755) + try localFS.setFilePermissions(linkPath, permissions: 0o644) + #expect(try localFS.getFilePermissions(linkPath) == 0o644) + #expect(try localFS.getFileInfo(linkPath).permissions == 0o644) } } @@ -804,16 +814,28 @@ import SWBTestSupport // Test default permissions. #expect(try fs.getFilePermissions(.root) == 0o755) + #expect(try fs.getFileInfo(.root).permissions == 0o755) let filePath = Path.root.join("file.txt") try fs.write(filePath, contents: []) #expect(try fs.getFilePermissions(filePath) == 0o644) + #expect(try fs.getFileInfo(filePath).permissions == 0o644) // Test setting file permissions. let execPath = Path.root.join("script.sh") try fs.write(execPath, contents: []) #expect(try fs.getFilePermissions(execPath) == 0o644) + #expect(try fs.getFileInfo(execPath).permissions == 0o644) try fs.setFilePermissions(execPath, permissions: 0o755) #expect(try fs.getFilePermissions(execPath) == 0o755) + #expect(try fs.getFileInfo(execPath).permissions == 0o755) + + let linkPath = Path.root.join("script") + try fs.symlink(linkPath, target: Path("script.sh")) + #expect(try fs.getFilePermissions(linkPath) == 0o755) + #expect(try fs.getFileInfo(linkPath).permissions == 0o755) + try fs.setFilePermissions(linkPath, permissions: 0o644) + #expect(try fs.getFilePermissions(linkPath) == 0o644) + #expect(try fs.getFileInfo(linkPath).permissions == 0o644) } @Test