From eafe322ceff0ecd9915cfa7960241b2797243f2b Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 16:28:24 -0400 Subject: [PATCH 1/6] Disable inheritance of file descriptors created by Swift Testing by default. This PR makes file descriptors created by Swift Testing `FD_CLOEXEC` by default (on Windows, `~HANDLE_FLAG_INHERIT`.) We then clear `FD_CLOEXEC` for specific file descriptors that should be inherited. On Darwin, this is effectively ignored because we use `POSIX_SPAWN_CLOEXEC_DEFAULT`, and on Windows we use `PROC_THREAD_ATTRIBUTE_HANDLE_LIST` which has much the same effect. (On non-Darwin POSIX platforms, there's no reliable way to ensure only one child process inherits a particular file descriptor.) This change is speculative. Resolves #1140. --- Sources/Testing/Attachments/Attachment.swift | 4 +- Sources/Testing/ExitTests/ExitTest.swift | 4 + Sources/Testing/Support/FileHandle.swift | 94 +++++++++++++++++++- 3 files changed, 96 insertions(+), 6 deletions(-) diff --git a/Sources/Testing/Attachments/Attachment.swift b/Sources/Testing/Attachments/Attachment.swift index 366e288d1..e9b98fb7b 100644 --- a/Sources/Testing/Attachments/Attachment.swift +++ b/Sources/Testing/Attachments/Attachment.swift @@ -462,7 +462,7 @@ extension Attachment where AttachableValue: ~Copyable { // file exists at this path (note "x" in the mode string), an error will // be thrown and we'll try again by adding a suffix. let preferredPath = appendPathComponent(preferredName, to: directoryPath) - file = try FileHandle(atPath: preferredPath, mode: "wxb") + file = try FileHandle(atPath: preferredPath, mode: "wxeb") result = preferredPath } catch { // Split the extension(s) off the preferred name. The first component in @@ -478,7 +478,7 @@ extension Attachment where AttachableValue: ~Copyable { // Propagate any error *except* EEXIST, which would indicate that the // name was already in use (so we should try again with a new suffix.) do { - file = try FileHandle(atPath: preferredPath, mode: "wxb") + file = try FileHandle(atPath: preferredPath, mode: "wxeb") result = preferredPath break } catch let error as CError where error.rawValue == swt_EEXIST() { diff --git a/Sources/Testing/ExitTests/ExitTest.swift b/Sources/Testing/ExitTests/ExitTest.swift index beda3eb4e..e4df9fbda 100644 --- a/Sources/Testing/ExitTests/ExitTest.swift +++ b/Sources/Testing/ExitTests/ExitTest.swift @@ -779,17 +779,21 @@ extension ExitTest { _ = setvbuf(stderr, nil, _IONBF, Int(BUFSIZ)) } } + try stdoutWriteEnd?.setInherited(true) + try stderrWriteEnd?.setInherited(true) // Create a "back channel" pipe to handle events from the child process. var backChannelReadEnd: FileHandle! var backChannelWriteEnd: FileHandle! try FileHandle.makePipe(readEnd: &backChannelReadEnd, writeEnd: &backChannelWriteEnd) + try backChannelWriteEnd.setInherited(true) // Create another pipe to send captured values (and possibly other state // in the future) to the child process. var capturedValuesReadEnd: FileHandle! var capturedValuesWriteEnd: FileHandle! try FileHandle.makePipe(readEnd: &capturedValuesReadEnd, writeEnd: &capturedValuesWriteEnd) + try capturedValuesReadEnd.setInherited(true) // Let the child process know how to find the back channel and // captured values channel by setting a known environment variable to diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index 2a2bfe967..9ab538d10 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -73,6 +73,13 @@ struct FileHandle: ~Copyable, Sendable { return } + // On Windows, "N" is used rather than "e" to signify that a file handle is + // not inherited. + var mode = mode + if let eIndex = mode.firstIndex(of: "e") { + mode.replaceSubrange(eIndex ... eIndex, with: "N") + } + // Windows deprecates fopen() as insecure, so call _wfopen_s() instead. let fileHandle = try path.withCString(encodedAs: UTF16.self) { path in try mode.withCString(encodedAs: UTF16.self) { mode in @@ -98,8 +105,13 @@ struct FileHandle: ~Copyable, Sendable { /// - path: The path to read from. /// /// - Throws: Any error preventing the stream from being opened. + /// + /// By default, the resulting file handle is not inherited by any child + /// processes (that is, `FD_CLOEXEC` is set on POSIX-like systems and + /// `HANDLE_FLAG_INHERIT` is cleared on Windows.) To make it inheritable, call + /// ``setInherited()``. init(forReadingAtPath path: String) throws { - try self.init(atPath: path, mode: "rb") + try self.init(atPath: path, mode: "reb") } /// Initialize an instance of this type to write to the given path. @@ -108,8 +120,13 @@ struct FileHandle: ~Copyable, Sendable { /// - path: The path to write to. /// /// - Throws: Any error preventing the stream from being opened. + /// + /// By default, the resulting file handle is not inherited by any child + /// processes (that is, `FD_CLOEXEC` is set on POSIX-like systems and + /// `HANDLE_FLAG_INHERIT` is cleared on Windows.) To make it inheritable, call + /// ``setInherited()``. init(forWritingAtPath path: String) throws { - try self.init(atPath: path, mode: "wb") + try self.init(atPath: path, mode: "web") } /// Initialize an instance of this type with an existing C file handle. @@ -461,16 +478,27 @@ extension FileHandle { /// - Bug: This function should return a tuple containing the file handles /// instead of returning them via `inout` arguments. Swift does not support /// tuples with move-only elements. ([104669935](rdar://104669935)) + /// + /// By default, the resulting file handles are not inherited by any child + /// processes (that is, `FD_CLOEXEC` is set on POSIX-like systems and + /// `HANDLE_FLAG_INHERIT` is cleared on Windows.) To make them inheritable, + /// call ``setInherited()``. static func makePipe(readEnd: inout FileHandle?, writeEnd: inout FileHandle?) throws { var (fdReadEnd, fdWriteEnd) = try withUnsafeTemporaryAllocation(of: CInt.self, capacity: 2) { fds in #if os(Windows) - guard 0 == _pipe(fds.baseAddress, 0, _O_BINARY) else { + guard 0 == _pipe(fds.baseAddress, 0, _O_BINARY | _O_NOINHERIT) else { throw CError(rawValue: swt_errno()) } -#else +#elseif SWT_TARGET_OS_APPLE + // Apple platforms do not currently implement pipe2(), so simulate it to + // the best of our ability. guard 0 == pipe(fds.baseAddress!) else { throw CError(rawValue: swt_errno()) } +#else + guard 0 == pipe2(fds.baseAddress!, O_CLOEXEC) else { + throw CError(rawValue: swt_errno()) + } #endif return (fds[0], fds[1]) } @@ -488,6 +516,13 @@ extension FileHandle { fdWriteEnd = -1 } try writeEnd = FileHandle(unsafePOSIXFileDescriptor: fdWriteEnd, mode: "wb") + +#if SWT_TARGET_OS_APPLE + // Apple platforms do not currently implement pipe2(), so simulate it to + // the best of our ability. + try readEnd?.setInherited(false) + try writeEnd?.setInherited(false) +#endif } catch { // Don't leak file handles! Ensure we've cleared both pointers before // returning so the state is consistent in the caller. @@ -553,6 +588,57 @@ extension FileHandle { #endif } #endif + + /// Set whether or not this file handle is inherited by child processes. + /// + /// - Parameters: + /// - inherited: Whether or not this file handle is inherited by child + /// processes (ignoring overriding functionality such as Apple's + /// `POSIX_SPAWN_CLOEXEC_DEFAULT` flag.) + /// + /// - Throws: Any error that occurred while setting the flag. + func setInherited(_ inherited: Bool) throws { +#if SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android) + try withUnsafePOSIXFileDescriptor { fd in + guard let fd else { + throw SystemError(description: "Cannot set whether a file handle is inherited unless it is backed by a file descriptor. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") + } + try withLock { + switch fcntl(fd, F_GETFD) { + case -1: + // An error occurred reading the flags for this file descriptor. + throw CError(rawValue: swt_errno()) + case let oldValue: + let newValue = if inherited { + oldValue & ~FD_CLOEXEC + } else { + oldValue | FD_CLOEXEC + } + if oldValue == newValue { + // No need to make a second syscall as nothing has changed. + return + } + if -1 == fcntl(fd, F_SETFD, newValue) { + // An error occurred setting the flags for this file descriptor. + throw CError(rawValue: swt_errno()) + } + } + } + } +#elseif os(Windows) + return withUnsafeWindowsHANDLE { handle in + guard let handle else { + throw SystemError(description: "Cannot set whether a file handle is inherited unless it is backed by a Windows file handle. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") + } + let newValue = inherited ? DWORD(HANDLE_FLAG_INHERIT) : 0 + guard SetHandleInformation(handle, DWORD(HANDLE_FLAG_INHERIT), newValue) else { + throw Win32Error(rawValue: GetLastError()) + } + } +#else +#warning("Platform-specific implementation missing: cannot set whether a file handle is inherited") +#endif + } } // MARK: - General path utilities From d5cd0f2e903c5191657c974f521284693f286336 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 16:57:48 -0400 Subject: [PATCH 2/6] Work around fcntl() and pipe2() being inconsistently available with glibc --- Sources/Testing/Support/FileHandle.swift | 98 +++++++++++++++-------- Sources/_TestingInternals/include/Stubs.h | 19 +++++ 2 files changed, 83 insertions(+), 34 deletions(-) diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index 9ab538d10..bd2625847 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -462,6 +462,17 @@ extension FileHandle { #if !SWT_NO_PIPES // MARK: - Pipes +#if !SWT_TARGET_OS_APPLE && !os(Windows) && !SWT_NO_DYNAMIC_LINKING +/// Create a pipe with flags. +/// +/// This function declaration is provided because `pipe2()` is only declared if +/// `_GNU_SOURCE` is set, but setting it causes build errors due to conflicts +/// with Swift's Glibc module. +private let _pipe2 = symbol(named: "pipe2").map { + castCFunction(at: $0, to: (@convention(c) (UnsafeMutablePointer, CInt) -> CInt).self) +} +#endif + extension FileHandle { /// Make a pipe connecting two new file handles. /// @@ -489,15 +500,26 @@ extension FileHandle { guard 0 == _pipe(fds.baseAddress, 0, _O_BINARY | _O_NOINHERIT) else { throw CError(rawValue: swt_errno()) } -#elseif SWT_TARGET_OS_APPLE - // Apple platforms do not currently implement pipe2(), so simulate it to - // the best of our ability. - guard 0 == pipe(fds.baseAddress!) else { - throw CError(rawValue: swt_errno()) - } #else - guard 0 == pipe2(fds.baseAddress!, O_CLOEXEC) else { - throw CError(rawValue: swt_errno()) + var pipe2Called = false +#if !SWT_TARGET_OS_APPLE && !os(Windows) && !SWT_NO_DYNAMIC_LINKING + if let _pipe2 { + guard 0 == _pipe2(fds.baseAddress!, O_CLOEXEC) else { + throw CError(rawValue: swt_errno()) + } + pipe2Called = true + } +#endif + + if !pipe2Called { + // pipe2() is not available. Use pipe() instead and simulate O_CLOEXEC + // to the best of our ability. + guard 0 == pipe(fds.baseAddress!) else { + throw CError(rawValue: swt_errno()) + } + for fd in fds { + try? setFileDescriptorInherited(fd, false) + } } #endif return (fds[0], fds[1]) @@ -516,13 +538,6 @@ extension FileHandle { fdWriteEnd = -1 } try writeEnd = FileHandle(unsafePOSIXFileDescriptor: fdWriteEnd, mode: "wb") - -#if SWT_TARGET_OS_APPLE - // Apple platforms do not currently implement pipe2(), so simulate it to - // the best of our ability. - try readEnd?.setInherited(false) - try writeEnd?.setInherited(false) -#endif } catch { // Don't leak file handles! Ensure we've cleared both pointers before // returning so the state is consistent in the caller. @@ -589,6 +604,39 @@ extension FileHandle { } #endif +#if SWT_TARGET_OS_APPLE || os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android) + /// Set whether or not the given file descriptor is inherited by child processes. + /// + /// - Parameters: + /// - fd: The file descriptor. + /// - inherited: Whether or not `fd` is inherited by child processes + /// (ignoring overriding functionality such as Apple's + /// `POSIX_SPAWN_CLOEXEC_DEFAULT` flag.) + /// + /// - Throws: Any error that occurred while setting the flag. + static func setFileDescriptorInherited(_ fd: CInt, _ inherited: Bool) throws { + switch swt_getfdflags(fd) { + case -1: + // An error occurred reading the flags for this file descriptor. + throw CError(rawValue: swt_errno()) + case let oldValue: + let newValue = if inherited { + oldValue & ~FD_CLOEXEC + } else { + oldValue | FD_CLOEXEC + } + if oldValue == newValue { + // No need to make a second syscall as nothing has changed. + return + } + if -1 == swt_setfdflags(fd, newValue) { + // An error occurred setting the flags for this file descriptor. + throw CError(rawValue: swt_errno()) + } + } + } +#endif + /// Set whether or not this file handle is inherited by child processes. /// /// - Parameters: @@ -604,25 +652,7 @@ extension FileHandle { throw SystemError(description: "Cannot set whether a file handle is inherited unless it is backed by a file descriptor. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") } try withLock { - switch fcntl(fd, F_GETFD) { - case -1: - // An error occurred reading the flags for this file descriptor. - throw CError(rawValue: swt_errno()) - case let oldValue: - let newValue = if inherited { - oldValue & ~FD_CLOEXEC - } else { - oldValue | FD_CLOEXEC - } - if oldValue == newValue { - // No need to make a second syscall as nothing has changed. - return - } - if -1 == fcntl(fd, F_SETFD, newValue) { - // An error occurred setting the flags for this file descriptor. - throw CError(rawValue: swt_errno()) - } - } + try Self.setFileDescriptorInherited(fd, inherited) } } #elseif os(Windows) diff --git a/Sources/_TestingInternals/include/Stubs.h b/Sources/_TestingInternals/include/Stubs.h index 8093a3722..acf0e1966 100644 --- a/Sources/_TestingInternals/include/Stubs.h +++ b/Sources/_TestingInternals/include/Stubs.h @@ -151,6 +151,25 @@ static int swt_EEXIST(void) { return EEXIST; } +#if __has_include() || __has_include() +/// Call `fcntl(F_GETFD)`. +/// +/// This function is provided because `fcntl()` is a variadic function and +/// cannot be imported directly into Swift. +static int swt_getfdflags(int fd) { + return fcntl(fd, F_GETFD); +} + +/// Call `fcntl(F_SETFD)`. +/// +/// This function is provided because `fcntl()` is a variadic function and +/// cannot be imported directly into Swift. +static int swt_setfdflags(int fd, int flags) { + return fcntl(fd, F_SETFD, flags); +} +#endif + + SWT_ASSUME_NONNULL_END #endif From 12accaa0f547cf9a0f306991e2ed1b4279d85ba0 Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 16:58:50 -0400 Subject: [PATCH 3/6] Fix missing try on Windows --- Sources/Testing/Support/FileHandle.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index bd2625847..8ad4e450d 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -656,7 +656,7 @@ extension FileHandle { } } #elseif os(Windows) - return withUnsafeWindowsHANDLE { handle in + return try withUnsafeWindowsHANDLE { handle in guard let handle else { throw SystemError(description: "Cannot set whether a file handle is inherited unless it is backed by a Windows file handle. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") } From cdc073717943a0f447b03455c37505beef4b2ebf Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 17:02:15 -0400 Subject: [PATCH 4/6] Don't bother clearing FD_CLOEXEC on Darwin when spawning a child process --- Sources/Testing/ExitTests/ExitTest.swift | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Sources/Testing/ExitTests/ExitTest.swift b/Sources/Testing/ExitTests/ExitTest.swift index e4df9fbda..9284310db 100644 --- a/Sources/Testing/ExitTests/ExitTest.swift +++ b/Sources/Testing/ExitTests/ExitTest.swift @@ -779,21 +779,17 @@ extension ExitTest { _ = setvbuf(stderr, nil, _IONBF, Int(BUFSIZ)) } } - try stdoutWriteEnd?.setInherited(true) - try stderrWriteEnd?.setInherited(true) // Create a "back channel" pipe to handle events from the child process. var backChannelReadEnd: FileHandle! var backChannelWriteEnd: FileHandle! try FileHandle.makePipe(readEnd: &backChannelReadEnd, writeEnd: &backChannelWriteEnd) - try backChannelWriteEnd.setInherited(true) // Create another pipe to send captured values (and possibly other state // in the future) to the child process. var capturedValuesReadEnd: FileHandle! var capturedValuesWriteEnd: FileHandle! try FileHandle.makePipe(readEnd: &capturedValuesReadEnd, writeEnd: &capturedValuesWriteEnd) - try capturedValuesReadEnd.setInherited(true) // Let the child process know how to find the back channel and // captured values channel by setting a known environment variable to @@ -805,6 +801,15 @@ extension ExitTest { childEnvironment["SWT_EXPERIMENTAL_CAPTURED_VALUES"] = capturedValuesEnvironmentVariable } +#if !SWT_TARGET_OS_APPLE + // Set inherited those file handles that the child process needs. On + // Darwin, this is a no-op because we use POSIX_SPAWN_CLOEXEC_DEFAULT. + try stdoutWriteEnd?.setInherited(true) + try stderrWriteEnd?.setInherited(true) + try backChannelWriteEnd.setInherited(true) + try capturedValuesReadEnd.setInherited(true) +#endif + // Spawn the child process. let processID = try withUnsafePointer(to: backChannelWriteEnd) { backChannelWriteEnd in try withUnsafePointer(to: capturedValuesReadEnd) { capturedValuesReadEnd in From 050301f310e5973a46b2ab2c90c70be93985f1dd Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 17:10:44 -0400 Subject: [PATCH 5/6] Work around Windows having fcntl.h but not fcntl()? --- Sources/_TestingInternals/include/Stubs.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Sources/_TestingInternals/include/Stubs.h b/Sources/_TestingInternals/include/Stubs.h index acf0e1966..8ddb0bf95 100644 --- a/Sources/_TestingInternals/include/Stubs.h +++ b/Sources/_TestingInternals/include/Stubs.h @@ -151,7 +151,7 @@ static int swt_EEXIST(void) { return EEXIST; } -#if __has_include() || __has_include() +#if defined(F_GETFD) /// Call `fcntl(F_GETFD)`. /// /// This function is provided because `fcntl()` is a variadic function and @@ -159,7 +159,9 @@ static int swt_EEXIST(void) { static int swt_getfdflags(int fd) { return fcntl(fd, F_GETFD); } +#endif +#if defined(F_SETFD) /// Call `fcntl(F_SETFD)`. /// /// This function is provided because `fcntl()` is a variadic function and From da36940c52ecdccd3bc8b43aa55a4ee4972d98dc Mon Sep 17 00:00:00 2001 From: Jonathan Grynspan Date: Mon, 9 Jun 2025 17:56:57 -0400 Subject: [PATCH 6/6] Incorporate feedback --- Sources/Testing/Support/FileHandle.swift | 27 +++++++++++++++-------- Sources/_TestingInternals/include/Stubs.h | 1 - 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/Sources/Testing/Support/FileHandle.swift b/Sources/Testing/Support/FileHandle.swift index 8ad4e450d..4e3c17372 100644 --- a/Sources/Testing/Support/FileHandle.swift +++ b/Sources/Testing/Support/FileHandle.swift @@ -495,13 +495,16 @@ extension FileHandle { /// `HANDLE_FLAG_INHERIT` is cleared on Windows.) To make them inheritable, /// call ``setInherited()``. static func makePipe(readEnd: inout FileHandle?, writeEnd: inout FileHandle?) throws { +#if !os(Windows) + var pipe2Called = false +#endif + var (fdReadEnd, fdWriteEnd) = try withUnsafeTemporaryAllocation(of: CInt.self, capacity: 2) { fds in #if os(Windows) guard 0 == _pipe(fds.baseAddress, 0, _O_BINARY | _O_NOINHERIT) else { throw CError(rawValue: swt_errno()) } #else - var pipe2Called = false #if !SWT_TARGET_OS_APPLE && !os(Windows) && !SWT_NO_DYNAMIC_LINKING if let _pipe2 { guard 0 == _pipe2(fds.baseAddress!, O_CLOEXEC) else { @@ -517,9 +520,6 @@ extension FileHandle { guard 0 == pipe(fds.baseAddress!) else { throw CError(rawValue: swt_errno()) } - for fd in fds { - try? setFileDescriptorInherited(fd, false) - } } #endif return (fds[0], fds[1]) @@ -529,6 +529,15 @@ extension FileHandle { Self._close(fdWriteEnd) } +#if !os(Windows) + if !pipe2Called { + // pipe2() is not available. Use pipe() instead and simulate O_CLOEXEC + // to the best of our ability. + try _setFileDescriptorInherited(fdReadEnd, false) + try _setFileDescriptorInherited(fdWriteEnd, false) + } +#endif + do { defer { fdReadEnd = -1 @@ -608,13 +617,13 @@ extension FileHandle { /// Set whether or not the given file descriptor is inherited by child processes. /// /// - Parameters: - /// - fd: The file descriptor. + /// - fd: The file descriptor. /// - inherited: Whether or not `fd` is inherited by child processes - /// (ignoring overriding functionality such as Apple's - /// `POSIX_SPAWN_CLOEXEC_DEFAULT` flag.) + /// (ignoring overriding functionality such as Apple's + /// `POSIX_SPAWN_CLOEXEC_DEFAULT` flag.) /// /// - Throws: Any error that occurred while setting the flag. - static func setFileDescriptorInherited(_ fd: CInt, _ inherited: Bool) throws { + private static func _setFileDescriptorInherited(_ fd: CInt, _ inherited: Bool) throws { switch swt_getfdflags(fd) { case -1: // An error occurred reading the flags for this file descriptor. @@ -652,7 +661,7 @@ extension FileHandle { throw SystemError(description: "Cannot set whether a file handle is inherited unless it is backed by a file descriptor. Please file a bug report at https://github.com/swiftlang/swift-testing/issues/new") } try withLock { - try Self.setFileDescriptorInherited(fd, inherited) + try Self._setFileDescriptorInherited(fd, inherited) } } #elseif os(Windows) diff --git a/Sources/_TestingInternals/include/Stubs.h b/Sources/_TestingInternals/include/Stubs.h index 8ddb0bf95..171cca6a5 100644 --- a/Sources/_TestingInternals/include/Stubs.h +++ b/Sources/_TestingInternals/include/Stubs.h @@ -171,7 +171,6 @@ static int swt_setfdflags(int fd, int flags) { } #endif - SWT_ASSUME_NONNULL_END #endif