TEST: macOS low-fd verification (not for upstream) - #1
Closed
bernardladenthin wants to merge 18 commits into
Closed
Conversation
sheredom#104) * Fix building against glibc older than 2.29 posix_spawn_file_actions_addchdir_np arrived in glibc 2.29, so subprocess_create_ex fails to compile and link on older systems such as manylinux2014. Add a SUBPROCESS_HAVE_CWD probe, report a requested cwd as ENOSYS where the call is unavailable, and skip the cwd test there. * Allow overriding SUBPROCESS_HAVE_CWD and document it musl gained posix_spawn_file_actions_addchdir_np in 1.1.24 but exposes no version macro, so the detection cannot cover older releases. Skip it when the macro is already defined, letting those users set it themselves. * Fix building against macOS older than 10.15 posix_spawn_file_actions_addchdir_np arrived in macOS 10.15, so older SDKs neither declare nor export it and subprocess_create_ex fails to compile, as reported for MacPorts on PowerPC in sheredom#108. A newer SDK paired with an older deployment target does compile, but warns and leaves the binary referencing a symbol the target system does not have. Extend the SUBPROCESS_HAVE_CWD probe with a Darwin arm. It deliberately omits a defined() guard on MAC_OS_X_VERSION_MIN_REQUIRED: an absent macro folds to 0, so a toolchain that does not supply it answers "no cwd" rather than calling a function that may not be there. That also gives the right answer on iOS, tvOS and watchOS, where the SDKs mark the call unavailable outright.
GCC 8 implements C++20 under the pre-release spelling only. CMake still lists cxx_std_20 as supported there because it maps the feature onto -std=c++2a, so the feature list cannot tell us which spelling the compiler accepts. Probe the flag itself instead.
* Fix building against glibc older than 2.29 posix_spawn_file_actions_addchdir_np arrived in glibc 2.29, so subprocess_create_ex fails to compile and link on older systems such as manylinux2014. Add a SUBPROCESS_HAVE_CWD probe, report a requested cwd as ENOSYS where the call is unavailable, and skip the cwd test there. * Allow overriding SUBPROCESS_HAVE_CWD and document it musl gained posix_spawn_file_actions_addchdir_np in 1.1.24 but exposes no version macro, so the detection cannot cover older releases. Skip it when the macro is already defined, letting those users set it themselves. * Report a missing executable on glibc older than 2.24 * Restore the exec-error probe lost in the merge Merging main after sheredom#104 dropped the SUBPROCESS_SPAWN_REPORTS_EXEC_ERRORS block while keeping its use site, so the macro was undefined and folded to 0. That turned the access() pre-check on unconditionally, on every platform, which is the opposite of what this change intends.
…om#101) * Fix Windows argument quoting for backslash runs before quotes The command line builder doubles a backslash only when the next source character is a quote. Per the CRT parsing rules, every backslash in a run preceding a double quote must be doubled, and this includes a run that ends up before the generated closing quote of a quoted argument. As a result, a quoted argument ending in a backslash swallows its closing quote and merges with every following argument, a run of two or more backslashes before an embedded quote is under-escaped, and a quoted argument ending in several backslashes loses half of them. Count backslash runs in the length and emit passes and double the whole run before an escaped quote or before the closing quote. Extend the special argv test with the three cases plus a merge canary. * Simplify Windows argument quoting passes Rework the two command line builder passes without changing the emitted bytes. The length pass becomes a linear scan that carries the current backslash run, and the emit pass folds the duplicated write loops into the explicit 2n + 1 and 2n CRT formulas. Extend the special argv test with a quoted argument combining a space and a backslash run before an embedded quote. * Add mixed Windows backslash quoting test 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol --------- Co-authored-by: Neil Henning <me@neilhenning.dev>
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
Merges mehendarkarprajwal/subprocess.h@e6cec1b ("Initial port for AIX"). AIX provides no posix_spawn_file_actions_addchdir[_np], so process_cwd cannot be honoured through posix_spawn there; the child has to be forked so it can chdir() before exec. The branch was based on 8671cee and conflicted with five commits that have since touched subprocess.h (sheredom#102, sheredom#104, sheredom#105, sheredom#106, sheredom#110). Rather than replay the incoming restructuring of the posix_spawn path, the conflict is resolved in favour of main for that path, and the fork() implementation is added beside it. The result is +183/-7 against main instead of +227/-97: the posix_spawn code is left byte-identical, so no other platform can regress. Changes relative to the merged commit: * Select the implementation through SUBPROCESS_SPAWN_VIA_FORK, defaulting to 1 on AIX, instead of testing _AIX at each site. The path can then be forced on for testing on any POSIX platform, which is how it was validated: with -DSUBPROCESS_SPAWN_VIA_FORK=1 on glibc, all 441 tests pass. * Derive SUBPROCESS_HAVE_CWD and SUBPROCESS_SPAWN_REPORTS_EXEC_ERRORS from it. Both are true whenever the fork path is used, because the child chdir()s itself and the error pipe reports exec failures. Previously both landed on 1 for AIX through the unknown-platform #else, which happened to be right for the wrong reason. * Drop the duplicate function-scope declarations of posix_error and actions, which shadowed the ones in the new inner block and left actions_created without a use. As merged, that broke every non-AIX build under -Werror: three unused-variable errors at subprocess.h:1137-1139. * Name the child's fallback exit status SUBPROCESS_EXEC_FAILURE_STATUS rather than repeating 127 at six call sites. No header defines the value; 127 is the shell convention for a command that could not be executed, and what glibc's posix_spawn used before it learned to report exec failures. * Restart read() and waitpid() on EINTR, and require a full sizeof(int) from the error pipe. A short or interrupted read previously reported success for a child whose exec had failed. * Only close the pipe ends in the child once they have been duplicated, and only those above STDERR_FILENO, so a pipe end that already occupies 0, 1 or 2 is not closed out from under the descriptor just installed. * Route the child's failure paths through a single exit point and keep every call it makes async-signal-safe, which fork() in a threaded process requires. Not yet verified on AIX itself; the test evidence above is from glibc with the fork path forced.
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.
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.
🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
* Restrict Windows subprocess handle inheritance 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol * Fix Windows test security attribute cast 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol * Match Windows attribute list API types 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol * Verify inherited event identity in Windows test 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol * Only define Windows handle test on Windows 🧙 Conjured by AI via [pi.dev](https://pi.dev/) using gpt-5.6-sol
Review feedback on sheredom#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.
Review feedback on sheredom#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.
Review feedback on sheredom#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.
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.
Not for upstream. This exists so the fork's own CI compiles the combination we cannot reproduce locally: no macOS here, and the defect below is invisible on Linux because glibc clears FD_CLOEXEC on a self-duplication where Apple does not. Contents, which live in five separate upstream PRs: subprocess.h sheredom#112 (aix-port at eadfac5) + the low-fd fix this branch exists to test test/test.c sheredom#112's fd-0 regression test + a new invariant test: no pipe end may sit on 0, 1 or 2 test/utest.h utest.h #188 reworked, #189 and #190, vendored together The defect being tested: sheredom#115 creates the stdio pipes close-on-exec, and both implementations then move them onto 0, 1 and 2 with dup2 — or with posix_spawn_file_actions_adddup2, which Apple documents as behaving "as if dup2() had been called". A duplication onto the same descriptor is a no-op, so a pipe end that already landed on a standard descriptor keeps FD_CLOEXEC and exec closes the child's stream. macOS CI caught this on sheredom#112 through the fd-0 test; Linux and AIX cannot see it. subprocess_fds_above_std moves such an end out of the way, which removes the self-duplication for both implementations rather than patching each. Measured before pushing: the new invariant test goes red on unpatched main and green with the fix; the full suite is 444/444 on x86_64 glibc in both implementations, and the -D_AIX arm type-checks. macOS is what this branch is for.
Owner
Author
|
Test branch, its purpose is served: subprocess.h#118 is merged. |
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
5 participants
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.
Throwaway branch to reach a macOS runner. Combines subprocess.h sheredom#112 + the low-fd fix + both regression tests, with utest.h #188/#189/#190 vendored together.
The defect: sheredom#115 creates the stdio pipes close-on-exec, and both implementations then move them onto 0, 1 and 2. A duplication onto the same descriptor is a no-op, so an end that already landed on a standard descriptor keeps FD_CLOEXEC and exec closes the child's stream. glibc clears the flag there, Apple does not — so Linux and AIX cannot see this.
Locally: invariant test red without the fix, green with it; 444/444 on x86_64 in both implementations. macOS is what this run is for.
Close when answered.