From 8550ee6a31726832218f7f3cc2e473603a5eb1d2 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 20 Jul 2026 13:47:25 +0200 Subject: [PATCH 1/2] util, refactor: Add ChildFail helper for post-fork child errors Factor out the write-message-then-_exit(126) pattern used when a syscall fails in the post-fork child of SpawnProcess, and document its constraints (async-signal-safe calls only, message must not allocate) in one place. The execvp failure path is intentionally left alone: replacing perror() there would lose the errno string, and improving it is already covered by the existing TODO about reporting errors to the parent via a pipe. Co-Authored-By: Claude Fable 5 --- src/mp/util.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 42cda9ce..0be367fc 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -60,6 +60,18 @@ size_t MaxFd() } } +//! Report an error and exit from the post-fork child of a multi-threaded +//! process, where only async-signal-safe calls (like write and _exit) are +//! allowed. Accepting only a reference to a char array (in practice a string +//! literal) ensures no allocation is needed at the call site. +template +[[noreturn]] void ChildFail(const char (&msg)[N]) noexcept +{ + const ssize_t written = ::write(STDERR_FILENO, msg, N - 1); + (void)written; + _exit(126); +} + } // namespace std::string ThreadName(const char* exe_name) @@ -147,10 +159,7 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ (void)close(fds[1]); throw std::system_error(errno, std::system_category(), "close"); } - static constexpr char msg[] = "SpawnProcess(child): close(fds[1]) failed\n"; - const ssize_t writeResult = ::write(STDERR_FILENO, msg, sizeof(msg) - 1); - (void)writeResult; - _exit(126); + ChildFail("SpawnProcess(child): close(fds[1]) failed\n"); } if (!pid) { From 1e0c7ff9a51aedf9ddbda13dc3d8f03428da1c87 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Mon, 20 Jul 2026 13:49:29 +0200 Subject: [PATCH 2/2] util: Clear FD_CLOEXEC in child instead of parent before fork SpawnProcess cleared FD_CLOEXEC on the child's socket in the parent before forking. Between that fcntl() and fork(), a concurrent fork+exec on another thread would snapshot the descriptor with the close-on-exec flag already cleared, leaking it into an unrelated child process where it survives exec. A leaked duplicate of the socket also prevents the parent from seeing EOF when the spawned process exits. Instead, keep both descriptors close-on-exec in the parent for their entire lifetime and have the intended child clear the flag on its own copy of the descriptor between fork() and exec(). Co-Authored-By: Claude Fable 5 --- src/mp/util.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/mp/util.cpp b/src/mp/util.cpp index 0be367fc..f524ab23 100644 --- a/src/mp/util.cpp +++ b/src/mp/util.cpp @@ -142,11 +142,6 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ const std::vector args{connect_info_to_args(std::to_string(fds[0]))}; const std::vector argv{MakeArgv(args)}; - // Clear FD_CLOEXEC on fds[0] before forking so it survives exec in the child. - int fds0_flags; - KJ_SYSCALL(fds0_flags = fcntl(fds[0], F_GETFD)); - KJ_SYSCALL(fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC)); - ProcessId pid = fork(); if (pid == -1) { throw std::system_error(errno, std::system_category(), "fork"); @@ -172,6 +167,16 @@ std::tuple SpawnProcess(SpawnConnectInfoToArgsFn&& connect_ } } + // Clear FD_CLOEXEC on socket 0 so it survives exec. Doing this here + // rather than in the parent before forking avoids a window where a + // concurrent fork in another thread would leak the descriptor into an + // unrelated child process (which would not clear it). + // fcntl is async-signal-safe. + const int fds0_flags = fcntl(fds[0], F_GETFD); + if (fds0_flags == -1 || fcntl(fds[0], F_SETFD, fds0_flags & ~FD_CLOEXEC) == -1) { + ChildFail("SpawnProcess(child): clearing FD_CLOEXEC failed\n"); + } + execvp(argv[0], argv.data()); // NOTE: perror() is not async-signal-safe; calling it here in a // post-fork child may deadlock in multithreaded parents.