Introduce preExecProcessAction on Darwin, rename preSpawnProcessConfigurator to preExecProcessAction on Linux (#140)

This patch introduces a preExecProcessAction on Darwin, which runs immediately before posix_spawn-as-exec, just as preSpawnProcessConfigurator (now called preExecProcessAction) runs immediately before exe on Linux.

The reason to introduce this API is because there is presently no way to run code _after_ fork on Darwin in the case where pre-forking occurs (which is true when setting a uid, gid, sgroups, or creating a session). This can be useful when performing operations that modify process global state and wanting to do so without affecting the parent.

The reason to rename the Linux API is that these are fundamentally two different logical operations: one is modifying flags to the process spawning mechanism (CreateProcessW, posix_spawn), while the other is performing an action at a specific point in the process lifecycle (before exec). Only Darwin uses posix_spawn, so there is no opportunity to modify any flags, and preSpawnProcessConfigurator is therefore not available on Linux/Android/FreeBSD/etc. On Windows, there is no fork/exec concept, so preExecProcessAction is not available on Windows.

This brings the set of APIs to:

- preSpawnProcessConfigurator (Windows, Darwin) - modifies flags bitset / STARTUPINFOW, or posix_spawnattr_t / posix_spawn_file_actions_t
- preExecProcessAction (Darwin, Linux/Android/FreeBSD/etc.) - runs before exec or posix_spawn-as-exec

or

- Windows: preSpawnProcessConfigurator
- Darwin: preSpawnProcessConfigurator + preExecProcessAction
- Linux/Android/FreeBSD/etc.: preExecProcessAction

This means that Darwin has both preSpawnProcessConfigurator and preExecProcessAction, because the pre-forking case uses posix_spawn _and_ fork. It also give us the opportunity to introduce preSpawnProcessConfigurator on Linux and other platforms in the future if they introduce a similar POSIX_SPAWN_CLOEXEC_DEFAULT flag or POSIX otherwise standardizes a mechanism to disable file descriptor inheritance by default, since that would allow us to use posix_spawn on those platforms.

Note that merely setting preExecProcessAction will also cause pre-forking to occur.

Closes #25

Author: jakepetroules

Sources/Subprocess/Platforms/Subprocess+Darwin.swift

@@ -81,6 +81,29 @@ 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() {}
 }
@@ -138,6 +161,7 @@ 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))
             """
     }
@@ -402,7 +426,8 @@ extension Configuration {
                             gidPtr,
                             Int32(supplementaryGroups?.count ?? 0),
                             sgroups?.baseAddress,
-                            self.platformOptions.createSession ? 1 : 0
+                            self.platformOptions.createSession ? 1 : 0,
+                            self.platformOptions.preExecProcessAction,
                         )
                     }
                 }

Sources/Subprocess/Platforms/Subprocess+Linux.swift

@@ -152,7 +152,7 @@ extension Configuration {
                                     CInt(supplementaryGroups?.count ?? 0),
                                     sgroups?.baseAddress,
                                     self.platformOptions.createSession ? 1 : 0,
-                                    self.platformOptions.preSpawnProcessConfigurator
+                                    self.platformOptions.preExecProcessAction
                                 )
                             }
                         }
@@ -261,27 +261,27 @@ extension ProcessIdentifier: CustomStringConvertible, CustomDebugStringConvertib
 /// The collection of platform-specific settings
 /// to configure the subprocess when running
 public struct PlatformOptions: Sendable {
-    // Set user ID for the subprocess
+    /// Set user ID for the subprocess
     public var userID: uid_t? = nil
     /// Set the real and effective group ID and the saved
     /// set-group-ID of the subprocess, equivalent to calling
     /// `setgid()` on the child process.
     /// Group ID is used to control permissions, particularly
     /// for file access.
     public var groupID: gid_t? = nil
-    // Set list of supplementary group IDs for the subprocess
+    /// Set list of supplementary group IDs for the subprocess
     public var supplementaryGroups: [gid_t]? = nil
     /// Set the process group for the subprocess, equivalent to
     /// calling `setpgid()` on the child process.
     /// Process group ID is used to group related processes for
     /// controlling signals.
     public var processGroupID: pid_t? = nil
-    // Creates a session and sets the process group ID
-    // i.e. Detach from the terminal.
+    /// Creates a session and sets the process group ID
+    /// i.e. Detach from the terminal.
     public var createSession: Bool = false
     /// An ordered list of steps in order to tear down the child
     /// process in case the parent task is cancelled before
-    /// the child proces terminates.
+    /// the child process terminates.
     /// Always ends in sending a `.kill` signal at the end.
     public var teardownSequence: [TeardownStep] = []
     /// A closure to configure platform-specific
@@ -295,7 +295,9 @@ public struct PlatformOptions: Sendable {
     /// underlying spawning mechanism. This closure is called
     /// after `fork()` but before `exec()`. You may use it to
     /// call any necessary process setup functions.
-    public var preSpawnProcessConfigurator: (@convention(c) @Sendable () -> Void)? = nil
+    ///
+    /// - 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() {}
 }
@@ -310,7 +312,7 @@ extension PlatformOptions: CustomStringConvertible, CustomDebugStringConvertible
             \(indent)    supplementaryGroups: \(String(describing: supplementaryGroups)),
             \(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))
             """
     }

Sources/_SubprocessCShims/include/process_shims.h

