From 9428f62793696d9a0cc1f26a63f63bb31da0516d Mon Sep 17 00:00:00 2001 From: George Barnett Date: Fri, 31 May 2024 09:30:50 +0100 Subject: [PATCH] Add a fallback path if renameat2 fails (#2733) Motivation: 'renameat2' can fail with EINVAL if the RENAME_NOREPLACE flag isn't supported. However, not all kernel versions support this flag which can result in an error when creating a file 'transactionally'. Using 'renameat2' only happens on Linux as a fallback when O_TMPFILE isn't available. Modifications: - On Linux, fallback to a combination of 'stat' and 'rename' if 'renameat2' fails with EINVAL. - Add a couple of tests Result: Files can still be created exclusively --- .../Internal/SystemFileHandle.swift | 91 +++++++++++++++---- .../FileHandleTests.swift | 49 ++++++++++ 2 files changed, 124 insertions(+), 16 deletions(-) diff --git a/Sources/NIOFileSystem/Internal/SystemFileHandle.swift b/Sources/NIOFileSystem/Internal/SystemFileHandle.swift index c56a24cf7b..54496673d3 100644 --- a/Sources/NIOFileSystem/Internal/SystemFileHandle.swift +++ b/Sources/NIOFileSystem/Internal/SystemFileHandle.swift @@ -650,7 +650,7 @@ extension SystemFileHandle.SendableView { } @_spi(Testing) - public func _close(materialize: Bool) -> Result { + public func _close(materialize: Bool, failRenameat2WithEINVAL: Bool = false) -> Result { let descriptor: FileDescriptor? = self.lifecycle.withLockedValue { lifecycle in switch lifecycle { case let .open(descriptor): @@ -666,7 +666,7 @@ extension SystemFileHandle.SendableView { } // Materialize then close. - let materializeResult = self._materialize(materialize, descriptor: descriptor) + let materializeResult = self._materialize(materialize, descriptor: descriptor, failRenameat2WithEINVAL: failRenameat2WithEINVAL) return Result { try descriptor.close() @@ -778,7 +778,8 @@ extension SystemFileHandle.SendableView { func _materialize( _ materialize: Bool, - descriptor: FileDescriptor + descriptor: FileDescriptor, + failRenameat2WithEINVAL: Bool ) -> Result { guard let materialization = self.materialization else { return .success(()) } @@ -803,7 +804,7 @@ extension SystemFileHandle.SendableView { case .rename: if materialize { - let renameResult: Result + var renameResult: Result let renameFunction: String #if canImport(Darwin) renameFunction = "renamex_np" @@ -817,19 +818,76 @@ extension SystemFileHandle.SendableView { // ignored. However, they must still be provided to 'rename' in order to pass // flags. renameFunction = "renameat2" - renameResult = Syscall.rename( - from: createdPath, - relativeTo: .currentWorkingDirectory, - to: desiredPath, - relativeTo: .currentWorkingDirectory, - flags: materialization.exclusive ? [.exclusive] : [] - ) + if materialization.exclusive, failRenameat2WithEINVAL { + renameResult = .failure(.invalidArgument) + } else { + renameResult = Syscall.rename( + from: createdPath, + relativeTo: .currentWorkingDirectory, + to: desiredPath, + relativeTo: .currentWorkingDirectory, + flags: materialization.exclusive ? [.exclusive] : [] + ) + } #endif - // A file exists at the desired path and the user specified exclusive creation, - // clear up by removing the file we did create. - if materialization.exclusive, case .failure(.fileExists) = renameResult { - _ = Libc.remove(createdPath) + if materialization.exclusive { + switch renameResult { + case .failure(.fileExists): + // A file exists at the desired path and the user specified exclusive + // creation, clear up by removing the file that we did create. + _ = Libc.remove(createdPath) + + case .failure(.invalidArgument): + // If 'renameat2' failed on Linux with EINVAL then in all likelihood the + // 'RENAME_NOREPLACE' option isn't supported. As we're doing an exclusive + // create, check the desired path doesn't exist then do a regular rename. + #if canImport(Glibc) || canImport(Musl) + switch Syscall.stat(path: desiredPath) { + case .failure(.noSuchFileOrDirectory): + // File doesn't exist, do a 'regular' rename. + renameResult = Syscall.rename(from: createdPath, to: desiredPath) + + case .success: + // File exists so exclusive create isn't possible. Remove the file + // we did create then throw. + _ = Libc.remove(createdPath) + let error = FileSystemError( + code: .fileAlreadyExists, + message: """ + Couldn't open '\(desiredPath)', it already exists and the \ + file was opened with the 'existingFile' option set to 'none'. + """, + cause: nil, + location: .here() + ) + return .failure(error) + + case .failure: + // Failed to stat the desired file for reasons unknown. Remove the file + // we did create then throw. + _ = Libc.remove(createdPath) + let error = FileSystemError( + code: .unknown, + message: "Couldn't open '\(desiredPath)'.", + cause: FileSystemError.rename( + "renameat2", + errno: .invalidArgument, + oldName: createdPath, + newName: desiredPath, + location: .here() + ), + location: .here() + ) + return .failure(error) + } + #else + () // Not Linux, use the normal error flow. + #endif + + case .success, .failure: + () + } } result = renameResult.mapError { errno in @@ -1238,7 +1296,8 @@ extension SystemFileHandle { } } - static func syncOpenWithMaterialization( + @_spi(Testing) + public static func syncOpenWithMaterialization( atPath path: FilePath, mode: FileDescriptor.AccessMode, options originalOptions: FileDescriptor.OpenOptions, diff --git a/Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift b/Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift index 5531ea35cb..7356a9f12d 100644 --- a/Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift +++ b/Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift @@ -995,6 +995,55 @@ final class FileHandleTests: XCTestCase { } } + func testOpenExclusiveCreateForFileWhichExistsWithoutOTMPFILE() async throws { + // Takes the path where 'O_TMPFILE' doesn't exist, so materializing the file is done via + // creating a temporary file and then renaming it using 'renamex_np'/'renameat2' (Darwin/Linux). + let temporaryDirectory = try await FileSystem.shared.temporaryDirectory + let path = temporaryDirectory.appending(Self.temporaryFileName().components) + let handle = try SystemFileHandle.syncOpenWithMaterialization( + atPath: path, + mode: .writeOnly, + options: [.exclusiveCreate, .create], + permissions: .ownerReadWrite, + threadPool: .singleton, + useTemporaryFileIfPossible: false + ).get() + + // Closing shouldn't throw and the file should now be visible. + try await handle.close() + let info = try await FileSystem.shared.info(forFileAt: path) + XCTAssertNotNil(info) + } + + func testOpenExclusiveCreateForFileWhichExistsWithoutOTMPFILEOrRenameat2() async throws { + // Takes the path where 'O_TMPFILE' doesn't exist, so materializing the file is done via + // creating a temporary file and then renaming it using 'renameat2' and then takes a further + // fallback path where 'renameat2' returns EINVAL so the 'rename' is used in combination + // with 'stat'. This path is only reachable on Linux. + #if canImport(Glibc) || canImport(Musl) + let temporaryDirectory = try await FileSystem.shared.temporaryDirectory + let path = temporaryDirectory.appending(Self.temporaryFileName().components) + let handle = try SystemFileHandle.syncOpenWithMaterialization( + atPath: path, + mode: .writeOnly, + options: [.exclusiveCreate, .create], + permissions: .ownerReadWrite, + threadPool: .singleton, + useTemporaryFileIfPossible: false + ).get() + + // Close, but take the path where 'renameat2' fails with EINVAL. This shouldn't throw and + // the file should be available. + let result = handle.sendableView._close(materialize: true, failRenameat2WithEINVAL: true) + try result.get() + + let info = try await FileSystem.shared.info(forFileAt: path) + XCTAssertNotNil(info) + #else + throw XCTSkip("This test requires 'renameat2' which isn't supported on this platform") + #endif + } + func testOpenExclusiveCreateForFileWhichDoesNotExist() async throws { try await self.withTestDataDirectory { dir in let path = Self.temporaryFileName()