From 31046a7bc03e9c31a7cc0c9cae7e3f12816e6570 Mon Sep 17 00:00:00 2001 From: Johannes Weiss Date: Tue, 8 Jul 2025 12:18:12 +0100 Subject: [PATCH] fd close loop: improve, fix & test --- Sources/CProcessSpawnSync/internal-helpers.h | 51 +++++++++++++- Sources/CProcessSpawnSync/spawner.c | 2 +- .../AsyncProcessTests/IntegrationTests.swift | 70 +++++++++++++++++++ 3 files changed, 121 insertions(+), 2 deletions(-) diff --git a/Sources/CProcessSpawnSync/internal-helpers.h b/Sources/CProcessSpawnSync/internal-helpers.h index b57f0f6..a8e9e7e 100644 --- a/Sources/CProcessSpawnSync/internal-helpers.h +++ b/Sources/CProcessSpawnSync/internal-helpers.h @@ -13,6 +13,8 @@ #ifndef INTERNAL_HELPERS_H #define INTERNAL_HELPERS_H #include +#include +#include static int positive_int_parse(const char *str) { int out = 0; @@ -49,6 +51,53 @@ static int highest_possibly_open_fd_dir(const char *fd_dir) { return highest_fd_so_far; } +#if defined(__linux__) +// 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; + int dir_fd = open(fd_dir, O_RDONLY); + if (dir_fd < 0) { + // errno set by `open`. + return -1; + } + + // 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; + } + } + +error: + close(dir_fd); + return highest_fd_so_far; +} +#endif + static int highest_possibly_open_fd(void) { #if defined(__APPLE__) int hi = highest_possibly_open_fd_dir("/dev/fd"); @@ -56,7 +105,7 @@ static int highest_possibly_open_fd(void) { hi = getdtablesize(); } #elif defined(__linux__) - int hi = highest_possibly_open_fd_dir("/proc/self/fd"); + int hi = highest_possibly_open_fd_dir_linux("/proc/self/fd"); if (hi < 0) { hi = getdtablesize(); } diff --git a/Sources/CProcessSpawnSync/spawner.c b/Sources/CProcessSpawnSync/spawner.c index 315bb75..ce20e79 100644 --- a/Sources/CProcessSpawnSync/spawner.c +++ b/Sources/CProcessSpawnSync/spawner.c @@ -169,7 +169,7 @@ static void setup_and_execve_child(ps_process_configuration *config, int error_p #endif if (close_range_err) { // close_range failed (or doesn't exist), let's fall back onto this - for (int i=config->psc_fd_setup_count; ipsc_fd_setup_count; i<=highest_possibly_open_fd(); i++) { if (i != error_pipe) { close(i); } diff --git a/Tests/AsyncProcessTests/IntegrationTests.swift b/Tests/AsyncProcessTests/IntegrationTests.swift index f79cd50..3974882 100644 --- a/Tests/AsyncProcessTests/IntegrationTests.swift +++ b/Tests/AsyncProcessTests/IntegrationTests.swift @@ -1333,6 +1333,76 @@ final class IntegrationTests: XCTestCase { } } + func testVeryHighFDs() async throws { + var openedFDs: [CInt] = [] + + // Open /dev/null to use as source for duplication + let devNullFD = open("/dev/null", O_RDONLY) + guard devNullFD != -1 else { + XCTFail("Failed to open /dev/null") + return + } + defer { + let closeResult = close(devNullFD) + XCTAssertEqual(0, closeResult, "Failed to close /dev/null FD") + } + + for candidate in sequence(first: CInt(1), next: { $0 <= CInt.max / 2 ? $0 * 2 : nil }) { + // Use fcntl with F_DUPFD to find next available FD >= candidate + let fd = fcntl(devNullFD, F_DUPFD, candidate) + if fd == -1 { + // Failed to allocate FD >= candidate, try next power of 2 + self.logger.debug( + "already unavailable, skipping", + metadata: ["candidate": "\(candidate)", "errno": "\(errno)"] + ) + continue + } else { + openedFDs.append(fd) + self.logger.debug("Opened FD in parent", metadata: ["fd": "\(fd)"]) + } + } + + defer { + for fd in openedFDs { + let closeResult = close(fd) + XCTAssertEqual(0, closeResult, "Failed to close FD \(fd)") + } + } + + // Create shell script that checks each FD passed as arguments + let shellScript = """ + for fd in "$@"; do + if [ -e "/proc/self/fd/$fd" ] || [ -e "/dev/fd/$fd" ]; then + echo "- fd: $fd: OPEN" + else + echo "- fd: $fd: CLOSED" + fi + done + """ + + var arguments = ["-c", shellScript, "--"] + arguments.append(contentsOf: openedFDs.map { "\($0)" }) + + let result = try await ProcessExecutor.runCollectingOutput( + group: self.group, + executable: "/bin/sh", + arguments, + standardInput: EOFSequence(), + collectStandardOutput: true, + collectStandardError: true, + logger: self.logger + ) + try result.exitReason.throwIfNonZero() + + // Assert stderr is empty + XCTAssertEqual("", String(buffer: result.standardError!)) + + // Assert stdout contains exactly the expected output (all FDs closed) + let expectedOutput = openedFDs.map { "- fd: \($0): CLOSED" }.joined(separator: "\n") + "\n" + XCTAssertEqual(expectedOutput, String(buffer: result.standardOutput!)) + } + // MARK: - Setup/teardown override func setUp() async throws { self.group = MultiThreadedEventLoopGroup(numberOfThreads: 3)