Use NSIG_MAX insead of a constant

Author: iCharlesHu

Sources/Subprocess/Platforms/Subprocess+Unix.swift

@@ -115,7 +115,7 @@ extension Execution {
         if shouldSendToProcessGroup {
             // pidfd_send_signal does not support sending signal to process group
             try _kill(pid, signal: signal)
-        } else {
+        } else if processIdentifier.processDescriptor > 0 {
             guard _pidfd_send_signal(
                 processIdentifier.processDescriptor,
                 signal.rawValue
@@ -125,6 +125,9 @@ extension Execution {
                     underlyingError: .init(rawValue: errno)
                 )
             }
+        } else {
+            // fall back to _kill()
+            try _kill(pid, signal: signal)
         }
         #else
         try _kill(pid, signal: signal)

Sources/_SubprocessCShims/process_shims.c

@@ -36,6 +36,7 @@
 #include <pthread.h>
 #include <dirent.h>
 #include <stdio.h>
+#include <limits.h>
 
 #if __has_include(<linux/close_range.h>)
 #include <linux/close_range.h>
@@ -272,6 +273,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 0xFFFF
+#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
 #ifndef SYS_clone3
@@ -320,10 +331,18 @@ static int _subprocess_make_critical_mask(sigset_t *old_mask) {
     } \
 } while(0)
 
-#if __DARWIN_NSIG
+#if __DARWIN_NSIG               /* Darwin */
 #  define _SUBPROCESS_SIG_MAX __DARWIN_NSIG
-#else
-#  define _SUBPROCESS_SIG_MAX 32
+#elif defined(NSIG_MAX)         /* POSIX issue 8 */
+# define _SUBPROCESS_SIG_MAX NSIG_MAX
+#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(
@@ -343,46 +362,66 @@ 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 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 dirent64 *entry = (struct 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;
 }
 
 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;
 }

Tests/SubprocessTests/SubprocessTests+Linux.swift

@@ -63,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))
-        }
     }
 }
 
@@ -124,15 +99,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"
@@ -158,7 +124,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 {
@@ -169,13 +135,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 {

Tests/SubprocessTests/SubprocessTests+Unix.swift

@@ -953,21 +953,21 @@ extension SubprocessUnixTests {
         try FileManager.default.removeItem(at: testFilePath)
     }
 
-    @Test func testDoesNotInheritRandomFileDescriptorsByDefault() async throws {
+    @Test(.requiresBash) func testDoesNotInheritRandomFileDescriptorsByDefault() async throws {
         // This tests makes sure POSIX_SPAWN_CLOEXEC_DEFAULT works on all platforms
         let pipe = try FileDescriptor.ssp_pipe()
         defer {
             close(pipe.readEnd.rawValue)
-            close(pipe.writeEnd.rawValue)
         }
         let writeFd = pipe.writeEnd.rawValue
-        let result = try await Subprocess.run(
-            .path("/bin/sh"),
-            arguments: ["-c", "echo hello from child >&\(writeFd); echo wrote into \(writeFd), echo exit code $?"],
-            output: .string,
-            error: .string
-        )
-        close(pipe.writeEnd.rawValue)
+        let result = try await pipe.writeEnd.closeAfter {
+            return try await Subprocess.run(
+                .path("/bin/bash"),
+                arguments: ["-c", "echo hello from child >&\(writeFd); echo wrote into \(writeFd), echo exit code $?"],
+                output: .string(limit: .max),
+                error: .string(limit: .max)
+            )
+        }
 
         #expect(result.terminationStatus.isSuccess)
         #expect(
@@ -984,6 +984,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,