From 29c687e262f08f39c66ffa48482ca8485f2a9ccc Mon Sep 17 00:00:00 2001 From: Carlos Cabanero Date: Mon, 16 Sep 2024 08:03:17 -0400 Subject: [PATCH] Fixes for overwrite mode on SFTP during copy operation - Removed flags from BlinkFiles::create. It was used as a way to pass it down to the open but it made things more confusing. - Create now uses by default the expected flags everywhere. - Additional tests on SFTP. --- Blink/Commands/ssh/CopyFiles.swift | 2 +- BlinkCode/CodeFileSystem.swift | 2 +- BlinkFiles/BlinkFiles.swift | 2 +- BlinkFiles/CopyFiles.swift | 4 +- BlinkFiles/LocalFiles.swift | 5 +-- BlinkFilesTests/LocalFilesTests.swift | 2 +- SSH/SCP.swift | 2 +- SSH/SFTP.swift | 4 +- SSHTests/SFTPTests.swift | 63 ++++++++++++++++++++++++--- 9 files changed, 67 insertions(+), 19 deletions(-) diff --git a/Blink/Commands/ssh/CopyFiles.swift b/Blink/Commands/ssh/CopyFiles.swift index 3db1fbb5f..f16adab9f 100644 --- a/Blink/Commands/ssh/CopyFiles.swift +++ b/Blink/Commands/ssh/CopyFiles.swift @@ -252,7 +252,7 @@ public class BlinkCopy: NSObject { let newFileName = (self.command.destination.filePath as NSString).lastPathComponent let parentPath = (self.command.destination.filePath as NSString).deletingLastPathComponent return d.cloneWalkTo(parentPath) - .flatMap { $0.create(name: newFileName, flags: O_WRONLY, mode: S_IRWXU) } + .flatMap { $0.create(name: newFileName, mode: S_IRWXU) } .flatMap { $0.close() } .flatMap { _ in d.cloneWalkTo(self.command.destination.filePath) } .eraseToAnyPublisher() diff --git a/BlinkCode/CodeFileSystem.swift b/BlinkCode/CodeFileSystem.swift index c42cc8d9c..6236dd66d 100644 --- a/BlinkCode/CodeFileSystem.swift +++ b/BlinkCode/CodeFileSystem.swift @@ -164,7 +164,7 @@ class CodeFileSystem { if !(options.create ?? false) { return .fail(error: CodeFileSystemError.fileNotFound(uri: self.uri)) } - return parentT.create(name: fileName, flags: O_WRONLY, mode: 0o644) + return parentT.create(name: fileName, mode: 0o644) } // 2. Write the content to the file .flatMap { file -> AnyPublisher in diff --git a/BlinkFiles/BlinkFiles.swift b/BlinkFiles/BlinkFiles.swift index e29edf130..f66237061 100644 --- a/BlinkFiles/BlinkFiles.swift +++ b/BlinkFiles/BlinkFiles.swift @@ -62,7 +62,7 @@ public protocol Translator: CopierFrom { // Creates the file name at the current walked path with the given permissions. // Default mode = S_IRWXU - func create(name: String, flags: Int32, mode: mode_t) -> AnyPublisher + func create(name: String, mode: mode_t) -> AnyPublisher // Creates the directory name at the current walked path with the given permissions. // Default mode = S_IRWUX | S_IRWXG | S_IRWXO func mkdir(name: String, mode: mode_t) -> AnyPublisher diff --git a/BlinkFiles/CopyFiles.swift b/BlinkFiles/CopyFiles.swift index 7930c3e64..589ccefe8 100644 --- a/BlinkFiles/CopyFiles.swift +++ b/BlinkFiles/CopyFiles.swift @@ -201,10 +201,10 @@ extension Translator { let fullFile: String let file: AnyPublisher - // If we are in a directory, we create the file, otherwise we open truncated. + // If we are a directory, we create the file. If we are a file, we open truncated. if self.isDirectory { fullFile = (self.current as NSString).appendingPathComponent(name) - file = self.create(name: name, flags: O_WRONLY, mode: S_IRWXU) + file = self.create(name: name, mode: S_IRWXU) } else { fullFile = self.current file = self.open(flags: O_WRONLY | O_TRUNC) diff --git a/BlinkFiles/LocalFiles.swift b/BlinkFiles/LocalFiles.swift index d663311a9..913cccbe8 100644 --- a/BlinkFiles/LocalFiles.swift +++ b/BlinkFiles/LocalFiles.swift @@ -134,8 +134,7 @@ public class Local : Translator { }.eraseToAnyPublisher() } - // // TODO Change permissions to more generic open options - public func create(name: String, flags: Int32, mode: mode_t = S_IRWXU) -> AnyPublisher { + public func create(name: String, mode: mode_t = S_IRWXU) -> AnyPublisher { if fileType != .typeDirectory { return fail(msg: "Not a directory.") } @@ -148,7 +147,7 @@ public class Local : Translator { throw LocalFileError(msg: "Could not create file.") } - return try LocalFile(at: absPath, flags: flags) + return try LocalFile(at: absPath, flags: O_WRONLY|O_CREAT|O_TRUNC) }.eraseToAnyPublisher() } diff --git a/BlinkFilesTests/LocalFilesTests.swift b/BlinkFilesTests/LocalFilesTests.swift index 149ea334a..c87b59964 100644 --- a/BlinkFilesTests/LocalFilesTests.swift +++ b/BlinkFilesTests/LocalFilesTests.swift @@ -176,7 +176,7 @@ class LocalFilesTests: XCTestCase { f.walkTo("/Users/carlos/Xcode_12.0.1.xip") .flatMap { $0.open(flags: O_RDONLY) } .flatMap { srcFile -> AnyPublisher in - return dst.create(name: "Docker-copy.dmg", flags: O_WRONLY, mode: 0o644) + return dst.create(name: "Docker-copy.dmg", mode: 0o644) .flatMap { dstFile in return (srcFile as! WriterTo).writeTo(dstFile) }.eraseToAnyPublisher() diff --git a/SSH/SCP.swift b/SSH/SCP.swift index 3730b3993..153fd1ba3 100644 --- a/SSH/SCP.swift +++ b/SSH/SCP.swift @@ -383,7 +383,7 @@ extension SCPClient { // Flow when receiving a file to copy. Create on Translator and then Write to it. func copyFileTo(translator: Translator, usingName name: String, length: UInt64, mode: mode_t) -> CopyProgressInfoPublisher { - return translator.create(name: name, flags: O_RDWR, mode: mode).flatMap { file -> CopyProgressInfoPublisher in + return translator.create(name: name, mode: mode).flatMap { file -> CopyProgressInfoPublisher in var totalWritten: UInt64 = 0 return self.writeTo(file, length: length).map { written in diff --git a/SSH/SFTP.swift b/SSH/SFTP.swift index 932a0f8b0..82460a777 100644 --- a/SSH/SFTP.swift +++ b/SSH/SFTP.swift @@ -247,7 +247,7 @@ public class SFTPTranslator: BlinkFiles.Translator { }.eraseToAnyPublisher() } - public func create(name: String, flags: Int32, mode: mode_t = S_IRWXU) -> AnyPublisher { + public func create(name: String, mode: mode_t = S_IRWXU) -> AnyPublisher { if fileType != .typeDirectory { return .fail(error: FileError(title: "Not a directory.", in: session)) } @@ -257,7 +257,7 @@ public class SFTPTranslator: BlinkFiles.Translator { defer { ssh_channel_set_blocking(self.channel, 0) } let filePath = (self.path as NSString).appendingPathComponent(name) - guard let file = sftp_open(sftp, filePath, flags | O_CREAT, mode) else { + guard let file = sftp_open(sftp, filePath, O_WRONLY|O_CREAT|O_TRUNC, mode) else { throw FileError(in: self.session) } diff --git a/SSHTests/SFTPTests.swift b/SSHTests/SFTPTests.swift index 1f1f5aca8..84b460eaf 100644 --- a/SSHTests/SFTPTests.swift +++ b/SSHTests/SFTPTests.swift @@ -133,19 +133,23 @@ class SFTPTests: XCTestCase { } func testWrite() throws { - let expectation = self.expectation(description: "Buffer Written") + let writeExpectation = self.expectation(description: "File Written") //var connection: SSHClient? - //var sftp: SFTPClient? + var root: SFTPTranslator? = nil var totalWritten = 0 let gen = RandomInputGenerator(fast: true) - let cancellable = SSHClient.dialWithTestConfig() + let cancelWrite = SSHClient.dialWithTestConfig() .flatMap() { $0.requestSFTP() } - .tryMap() { try SFTPTranslator(on: $0) } + .tryMap() { t in + let r = try SFTPTranslator(on: t) + root = r + return r + } .flatMap() { $0.walkTo("/tmp") } - .flatMap() { $0.create(name: "newfile", flags: O_WRONLY, mode: S_IRWXU) } + .flatMap() { $0.create(name: "newfile", mode: S_IRWXU) } .flatMap() { file in return gen.read(max: 5 * 1024 * 1024) .flatMap() { data in @@ -154,7 +158,7 @@ class SFTPTests: XCTestCase { }.sink(receiveCompletion: { completion in switch completion { case .finished: - expectation.fulfill() + writeExpectation.fulfill() case .failure(let error): // Problem here is we can have both SFTP and SSHError if let err = error as? SSH.FileError { @@ -169,6 +173,51 @@ class SFTPTests: XCTestCase { waitForExpectations(timeout: 15, handler: nil) XCTAssert(totalWritten == 5 * 1024 * 1024, "Did not write all data") + + totalWritten = 0 + let overwriteExpectation = self.expectation(description: "File Overwritten") + + guard let root = root else { + XCTFail("No root translator.") + return + } + + let cancelOverwrite = root.walkTo("/tmp") + .flatMap() { $0.create(name: "newfile", mode: S_IRWXU) } + .flatMap() { file in + return gen.read(max: 4 * 1024 * 1024) + .flatMap() { data in + return file.write(data, max: data.count) + } + }.sink(receiveCompletion: { completion in + switch completion { + case .finished: + overwriteExpectation.fulfill() + case .failure(let error): + // Problem here is we can have both SFTP and SSHError + if let err = error as? SSH.FileError { + XCTFail(err.description) + } else { + XCTFail("Crash") + } + } + }, receiveValue: { written in + totalWritten += written + }) + + waitForExpectations(timeout: 15, handler: nil) + XCTAssert(totalWritten == 4 * 1024 * 1024, "Did not write all data") + + let statExpectation = self.expectation(description: "File Stat") + let cancelStat = root.walkTo("/tmp/newfile") + .flatMap { (t: Translator) -> AnyPublisher in t.stat() } + .assertNoFailure() + .sink { (stats: FileAttributes) in + XCTAssertTrue(stats[.size] as! Int == 4 * 1024 * 1024) + statExpectation.fulfill() + } + + waitForExpectations(timeout: 15, handler: nil) } func testWriteToWriter() throws { @@ -190,7 +239,7 @@ class SFTPTests: XCTestCase { }.flatMap() { f -> AnyPublisher in let file = f as! SFTPFile return translator!.walkTo("/tmp/") - .flatMap { $0.create(name: "linux.tar.xz", flags: O_WRONLY, mode: S_IRWXU) } + .flatMap { $0.create(name: "linux.tar.xz", mode: S_IRWXU) } .flatMap() { file.writeTo($0) }.eraseToAnyPublisher() }.sink(receiveCompletion: { completion in switch completion {