Repository navigation
fix: report an exec failure as a spawn error, not a SIGABRT child - #566
Merged
Merged
Conversation
7 tasks done
vyncint
force-pushed
the
fix/exec-failure-is-a-spawn-error
branch
from
October 14, 2026 09:40
2aae388 to
12a4fed
Compare
On Unix, a program that exists but cannot be executed made `spawn` return Ok with a child already killed by SIGABRT. Examples are a missing #! interpreter, a shebang ending in \r from CRLF line endings, a missing dynamic loader, and an argument list over the kernel's limit. portable-pty's pre_exec closes every descriptor above stdio, and std's exec-error pipe is one of them, so the child aborted on the write that would have reported the failure. termlens inspect showed it as "exited: killed by signal: Aborted" and exited 0. On Unix termlens now starts the child itself (src/spawn.rs). It builds the command portable-pty would build: the same program resolution, working directory, environment, session and signals. The one change is that inherited descriptors are marked close-on-exec instead of closed. A successful exec closes the same set as before, and a failed one reports through std. The error names the culprit: the #! interpreter, shown escaped so that a stray \r is visible, or the dynamic loader. Cases the new path is not sure of still go to portable-pty unchanged, with its diagnostics, and Windows is untouched. The responder thread's duplicate of the PTY master is now opened with F_DUPFD_CLOEXEC. As a plain dup it leaked into every other child the test process started while a terminal was alive. Closes #563 Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The diagnosis table gains the message termlens now gives for a script whose #! interpreter does not exist, and what a \r at the end of the quoted name means. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
vyncint
force-pushed
the
fix/exec-failure-is-a-spawn-error
branch
from
October 14, 2026 09:49
12a4fed to
5568427
Compare
AGENTS.md's layout names the module that now starts the child on Unix, and lists it with the files to read DESIGN before touching. CONTRIBUTING §3 asks a change to it for the stress workflow, as it asks of wait semantics: every test in the suite goes through it. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Review findings on the new spawn path: - A failure before the exec no longer reads "cannot execute <program>". That covers a NUL in a variable, a directory the child cannot enter, and the fork itself. Such a failure reads as std reports it, and an unenterable directory is named. - EACCES from an interpreter that is not a program names the interpreter. A script that can be run but not read names both suspects instead of guessing the loader. The #! line now ends at a NUL, and on macOS at '#', as the kernels read it, from 512 bytes. - A working directory that is not a directory gives way to the child's HOME, as in portable-pty, instead of handing the spawn back to portable-pty. A process whose children the kernel reaps (SIGCHLD ignored, or SA_NOCLDWAIT) is handed back, because std would panic reporting the failure. - A unit test starts one probe through both spawns and requires the same argv[0], directory, environment, descriptors, umask, process group and controlling terminal, so a portable-pty update cannot drift unseen. - dup_writer duplicates with BorrowedFd::try_clone_to_owned, which is close-on-exec, and one unsafe block replaces two. - XTGETTCAP's TN reports the last TERM set, the one the child gets. - The descriptor probe runs in a hermetic environment and reads a line before it exits. The E2BIG test no longer Debug-prints a 2 MiB command line when it fails. - DESIGN, SECURITY and the CHANGELOG describe the remaining fallback cases, the lifecycle lock now held through the exec, the openpty close-on-exec window, and the upstream issue (wezterm/wezterm#7893). Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
The comparison test stopped reading once "probe-finished" had arrived, and a read can end between the marker and its line ending. That side was then one line shorter, and the two spawns looked different. It failed on CI and on every Linux stress shard. Locally it failed 6 times in 200 concurrent runs before this change and 0 times in 800 after. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
On macOS the comparison test hung on every stress shard until the 30-minute job limit. Its teardown dropped the lifecycle guard before the reader and writer, which are duplicates of the master, so the last master close, the one macOS revokes the device on, happened outside the lock. The next probe opened a PTY at once, and a late revoke landing on that recycled device is the hazard DESIGN.md describes. Every master handle now closes under the lock, as a Terminal's do. Each probe also runs on a thread of its own under a 60-second deadline. A hang then fails the test, naming the step it hung in and what the probe had printed, instead of stalling the suite. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
With the lock fix in, macOS reported where the probe hung: in wait(), after the test had answered the shell's read and stopped reading. The terminal echoes the answer, and on macOS a process exiting with output still queued waits for it to be read. The shell could not finish exiting while the test waited for it. The probe now reads to the end, as a Terminal's reader does, before it waits. Both probes now run even when the first fails. A probe that hangs reports the shell's state from ps and the terminal's from stty, beside what it printed. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.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
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.
What & why
On Unix, a program that exists but cannot be executed made
spawnreturnOkwith a child already dead of SIGABRT.termlens inspectdrew a screenholding
fatal runtime error: assertion failed: output.write(&bytes).is_ok(),reported
exited: killed by signal: Aborted, and exited 0. portable-pty'spre_execcloses every descriptor above stdio, and std's close-on-execexec-error pipe is one of them (upstream: wezterm/wezterm#7893, still open).
On Unix termlens now starts the child itself (
src/spawn.rs), building thecommand portable-pty 0.9.0 would build: the same resolution,
argv[0],working directory (with portable-pty's fallback to the child's
HOME),environment, session and signals. Descriptors above stdio are marked
close-on-exec instead of closed, so a failed exec reports through std. A
unit test starts one probe through both spawns and requires the same
answers (argv[0], directory, environment names plus a checksum of the
values, descriptors, umask, process group, controlling terminal), so a
portable-pty update cannot drift away unseen.
The error names the culprit where the errno would point at the wrong file:
A failure before the exec (a NUL in a variable, the fork) reads as std
reports it, as before. Four rare setups still go to portable-pty unchanged,
and
docs/DESIGN.mdlists them: children reaped by the kernel (std wouldpanic), no usable directory and no
HOME, a program that does not resolve(portable-pty's own diagnosis), and a slave that cannot be opened by name.
Also in this PR:
dup_writerduplicates the master with std's close-on-execBorrowedFd::try_clone_to_owned. Its plaindupleaked into every otherchild the test process started while a terminal was alive.
TNreports the lastTERMset, the one the child gets.SECURITY.mdlists the fiveunsafeblocks:borrow_raw,access(2),a
sigaction(2)query,kill(2), and thepre_exechook. DESIGN §1gains "On Unix termlens starts the child itself", which covers the
lifecycle lock now held through the exec and the openpty close-on-exec
window in the PTY layer. AGENTS.md and CONTRIBUTING §3 point at
spawn.rs, and the skill's failure table gains the new message.Closes #563
Review
The code-review skill ran at max effort on the first version and returned
15 findings. All of them are addressed in 95f1e77, or documented where they
are inherent:
That is the price of reporting the exec at all.
set_var.CI then found two problems in the new comparison test itself, both fixed:
a process exiting there waits for its queued output (the echoed answer)
to be read, so every macOS stress shard hung (088aca6, b6c101a).
Each probe now closes its PTY under the lifecycle lock and runs under a
deadline. A hang reports
psandsttystate instead of stalling the job.Tests
Each was checked to fail without its part of the fix:
process.rs: a missing interpreter, a CRLF interpreter, an interpreterthat is not a program, an execute-only script, an E2BIG argument, a NUL
in a variable and an unenterable directory.
descriptors.rs, a binary of its own: no termlens descriptor reaches anordinary child, and none of the test's reaches a termlens child.
inspect.rs: the example andtermlens inspectboth exit 2 on a missinginterpreter.
queries.rs:TERMset twice.spawn.rsunit test: the two spawns cannot be told apart. It fails on achanged
argv[0], a missingsetsid, or a changed variable value.Verification
covering render in five formats over 104 saved screens, diff pairs, the
CLI surface, and 35 inspect programs. 967 are identical; the two that
differ are the missing-interpreter and CRLF cases. Also checked by hand:
the SIGCHLD-ignored case matches 0.11.5, and a deleted working directory
runs in HOME as before.
-D warningsfor Linux,x86_64-pc-windows-msvcandaarch64-apple-darwin; the MSRV checks at 1.85.1; the full suite, 625passed and 0 failed; preflight green on the previous head.
Checklist
cargo fmt --allandcargo clippy --workspace --all-targets --all-featuresare cleangit commit -s) — see CONTRIBUTING.md §5CHANGELOG.mdupdated under[Unreleased](user-facing changes only)cargo insta review, not blind-accepted