Skip to content

Commit

Permalink
Add a fallback path if renameat2 fails (#2733)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
glbrntt authored May 31, 2024
1 parent e2aef20 commit 9428f62
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 16 deletions.
91 changes: 75 additions & 16 deletions Sources/NIOFileSystem/Internal/SystemFileHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,7 @@ extension SystemFileHandle.SendableView {
}

@_spi(Testing)
public func _close(materialize: Bool) -> Result<Void, FileSystemError> {
public func _close(materialize: Bool, failRenameat2WithEINVAL: Bool = false) -> Result<Void, FileSystemError> {
let descriptor: FileDescriptor? = self.lifecycle.withLockedValue { lifecycle in
switch lifecycle {
case let .open(descriptor):
Expand All @@ -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()
Expand Down Expand Up @@ -778,7 +778,8 @@ extension SystemFileHandle.SendableView {

func _materialize(
_ materialize: Bool,
descriptor: FileDescriptor
descriptor: FileDescriptor,
failRenameat2WithEINVAL: Bool
) -> Result<Void, FileSystemError> {
guard let materialization = self.materialization else { return .success(()) }

Expand All @@ -803,7 +804,7 @@ extension SystemFileHandle.SendableView {

case .rename:
if materialize {
let renameResult: Result<Void, Errno>
var renameResult: Result<Void, Errno>
let renameFunction: String
#if canImport(Darwin)
renameFunction = "renamex_np"
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down
49 changes: 49 additions & 0 deletions Tests/NIOFileSystemIntegrationTests/FileHandleTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit 9428f62

Please sign in to comment.