From dfd5a0fc3ef583390a0468bc4915ad5fc381a2b5 Mon Sep 17 00:00:00 2001 From: Charles Hu Date: Fri, 13 Jun 2025 15:51:52 -0700 Subject: [PATCH 1/4] Emulate POSIX_SPAWN_CLOEXEC_DEFAULT in fork/exec. POSIX_SPAWN_CLOEXEC_DEFAULT is only available on Darwin. Emulate POSIX_SPAWN_CLOEXEC_DEFAULT on other platforms by calling close after fork, before exec. This commit also removes _subprocess_posix_spawn_fallback because we can't emulate POSIX_SPAWN_CLOEXEC_DEFAULT in a thread-safe manner while using posix_spawn. --- Sources/_SubprocessCShims/process_shims.c | 172 +++++++++++++++------- 1 file changed, 118 insertions(+), 54 deletions(-) diff --git a/Sources/_SubprocessCShims/process_shims.c b/Sources/_SubprocessCShims/process_shims.c index 2b17daa1..2657f009 100644 --- a/Sources/_SubprocessCShims/process_shims.c +++ b/Sources/_SubprocessCShims/process_shims.c @@ -34,9 +34,13 @@ #include #include #include - +#include #include +#if __has_include() +#include +#endif + #if __has_include() #include #elif defined(_WIN32) @@ -50,6 +54,7 @@ extern char **environ; #include #endif +#if !TARGET_OS_WINDOWS int _was_process_exited(int status) { return WIFEXITED(status); } @@ -70,8 +75,7 @@ int _was_process_suspended(int status) { return WIFSTOPPED(status); } -#if TARGET_OS_LINUX -#include +#endif // !TARGET_OS_WINDOWS #ifndef SYS_pidfd_send_signal #define SYS_pidfd_send_signal 424 @@ -100,40 +104,6 @@ vm_size_t _subprocess_vm_size(void) { } #endif -// MARK: - Private Helpers -static pthread_mutex_t _subprocess_fork_lock = PTHREAD_MUTEX_INITIALIZER; - -static int _subprocess_block_everything_but_something_went_seriously_wrong_signals(sigset_t *old_mask) { - sigset_t mask; - int r = 0; - r |= sigfillset(&mask); - r |= sigdelset(&mask, SIGABRT); - r |= sigdelset(&mask, SIGBUS); - r |= sigdelset(&mask, SIGFPE); - r |= sigdelset(&mask, SIGILL); - r |= sigdelset(&mask, SIGKILL); - r |= sigdelset(&mask, SIGSEGV); - r |= sigdelset(&mask, SIGSTOP); - r |= sigdelset(&mask, SIGSYS); - r |= sigdelset(&mask, SIGTRAP); - - r |= pthread_sigmask(SIG_BLOCK, &mask, old_mask); - return r; -} - -#define _subprocess_precondition(__cond) do { \ - int eval = (__cond); \ - if (!eval) { \ - __builtin_trap(); \ - } \ -} while(0) - -#if __DARWIN_NSIG -# define _SUBPROCESS_SIG_MAX __DARWIN_NSIG -#else -# define _SUBPROCESS_SIG_MAX 32 -#endif - // MARK: - Darwin (posix_spawn) #if TARGET_OS_MAC @@ -374,6 +344,100 @@ static int _clone3(int *pidfd) { return syscall(SYS_clone3, &args, sizeof(args)); } +static pthread_mutex_t _subprocess_fork_lock = PTHREAD_MUTEX_INITIALIZER; + +static int _subprocess_make_critical_mask(sigset_t *old_mask) { + sigset_t mask; + int r = 0; + r |= sigfillset(&mask); + r |= sigdelset(&mask, SIGABRT); + r |= sigdelset(&mask, SIGBUS); + r |= sigdelset(&mask, SIGFPE); + r |= sigdelset(&mask, SIGILL); + r |= sigdelset(&mask, SIGKILL); + r |= sigdelset(&mask, SIGSEGV); + r |= sigdelset(&mask, SIGSTOP); + r |= sigdelset(&mask, SIGSYS); + r |= sigdelset(&mask, SIGTRAP); + + r |= pthread_sigmask(SIG_BLOCK, &mask, old_mask); + return r; +} + +#define _subprocess_precondition(__cond) do { \ + int eval = (__cond); \ + if (!eval) { \ + __builtin_trap(); \ + } \ +} while(0) + +#if __DARWIN_NSIG +# define _SUBPROCESS_SIG_MAX __DARWIN_NSIG +#else +# define _SUBPROCESS_SIG_MAX 32 +#endif + +int _shims_snprintf( + char * _Nonnull str, + int len, + const char * _Nonnull format, + char * _Nonnull str1, + char * _Nonnull str2 +) { + return snprintf(str, len, format, str1, str2); +} + +static int _positive_int_parse(const char *str) { + char *end; + long value = strtol(str, &end, 10); + if (end == str) { + // No digits found + return -1; + } + if (errno == ERANGE || val <= 0 || val > INT_MAX) { + // Out of range + return -1; + } + return (int)value; +} + +static int _highest_possibly_open_fd_dir(const char *fd_dir) { + int highest_fd_so_far = 0; + DIR *dir_ptr = opendir(fd_dir); + if (dir_ptr == NULL) { + return -1; + } + + struct dirent *dir_entry = NULL; + while ((dir_entry = readdir(dir_ptr)) != NULL) { + char *entry_name = dir_entry->d_name; + int number = _positive_int_parse(entry_name); + if (number > (long)highest_fd_so_far) { + highest_fd_so_far = number; + } + } + + closedir(dir_ptr); + return highest_fd_so_far; +} + +static int _highest_possibly_open_fd(void) { +#if defined(__APPLE__) + int hi = _highest_possibly_open_fd_dir("/dev/fd"); + if (hi < 0) { + hi = getdtablesize(); + } +#elif defined(__linux__) + int hi = _highest_possibly_open_fd_dir("/proc/self/fd"); + if (hi < 0) { + hi = getdtablesize(); + } +#else + int hi = getdtablesize(); +#endif + return hi; +} + int _subprocess_fork_exec( pid_t * _Nonnull pid, int * _Nonnull pidfd, @@ -433,7 +497,7 @@ int _subprocess_fork_exec( _subprocess_precondition(rc == 0); // Block all signals on this thread sigset_t old_sigmask; - rc = _subprocess_block_everything_but_something_went_seriously_wrong_signals(&old_sigmask); + rc = _subprocess_make_critical_mask(&old_sigmask); if (rc != 0) { close(pipefd[0]); close(pipefd[1]); @@ -556,20 +620,22 @@ int _subprocess_fork_exec( if (rc < 0) { write_error_and_exit; } - - // Close parent side - if (file_descriptors[1] >= 0) { - rc = close(file_descriptors[1]); - } - if (file_descriptors[3] >= 0) { - rc = close(file_descriptors[3]); - } - if (file_descriptors[5] >= 0) { - rc = close(file_descriptors[5]); - } - - if (rc < 0) { - write_error_and_exit; + // Close all other file descriptors + rc = -1; + errno = ENOSYS; +#if __has_include() || defined(__FreeBSD__) + // We must NOT close pipefd[1] for writing errors + rc = close_range(STDERR_FILENO + 1, pipefd[1] - 1, 0); + rc |= close_range(pipefd[1] + 1, ~0U, 0); +#endif + if (rc != 0) { + // close_range failed (or doesn't exist), fall back to close() + for (int fd = STDERR_FILENO + 1; fd < _highest_possibly_open_fd(); fd++) { + // We must NOT close pipefd[1] for writing errors + if (fd != pipefd[1]) { + close(fd); + } + } } // Run custom configuratior @@ -647,8 +713,6 @@ int _subprocess_fork_exec( #endif // TARGET_OS_LINUX -#endif // !TARGET_OS_WINDOWS - #pragma mark - Environment Locking #if __has_include() From b2e57ea0b7a6e8e6cc2619c5bd75dfd4de8f1fee Mon Sep 17 00:00:00 2001 From: Charles Hu Date: Thu, 3 Jul 2025 21:22:22 -0700 Subject: [PATCH 2/4] Use NSIG_MAX insead of a constant --- .../Platforms/Subprocess+Linux.swift | 12 +- .../Platforms/Subprocess+Unix.swift | 25 +++- Sources/_SubprocessCShims/process_shims.c | 129 +++++++++++------- Tests/SubprocessTests/IntegrationTests.swift | 12 +- Tests/SubprocessTests/LinuxTests.swift | 79 +++-------- Tests/SubprocessTests/UnixTests.swift | 26 +--- 6 files changed, 128 insertions(+), 155 deletions(-) diff --git a/Sources/Subprocess/Platforms/Subprocess+Linux.swift b/Sources/Subprocess/Platforms/Subprocess+Linux.swift index a0a8e198..3051ea25 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Linux.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Linux.swift @@ -389,7 +389,7 @@ internal func monitorProcessTermination( if siginfo.si_pid == 0 && siginfo.si_signo == 0 { // Save this continuation to be called by signal hander var newState = storage - newState.continuations[processIdentifier.processDescriptor] = continuation + newState.continuations[processIdentifier.value] = continuation state = .started(newState) return nil } @@ -472,7 +472,7 @@ internal extension siginfo_t { // Okay to be unlocked global mutable because this value is only set once like dispatch_once private nonisolated(unsafe) var _signalPipe: (readEnd: CInt, writeEnd: CInt) = (readEnd: -1, writeEnd: -1) // Okay to be unlocked global mutable because this value is only set once like dispatch_once -private nonisolated(unsafe) var _waitprocessDescriptorSupported = false +private nonisolated(unsafe) var _waitProcessDescriptorSupported = false private let _processMonitorState: Mutex = .init(.notStarted) private func shutdown() { @@ -568,8 +568,8 @@ private func monitorThreadFunc(context: MonitorThreadContext) { } // P_PIDFD requires Linux Kernel 5.4 and above - if _waitprocessDescriptorSupported { - _blockAndWaitForprocessDescriptor(targetFileDescriptor, context: context) + if _waitProcessDescriptorSupported { + _blockAndWaitForProcessDescriptor(targetFileDescriptor, context: context) } else { _reapAllKnownChildProcesses(targetFileDescriptor, context: context) } @@ -658,7 +658,7 @@ private let setup: () = { } } else { // Mark waitid(P_PIDFD) as supported - _waitprocessDescriptorSupported = true + _waitProcessDescriptorSupported = true } let monitorThreadContext = MonitorThreadContext( epollFileDescriptor: epollFileDescriptor, @@ -705,7 +705,7 @@ internal func _setupMonitorSignalHandler() { setup } -private func _blockAndWaitForprocessDescriptor(_ pidfd: CInt, context: MonitorThreadContext) { +private func _blockAndWaitForProcessDescriptor(_ pidfd: CInt, context: MonitorThreadContext) { var terminationStatus: Result var siginfo = siginfo_t() diff --git a/Sources/Subprocess/Platforms/Subprocess+Unix.swift b/Sources/Subprocess/Platforms/Subprocess+Unix.swift index 92f11d4e..e55ff510 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Unix.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Unix.swift @@ -112,19 +112,30 @@ extension Execution { #if os(Linux) || os(Android) // On linux, use pidfd_send_signal if possible - if shouldSendToProcessGroup { + if shouldSendToProcessGroup || self.processIdentifier.processDescriptor < 0 { // pidfd_send_signal does not support sending signal to process group try _kill(pid, signal: signal) } else { - guard _pidfd_send_signal( + let rc = _pidfd_send_signal( processIdentifier.processDescriptor, signal.rawValue - ) == 0 else { - throw SubprocessError( - code: .init(.failedToSendSignal(signal.rawValue)), - underlyingError: .init(rawValue: errno) - ) + ) + let capturedErrno = errno + if rc == 0 { + // _pidfd_send_signal succeeded + return } + if capturedErrno == ENOSYS { + // _pidfd_send_signal is not implemented. Fallback to kill + try _kill(pid, signal: signal) + return + } + + // Throw all other errors + throw SubprocessError( + code: .init(.failedToSendSignal(signal.rawValue)), + underlyingError: .init(rawValue: capturedErrno) + ) } #else try _kill(pid, signal: signal) diff --git a/Sources/_SubprocessCShims/process_shims.c b/Sources/_SubprocessCShims/process_shims.c index 2657f009..4f07e24f 100644 --- a/Sources/_SubprocessCShims/process_shims.c +++ b/Sources/_SubprocessCShims/process_shims.c @@ -36,6 +36,7 @@ #include #include #include +#include #if __has_include() #include @@ -54,7 +55,6 @@ extern char **environ; #include #endif -#if !TARGET_OS_WINDOWS int _was_process_exited(int status) { return WIFEXITED(status); } @@ -75,26 +75,6 @@ int _was_process_suspended(int status) { return WIFSTOPPED(status); } -#endif // !TARGET_OS_WINDOWS - -#ifndef SYS_pidfd_send_signal -#define SYS_pidfd_send_signal 424 -#endif - -int _shims_snprintf( - char * _Nonnull str, - int len, - const char * _Nonnull format, - char * _Nonnull str1, - char * _Nonnull str2 -) { - return snprintf(str, len, format, str1, str2); -} - -int _pidfd_send_signal(int pidfd, int signal) { - return syscall(SYS_pidfd_send_signal, pidfd, signal, NULL, 0); -} - #endif #if __has_include() @@ -304,6 +284,16 @@ int _pidfd_open(pid_t pid) { return syscall(SYS_pidfd_open, pid, 0); } +// SYS_pidfd_send_signal is only defined on Linux Kernel 5.1 and above +// Define our dummy value if it's not available +#ifndef SYS_pidfd_send_signal +#define SYS_pidfd_send_signal 424 +#endif + +int _pidfd_send_signal(int pidfd, int signal) { + return syscall(SYS_pidfd_send_signal, pidfd, signal, NULL, 0); +} + // SYS_clone3 is only defined on Linux Kernel 5.3 and above // Define our dummy value if it's not available (as is the case with Musl libc) #ifndef SYS_clone3 @@ -344,6 +334,18 @@ static int _clone3(int *pidfd) { return syscall(SYS_clone3, &args, sizeof(args)); } +struct linux_dirent64 { + unsigned long d_ino; + unsigned long d_off; + unsigned short d_reclen; + unsigned char d_type; + char d_name[]; +}; + +static int _getdents64(int fd, struct linux_dirent64 *dirp, size_t nbytes) { + return syscall(SYS_getdents64, fd, dirp, nbytes); +} + static pthread_mutex_t _subprocess_fork_lock = PTHREAD_MUTEX_INITIALIZER; static int _subprocess_make_critical_mask(sigset_t *old_mask) { @@ -371,10 +373,18 @@ static int _subprocess_make_critical_mask(sigset_t *old_mask) { } \ } while(0) -#if __DARWIN_NSIG -# define _SUBPROCESS_SIG_MAX __DARWIN_NSIG -#else -# define _SUBPROCESS_SIG_MAX 32 +#if defined(NSIG_MAX) /* POSIX issue 8 */ +# define _SUBPROCESS_SIG_MAX NSIG_MAX +#elif defined(__DARWIN_NSIG) /* Darwin */ +# define _SUBPROCESS_SIG_MAX __DARWIN_NSIG +#elif defined(_SIG_MAXSIG) /* FreeBSD */ +# define _SUBPROCESS_SIG_MAX _SIG_MAXSIG +#elif defined(_SIGMAX) /* QNX */ +# define _SUBPROCESS_SIG_MAX (_SIGMAX + 1) +#elif defined(NSIG) /* 99% of everything else */ +# define _SUBPROCESS_SIG_MAX NSIG +#else /* Last resort */ +# define _SUBPROCESS_SIG_MAX (sizeof(sigset_t) * CHAR_BIT + 1) #endif int _shims_snprintf( @@ -394,46 +404,68 @@ static int _positive_int_parse(const char *str) { // No digits found return -1; } - if (errno == ERANGE || val <= 0 || val > INT_MAX) { + if (errno == ERANGE || value <= 0 || value > INT_MAX) { // Out of range return -1; } return (int)value; } -static int _highest_possibly_open_fd_dir(const char *fd_dir) { +// Linux-specific version that uses syscalls directly and doesn't allocate heap memory. +// Safe to use after vfork() and before execve() +static int _highest_possibly_open_fd_dir_linux(const char *fd_dir) { int highest_fd_so_far = 0; - DIR *dir_ptr = opendir(fd_dir); - if (dir_ptr == NULL) { + int dir_fd = open(fd_dir, O_RDONLY); + if (dir_fd < 0) { + // errno set by `open`. return -1; } - struct dirent *dir_entry = NULL; - while ((dir_entry = readdir(dir_ptr)) != NULL) { - char *entry_name = dir_entry->d_name; - int number = _positive_int_parse(entry_name); - if (number > (long)highest_fd_so_far) { - highest_fd_so_far = number; + // Buffer for directory entries - allocated on stack, no heap allocation + char buffer[4096] = {0}; + long bytes_read = -1; + + while ((bytes_read = _getdents64(dir_fd, (struct linux_dirent64 *)buffer, sizeof(buffer))) > 0) { + if (bytes_read < 0) { + if (errno == EINTR) { + continue; + } else { + // `errno` set by _getdents64. + highest_fd_so_far = -1; + goto error; + } + } + long offset = 0; + while (offset < bytes_read) { + struct linux_dirent64 *entry = (struct linux_dirent64 *)(buffer + offset); + + // Skip "." and ".." entries + if (entry->d_name[0] != '.') { + int number = _positive_int_parse(entry->d_name); + if (number > highest_fd_so_far) { + highest_fd_so_far = number; + } + } + + offset += entry->d_reclen; } } - closedir(dir_ptr); +error: + close(dir_fd); return highest_fd_so_far; } +// This function is only used on systems with Linux kernel 5.9 or lower. +// On newer systems, `close_range` is used instead. static int _highest_possibly_open_fd(void) { -#if defined(__APPLE__) - int hi = _highest_possibly_open_fd_dir("/dev/fd"); - if (hi < 0) { - hi = getdtablesize(); - } -#elif defined(__linux__) - int hi = _highest_possibly_open_fd_dir("/proc/self/fd"); +#if defined(__linux__) + int hi = _highest_possibly_open_fd_dir_linux("/dev/fd"); if (hi < 0) { - hi = getdtablesize(); + hi = sysconf(_SC_OPEN_MAX); } #else - int hi = getdtablesize(); + int hi = sysconf(_SC_OPEN_MAX); #endif return hi; } @@ -623,11 +655,11 @@ int _subprocess_fork_exec( // Close all other file descriptors rc = -1; errno = ENOSYS; -#if __has_include() || defined(__FreeBSD__) + #if (__has_include() && (!defined(__ANDROID__) || __ANDROID_API__ >= 34)) || defined(__FreeBSD__) // We must NOT close pipefd[1] for writing errors rc = close_range(STDERR_FILENO + 1, pipefd[1] - 1, 0); rc |= close_range(pipefd[1] + 1, ~0U, 0); -#endif + #endif if (rc != 0) { // close_range failed (or doesn't exist), fall back to close() for (int fd = STDERR_FILENO + 1; fd < _highest_possibly_open_fd(); fd++) { @@ -657,9 +689,6 @@ int _subprocess_fork_exec( // Newer Linux supports clone3 which returns pidfd directly if (_pidfd < 0) { _pidfd = _pidfd_open(childPid); - if (_pidfd < 0) { - reap_child_process_and_return_errno; - } } // Parent process diff --git a/Tests/SubprocessTests/IntegrationTests.swift b/Tests/SubprocessTests/IntegrationTests.swift index 99045d9b..a800ef3c 100644 --- a/Tests/SubprocessTests/IntegrationTests.swift +++ b/Tests/SubprocessTests/IntegrationTests.swift @@ -1935,17 +1935,7 @@ extension SubprocessIntegrationTests { #expect(result.value.error == "hello stderr") } - @Test( - .disabled("Linux requires #46 to be fixed", { - #if os(Linux) - return true - #else - return false - #endif - }), - .bug("https://github.com/swiftlang/swift-subprocess/issues/46") - ) - func testSubprocessPipeChain() async throws { + @Test func testSubprocessPipeChain() async throws { struct Pipe: @unchecked Sendable { #if os(Windows) let readEnd: HANDLE diff --git a/Tests/SubprocessTests/LinuxTests.swift b/Tests/SubprocessTests/LinuxTests.swift index 17de67f5..cda616f9 100644 --- a/Tests/SubprocessTests/LinuxTests.swift +++ b/Tests/SubprocessTests/LinuxTests.swift @@ -43,7 +43,6 @@ struct SubprocessLinuxTests { // CAP_SETGID capability in its user namespace), and gid does // not match the real group ID or saved set-group-ID of the // calling process. - perror("setgid") abort() } } @@ -64,56 +63,31 @@ struct SubprocessLinuxTests { } @Test func testSuspendResumeProcess() async throws { - let result = try await Subprocess.run( + _ = try await Subprocess.run( // This will intentionally hang .path("/usr/bin/sleep"), arguments: ["infinity"], - output: .discarded, error: .discarded - ) { subprocess -> Error? in - do { - try await tryFinally { - // First suspend the process - try subprocess.send(signal: .suspend) - try await waitForCondition(timeout: .seconds(30), comment: "Process did not transition from running to stopped state after $$") { - let state = try subprocess.state() - switch state { - case .running: - return false - case .zombie: - throw ProcessStateError(expectedState: .stopped, actualState: state) - case .stopped, .sleeping, .uninterruptibleWait: - return true - } - } - // Now resume the process - try subprocess.send(signal: .resume) - try await waitForCondition(timeout: .seconds(30), comment: "Process did not transition from stopped to running state after $$") { - let state = try subprocess.state() - switch state { - case .running, .sleeping, .uninterruptibleWait: - return true - case .zombie: - throw ProcessStateError(expectedState: .running, actualState: state) - case .stopped: - return false - } - } - } finally: { error in - // Now kill the process - try subprocess.send(signal: error != nil ? .kill : .terminate) + ) { subprocess, standardOutput in + try await tryFinally { + // First suspend the process + try subprocess.send(signal: .suspend) + try await waitForCondition(timeout: .seconds(30)) { + let state = try subprocess.state() + return state == .stopped } - return nil - } catch { - return error + // Now resume the process + try subprocess.send(signal: .resume) + try await waitForCondition(timeout: .seconds(30)) { + let state = try subprocess.state() + return state == .running + } + } finally: { error in + // Now kill the process + try subprocess.send(signal: error != nil ? .kill : .terminate) + for try await _ in standardOutput {} } } - if let error = result.value { - #expect(result.terminationStatus == .unhandledException(SIGKILL)) - throw error - } else { - #expect(result.terminationStatus == .unhandledException(SIGTERM)) - } } @Test func testUniqueProcessIdentifier() async throws { @@ -141,15 +115,6 @@ fileprivate enum ProcessState: String { case stopped = "T" } -fileprivate struct ProcessStateError: Error, CustomStringConvertible { - let expectedState: ProcessState - let actualState: ProcessState - - var description: String { - "Process did not transition to \(expectedState) state, but was actually \(actualState)" - } -} - extension Execution { fileprivate func state() throws -> ProcessState { let processStatusFile = "/proc/\(processIdentifier.value)/status" @@ -175,7 +140,7 @@ extension Execution { } } -func waitForCondition(timeout: Duration, comment: Comment, _ evaluateCondition: () throws -> Bool) async throws { +func waitForCondition(timeout: Duration, _ evaluateCondition: () throws -> Bool) async throws { var currentCondition = try evaluateCondition() let deadline = ContinuousClock.now + timeout while ContinuousClock.now < deadline { @@ -186,13 +151,11 @@ func waitForCondition(timeout: Duration, comment: Comment, _ evaluateCondition: currentCondition = try evaluateCondition() } struct TimeoutError: Error, CustomStringConvertible { - let timeout: Duration - let comment: Comment var description: String { - comment.description.replacingOccurrences(of: "$$", with: "\(timeout)") + "Timed out waiting for condition to be true" } } - throw TimeoutError(timeout: timeout, comment: comment) + throw TimeoutError() } func tryFinally(_ work: () async throws -> (), finally: (Error?) async throws -> ()) async throws { diff --git a/Tests/SubprocessTests/UnixTests.swift b/Tests/SubprocessTests/UnixTests.swift index 66f4342e..f376f4c0 100644 --- a/Tests/SubprocessTests/UnixTests.swift +++ b/Tests/SubprocessTests/UnixTests.swift @@ -312,17 +312,7 @@ extension SubprocessUnixTests { } } - @Test( - .disabled("Linux requires #46 to be fixed", { - #if os(Linux) - return true - #else - return false - #endif - }), - .bug("https://github.com/swiftlang/swift-subprocess/issues/46") - ) - func testSubprocessDoesNotInheritVeryHighFileDescriptors() async throws { + @Test func testSubprocessDoesNotInheritVeryHighFileDescriptors() async throws { var openedFileDescriptors: [CInt] = [] // Open /dev/null to use as source for duplication let devnull: FileDescriptor = try .openDevNull(withAccessMode: .readOnly) @@ -389,17 +379,7 @@ extension SubprocessUnixTests { #expect(checklist.isEmpty) } - @Test( - .disabled("Linux requires #46 to be fixed", { - #if os(Linux) - return true - #else - return false - #endif - }), - .bug("https://github.com/swiftlang/swift-subprocess/issues/46") - ) - func testSubprocessDoesNotInheritRandomFileDescriptors() async throws { + @Test(.requiresBash) func testSubprocessDoesNotInheritRandomFileDescriptors() async throws { let pipe = try FileDescriptor.ssp_pipe() defer { try? pipe.readEnd.close() @@ -407,7 +387,7 @@ extension SubprocessUnixTests { } // Spawn bash and then attempt to write to the write end let result = try await Subprocess.run( - .path("/bin/sh"), + .path("/bin/bash"), arguments: [ "-c", """ From 7aae589be5e14a933960a9e6c2ed6e9ba8839f11 Mon Sep 17 00:00:00 2001 From: Charles Hu Date: Wed, 20 Aug 2025 00:00:05 -0700 Subject: [PATCH 3/4] Update Linux testSuspendResumeProcess to use waitpid instead of polling --- Sources/_SubprocessCShims/process_shims.c | 2 +- Tests/SubprocessTests/LinuxTests.swift | 134 ++++++++-------------- 2 files changed, 52 insertions(+), 84 deletions(-) diff --git a/Sources/_SubprocessCShims/process_shims.c b/Sources/_SubprocessCShims/process_shims.c index 4f07e24f..d1372217 100644 --- a/Sources/_SubprocessCShims/process_shims.c +++ b/Sources/_SubprocessCShims/process_shims.c @@ -662,7 +662,7 @@ int _subprocess_fork_exec( #endif if (rc != 0) { // close_range failed (or doesn't exist), fall back to close() - for (int fd = STDERR_FILENO + 1; fd < _highest_possibly_open_fd(); fd++) { + for (int fd = STDERR_FILENO + 1; fd <= _highest_possibly_open_fd(); fd++) { // We must NOT close pipefd[1] for writing errors if (fd != pipefd[1]) { close(fd); diff --git a/Tests/SubprocessTests/LinuxTests.swift b/Tests/SubprocessTests/LinuxTests.swift index cda616f9..0f4ab970 100644 --- a/Tests/SubprocessTests/LinuxTests.swift +++ b/Tests/SubprocessTests/LinuxTests.swift @@ -23,6 +23,7 @@ import Musl import FoundationEssentials import Testing +import _SubprocessCShims @testable import Subprocess // MARK: PlatformOption Tests @@ -63,29 +64,62 @@ struct SubprocessLinuxTests { } @Test func testSuspendResumeProcess() async throws { + func blockAndWaitForStatus( + pid: pid_t, + waitThread: inout pthread_t?, + targetSignal: Int32, + _ handler: @Sendable @escaping (Int32) -> Void + ) async throws { + try await withCheckedThrowingContinuation { (continuation: CheckedContinuation) in + do { + waitThread = try pthread_create { + var suspendedStatus: Int32 = 0 + let rc = waitpid(pid, &suspendedStatus, targetSignal) + handler(suspendedStatus) + continuation.resume() + } + } catch { + continuation.resume(throwing: error) + } + } + } + _ = try await Subprocess.run( // This will intentionally hang .path("/usr/bin/sleep"), arguments: ["infinity"], error: .discarded ) { subprocess, standardOutput in - try await tryFinally { - // First suspend the process - try subprocess.send(signal: .suspend) - try await waitForCondition(timeout: .seconds(30)) { - let state = try subprocess.state() - return state == .stopped - } - // Now resume the process - try subprocess.send(signal: .resume) - try await waitForCondition(timeout: .seconds(30)) { - let state = try subprocess.state() - return state == .running - } - } finally: { error in - // Now kill the process - try subprocess.send(signal: error != nil ? .kill : .terminate) - for try await _ in standardOutput {} + // First suspend the process + try subprocess.send(signal: .suspend) + var thread1: pthread_t? = nil + try await blockAndWaitForStatus( + pid: subprocess.processIdentifier.value, + waitThread: &thread1, + targetSignal: WSTOPPED + ) { status in + #expect(_was_process_suspended(status) > 0) + } + // Now resume the process + try subprocess.send(signal: .resume) + var thread2: pthread_t? = nil + try await blockAndWaitForStatus( + pid: subprocess.processIdentifier.value, + waitThread: &thread2, + targetSignal: WCONTINUED + ) { status in + #expect(_was_process_suspended(status) == 0) + } + + // Now kill the process + try subprocess.send(signal: .terminate) + for try await _ in standardOutput {} + + if let thread1 { + pthread_join(thread1, nil) + } + if let thread2 { + pthread_join(thread2, nil) } } } @@ -106,70 +140,4 @@ struct SubprocessLinuxTests { } } } - -fileprivate enum ProcessState: String { - case running = "R" - case sleeping = "S" - case uninterruptibleWait = "D" - case zombie = "Z" - case stopped = "T" -} - -extension Execution { - fileprivate func state() throws -> ProcessState { - let processStatusFile = "/proc/\(processIdentifier.value)/status" - let processStatusData = try Data( - contentsOf: URL(filePath: processStatusFile) - ) - let stateMatches = try String(decoding: processStatusData, as: UTF8.self) - .split(separator: "\n") - .compactMap({ line in - return try #/^State:\s+(?[A-Z])\s+.*/#.wholeMatch(in: line) - }) - guard let status = stateMatches.first, stateMatches.count == 1, let processState = ProcessState(rawValue: String(status.output.status)) else { - struct ProcStatusParseError: Error, CustomStringConvertible { - let filePath: String - let contents: Data - var description: String { - "Could not parse \(filePath):\n\(String(decoding: contents, as: UTF8.self))" - } - } - throw ProcStatusParseError(filePath: processStatusFile, contents: processStatusData) - } - return processState - } -} - -func waitForCondition(timeout: Duration, _ evaluateCondition: () throws -> Bool) async throws { - var currentCondition = try evaluateCondition() - let deadline = ContinuousClock.now + timeout - while ContinuousClock.now < deadline { - if currentCondition { - return - } - try await Task.sleep(for: .milliseconds(10)) - currentCondition = try evaluateCondition() - } - struct TimeoutError: Error, CustomStringConvertible { - var description: String { - "Timed out waiting for condition to be true" - } - } - throw TimeoutError() -} - -func tryFinally(_ work: () async throws -> (), finally: (Error?) async throws -> ()) async throws { - let error: Error? - do { - try await work() - error = nil - } catch let e { - error = e - } - try await finally(error) - if let error { - throw error - } -} - #endif // canImport(Glibc) || canImport(Bionic) || canImport(Musl) From 44a66275a0c262bc6783fa1d5eb423b380a08cce Mon Sep 17 00:00:00 2001 From: Charles Hu Date: Wed, 20 Aug 2025 14:16:30 -0700 Subject: [PATCH 4/4] Use closefrom on OpenBSD instead of looping through fds to close --- Sources/_SubprocessCShims/process_shims.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/Sources/_SubprocessCShims/process_shims.c b/Sources/_SubprocessCShims/process_shims.c index d1372217..ee8f4d5f 100644 --- a/Sources/_SubprocessCShims/process_shims.c +++ b/Sources/_SubprocessCShims/process_shims.c @@ -423,18 +423,23 @@ static int _highest_possibly_open_fd_dir_linux(const char *fd_dir) { // Buffer for directory entries - allocated on stack, no heap allocation char buffer[4096] = {0}; - long bytes_read = -1; - while ((bytes_read = _getdents64(dir_fd, (struct linux_dirent64 *)buffer, sizeof(buffer))) > 0) { + while (1) { + long bytes_read = _getdents64(dir_fd, (struct linux_dirent64 *)buffer, sizeof(buffer)); if (bytes_read < 0) { if (errno == EINTR) { continue; } else { // `errno` set by _getdents64. highest_fd_so_far = -1; - goto error; + close(dir_fd); + return highest_fd_so_far; } } + if (bytes_read == 0) { + close(dir_fd); + return highest_fd_so_far; + } long offset = 0; while (offset < bytes_read) { struct linux_dirent64 *entry = (struct linux_dirent64 *)(buffer + offset); @@ -451,7 +456,6 @@ static int _highest_possibly_open_fd_dir_linux(const char *fd_dir) { } } -error: close(dir_fd); return highest_fd_so_far; } @@ -659,6 +663,13 @@ int _subprocess_fork_exec( // We must NOT close pipefd[1] for writing errors rc = close_range(STDERR_FILENO + 1, pipefd[1] - 1, 0); rc |= close_range(pipefd[1] + 1, ~0U, 0); + #elif defined(__OpenBSD__) + // OpenBSD Supports closefrom, but not close_range + // See https://man.openbsd.org/closefrom + for (int fd = STDERR_FILENO + 1; fd <= pipefd[1] - 1; fd++) { + close(fd); + } + rc = closefrom(pipefd[1] + 1); #endif if (rc != 0) { // close_range failed (or doesn't exist), fall back to close()