Prevent POSIX subprocesses from inheriting pipes - #115
Merged
Conversation
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
bernardladenthin
added a commit
to bernardladenthin/subprocess.h
that referenced
this pull request
Aug 14, 2026
The merge brings in sheredom#115, which creates the stdio pipes close-on-exec. The forked child relies on dup2 to strip that flag when it installs them on 0, 1 and 2 — and dup2 does clear it, except when source and target are the same descriptor, where it is a no-op. That case is reachable: with fd 0 free in the parent, the stdin pipe's read end lands on 0 and the child's dup2(stdinfd[0], STDIN_FILENO) becomes dup2(0, 0). exec then closes the child's stdin. Measured with a parent that closes fd 0 before subprocess_create: this branch before the merge works, upstream main works, the merge of the two does not, and clearing the flag fixes it. fcntl is async-signal-safe, so the child stays within what it may call before exec. Nothing in the suite could see this. It stayed at 442/442 before and after the fix, in both implementations, because no test ran with a standard descriptor closed. The test added here does: it closes fd 0 around a subprocess_create and asserts the child still has a stdin, using the process_is_fd_open helper sheredom#115 brought along. It goes red on the fork path without the fix above and green with it; on the posix_spawn path, which never had the defect, it is green either way. The suite is now 443/443. The test saves and restores fd 0 and defers its assertions until after the restore — an assertion returns early in utest, which would otherwise leave every later test in the binary without a stdin.
This was referenced Aug 14, 2026
sheredom
pushed a commit
that referenced
this pull request
Aug 18, 2026
#115 creates the stdio pipes close-on-exec, and subprocess_create_ex then installs them on 0, 1 and 2. Duplicating a descriptor onto itself is a no-op, so an end that already landed on a standard descriptor keeps FD_CLOEXEC and exec closes the child's stream. It happens whenever the parent runs with one of those descriptors free. posix_spawn_file_actions_adddup2 is documented on macOS as behaving "as if dup2() had been called", with no special case for equal descriptors, and that is where it bites. glibc applies the POSIX clarification and clears the flag, which is why no Linux runner shows this. subprocess_fds_above_std moves such an end out of the way at creation time. That removes the self-duplication for every caller rather than patching each site, and it needs no platform test. The added test checks the precondition rather than the symptom: no pipe end may sit on 0, 1 or 2. That is observable everywhere, and it goes red on unpatched main — the symptom itself cannot be reproduced off macOS. Measured: full suite green on x86_64 glibc, and green on macOS in a run that combined this with the AIX branch and the pending utest.h changes, where the same suite had been failing on exactly this before.
sheredom
pushed a commit
that referenced
this pull request
Aug 19, 2026
* Initial port for AIX * Detect both the presence and the spelling of the chdir file action Adds SUBPROCESS_ADDCHDIR_IS_POSIX, which says whether the platform provides posix_spawn_file_actions_addchdir under its POSIX 2024 name rather than the older ..._np one, and extends SUBPROCESS_SPAWN_VIA_FORK to the platforms that provide neither and therefore need fork()+exec(). The call site now asks those two macros instead of naming platforms itself. SUBPROCESS_ADDCHDIR_IS_POSIX macOS 26+, NetBSD 10+ SUBPROCESS_SPAWN_VIA_FORK AIX, OpenBSD, NetBSD < 10 The reason for two macros rather than one is that platforms differ in two independent ways: whether the chdir file action exists at all, and what it is called. Taken from each vendor's own spawn.h and libc: FreeBSD 13.1, 13.2, 14.3, 15.0 ..._addchdir_np NetBSD 10.0, 10.1 ..._addchdir, no _np NetBSD 9.3, 9.4 neither OpenBSD 7.7, 7.8, 7.9 neither AIX 7.2 TL04 neither, in header and in libc This also corrects the call site's previous test, defined(__NetBSD__) with no version check, which was right for NetBSD 10 and wrong for 9 and older, where neither spelling exists. __NetBSD_Version__ comes from <sys/param.h>, not from the compiler's predefines, so that header is now included on NetBSD before the probes run. If it is ever absent the version folds to 0 and every NetBSD takes the fork path: heavier than needed, never wrong. Verified by compiling against sysroots built from each vendor's release sets, so nothing here rests on documentation. Every C source of the test suite compiles for NetBSD 9.4 and 10.1, OpenBSD 7.9 and FreeBSD 14.3 under the flags CMakeLists assigns. macOS 10.13 through 14.0 compile through osxcross against a real 15.5 SDK, with SUBPROCESS_HAVE_CWD 0 below 10.15 and 1 from 10.15 up; the macOS 26 branch cannot be checked because no mirrored SDK goes that far. Unchanged where it was already working: Linux glibc and musl 441/441, AIX 7.2 on POWER8 441/441, Windows MSVC 387/387 in 32- and 64-bit and against both CRTs. Forcing -DSUBPROCESS_SPAWN_VIA_FORK=1 on glibc also passes 441/441, which is how the fork path is exercised without leaving Linux. clang's static analyser reports nothing on either path, and a purpose-built test spawning from eight threads, 640 spawns, passes on both paths under AddressSanitizer. * Skip the divide-by-zero assertion on PowerPC and RISC-V The test already excludes AArch64 because SDIV by zero yields 0 instead of trapping. Two more architectures behave the same way, and are specified to: PowerPC's divw/divd leave the result undefined and raise no exception unless OE is set, and RISC-V's DIV returns -1, all bits set. On both the child exits 0, so subprocess_fail_divzero cannot hold. Measured with the same source and compiler on each architecture rather than taken from the manuals. x86_64, i386, armv7l, armv6 and s390x die on signal 8; aarch64 exits 0 printing 0, ppc64le exits 0, and riscv64 exits 0 printing -1 — each matching what its ISA specifies. Reproduced on AIX 7.2 TL04 on POWER8, where the suite now passes 441 of 441. 32-bit ARM was checked deliberately and does trap, including ARMv6, which has no divide instruction at all and goes through __aeabi_idiv. The plausible assumption that a software divide routine returns 0 quietly is wrong, so __arm__ is not excluded. _ARCH_PPC is listed beside __powerpc__ because IBM XL C and OpenXL define only the former, and that is the compiler most AIX users reach for. * Shorten the two probe comments the review flagged Review feedback on #112. The paragraph above the <sys/param.h> include is gone. It explained that __NetBSD_Version__ is not a compiler predefine and that omitting the header would fold the version to 0 — true, but the include sits directly under #if defined(__NetBSD__) and needs no defence. The SUBPROCESS_SPAWN_VIA_FORK comment keeps what the macro does, why forking buys anything, and that it can be overridden. Dropped: the list of which platforms lack addchdir, which the #if immediately below states in code, and the sentence about the capabilities this enables further down. Comments only. Preprocessing both revisions with -fpreprocessed -dD -E -P leaves 1563 identical lines of code. Rebuilt anyway: 441/441 on x86_64 glibc, and 441/441 again with -DSUBPROCESS_SPAWN_VIA_FORK=1. * Drop the exec-failure status macro and exit 127 directly Review feedback on #112: "Why do we even need this define?" We do not. The value was used exactly once, no test referenced it, the README never mentioned it, and it only ever reaches a caller down a path that practically cannot happen — the parent reads the real errno off the pipe and reaps the child with a discarded status, so 127 surfaces only if that 4-byte write into an empty pipe is lost. Making it configurable was worse than pointless. POSIX requires posix_spawn's child to exit with status 127 when exec fails, so 127 is what keeps the fork path indistinguishable from the posix_spawn path for a caller. The knob's only possible use was to break that equivalence, and it offered nothing on the posix_spawn side, where the value comes from libc and cannot be overridden. The macro was introduced by this PR and never released, so nothing depends on it. 441/441 on x86_64 glibc, and again with -DSUBPROCESS_SPAWN_VIA_FORK=1. * Declare execvpe above the function instead of inside it Review feedback on #112: "Could we only extern to this in a define block for AIX above the function instead then?" Moved out of the function body, but guarded on SUBPROCESS_SPAWN_VIA_FORK rather than _AIX. Restricting it to AIX breaks glibc: execvpe is a GNU extension there, so <unistd.h> only declares it when _GNU_SOURCE is set, and a consumer forcing the fork path without that macro would lose the declaration. Measured both ways with -Werror=implicit-function-declaration — the AIX-only variant fails without _GNU_SOURCE and builds with it. The suite cannot see this either way, since test/CMakeLists.txt defines _GNU_SOURCE on Linux. musl hides execvpe the same way. The declaration is also no longer AIX-specific in what it claims: OpenBSD and NetBSD reach this code too, and both do have execvpe. 441/441 on x86_64 glibc, and again with -DSUBPROCESS_SPAWN_VIA_FORK=1. The -D_AIX arm still type-checks, and the fork path compiles on glibc and musl in C and C++, with and without _GNU_SOURCE — the C++ runs matter because glibc declares execvpe __THROW, and a file-scope redeclaration without noexcept would clash where the in-body one did not. * Clear FD_CLOEXEC on the child's standard descriptors after dup2 The merge brings in #115, which creates the stdio pipes close-on-exec. The forked child relies on dup2 to strip that flag when it installs them on 0, 1 and 2 — and dup2 does clear it, except when source and target are the same descriptor, where it is a no-op. That case is reachable: with fd 0 free in the parent, the stdin pipe's read end lands on 0 and the child's dup2(stdinfd[0], STDIN_FILENO) becomes dup2(0, 0). exec then closes the child's stdin. Measured with a parent that closes fd 0 before subprocess_create: this branch before the merge works, upstream main works, the merge of the two does not, and clearing the flag fixes it. fcntl is async-signal-safe, so the child stays within what it may call before exec. Nothing in the suite could see this. It stayed at 442/442 before and after the fix, in both implementations, because no test ran with a standard descriptor closed. The test added here does: it closes fd 0 around a subprocess_create and asserts the child still has a stdin, using the process_is_fd_open helper #115 brought along. It goes red on the fork path without the fix above and green with it; on the posix_spawn path, which never had the defect, it is green either way. The suite is now 443/443. The test saves and restores fd 0 and defers its assertions until after the restore — an assertion returns early in utest, which would otherwise leave every later test in the binary without a stdin. * Drop the FD_CLOEXEC clearing now that pipe ends avoid 0, 1 and 2 subprocess_fds_above_std, added in #118, moves every pipe end above STDERR on both pipe-creation paths, so dup2 in the child can no longer be a no-op on a standard descriptor. Reverts the addition from this branch as promised in #118. --------- Co-authored-by: mehendarkarprajwal <prajwal.b.mehendarkar@ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pipe2(O_CLOEXEC)where available, with apipe/fcntlfallbackFixes #114.
Testing
ctest --test-dir build --output-on-failure0 1 2 3 4🧙 Conjured by AI via pi.dev using gpt-5.6-sol