From c7fc84487ce0a9b05c1b8630136be6c37d611b4c Mon Sep 17 00:00:00 2001 From: Jake Petroules Date: Fri, 15 Aug 2025 16:14:41 -0700 Subject: [PATCH] Remove preExecProcessAction It's been deemed too tricky to make this API safe and there are relatively few use cases anyways, considering we want to use posix_spawn internally as much as possible. Closes #148 --- .../Platforms/Subprocess+Darwin.swift | 27 +------ .../Platforms/Subprocess+Linux.swift | 20 +----- .../_SubprocessCShims/include/process_shims.h | 6 +- Sources/_SubprocessCShims/process_shims.c | 23 ++---- Tests/SubprocessTests/DarwinTests.swift | 72 ------------------- Tests/SubprocessTests/LinuxTests.swift | 34 --------- 6 files changed, 10 insertions(+), 172 deletions(-) diff --git a/Sources/Subprocess/Platforms/Subprocess+Darwin.swift b/Sources/Subprocess/Platforms/Subprocess+Darwin.swift index 99fe532d..99ff9d65 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Darwin.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Darwin.swift @@ -81,29 +81,6 @@ public struct PlatformOptions: Sendable { inout posix_spawn_file_actions_t? ) throws -> Void )? = nil - /// A closure to configure platform-specific - /// spawning constructs. This closure enables direct - /// configuration or override of underlying platform-specific - /// spawn settings that `Subprocess` utilizes internally, - /// in cases where Subprocess does not provide higher-level - /// APIs for such modifications. - /// - /// On Darwin, Subprocess uses `posix_spawn()` as the - /// underlying spawning mechanism, but may require an initial `fork()` - /// depending on the configured `PlatformOptions`. - /// This closure is called after `fork()` but before `posix_spawn()` - /// (with the `POSIX_SPAWN_SETEXEC` flag set). - /// You may use it to call any necessary process setup functions. - /// - /// - note: You can set both `preExecProcessAction` and - /// `preSpawnProcessConfigurator` and both will be called. - /// Setting `preExecProcessAction` will always cause Subprocess - /// to pre-`fork()` before calling `posix_spawn()` (with the - /// `POSIX_SPAWN_SETEXEC` flag set) even if it would not have otherwise - /// done so based on the configured `PlatformOptions`. - /// - /// - warning: You may ONLY call [async-signal-safe functions](https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html) within this closure (note _"The following table defines a set of functions and function-like macros that shall be async-signal-safe."_). - public var preExecProcessAction: (@convention(c) @Sendable () -> Void)? = nil public init() {} } @@ -161,7 +138,6 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible \(indent) processGroupID: \(String(describing: processGroupID)), \(indent) createSession: \(createSession), \(indent) preSpawnProcessConfigurator: \(self.preSpawnProcessConfigurator == nil ? "not set" : "set") - \(indent) preExecProcessAction: \(self.preExecProcessAction == nil ? "not set" : "set") \(indent)) """ } @@ -426,8 +402,7 @@ extension Configuration { gidPtr, Int32(supplementaryGroups?.count ?? 0), sgroups?.baseAddress, - self.platformOptions.createSession ? 1 : 0, - self.platformOptions.preExecProcessAction, + self.platformOptions.createSession ? 1 : 0 ) } } diff --git a/Sources/Subprocess/Platforms/Subprocess+Linux.swift b/Sources/Subprocess/Platforms/Subprocess+Linux.swift index 3051ea25..512eeed2 100644 --- a/Sources/Subprocess/Platforms/Subprocess+Linux.swift +++ b/Sources/Subprocess/Platforms/Subprocess+Linux.swift @@ -151,8 +151,7 @@ extension Configuration { processGroupIDPtr, CInt(supplementaryGroups?.count ?? 0), sgroups?.baseAddress, - self.platformOptions.createSession ? 1 : 0, - self.platformOptions.preExecProcessAction + self.platformOptions.createSession ? 1 : 0 ) } } @@ -284,20 +283,6 @@ public struct PlatformOptions: Sendable { /// the child process terminates. /// Always ends in sending a `.kill` signal at the end. public var teardownSequence: [TeardownStep] = [] - /// A closure to configure platform-specific - /// spawning constructs. This closure enables direct - /// configuration or override of underlying platform-specific - /// spawn settings that `Subprocess` utilizes internally, - /// in cases where Subprocess does not provide higher-level - /// APIs for such modifications. - /// - /// On Linux, Subprocess uses `fork/exec` as the - /// underlying spawning mechanism. This closure is called - /// after `fork()` but before `exec()`. You may use it to - /// call any necessary process setup functions. - /// - /// - warning: You may ONLY call [async-signal-safe functions](https://pubs.opengroup.org/onlinepubs/9799919799/functions/V2_chap02.html) within this closure (note _"The following table defines a set of functions and function-like macros that shall be async-signal-safe."_). - public var preExecProcessAction: (@convention(c) @Sendable () -> Void)? = nil public init() {} } @@ -311,8 +296,7 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible \(indent) groupID: \(String(describing: groupID)), \(indent) supplementaryGroups: \(String(describing: supplementaryGroups)), \(indent) processGroupID: \(String(describing: processGroupID)), - \(indent) createSession: \(createSession), - \(indent) preExecProcessAction: \(self.preExecProcessAction == nil ? "not set" : "set") + \(indent) createSession: \(createSession) \(indent)) """ } diff --git a/Sources/_SubprocessCShims/include/process_shims.h b/Sources/_SubprocessCShims/include/process_shims.h index 2fa4d54a..85c97bb3 100644 --- a/Sources/_SubprocessCShims/include/process_shims.h +++ b/Sources/_SubprocessCShims/include/process_shims.h @@ -43,8 +43,7 @@ int _subprocess_spawn( uid_t * _Nullable uid, gid_t * _Nullable gid, int number_of_sgroups, const gid_t * _Nullable sgroups, - int create_session, - void (* _Nullable configurator)(void) + int create_session ); #endif // TARGET_OS_MAC @@ -60,8 +59,7 @@ int _subprocess_fork_exec( gid_t * _Nullable gid, gid_t * _Nullable process_group_id, int number_of_sgroups, const gid_t * _Nullable sgroups, - int create_session, - void (* _Nullable configurator)(void) + int create_session ); int _was_process_exited(int status); diff --git a/Sources/_SubprocessCShims/process_shims.c b/Sources/_SubprocessCShims/process_shims.c index ee8f4d5f..5fa1d2cc 100644 --- a/Sources/_SubprocessCShims/process_shims.c +++ b/Sources/_SubprocessCShims/process_shims.c @@ -97,8 +97,7 @@ static int _subprocess_spawn_prefork( uid_t * _Nullable uid, gid_t * _Nullable gid, int number_of_sgroups, const gid_t * _Nullable sgroups, - int create_session, - void (* _Nullable configurator)(void) + int create_session ) { #define write_error_and_exit int error = errno; \ write(pipefd[1], &error, sizeof(error));\ @@ -191,11 +190,6 @@ static int _subprocess_spawn_prefork( (void)setsid(); } - // Run custom configuratior - if (configurator != NULL) { - configurator(); - } - // Use posix_spawnas exec int error = posix_spawn(pid, exec_path, file_actions, spawn_attrs, args, env); // If we reached this point, something went wrong @@ -244,14 +238,12 @@ int _subprocess_spawn( uid_t * _Nullable uid, gid_t * _Nullable gid, int number_of_sgroups, const gid_t * _Nullable sgroups, - int create_session, - void (* _Nullable configurator)(void) + int create_session ) { int require_pre_fork = uid != NULL || gid != NULL || number_of_sgroups > 0 || - create_session > 0 || - configurator != NULL; + create_session > 0; if (require_pre_fork != 0) { int rc = _subprocess_spawn_prefork( @@ -259,7 +251,7 @@ int _subprocess_spawn( exec_path, file_actions, spawn_attrs, args, env, - uid, gid, number_of_sgroups, sgroups, create_session, configurator + uid, gid, number_of_sgroups, sgroups, create_session ); return rc; } @@ -486,8 +478,7 @@ int _subprocess_fork_exec( gid_t * _Nullable gid, gid_t * _Nullable process_group_id, int number_of_sgroups, const gid_t * _Nullable sgroups, - int create_session, - void (* _Nullable configurator)(void) + int create_session ) { #define write_error_and_exit int error = errno; \ write(pipefd[1], &error, sizeof(error));\ @@ -681,10 +672,6 @@ int _subprocess_fork_exec( } } - // Run custom configuratior - if (configurator != NULL) { - configurator(); - } // Finally, exec execve(exec_path, args, env); // If we reached this point, something went wrong diff --git a/Tests/SubprocessTests/DarwinTests.swift b/Tests/SubprocessTests/DarwinTests.swift index d8893415..522225ed 100644 --- a/Tests/SubprocessTests/DarwinTests.swift +++ b/Tests/SubprocessTests/DarwinTests.swift @@ -26,78 +26,6 @@ import Testing // MARK: PlatformOptions Tests @Suite(.serialized) struct SubprocessDarwinTests { - @Test func testSubprocessPlatformOptionsPreExecProcessAction() async throws { - var platformOptions = PlatformOptions() - platformOptions.preExecProcessAction = { - exit(1234567) - } - let idResult = try await Subprocess.run( - .path("/bin/pwd"), - platformOptions: platformOptions, - output: .discarded - ) - #expect(idResult.terminationStatus == .exited(1234567)) - } - - @Test( - .disabled("Constantly fails on macOS 26 and Swift 6.2"), - .bug("https://github.com/swiftlang/swift-subprocess/issues/148") - ) func testSubprocessPlatformOptionsPreExecProcessActionAndProcessConfigurator() async throws { - let (readFD, writeFD) = try FileDescriptor.pipe() - try await readFD.closeAfter { - let childPID = try await writeFD.closeAfter { - // Allocate some constant high-numbered FD that's unlikely to be used. - let specialFD = try writeFD.duplicate(as: FileDescriptor(rawValue: 9000)) - return try await specialFD.closeAfter { - // Make the fd non-blocking just to avoid the test hanging if it fails - let opts = fcntl(specialFD.rawValue, F_GETFD) - #expect(opts >= 0) - #expect(fcntl(specialFD.rawValue, F_SETFD, opts | O_NONBLOCK) >= 0) - - var platformOptions = PlatformOptions() - platformOptions.preExecProcessAction = { - var pid: Int32 = getpid() - if write(9000, &pid, 4) != 4 { - exit(EXIT_FAILURE) - } - } - platformOptions.preSpawnProcessConfigurator = { spawnAttr, _ in - // Set POSIX_SPAWN_SETSID flag, which implies calls - // to setsid - var flags: Int16 = 0 - posix_spawnattr_getflags(&spawnAttr, &flags) - posix_spawnattr_setflags(&spawnAttr, flags | Int16(POSIX_SPAWN_SETSID)) - } - // Check the process ID (pid), process group ID (pgid), and - // controlling terminal's process group ID (tpgid) - let psResult = try await Subprocess.run( - .path("/bin/bash"), - arguments: ["-c", "ps -o pid,pgid,tpgid -p $$"], - platformOptions: platformOptions, - input: .none, - error: .discarded - ) { execution, standardOutput in - var buffer = Data() - for try await chunk in standardOutput { - let currentChunk = chunk.withUnsafeBytes { Data($0) } - buffer += currentChunk - } - return (pid: execution.processIdentifier.value, standardOutput: buffer) - } - try assertNewSessionCreated(terminationStatus: psResult.terminationStatus, output: String(decoding: psResult.value.standardOutput, as: UTF8.self)) - return psResult.value.pid - } - } - - let bytes = try await readFD.readUntilEOF(upToLength: 4) - var pid: Int32 = -1 - _ = withUnsafeMutableBytes(of: &pid) { ptr in - bytes.copyBytes(to: ptr) - } - #expect(pid == childPID) - } - } - @Test func testSubprocessPlatformOptionsProcessConfiguratorUpdateSpawnAttr() async throws { var platformOptions = PlatformOptions() platformOptions.preSpawnProcessConfigurator = { spawnAttr, _ in diff --git a/Tests/SubprocessTests/LinuxTests.swift b/Tests/SubprocessTests/LinuxTests.swift index 0f4ab970..a965cb0d 100644 --- a/Tests/SubprocessTests/LinuxTests.swift +++ b/Tests/SubprocessTests/LinuxTests.swift @@ -29,40 +29,6 @@ import _SubprocessCShims // MARK: PlatformOption Tests @Suite(.serialized) struct SubprocessLinuxTests { - @Test( - .enabled( - if: getgid() == 0, - "This test requires root privileges" - ) - ) - func testSubprocessPlatformOptionsPreExecProcessAction() async throws { - var platformOptions = PlatformOptions() - platformOptions.preExecProcessAction = { - guard setgid(4321) == 0 else { - // Returns EPERM when: - // The calling process is not privileged (does not have the - // 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. - abort() - } - } - let idResult = try await Subprocess.run( - .path("/usr/bin/id"), - arguments: ["-g"], - platformOptions: platformOptions, - output: .string(limit: 32), - error: .string(limit: 32) - ) - let error = try #require(idResult.standardError) - try #require(error == "") - #expect(idResult.terminationStatus.isSuccess) - let id = try #require(idResult.standardOutput) - #expect( - id.trimmingCharacters(in: .whitespacesAndNewlines) == "\(4321)" - ) - } - @Test func testSuspendResumeProcess() async throws { func blockAndWaitForStatus( pid: pid_t,