@@ -43,7 +43,8 @@ int _subprocess_spawn(
     uid_t * _Nullable uid,
     gid_t * _Nullable gid,
     int number_of_sgroups, const gid_t * _Nullable sgroups,
-    int create_session
+    int create_session,
+    void (* _Nullable configurator)(void)
 );
 #endif // TARGET_OS_MAC
 

Sources/_SubprocessCShims/process_shims.c

@@ -143,7 +143,8 @@ 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
+    int create_session,
+    void (* _Nullable configurator)(void)
 ) {
 #define write_error_and_exit int error = errno; \
     write(pipefd[1], &error, sizeof(error));\
@@ -212,6 +213,9 @@ static int _subprocess_spawn_prefork(
 
         // Perform setups
         if (number_of_sgroups > 0 && sgroups != NULL) {
+            // POSIX doesn't define setgroups (only getgroups) and therefore makes no guarantee of async-signal-safety,
+            // but we'll assume in practice it should be async-signal-safe on any reasonable platform based on the fact
+            // that getgroups is async-signal-safe.
             if (setgroups(number_of_sgroups, sgroups) != 0) {
                 write_error_and_exit;
             }
@@ -233,6 +237,11 @@ 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
@@ -281,20 +290,22 @@ int _subprocess_spawn(
     uid_t * _Nullable uid,
     gid_t * _Nullable gid,
     int number_of_sgroups, const gid_t * _Nullable sgroups,
-    int create_session
+    int create_session,
+    void (* _Nullable configurator)(void)
 ) {
     int require_pre_fork = uid != NULL ||
         gid != NULL ||
         number_of_sgroups > 0 ||
-        create_session > 0;
+        create_session > 0 ||
+        configurator != NULL;
 
     if (require_pre_fork != 0) {
         int rc = _subprocess_spawn_prefork(
             pid,
             exec_path,
             file_actions, spawn_attrs,
             args, env,
-            uid, gid, number_of_sgroups, sgroups, create_session
+            uid, gid, number_of_sgroups, sgroups, create_session, configurator
         );
         return rc;
     }
@@ -490,6 +501,9 @@ int _subprocess_fork_exec(
         }
 
         if (number_of_sgroups > 0 && sgroups != NULL) {
+            // POSIX doesn't define setgroups (only getgroups) and therefore makes no guarantee of async-signal-safety,
+            // but we'll assume in practice it should be async-signal-safe on any reasonable platform based on the fact
+            // that getgroups is async-signal-safe.
             if (setgroups(number_of_sgroups, sgroups) != 0) {
                 write_error_and_exit;
             }

Tests/SubprocessTests/SubprocessTests+Darwin.swift

@@ -26,6 +26,75 @@ 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 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

Tests/SubprocessTests/SubprocessTests+Linux.swift

@@ -34,9 +34,9 @@ struct SubprocessLinuxTests {
             "This test requires root privileges"
         )
     )
-    func testSubprocessPlatformOptionsPreSpawnProcessConfigurator() async throws {
+    func testSubprocessPlatformOptionsPreExecProcessAction() async throws {
         var platformOptions = PlatformOptions()
-        platformOptions.preSpawnProcessConfigurator = {
+        platformOptions.preExecProcessAction = {
             guard setgid(4321) == 0 else {
                 // Returns EPERM when:
                 //   The calling process is not privileged (does not have the

Tests/SubprocessTests/SubprocessTests+Unix.swift

@@ -955,6 +955,32 @@ extension SubprocessUnixTests {
 }
 
 // MARK: - Utils
+extension FileDescriptor {
+    /// Runs a closure and then closes the FileDescriptor, even if an error occurs.
+    ///
+    /// - Parameter body: The closure to run.
+    ///   If the closure throws an error,
+    ///   this method closes the file descriptor before it rethrows that error.
+    ///
+    /// - Returns: The value returned by the closure.
+    ///
+    /// If `body` throws an error
+    /// or an error occurs while closing the file descriptor,
+    /// this method rethrows that error.
+    public func closeAfter<R>(_ body: () async throws -> R) async throws -> R {
+        // No underscore helper, since the closure's throw isn't necessarily typed.
+        let result: R
+        do {
+            result = try await body()
+        } catch {
+            _ = try? self.close() // Squash close error and throw closure's
+            throw error
+        }
+        try self.close()
+        return result
+    }
+}
+
 extension SubprocessUnixTests {
     private func assertID(
         withArgument argument: String,
@@ -981,10 +1007,18 @@ internal func assertNewSessionCreated<Output: OutputProtocol>(
         Output
     >
 ) throws {
-    #expect(result.terminationStatus.isSuccess)
-    let psValue = try #require(
-        result.standardOutput
+    try assertNewSessionCreated(
+        terminationStatus: result.terminationStatus,
+        output: #require(result.standardOutput)
     )
+}
+
+internal func assertNewSessionCreated(
+    terminationStatus: TerminationStatus,
+    output psValue: String
+) throws {
+    #expect(terminationStatus.isSuccess)
+
     let match = try #require(try #/\s*PID\s*PGID\s*TPGID\s*(?<pid>[\-]?[0-9]+)\s*(?<pgid>[\-]?[0-9]+)\s*(?<tpgid>[\-]?[0-9]+)\s*/#.wholeMatch(in: psValue), "ps output was in an unexpected format:\n\n\(psValue)")
     // If setsid() has been called successfully, we should observe:
     // - pid == pgid