Skip to content

Commit 3f4a6a8

Browse files
authored
AsyncProcess: improve, fix & test fd close loop (#223)
- Replaces the `highest_possibly_open_fd` function for Linux with one that doesn't allocate - Added a test to test that we're actually closing all those fds in the child - This revealed a long-standing off-by-one bug in the driver loop to close the fds!
1 parent 810f0c1 commit 3f4a6a8

File tree

3 files changed

+121
-2
lines changed

3 files changed

+121
-2
lines changed

Sources/CProcessSpawnSync/internal-helpers.h

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,8 @@
1313
#ifndef INTERNAL_HELPERS_H
1414
#define INTERNAL_HELPERS_H
1515
#include <signal.h>
16+
#include <unistd.h>
17+
#include <fcntl.h>
1618

1719
static int positive_int_parse(const char *str) {
1820
int out = 0;
@@ -49,14 +51,61 @@ static int highest_possibly_open_fd_dir(const char *fd_dir) {
4951
return highest_fd_so_far;
5052
}
5153

54+
#if defined(__linux__)
55+
// Linux-specific version that uses syscalls directly and doesn't allocate heap memory.
56+
// Safe to use after vfork() and before execve()
57+
static int highest_possibly_open_fd_dir_linux(const char *fd_dir) {
58+
int highest_fd_so_far = 0;
59+
int dir_fd = open(fd_dir, O_RDONLY);
60+
if (dir_fd < 0) {
61+
// errno set by `open`.
62+
return -1;
63+
}
64+
65+
// Buffer for directory entries - allocated on stack, no heap allocation
66+
char buffer[4096] = {0};
67+
long bytes_read = -1;
68+
69+
while ((bytes_read = getdents64(dir_fd, (struct dirent64 *)buffer, sizeof(buffer))) > 0) {
70+
if (bytes_read < 0) {
71+
if (errno == EINTR) {
72+
continue;
73+
} else {
74+
// `errno` set by getdents64.
75+
highest_fd_so_far = -1;
76+
goto error;
77+
}
78+
}
79+
long offset = 0;
80+
while (offset < bytes_read) {
81+
struct dirent64 *entry = (struct dirent64 *)(buffer + offset);
82+
83+
// Skip "." and ".." entries
84+
if (entry->d_name[0] != '.') {
85+
int number = positive_int_parse(entry->d_name);
86+
if (number > highest_fd_so_far) {
87+
highest_fd_so_far = number;
88+
}
89+
}
90+
91+
offset += entry->d_reclen;
92+
}
93+
}
94+
95+
error:
96+
close(dir_fd);
97+
return highest_fd_so_far;
98+
}
99+
#endif
100+
52101
static int highest_possibly_open_fd(void) {
53102
#if defined(__APPLE__)
54103
int hi = highest_possibly_open_fd_dir("/dev/fd");
55104
if (hi < 0) {
56105
hi = getdtablesize();
57106
}
58107
#elif defined(__linux__)
59-
int hi = highest_possibly_open_fd_dir("/proc/self/fd");
108+
int hi = highest_possibly_open_fd_dir_linux("/proc/self/fd");
60109
if (hi < 0) {
61110
hi = getdtablesize();
62111
}

Sources/CProcessSpawnSync/spawner.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,7 @@ static void setup_and_execve_child(ps_process_configuration *config, int error_p
169169
#endif
170170
if (close_range_err) {
171171
// close_range failed (or doesn't exist), let's fall back onto this
172-
for (int i=config->psc_fd_setup_count; i<highest_possibly_open_fd(); i++) {
172+
for (int i=config->psc_fd_setup_count; i<=highest_possibly_open_fd(); i++) {
173173
if (i != error_pipe) {
174174
close(i);
175175
}

Tests/AsyncProcessTests/IntegrationTests.swift

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1333,6 +1333,76 @@ final class IntegrationTests: XCTestCase {
13331333
}
13341334
}
13351335

1336+
func testVeryHighFDs() async throws {
1337+
var openedFDs: [CInt] = []
1338+
1339+
// Open /dev/null to use as source for duplication
1340+
let devNullFD = open("/dev/null", O_RDONLY)
1341+
guard devNullFD != -1 else {
1342+
XCTFail("Failed to open /dev/null")
1343+
return
1344+
}
1345+
defer {
1346+
let closeResult = close(devNullFD)
1347+
XCTAssertEqual(0, closeResult, "Failed to close /dev/null FD")
1348+
}
1349+
1350+
for candidate in sequence(first: CInt(1), next: { $0 <= CInt.max / 2 ? $0 * 2 : nil }) {
1351+
// Use fcntl with F_DUPFD to find next available FD >= candidate
1352+
let fd = fcntl(devNullFD, F_DUPFD, candidate)
1353+
if fd == -1 {
1354+
// Failed to allocate FD >= candidate, try next power of 2
1355+
self.logger.debug(
1356+
"already unavailable, skipping",
1357+
metadata: ["candidate": "\(candidate)", "errno": "\(errno)"]
1358+
)
1359+
continue
1360+
} else {
1361+
openedFDs.append(fd)
1362+
self.logger.debug("Opened FD in parent", metadata: ["fd": "\(fd)"])
1363+
}
1364+
}
1365+
1366+
defer {
1367+
for fd in openedFDs {
1368+
let closeResult = close(fd)
1369+
XCTAssertEqual(0, closeResult, "Failed to close FD \(fd)")
1370+
}
1371+
}
1372+
1373+
// Create shell script that checks each FD passed as arguments
1374+
let shellScript = """
1375+
for fd in "$@"; do
1376+
if [ -e "/proc/self/fd/$fd" ] || [ -e "/dev/fd/$fd" ]; then
1377+
echo "- fd: $fd: OPEN"
1378+
else
1379+
echo "- fd: $fd: CLOSED"
1380+
fi
1381+
done
1382+
"""
1383+
1384+
var arguments = ["-c", shellScript, "--"]
1385+
arguments.append(contentsOf: openedFDs.map { "\($0)" })
1386+
1387+
let result = try await ProcessExecutor.runCollectingOutput(
1388+
group: self.group,
1389+
executable: "/bin/sh",
1390+
arguments,
1391+
standardInput: EOFSequence(),
1392+
collectStandardOutput: true,
1393+
collectStandardError: true,
1394+
logger: self.logger
1395+
)
1396+
try result.exitReason.throwIfNonZero()
1397+
1398+
// Assert stderr is empty
1399+
XCTAssertEqual("", String(buffer: result.standardError!))
1400+
1401+
// Assert stdout contains exactly the expected output (all FDs closed)
1402+
let expectedOutput = openedFDs.map { "- fd: \($0): CLOSED" }.joined(separator: "\n") + "\n"
1403+
XCTAssertEqual(expectedOutput, String(buffer: result.standardOutput!))
1404+
}
1405+
13361406
// MARK: - Setup/teardown
13371407
override func setUp() async throws {
13381408
self.group = MultiThreadedEventLoopGroup(numberOfThreads: 3)

0 commit comments

Comments
 (0)