From 15500e0b9f954f845f8e43589b3607abb1dff042 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 11 Oct 2026 12:45:31 +0700 Subject: [PATCH 1/7] fix: an exec failure is a spawn error, not a child dead of SIGABRT 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> --- CHANGELOG.md | 19 ++ SECURITY.md | 28 +-- crates/termlens/src/lib.rs | 1 + crates/termlens/src/spawn.rs | 257 +++++++++++++++++++++++++++ crates/termlens/src/terminal.rs | 47 ++--- crates/termlens/tests/common/mod.rs | 17 ++ crates/termlens/tests/descriptors.rs | 91 ++++++++++ crates/termlens/tests/inspect.rs | 28 +++ crates/termlens/tests/process.rs | 60 +++++++ docs/DESIGN.md | 41 +++++ 10 files changed, 557 insertions(+), 32 deletions(-) create mode 100644 crates/termlens/src/spawn.rs create mode 100644 crates/termlens/tests/descriptors.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index daed3ea..1ee7782 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,25 @@ reads that marker. ## [Unreleased] +### Fixed + +- On Unix, a program that exists but cannot be executed now fails + `TerminalBuilder::spawn` with an `Error::Spawn` that says why, and + `termlens inspect` exits 2 with that message (#563). Typical causes are a + missing `#!` interpreter, a missing dynamic loader, and an argument list + over the kernel's limit. The message names the interpreter, with any `\r` + that CRLF line endings left on it shown escaped. `spawn` used to return + `Ok` with a child already killed by SIGABRT (`fatal runtime error: + assertion failed: output.write(&bytes).is_ok()`), which inspect showed as + `exited: killed by signal: Aborted`. portable-pty closed the pipe std + reports a failed exec through, so on Unix termlens now starts the child + itself. Program resolution, the environment, the session and the + signals are unchanged, and the child still inherits no descriptor besides + its terminal. +- The second writer termlens opens on the PTY master is close-on-exec. A + process the test started while a terminal was alive inherited it before, + which kept the master open in a process unrelated to the terminal. + ## [0.11.5] - 2026-10-07 ### Changed diff --git a/SECURITY.md b/SECURITY.md index 888f844..ef9866a 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -10,17 +10,23 @@ issues in escape-sequence handling) are treated as security issues. What the project does continuously, enforced by required CI on every change: -- **`unsafe` is confined to three audited single-line blocks** — `dup(2)`, - to open a second writer on the PTY master for the responder thread; - `File::from_raw_fd`, which takes sole ownership of that duplicate so it is - closed exactly once; and `kill(2)` behind `Terminal::signal`. They sit - under `SAFETY` comments (the first two share one) and each carries its own - `#[allow(unsafe_code)]`. The workspace sets `unsafe_code = "warn"` and CI - runs clippy with `-D warnings`, so a new block fails unless it carries - that `#[allow]` as well: the attribute is what review has to see, not a - count the lint keeps. None touches memory the child can influence. - Everything on the parsing path is pure Rust: escape sequences from the - child are parsed, never executed or evaluated. +- **`unsafe` is confined to five audited blocks, all Unix-only.** Four are + single lines. `fcntl(2)` with `F_DUPFD_CLOEXEC` opens a second writer on + the PTY master for the responder thread. `File::from_raw_fd` takes sole + ownership of that duplicate, so it is closed exactly once. `access(2)` + resolves the program the way the PTY layer does, and `kill(2)` sits + behind `Terminal::signal`. The fifth is the `pre_exec` hook in + `src/spawn.rs`, which runs in the forked child before exec. It repeats + portable-pty's own hook step by step, with one change: inherited + descriptors are marked close-on-exec rather than closed, so that a failed + exec can be reported (#563). Each block sits under a `SAFETY` comment + (the first two share one) and carries its own `#[allow(unsafe_code)]`. + The workspace sets `unsafe_code = "warn"` and CI runs clippy with + `-D warnings`, so a new block fails unless it carries that `#[allow]` as + well: the attribute is what review has to see, not a count the lint + keeps. None touches memory the child can influence. Everything on the + parsing path is pure Rust: escape sequences from the child are parsed, + never executed or evaluated. - **Dependency policy** (`cargo-deny` job): RUSTSEC advisories, yanked crates, license allowlist, and crates.io-only sources — on every PR, with weekly grouped Dependabot updates and Dependabot alerts enabled. diff --git a/crates/termlens/src/lib.rs b/crates/termlens/src/lib.rs index 79b8572..d2ae601 100644 --- a/crates/termlens/src/lib.rs +++ b/crates/termlens/src/lib.rs @@ -112,6 +112,7 @@ mod error; mod graphics; mod keys; mod screen; +mod spawn; mod terminal; mod utf8; mod wait; diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs new file mode 100644 index 0000000..e5abbec --- /dev/null +++ b/crates/termlens/src/spawn.rs @@ -0,0 +1,257 @@ +//! Starting the child inside the PTY. +//! +//! On Unix termlens forks and executes the child itself instead of handing +//! the command to the PTY layer's `spawn_command`. Between fork and exec, +//! portable-pty closes every descriptor above stdio, and one of them is the +//! pipe through which std reports a failed `exec`. A program that exists but +//! cannot be executed, such as a script whose `#!` interpreter is missing, +//! then died of a runtime abort, and `spawn` returned `Ok` with a child +//! already killed by SIGABRT (#563). +//! +//! So the command is built the way the PTY layer builds it, with the same +//! program resolution, working directory, environment, session and signals, +//! and one difference: descriptors above stdio are marked close-on-exec +//! instead of closed. A successful exec closes the same set the PTY layer +//! closed, and a failed one can still report why. +//! +//! Whatever this path is not sure of goes to the PTY layer unchanged: a +//! working directory that is not a directory (the PTY layer substitutes the +//! home directory), a program that does not resolve (the PTY layer says why, +//! in the words termlens has always reported), a slave device it cannot name +//! or open. Windows always goes there, because ConPTY reports a failed +//! `CreateProcess` directly. + +use std::ffi::OsString; + +use portable_pty::{Child, CommandBuilder, PtyPair}; + +/// A child running in the PTY, as the PTY layer hands one back. +pub(crate) type PtyChild = Box; + +/// Start `cmd` on the slave side of `pair`, with `env` set on top of the +/// inherited environment, or on top of nothing when `env_clear` is set. +/// Later entries in `env` win. +pub(crate) fn spawn_child( + pair: &PtyPair, + mut cmd: CommandBuilder, + env_clear: bool, + env: &[(OsString, OsString)], +) -> Result { + if env_clear { + cmd.env_clear(); + } + for (key, value) in env { + cmd.env(key, value); + } + #[cfg(unix)] + if let Some(spawned) = unix::spawn(pair.master.as_ref(), &cmd, env_clear, env) { + return spawned; + } + pair.slave + .spawn_command(cmd) + .map_err(|error| error.to_string()) +} + +#[cfg(unix)] +mod unix { + use std::ffi::{CString, OsStr, OsString}; + use std::io::{self, Read}; + use std::os::unix::ffi::OsStrExt; + use std::os::unix::fs::OpenOptionsExt; + use std::os::unix::process::CommandExt; + use std::path::{Component, Path, PathBuf}; + use std::process::Command; + + use portable_pty::{CommandBuilder, MasterPty}; + + use super::PtyChild; + + /// Spawn `cmd` the way the PTY layer would, or `None` to leave it to the + /// PTY layer. + pub(super) fn spawn( + master: &dyn MasterPty, + cmd: &CommandBuilder, + env_clear: bool, + env: &[(OsString, OsString)], + ) -> Option> { + let argv = cmd.get_argv(); + let program = argv.first()?; + let dir = Path::new(cmd.get_cwd()?); + if !dir.is_dir() { + return None; + } + let resolved = resolve(program, dir, cmd.get_env("PATH"))?; + // A second handle on the slave, by name, because the PTY layer does + // not expose its own. O_NOCTTY keeps the device from becoming this + // process's controlling terminal; std adds O_CLOEXEC. + let slave = std::fs::OpenOptions::new() + .read(true) + .write(true) + .custom_flags(libc::O_NOCTTY) + .open(master.tty_name()?) + .ok()?; + let stdin = slave.try_clone().ok()?; + let stdout = slave.try_clone().ok()?; + + let mut command = Command::new(&resolved); + command + .arg0(program) + .args(&argv[1..]) + .current_dir(dir) + .stdin(stdin) + .stdout(stdout) + .stderr(slave); + if env_clear { + command.env_clear(); + } + // When the test process has no SHELL, the PTY layer fills one in + // from the login shell, and the builder holds the value it chose. + if let Some(shell) = cmd.get_env("SHELL") { + command.env("SHELL", shell); + } + for (key, value) in env { + command.env(key, value); + } + let controlling_tty = cmd.get_controlling_tty(); + // SAFETY: the closure runs in the forked child before exec, where a + // lock held by another thread of this process stays held forever. It + // takes the PTY layer's own pre_exec step by step. signal, sigemptyset, + // sigprocmask, setsid, ioctl and fcntl are async-signal-safe and act + // on the child alone. Listing /dev/fd allocates, as the PTY layer's + // listing always did at this point. + #[allow(unsafe_code)] + unsafe { + command.pre_exec(move || { + // Defaults for the signals a test runner tends to change, and + // an empty mask, so the child does not inherit an ignored + // SIGINT from whatever started the test. + for signal in [ + libc::SIGCHLD, + libc::SIGHUP, + libc::SIGINT, + libc::SIGQUIT, + libc::SIGTERM, + libc::SIGALRM, + ] { + libc::signal(signal, libc::SIG_DFL); + } + let mut none = std::mem::MaybeUninit::::uninit(); + libc::sigemptyset(none.as_mut_ptr()); + libc::sigprocmask(libc::SIG_SETMASK, none.as_ptr(), std::ptr::null_mut()); + // A session of its own with the PTY as its controlling + // terminal, which is what turns ^C into SIGINT and a closed + // master into SIGHUP. + if libc::setsid() == -1 { + return Err(io::Error::last_os_error()); + } + if controlling_tty && libc::ioctl(0, libc::TIOCSCTTY as _, 0) == -1 { + return Err(io::Error::last_os_error()); + } + // The descriptors the PTY layer closes here, marked + // close-on-exec instead. A successful exec closes the same + // set; a failed one leaves std the pipe it reports through. + if let Ok(entries) = std::fs::read_dir("/dev/fd") { + let fds: Vec = entries + .filter_map(|entry| entry.ok()?.file_name().to_str()?.parse().ok()) + .filter(|&fd| fd > 2) + .collect(); + for fd in fds { + libc::fcntl(fd, libc::F_SETFD, libc::FD_CLOEXEC); + } + } + Ok(()) + }); + } + Some(match command.spawn() { + Ok(child) => Ok(Box::new(child)), + Err(error) => Err(explain(&error, &resolved, dir)), + }) + } + + /// The file the PTY layer would execute for `program` run in `dir`, + /// chosen by its rules, or `None` for the PTY layer to say why there is + /// none. + fn resolve(program: &OsStr, dir: &Path, path: Option<&OsStr>) -> Option { + let program = Path::new(program); + let runnable = |file: &Path| !file.is_dir() && executable(file); + if program.is_absolute() { + return runnable(program).then(|| program.to_owned()); + } + // `./x` and `../x` are looked up in `dir` alone. + if matches!( + program.components().next(), + Some(Component::CurDir | Component::ParentDir) + ) { + let file = dir.join(program); + return runnable(&file).then_some(file); + } + // Anything else, `sub/x` included, along PATH, each entry taken + // relative to `dir`. + std::env::split_paths(path?) + .map(|entry| dir.join(entry).join(program)) + .find(|file| runnable(file)) + } + + /// access(2) with `X_OK`, as the PTY layer's resolution asks it: the + /// kernel's verdict for this process, ACLs and noexec mounts included, + /// where the mode bits would be a guess. + fn executable(file: &Path) -> bool { + let Ok(file) = CString::new(file.as_os_str().as_bytes()) else { + return false; + }; + // SAFETY: `file` is a NUL-terminated string that outlives the call, + // and access(2) only reads it. + #[allow(unsafe_code)] + let status = unsafe { libc::access(file.as_ptr(), libc::X_OK) }; + status == 0 + } + + /// Why the child could not be started, naming the culprit where the + /// errno alone would mislead. `ENOENT` for a file that plainly exists + /// means a file the kernel went looking for is missing: the `#!` + /// interpreter of a script, or the dynamic loader of a binary. + fn explain(error: &io::Error, program: &Path, dir: &Path) -> String { + if error.raw_os_error() == Some(libc::ENOENT) { + if !dir.is_dir() { + return format!("cannot enter {}: {error}", dir.display()); + } + if program.is_file() { + return match interpreter(program) { + Some(interpreter) if !dir.join(&interpreter).exists() => format!( + "cannot execute {}: its #! interpreter {interpreter:?} does not exist", + program.display() + ), + Some(interpreter) => format!( + "cannot execute {}: {error}; its #! interpreter {interpreter:?} \ + exists, so what is missing is something the interpreter needs", + program.display() + ), + None => format!( + "cannot execute {}: {error}; the file exists, so what is missing \ + is its dynamic loader", + program.display() + ), + }; + } + } + format!("cannot execute {}: {error}", program.display()) + } + + /// The interpreter a script's `#!` line names, read the way the kernel + /// reads it: the first word after `#!`, ended by a space, a tab or the + /// newline. A `\r` stays part of the name, which is why a script saved + /// with CRLF line endings cannot run. + fn interpreter(script: &Path) -> Option { + let mut head = Vec::with_capacity(256); + std::fs::File::open(script) + .ok()? + .take(256) + .read_to_end(&mut head) + .ok()?; + let line = head.strip_prefix(b"#!")?.split(|&b| b == b'\n').next()?; + let name = line + .split(|&b| b == b' ' || b == b'\t') + .find(|word| !word.is_empty())?; + Some(PathBuf::from(OsStr::from_bytes(name))) + } +} diff --git a/crates/termlens/src/terminal.rs b/crates/termlens/src/terminal.rs index dc9683e..6ba3519 100644 --- a/crates/termlens/src/terminal.rs +++ b/crates/termlens/src/terminal.rs @@ -28,6 +28,7 @@ use crate::error::{Error, Result}; use crate::keys::Input; use crate::keys::{mouse_legacy, mouse_sgr, mouse_utf8}; use crate::screen::{MouseMode, Screen}; +use crate::spawn::{spawn_child, PtyChild}; use crate::utf8::Utf8Sanitizer; use crate::wait::{next_backoff, Expired, Monitor, INITIAL_BACKOFF, POLL_CAP}; @@ -64,7 +65,7 @@ struct ExitWatch { impl ExitWatch { fn tick( &mut self, - child: &mut Box, + child: &mut PtyChild, exit_status: &mut Option, state: &mut EmuState, ) { @@ -324,16 +325,21 @@ type SharedWriter = Arc>>>; /// thread. `take_writer` may only be called once, so duplicate the /// descriptor instead: both refer to the same open file description, /// which is what we want — same terminal, independent blocking. +/// +/// Close-on-exec, like every other descriptor the PTY layer opens. A plain +/// `dup` was inherited by every child the test process started while the +/// terminal was alive, holding the master open in processes that have no +/// business with it. #[cfg(unix)] fn dup_writer(master: &dyn portable_pty::MasterPty) -> Option { use std::os::unix::io::FromRawFd; let fd = master.as_raw_fd()?; // SAFETY: `fd` is the live master descriptor (the caller still owns - // the master). dup(2) returns a fresh descriptor we take sole + // the master). F_DUPFD_CLOEXEC returns a fresh descriptor we take sole // ownership of; `File` closes it exactly once on drop. #[allow(unsafe_code)] - let duped = unsafe { libc::dup(fd) }; + let duped = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 0) }; if duped < 0 { return None; } @@ -1863,7 +1869,8 @@ impl TerminalBuilder { /// command (empty program name, missing /// [`current_dir`](Self::current_dir), a bare program name under /// [`env_clear`](Self::env_clear) with no `PATH` to resolve it) or the - /// program cannot be executed; [`Error::Size`] when the configured + /// program cannot be executed, with the reason the OS gave (on Unix a + /// missing `#!` interpreter is named); [`Error::Size`] when the configured /// [`size`](Self::size) has a dimension below 2 or past the /// 1000-per-axis limit; [`Error::Pty`] when the PTY cannot be opened. pub fn spawn(self, program: impl AsRef) -> Result { @@ -1904,10 +1911,9 @@ impl TerminalBuilder { // `std::process::Command`, so it is made absolute here. Handed // over relative, the PTY layer joins it onto a relative program // path and the child then enters it before exec: `dir/./prog` - // looked up from inside `dir`, which does not exist. The exec - // failure could not even be reported, because the PTY layer - // closes the pipe std reports it through, so the child died of - // a runtime abort instead. `absolute` fails only on an empty + // looked up from inside `dir`, which does not exist. Until + // #563 that exec failure was not even reported: the child died + // of a runtime abort instead. `absolute` fails only on an empty // path or an unreadable cwd; the given path stands then. Some(dir) => cmd.cwd(std::path::absolute(dir).unwrap_or_else(|_| dir.clone())), None => { @@ -1916,9 +1922,6 @@ impl TerminalBuilder { } } } - if self.env_clear { - cmd.env_clear(); - } // Whatever the child is told TERM is, XTGETTCAP's `TN` reports the // same string — an application must not be able to get two different // answers to "which terminal is this?". @@ -1926,19 +1929,20 @@ impl TerminalBuilder { || "xterm-256color".to_owned(), |(_, v)| v.to_string_lossy().into_owned(), ); + // What the child gets on top of the inherited environment, or on top + // of nothing under env_clear. Later entries win. + let mut env: Vec<(OsString, OsString)> = Vec::new(); if !self.envs.iter().any(|(k, _)| k == "TERM") { - cmd.env("TERM", "xterm-256color"); + env.push(("TERM".into(), "xterm-256color".into())); } // The PTY layer fills SHELL from the host's login shell when the // variable is absent, which put one machine-shaped value inside an // environment env_clear promises is hermetic (#221). Pinned the way // TERM is; an explicit `.env("SHELL", …)` still wins. if self.env_clear && !self.envs.iter().any(|(k, _)| k == "SHELL") { - cmd.env("SHELL", "/bin/sh"); - } - for (key, value) in &self.envs { - cmd.env(key, value); + env.push(("SHELL".into(), "/bin/sh".into())); } + env.extend(self.envs.iter().cloned()); // Attach the reader thread BEFORE spawning the child: a program that // writes and exits within its first millisecond must find a drain @@ -2064,10 +2068,11 @@ impl TerminalBuilder { }) .map_err(Error::Io)?; - let child = pair.slave.spawn_command(cmd).map_err(|e| Error::Spawn { - command: command_desc.clone(), - reason: e.to_string(), - })?; + let child = + spawn_child(&pair, cmd, self.env_clear, &env).map_err(|reason| Error::Spawn { + command: command_desc.clone(), + reason, + })?; // Close the parent's slave handle so the master sees EOF once the // child (and its descendants) release the terminal. drop(pair.slave); @@ -2556,7 +2561,7 @@ fn reader_loop( /// See the crate-level docs for a full example. Dropping a `Terminal` kills /// and reaps the child — tests never leak zombies, even on panic. pub struct Terminal { - child: Box, + child: PtyChild, // Option only so Drop can close it under the PTY lifecycle lock; // Some for the entire life of the value outside Drop. master: Option>, diff --git a/crates/termlens/tests/common/mod.rs b/crates/termlens/tests/common/mod.rs index 31a9acd..c065c70 100644 --- a/crates/termlens/tests/common/mod.rs +++ b/crates/termlens/tests/common/mod.rs @@ -26,6 +26,23 @@ pub(crate) fn emit(steps: &[&str]) -> termlens::Result { spawn_emit(Terminal::builder(), steps) } +/// Write an executable script at `path` through `sh`, so that this process +/// never holds the file open for writing. A test binary forks all the time, +/// and a child forked while the file was open keeps a copy of the +/// descriptor until it execs. Running the script in that window fails with +/// ETXTBSY, "Text file busy", which is never the failure under test. +#[cfg(unix)] +#[allow(dead_code)] +pub(crate) fn write_script(path: &std::path::Path, contents: &str) { + let status = Command::new("sh") + .args(["-c", r#"printf '%s' "$2" > "$1" && chmod 755 "$1""#, "sh"]) + .arg(path) + .arg(contents) + .status() + .expect("sh runs"); + assert!(status.success(), "writing {} failed", path.display()); +} + /// Fixture names already rebuilt by this test process. static BUILT: Mutex> = Mutex::new(Vec::new()); diff --git a/crates/termlens/tests/descriptors.rs b/crates/termlens/tests/descriptors.rs new file mode 100644 index 0000000..0aa47c8 --- /dev/null +++ b/crates/termlens/tests/descriptors.rs @@ -0,0 +1,91 @@ +//! What a child inherits from the test process: its terminal, and nothing +//! termlens holds. A binary of its own, because the assertions are about the +//! whole process's descriptor table, which tests running beside them would +//! change. +#![cfg(unix)] + +use std::collections::BTreeSet; +use std::os::fd::AsRawFd; +use std::process::Command; +use std::sync::{Mutex, PoisonError}; +use std::time::Duration; + +use termlens::Terminal; + +/// The tests below measure and change one descriptor table, so they take +/// turns: one leaves a descriptor inheritable that the other would count. +static TABLE: Mutex<()> = Mutex::new(()); + +/// The descriptors an ordinary child of this process has open, as `ls` +/// lists them. The child's own listing descriptor is among them, the same +/// one each time. +fn in_an_ordinary_child() -> BTreeSet { + let out = Command::new("ls").arg("/dev/fd").output().expect("ls runs"); + assert!(out.status.success(), "{out:?}"); + String::from_utf8_lossy(&out.stdout) + .split_whitespace() + .map(|fd| fd.parse().expect("a descriptor number")) + .collect() +} + +/// A shell command that prints whether descriptor `fd` is open in it. +fn probe(fd: i32) -> String { + format!("if [ -e /dev/fd/{fd} ]; then echo inherited; else echo closed; fi") +} + +/// A terminal alive in the test process adds nothing to what the process's +/// other children inherit. The writer termlens duplicates from the master +/// was a plain `dup`, so every child started while a terminal lived held +/// that master open. +#[test] +fn a_live_terminal_leaks_no_descriptor_into_an_ordinary_child() -> termlens::Result<()> { + let _table = TABLE.lock().unwrap_or_else(PoisonError::into_inner); + let before = in_an_ordinary_child(); + let t = Terminal::builder() + .timeout(Duration::from_secs(10)) + .spawn("cat")?; + let during = in_an_ordinary_child(); + drop(t); + assert_eq!( + during, before, + "a descriptor of the live terminal reached an unrelated child" + ); + Ok(()) +} + +/// A child of termlens gets stdio and nothing else, even from a test process +/// holding a descriptor that any other child would inherit. The PTY layer +/// closed every such descriptor between fork and exec. termlens, which now +/// spawns the child itself, marks them close-on-exec instead, so that a +/// failed exec can still report through std's pipe (#563). +#[test] +fn a_termlens_child_inherits_nothing_but_its_terminal() -> termlens::Result<()> { + let _table = TABLE.lock().unwrap_or_else(PoisonError::into_inner); + let file = std::fs::File::open(concat!(env!("CARGO_MANIFEST_DIR"), "/Cargo.toml"))?; + let fd = file.as_raw_fd(); + // SAFETY: `fd` is open for as long as `file` lives, and F_SETFD changes + // only its descriptor flags. + #[allow(unsafe_code)] + let cleared = unsafe { libc::fcntl(fd, libc::F_SETFD, 0) }; + assert_eq!(cleared, 0, "clearing close-on-exec on {fd}"); + + let ordinary = Command::new("sh") + .args(["-c", &probe(fd)]) + .output() + .expect("sh runs"); + assert_eq!( + String::from_utf8_lossy(&ordinary.stdout).trim(), + "inherited", + "an ordinary child must inherit {fd}, or this test proves nothing" + ); + + let mut t = Terminal::builder() + .timeout(Duration::from_secs(10)) + .args(["-c", &probe(fd)]) + .spawn("sh")?; + assert!(t.wait_exit()?.success()); + let screen = t.screen().text(); + assert_eq!(screen.trim(), "closed", "descriptor {fd} reached the child"); + drop(file); + Ok(()) +} diff --git a/crates/termlens/tests/inspect.rs b/crates/termlens/tests/inspect.rs index 649f828..f47b7c5 100644 --- a/crates/termlens/tests/inspect.rs +++ b/crates/termlens/tests/inspect.rs @@ -775,6 +775,34 @@ fn inspect_example_and_command_agree_on_exit_codes_when_inspect_cannot_run() { } } +/// A program that exists but cannot be executed is inspect failing to run +/// it, exit 2 with the missing interpreter named, in both. Both used to +/// paint a screen for a child killed by SIGABRT and exit 0 (#563). +#[cfg(unix)] +#[test] +fn inspect_example_and_command_agree_that_a_missing_interpreter_is_a_spawn_failure() { + let scratch = std::path::Path::new(env!("CARGO_TARGET_TMPDIR")) + .join(format!("inspect-exec-failure-{}", std::process::id())); + std::fs::create_dir_all(&scratch).expect("scratch directory"); + let script = scratch.join("missing-interpreter"); + common::write_script(&script, "#!/nonexistent/interpreter\n"); + let script = script.to_str().expect("utf-8 scratch directory"); + let (example, command) = run_both(&["--size", "40x3", script]); + for (who, out) in [("example", &example), ("command", &command)] { + let stderr = String::from_utf8_lossy(&out.stderr); + assert_eq!(out.status.code(), Some(2), "{who}: {stderr}"); + assert!( + out.stdout.is_empty(), + "{who} painted a screen for a child that never ran" + ); + assert!( + stderr.contains("failed to spawn") && stderr.contains("/nonexistent/interpreter"), + "{who}: {stderr}" + ); + } + let _ = std::fs::remove_dir_all(&scratch); +} + /// `--help` and `--version` succeed on stdout with nothing on stderr, and /// `--version` is the one line both print. The usage text itself is not /// compared: it names the tool, and the two legitimately differ (#453). diff --git a/crates/termlens/tests/process.rs b/crates/termlens/tests/process.rs index 65dc3ef..f364140 100644 --- a/crates/termlens/tests/process.rs +++ b/crates/termlens/tests/process.rs @@ -91,6 +91,66 @@ fn a_relative_current_dir_is_the_test_process_s_and_a_relative_program_resolves_ Ok(()) } +/// A program that exists but cannot be executed is a spawn error naming what +/// is missing, not a child dead of SIGABRT (#563). The PTY layer closed the +/// pipe std reports a failed exec through, so the child aborted writing to +/// it and `spawn` returned `Ok`. The usual culprit is the interpreter a +/// script's `#!` line names. The sneakiest is the `\r` that CRLF line +/// endings leave on it, which the message shows escaped. +#[cfg(unix)] +#[test] +fn a_script_whose_interpreter_is_missing_fails_to_spawn_and_names_it() -> termlens::Result<()> { + let scratch = std::path::Path::new(env!("CARGO_TARGET_TMPDIR")) + .join(format!("exec-failure-{}", std::process::id())); + std::fs::create_dir_all(&scratch)?; + for (name, script, culprit) in [ + ( + "missing", + "#!/nonexistent/interpreter\necho unreachable\n", + r#""/nonexistent/interpreter""#, + ), + ( + "crlf", + "#!/bin/sh\r\necho unreachable\r\n", + r#""/bin/sh\r""#, + ), + ] { + let path = scratch.join(name); + common::write_script(&path, script); + let err = Terminal::builder() + .timeout(Duration::from_secs(10)) + .spawn(&path) + .expect_err("a script whose interpreter does not exist cannot run"); + let Error::Spawn { reason, .. } = &err else { + panic!("{name}: not a spawn error: {err}"); + }; + assert!( + reason.contains(culprit) && reason.contains("does not exist"), + "{name}: the message must name the interpreter: {reason}" + ); + } + let _ = std::fs::remove_dir_all(&scratch); + Ok(()) +} + +/// Any exec the kernel refuses is a spawn error carrying its reason, not +/// only a missing interpreter. Every Unix refuses an argument this long, +/// with E2BIG. The error's command line holds the argument, so only the +/// reason is printed. +#[cfg(unix)] +#[test] +fn an_argument_the_kernel_refuses_is_a_spawn_error_with_its_reason() { + let err = Terminal::builder() + .timeout(Duration::from_secs(10)) + .args(["-c", "true", &"x".repeat(2 << 20)]) + .spawn("sh") + .expect_err("a 2 MiB argument is past every Unix's limit"); + let Error::Spawn { reason, .. } = &err else { + panic!("not a spawn error"); + }; + assert!(reason.contains("Argument list too long"), "{reason}"); +} + #[test] fn current_dir_runs_the_child_where_asked() -> termlens::Result<()> { // Canonicalize: /tmp is a symlink on macOS and `--cwd` reports the real diff --git a/docs/DESIGN.md b/docs/DESIGN.md index a31c8db..353bd7a 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -201,6 +201,47 @@ answer" — a hang becomes a diagnosis. `answer_queries(false)` mutes the responder for tests that need a silent terminal; the diagnosis still works. +### On Unix termlens starts the child itself + +portable-pty opens the PTY, but on Unix it no longer starts the child. Its +`spawn_command` closes every descriptor above stdio between fork and exec, +and one of them is the close-on-exec pipe through which std reports a +failed `exec`. When the program exists but cannot be executed, the child's +write into that pipe failed and std aborted the child. `spawn` then +returned `Ok` with a process already dead of SIGABRT (#563). The usual +causes are a missing `#!` interpreter (a `\r` left by CRLF line endings +counts), a missing dynamic loader, and an argument list over the kernel's +limit. + +`src/spawn.rs` builds the command portable-pty would build and runs it with +`std::process::Command`: + +- **Resolution** finds the same file by the same rules. `./x` and `../x` + are looked up in the working directory alone. Any other relative name is + searched for along the child's `PATH`, each entry taken from the working + directory. A candidate must pass `access(X_OK)` and must not be a + directory. `argv[0]` is the name as given. +- **The environment** is the same: the inherited one unless `env_clear`, + plus the `SHELL` that portable-pty fills in from the login shell when the + process has none, then termlens's pins and the test's own variables. +- **The session** gets the same `pre_exec` steps. Six signals go back to + their defaults and the mask is emptied. `setsid` gives the child a + session of its own, and `TIOCSCTTY` makes the PTY its controlling + terminal. +- **Descriptors** above stdio are marked close-on-exec rather than closed. + A successful exec closes exactly what portable-pty closed, and a failed + one still has std's pipe to report through. Every descriptor termlens + opens is close-on-exec too, so no other child of the test process + inherits one. + +The slave is opened by name (`tty_name`, `O_NOCTTY`), because portable-pty +does not expose its own handle. Three cases still go to portable-pty +unchanged. A working directory that is not a directory gets `$HOME` +substituted. A program that does not resolve gets portable-pty's diagnosis, +word for word, which is what termlens has always reported. A slave that +cannot be named or opened is the third. On Windows ConPTY reports a failed +`CreateProcess` itself, and portable-pty spawns the child as before. + ### Windows: ConPTY renders, and that decides what is claimed `portable-pty` gives the same four layers a ConPTY backend, and the crate From 5568427170c9ab29e659f8376ce361d1c168fdcb Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Sun, 11 Oct 2026 12:47:56 +0700 Subject: [PATCH 2/7] docs(skill): read the missing-interpreter spawn error 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> --- skills/termlens/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/skills/termlens/SKILL.md b/skills/termlens/SKILL.md index f29c11b..e525da1 100644 --- a/skills/termlens/SKILL.md +++ b/skills/termlens/SKILL.md @@ -424,6 +424,7 @@ included in `X`, and `terminal closed (EOF) while waiting for X` is | `input not receivable: the application has not enabled mouse tracking` | `click`/`drag`/`scroll` before the app enabled the mouse | `wait_until(|s| s.mouse_mode() != MouseMode::None)` first | | `input not receivable: mouse at (50, 2) is outside the 20x5 grid` | coordinates swapped or out of range | rule 6 | | `failed to spawn \`sh\`: \`sh\` is a bare program name and env_clear() removed PATH` | bare program name under `env_clear` | absolute path, `bin!`, or `.env("PATH", …)` | +| ``failed to spawn `./x`: cannot execute …: its #! interpreter "…" does not exist`` | the script's `#!` line names a file that is not there; a `\r` at the end of the quoted name means CRLF line endings | install the interpreter, point the `#!` line at one that exists, or save the script with LF endings | | `invalid terminal size: a terminal needs at least 2 columns and 2 rows` | geometry below 2x2 (past 1000 has its own message) | rule 5 | | `the terminal emulator failed and the screen stopped advancing` (`Error::Emulator`) | a bug in the emulation, not in your app | report it to termlens with the detail it names | From 48e2a1249f61c8e0bdd9ed5388c8d435831baf4e Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:53:05 +0700 Subject: [PATCH 3/7] docs: point contributors at spawn.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- AGENTS.md | 4 +++- CONTRIBUTING.md | 9 +++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6b0bf37..02be51b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -8,6 +8,7 @@ is the full contributor document and wins wherever the two disagree. ## Layout - `crates/termlens/` — the published library. `terminal.rs` is the runtime, + `spawn.rs` starts the child (on Unix by itself, see DESIGN §1), `screen.rs` the snapshot types, `emu/` the VT emulation behind a small internal trait, `graphics.rs` the inline-image capture and decoder. - `crates/termlens-cli/` — the `termlens` command (`inspect`, `diff`, @@ -17,7 +18,8 @@ is the full contributor document and wins wherever the two disagree. - `fixtures/` — deterministic terminal programs the integration suite spawns in real PTYs. Never published; `publish = false`. - `docs/DESIGN.md` — four layers, the wait-semantics contract, the snapshot - format. **Read it before touching `wait.rs`, `terminal.rs`, or `emu/`.** + format. **Read it before touching `wait.rs`, `terminal.rs`, `spawn.rs`, + or `emu/`.** ## Build and test diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 596570d..3a767ad 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -134,10 +134,11 @@ cargo insta review # inspect and accept/reject each diff `wait_idle`) must pass the **stress workflow** (`stress.yml` — the suite in a 100-iteration loop on Ubuntu, macOS and Windows, sharded across five `--test-threads` values). Trigger it from the Actions tab on your branch, - or ask a maintainer to. So must a test that *adds* PTY spawns: three of - the tests added on one day in September 2026 passed locally and went red - only under stress, and stress on `main` is what told one author's flake - apart from an inherited one. + or ask a maintainer to. So must a change to how the child is started + (`spawn.rs`), which every test goes through, and a test that *adds* PTY + spawns: three of the tests added on one day in September 2026 passed + locally and went red only under stress, and stress on `main` is what told + one author's flake apart from an inherited one. - **No `sleep` as a wait.** A sleep is either too short and flaky under load, or too long and slow on every run, and it names nothing: when it fails, the log says only that time passed. Wait on what you mean — From 95f1e770d6cdce4d09295ad235ca6fcc580ff06f Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 14 Oct 2026 18:24:37 +0700 Subject: [PATCH 4/7] fix: name the right culprit and fall back less in the Unix spawn Review findings on the new spawn path: - A failure before the exec no longer reads "cannot execute ". 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> --- CHANGELOG.md | 26 ++- SECURITY.md | 23 ++- crates/termlens/src/spawn.rs | 288 +++++++++++++++++++++------ crates/termlens/src/terminal.rs | 36 ++-- crates/termlens/tests/descriptors.rs | 32 ++- crates/termlens/tests/process.rs | 120 ++++++++--- crates/termlens/tests/queries.rs | 3 + docs/DESIGN.md | 69 +++++-- skills/termlens/SKILL.md | 2 +- 9 files changed, 449 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ee7782..447b0eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,19 +22,25 @@ reads that marker. - On Unix, a program that exists but cannot be executed now fails `TerminalBuilder::spawn` with an `Error::Spawn` that says why, and `termlens inspect` exits 2 with that message (#563). Typical causes are a - missing `#!` interpreter, a missing dynamic loader, and an argument list - over the kernel's limit. The message names the interpreter, with any `\r` - that CRLF line endings left on it shown escaped. `spawn` used to return - `Ok` with a child already killed by SIGABRT (`fatal runtime error: - assertion failed: output.write(&bytes).is_ok()`), which inspect showed as - `exited: killed by signal: Aborted`. portable-pty closed the pipe std - reports a failed exec through, so on Unix termlens now starts the child - itself. Program resolution, the environment, the session and the - signals are unchanged, and the child still inherits no descriptor besides - its terminal. + missing `#!` interpreter, an interpreter that is not a program, a missing + dynamic loader, and an argument list over the kernel's limit. The message + names the interpreter, with any `\r` that CRLF line endings left on it + shown escaped. `spawn` used to return `Ok` with a child already killed by + SIGABRT (`fatal runtime error: assertion failed: + output.write(&bytes).is_ok()`), which inspect showed as `exited: killed + by signal: Aborted`. portable-pty closed the pipe std reports a failed + exec through, so on Unix termlens now starts the child itself. Program + resolution, the environment, the session and the signals are unchanged, + and the child still inherits no descriptor besides its terminal. A + working directory the child cannot enter is now named in the error. A + few rare setups are still left to portable-pty, as before, and + `docs/DESIGN.md` lists them. - The second writer termlens opens on the PTY master is close-on-exec. A process the test started while a terminal was alive inherited it before, which kept the master open in a process unrelated to the terminal. +- With `TERM` set twice through `.env`, XTGETTCAP's `TN` reply reports the + later value, the one the child gets. It reported the first, so an + application could get two answers to "which terminal is this?". ## [0.11.5] - 2026-10-07 diff --git a/SECURITY.md b/SECURITY.md index ef9866a..a8716f6 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -10,17 +10,18 @@ issues in escape-sequence handling) are treated as security issues. What the project does continuously, enforced by required CI on every change: -- **`unsafe` is confined to five audited blocks, all Unix-only.** Four are - single lines. `fcntl(2)` with `F_DUPFD_CLOEXEC` opens a second writer on - the PTY master for the responder thread. `File::from_raw_fd` takes sole - ownership of that duplicate, so it is closed exactly once. `access(2)` - resolves the program the way the PTY layer does, and `kill(2)` sits - behind `Terminal::signal`. The fifth is the `pre_exec` hook in - `src/spawn.rs`, which runs in the forked child before exec. It repeats - portable-pty's own hook step by step, with one change: inherited - descriptors are marked close-on-exec rather than closed, so that a failed - exec can be reported (#563). Each block sits under a `SAFETY` comment - (the first two share one) and carries its own `#[allow(unsafe_code)]`. +- **`unsafe` is confined to five audited blocks, all Unix-only.** Three + are single lines. `BorrowedFd::borrow_raw` borrows the PTY master's + descriptor, which std then duplicates, close-on-exec, into the responder + thread's second writer. `access(2)` resolves the program the way the PTY + layer does. `kill(2)` sits behind `Terminal::signal`. The fourth asks + `sigaction(2)` whether the kernel reaps this process's children. The + fifth is the `pre_exec` hook in `src/spawn.rs`, which runs in the forked + child before exec. It repeats portable-pty's own hook step by step, with + two changes: inherited descriptors are marked close-on-exec rather than + closed, so that a failed exec can be reported (#563), and the umask step + is left out because termlens never sets a umask. Each block sits under + its own `SAFETY` comment and carries its own `#[allow(unsafe_code)]`. The workspace sets `unsafe_code = "warn"` and CI runs clippy with `-D warnings`, so a new block fails unless it carries that `#[allow]` as well: the attribute is what review has to see, not a count the lint diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs index e5abbec..3ada5e7 100644 --- a/crates/termlens/src/spawn.rs +++ b/crates/termlens/src/spawn.rs @@ -6,19 +6,24 @@ //! pipe through which std reports a failed `exec`. A program that exists but //! cannot be executed, such as a script whose `#!` interpreter is missing, //! then died of a runtime abort, and `spawn` returned `Ok` with a child -//! already killed by SIGABRT (#563). +//! already killed by SIGABRT (#563; upstream, wezterm/wezterm#7893). //! -//! So the command is built the way the PTY layer builds it, with the same -//! program resolution, working directory, environment, session and signals, -//! and one difference: descriptors above stdio are marked close-on-exec -//! instead of closed. A successful exec closes the same set the PTY layer -//! closed, and a failed one can still report why. +//! So the command is built the way portable-pty 0.9.0 builds it, with the +//! same program resolution, working directory, environment, session and +//! signals, and one difference: descriptors above stdio are marked +//! close-on-exec instead of closed. A successful exec closes the same set the +//! PTY layer closed, and a failed one can still report why. The test at the +//! foot of this file starts one probe both ways and requires the same +//! answers, so a portable-pty release that changes its own spawn fails there +//! rather than drifting apart from this one unseen. //! //! Whatever this path is not sure of goes to the PTY layer unchanged: a -//! working directory that is not a directory (the PTY layer substitutes the -//! home directory), a program that does not resolve (the PTY layer says why, -//! in the words termlens has always reported), a slave device it cannot name -//! or open. Windows always goes there, because ConPTY reports a failed +//! process whose children the kernel reaps (std would panic reporting the +//! failure), a working directory that is not one with no `HOME` in the +//! child's environment to fall back to (the PTY layer reads the passwd +//! entry), a program that does not resolve (the PTY layer says why, in the +//! words termlens has always reported), a slave device it cannot name or +//! open. Windows always goes there, because ConPTY reports a failed //! `CreateProcess` directly. use std::ffi::OsString; @@ -74,13 +79,23 @@ mod unix { env_clear: bool, env: &[(OsString, OsString)], ) -> Option> { - let argv = cmd.get_argv(); - let program = argv.first()?; - let dir = Path::new(cmd.get_cwd()?); - if !dir.is_dir() { + // std reports a failed exec only after reaping the child, and panics + // when the reap fails, which it does wherever the kernel reaps + // children itself. The PTY layer never reports the failure, so it + // never gets that far. + if children_reaped_by_the_kernel() { return None; } - let resolved = resolve(program, dir, cmd.get_env("PATH"))?; + let argv = cmd.get_argv(); + let program = argv.first()?; + // Where the PTY layer runs the child: the working directory, or the + // child's HOME when that is not a directory. Without a HOME it reads + // the passwd entry, which is left to it. + let dir = match cmd.get_cwd().map(Path::new).filter(|dir| dir.is_dir()) { + Some(dir) => dir.to_owned(), + None => PathBuf::from(cmd.get_env("HOME")?.to_str()?), + }; + let resolved = resolve(program, &dir, cmd.get_env("PATH"))?; // A second handle on the slave, by name, because the PTY layer does // not expose its own. O_NOCTTY keeps the device from becoming this // process's controlling terminal; std adds O_CLOEXEC. @@ -97,7 +112,7 @@ mod unix { command .arg0(program) .args(&argv[1..]) - .current_dir(dir) + .current_dir(&dir) .stdin(stdin) .stdout(stdout) .stderr(slave); @@ -115,10 +130,12 @@ mod unix { let controlling_tty = cmd.get_controlling_tty(); // SAFETY: the closure runs in the forked child before exec, where a // lock held by another thread of this process stays held forever. It - // takes the PTY layer's own pre_exec step by step. signal, sigemptyset, - // sigprocmask, setsid, ioctl and fcntl are async-signal-safe and act - // on the child alone. Listing /dev/fd allocates, as the PTY layer's - // listing always did at this point. + // takes the PTY layer's own pre_exec step by step, but for two: the + // umask step, which acts only on a umask termlens never sets, is left + // out, and descriptors are marked close-on-exec rather than closed. + // signal, sigemptyset, sigprocmask, setsid, ioctl and fcntl are + // async-signal-safe and act on the child alone. Listing /dev/fd + // allocates, as the PTY layer's listing always did at this point. #[allow(unsafe_code)] unsafe { command.pre_exec(move || { @@ -164,7 +181,24 @@ mod unix { } Some(match command.spawn() { Ok(child) => Ok(Box::new(child)), - Err(error) => Err(explain(&error, &resolved, dir)), + Err(error) => Err(explain(&error, &resolved, &dir)), + }) + } + + /// Whether this process's children are reaped by the kernel the moment + /// they exit: SIGCHLD ignored, or `SA_NOCLDWAIT` set on it, either + /// inherited from whatever started the test. + fn children_reaped_by_the_kernel() -> bool { + // SAFETY: with a null new action, sigaction only writes the current + // one into `current`, and a zeroed sigaction is a valid value for it + // to overwrite. + #[allow(unsafe_code)] + let current = unsafe { + let mut current: libc::sigaction = std::mem::zeroed(); + (libc::sigaction(libc::SIGCHLD, std::ptr::null(), &mut current) == 0).then_some(current) + }; + current.is_some_and(|action| { + action.sa_sigaction == libc::SIG_IGN || action.sa_flags & libc::SA_NOCLDWAIT != 0 }) } @@ -194,7 +228,8 @@ mod unix { /// access(2) with `X_OK`, as the PTY layer's resolution asks it: the /// kernel's verdict for this process, ACLs and noexec mounts included, - /// where the mode bits would be a guess. + /// where the mode bits would be a guess. For a directory, whether it can + /// be entered. fn executable(file: &Path) -> bool { let Ok(file) = CString::new(file.as_os_str().as_bytes()) else { return false; @@ -206,52 +241,181 @@ mod unix { status == 0 } - /// Why the child could not be started, naming the culprit where the - /// errno alone would mislead. `ENOENT` for a file that plainly exists - /// means a file the kernel went looking for is missing: the `#!` - /// interpreter of a script, or the dynamic loader of a binary. + /// Why the child could not be started. std hands back the errno of + /// whichever step failed (the fork, a NUL in an argument, entering the + /// working directory, the session setup, the exec), and the errno alone + /// does not say which. So the program is blamed only for what the exec + /// alone could raise, and where the kernel's `ENOENT` or `EACCES` points + /// at a file that plainly is there, the file it went looking for is + /// named: the `#!` interpreter of a script, or the loader of a binary. + /// Anything else reads as std gave it, as the PTY layer's spawn did. fn explain(error: &io::Error, program: &Path, dir: &Path) -> String { - if error.raw_os_error() == Some(libc::ENOENT) { - if !dir.is_dir() { - return format!("cannot enter {}: {error}", dir.display()); - } - if program.is_file() { - return match interpreter(program) { - Some(interpreter) if !dir.join(&interpreter).exists() => format!( - "cannot execute {}: its #! interpreter {interpreter:?} does not exist", - program.display() - ), - Some(interpreter) => format!( - "cannot execute {}: {error}; its #! interpreter {interpreter:?} \ - exists, so what is missing is something the interpreter needs", - program.display() - ), - None => format!( - "cannot execute {}: {error}; the file exists, so what is missing \ - is its dynamic loader", - program.display() - ), - }; + let shown = program.display(); + match error.raw_os_error() { + // Entering the directory fails with these too, and fails first. + Some( + libc::ENOENT | libc::EACCES | libc::ENOTDIR | libc::ELOOP | libc::ENAMETOOLONG, + ) if !dir.is_dir() || !executable(dir) => { + format!("cannot enter {}: {error}", dir.display()) } + Some(libc::ENOENT) if program.is_file() => match interpreter(program) { + Ok(Some(interpreter)) if !present(dir, &interpreter) => format!( + "cannot execute {shown}: its #! interpreter {interpreter:?} does not exist" + ), + Ok(Some(interpreter)) => format!( + "cannot execute {shown}: {error}; its #! interpreter {interpreter:?} \ + exists, so what is missing is something the interpreter needs" + ), + Ok(None) => format!( + "cannot execute {shown}: {error}; the file exists, so what is missing \ + is its dynamic loader" + ), + Err(_) => format!( + "cannot execute {shown}: {error}; the file exists, so what is missing \ + is its #! interpreter or its dynamic loader" + ), + }, + Some(libc::EACCES) => match interpreter(program) { + Ok(Some(interpreter)) if !runnable(dir, &interpreter) => format!( + "cannot execute {shown}: its #! interpreter {interpreter:?} is not an \ + executable file" + ), + _ => format!("cannot execute {shown}: {error}"), + }, + Some( + libc::ENOENT + | libc::E2BIG + | libc::ETXTBSY + | libc::ENOEXEC + | libc::EISDIR + | libc::ENOTDIR + | libc::ELOOP + | libc::ENAMETOOLONG, + ) => format!("cannot execute {shown}: {error}"), + _ => error.to_string(), } - format!("cannot execute {}: {error}", program.display()) } - /// The interpreter a script's `#!` line names, read the way the kernel - /// reads it: the first word after `#!`, ended by a space, a tab or the - /// newline. A `\r` stays part of the name, which is why a script saved - /// with CRLF line endings cannot run. - fn interpreter(script: &Path) -> Option { - let mut head = Vec::with_capacity(256); - std::fs::File::open(script) - .ok()? - .take(256) - .read_to_end(&mut head) - .ok()?; - let line = head.strip_prefix(b"#!")?.split(|&b| b == b'\n').next()?; + /// Whether an interpreter path names something, looked up as the kernel + /// looks it up: relative to the directory the child runs in. + fn present(dir: &Path, interpreter: &Path) -> bool { + !interpreter.as_os_str().is_empty() && dir.join(interpreter).exists() + } + + /// Whether an interpreter path names a file the kernel would execute. + fn runnable(dir: &Path, interpreter: &Path) -> bool { + let file = dir.join(interpreter); + present(dir, interpreter) && file.is_file() && executable(&file) + } + + /// What a script's `#!` line names, read the way the kernel reads it: + /// the first word after `#!` on the first line, ended by a space, a tab + /// or a NUL, and on macOS by a `#` as well. A `\r` stays part of the + /// name, which is why a script saved with CRLF line endings cannot run. + /// `Ok(None)` for a file that is not a script, and `Err` for one that + /// cannot be read, which may be either. + fn interpreter(file: &Path) -> io::Result> { + let mut head = Vec::with_capacity(512); + std::fs::File::open(file)? + .take(512) + .read_to_end(&mut head)?; + let Some(rest) = head.strip_prefix(b"#!") else { + return Ok(None); + }; + let line_end = + |b: &u8| matches!(b, b'\n' | b'\0') || (cfg!(target_os = "macos") && *b == b'#'); + let line = rest.split(line_end).next().unwrap_or_default(); let name = line .split(|&b| b == b' ' || b == b'\t') - .find(|word| !word.is_empty())?; - Some(PathBuf::from(OsStr::from_bytes(name))) + .find(|word| !word.is_empty()) + .unwrap_or_default(); + Ok(Some(PathBuf::from(OsStr::from_bytes(name)))) + } +} + +#[cfg(all(test, unix))] +mod tests { + use std::ffi::OsString; + use std::io::{Read, Write}; + + use portable_pty::{CommandBuilder, PtySize}; + + /// What a child sees of how it was started: argv[0], the working + /// directory, the environment (its names, and a checksum of the whole, so + /// that a failure never prints a value), the descriptors it holds, the + /// umask, and whether it leads a process group that is the PTY's + /// foreground one with the PTY as its controlling terminal. It then waits + /// for a line, so that nothing it printed can be lost to an exit before + /// the read. + const PROBE: &str = r#"printf 'argv0=%s\n' "$0" +pwd +env | sed 's/=.*//' | LC_ALL=C sort | tr '\n' ' '; echo +env | LC_ALL=C sort | cksum +ls /dev/fd | tr '\n' ' '; echo +umask +[ "$(ps -o pgid= -p $$ | tr -d ' ')" = "$$" ] && echo group-leader +[ "$(ps -o tpgid= -p $$ | tr -d ' ')" = "$$" ] && echo foreground +exec 3 String { + let (pair, lifecycle) = crate::terminal::open_pty(PtySize { + rows: 80, + cols: 300, + pixel_width: 0, + pixel_height: 0, + }) + .expect("a PTY opens"); + let mut cmd = CommandBuilder::new("sh"); + cmd.args(["-c", PROBE]); + cmd.cwd(std::env::current_dir().expect("a working directory")); + let env = [(OsString::from("TERM"), OsString::from("xterm-256color"))]; + for (key, value) in &env { + cmd.env(key, value); + } + let mut reader = pair.master.try_clone_reader().expect("a reader"); + let mut writer = pair.master.take_writer().expect("a writer"); + let mut child = if through_termlens { + super::unix::spawn(pair.master.as_ref(), &cmd, false, &env) + .expect("termlens's own spawn takes this command") + .expect("sh starts") + } else { + pair.slave.spawn_command(cmd).expect("sh starts") + }; + drop(pair.slave); + drop(lifecycle); + + let mut seen = Vec::new(); + let mut chunk = [0; 4096]; + while !String::from_utf8_lossy(&seen).contains("probe-finished") { + let read = reader.read(&mut chunk).expect("the PTY reads"); + assert!( + read > 0, + "the probe ended early:\n{}", + String::from_utf8_lossy(&seen) + ); + seen.extend_from_slice(&chunk[..read]); + } + writer.write_all(b"\n").expect("the PTY writes"); + child.wait().expect("sh exits"); + let _teardown = crate::terminal::pty_lifecycle_guard(); + drop(pair.master); + String::from_utf8_lossy(&seen).replace("\r\n", "\n") + } + + /// termlens's own spawn starts the child the PTY layer would have + /// started, as far as the child can tell, so a portable-pty release that + /// changes its spawn fails here instead of drifting apart from this one. + #[test] + fn the_child_cannot_tell_which_spawn_started_it() { + let ours = probe(true); + assert!( + ours.contains("group-leader\nforeground\ncontrolling-tty\nprobe-finished"), + "the probe did not run to the end:\n{ours}" + ); + assert_eq!(ours, probe(false)); } } diff --git a/crates/termlens/src/terminal.rs b/crates/termlens/src/terminal.rs index 6ba3519..bdd73c2 100644 --- a/crates/termlens/src/terminal.rs +++ b/crates/termlens/src/terminal.rs @@ -255,15 +255,15 @@ static PTY_LIFECYCLE: Mutex<()> = Mutex::new(()); /// The lifecycle lock, held — or `None` on a platform with nothing to /// serialize. Callers hold and drop it the same way either way. -type LifecycleGuard = Option>; +pub(crate) type LifecycleGuard = Option>; #[cfg(unix)] -fn pty_lifecycle_guard() -> LifecycleGuard { +pub(crate) fn pty_lifecycle_guard() -> LifecycleGuard { Some(PTY_LIFECYCLE.lock().unwrap_or_else(PoisonError::into_inner)) } #[cfg(not(unix))] -fn pty_lifecycle_guard() -> LifecycleGuard { +pub(crate) fn pty_lifecycle_guard() -> LifecycleGuard { None } @@ -295,7 +295,7 @@ const PTY_OPEN_BACKOFF: Duration = Duration::from_millis(25); /// classifying an `anyhow` error by its text is guesswork, a permanent fault /// still reports its own message at the end, and the cost of being wrong is /// under two seconds on a spawn that was going to fail anyway. -fn open_pty(size: PtySize) -> Result<(PtyPair, LifecycleGuard)> { +pub(crate) fn open_pty(size: PtySize) -> Result<(PtyPair, LifecycleGuard)> { let pty = native_pty_system(); let mut last = String::new(); for attempt in 0..PTY_OPEN_ATTEMPTS { @@ -326,25 +326,18 @@ type SharedWriter = Arc>>>; /// descriptor instead: both refer to the same open file description, /// which is what we want — same terminal, independent blocking. /// -/// Close-on-exec, like every other descriptor the PTY layer opens. A plain -/// `dup` was inherited by every child the test process started while the -/// terminal was alive, holding the master open in processes that have no -/// business with it. +/// Close-on-exec, as std makes every descriptor it opens. A plain `dup` was +/// inherited by every child the test process started while the terminal was +/// alive, holding the master open in processes that have no business with +/// it. #[cfg(unix)] fn dup_writer(master: &dyn portable_pty::MasterPty) -> Option { - use std::os::unix::io::FromRawFd; - let fd = master.as_raw_fd()?; - // SAFETY: `fd` is the live master descriptor (the caller still owns - // the master). F_DUPFD_CLOEXEC returns a fresh descriptor we take sole - // ownership of; `File` closes it exactly once on drop. - #[allow(unsafe_code)] - let duped = unsafe { libc::fcntl(fd, libc::F_DUPFD_CLOEXEC, 0) }; - if duped < 0 { - return None; - } + // SAFETY: `fd` is the live master descriptor, owned by the master the + // caller holds for the whole of this borrow. #[allow(unsafe_code)] - Some(unsafe { std::fs::File::from_raw_fd(duped) }) + let master = unsafe { std::os::fd::BorrowedFd::borrow_raw(fd) }; + master.try_clone_to_owned().ok().map(std::fs::File::from) } #[cfg(not(unix))] @@ -1924,8 +1917,9 @@ impl TerminalBuilder { } // Whatever the child is told TERM is, XTGETTCAP's `TN` reports the // same string — an application must not be able to get two different - // answers to "which terminal is this?". - let term_name = self.envs.iter().find(|(k, _)| k == "TERM").map_or_else( + // answers to "which terminal is this?". Set twice, the later value is + // the one the child gets, so it is the one reported. + let term_name = self.envs.iter().rfind(|(k, _)| k == "TERM").map_or_else( || "xterm-256color".to_owned(), |(_, v)| v.to_string_lossy().into_owned(), ); diff --git a/crates/termlens/tests/descriptors.rs b/crates/termlens/tests/descriptors.rs index 0aa47c8..a8099c5 100644 --- a/crates/termlens/tests/descriptors.rs +++ b/crates/termlens/tests/descriptors.rs @@ -10,17 +10,28 @@ use std::process::Command; use std::sync::{Mutex, PoisonError}; use std::time::Duration; -use termlens::Terminal; +use termlens::{Key, Terminal}; /// The tests below measure and change one descriptor table, so they take /// turns: one leaves a descriptor inheritable that the other would count. static TABLE: Mutex<()> = Mutex::new(()); +/// PATH and nothing else, so that no colour or locale setting of the +/// machine's reaches what is parsed. +fn path() -> std::ffi::OsString { + std::env::var_os("PATH").unwrap_or_default() +} + /// The descriptors an ordinary child of this process has open, as `ls` /// lists them. The child's own listing descriptor is among them, the same /// one each time. fn in_an_ordinary_child() -> BTreeSet { - let out = Command::new("ls").arg("/dev/fd").output().expect("ls runs"); + let out = Command::new("ls") + .arg("/dev/fd") + .env_clear() + .env("PATH", path()) + .output() + .expect("ls runs"); assert!(out.status.success(), "{out:?}"); String::from_utf8_lossy(&out.stdout) .split_whitespace() @@ -71,6 +82,8 @@ fn a_termlens_child_inherits_nothing_but_its_terminal() -> termlens::Result<()> let ordinary = Command::new("sh") .args(["-c", &probe(fd)]) + .env_clear() + .env("PATH", path()) .output() .expect("sh runs"); assert_eq!( @@ -79,13 +92,22 @@ fn a_termlens_child_inherits_nothing_but_its_terminal() -> termlens::Result<()> "an ordinary child must inherit {fd}, or this test proves nothing" ); + // The child reads a line before it exits, so its answer cannot be lost + // to a teardown racing the read (DESIGN.md §2). let mut t = Terminal::builder() .timeout(Duration::from_secs(10)) - .args(["-c", &probe(fd)]) + .env_clear() + .env("PATH", path()) + .args(["-c", &format!("{}; read _", probe(fd))]) .spawn("sh")?; - assert!(t.wait_exit()?.success()); + t.wait_until(|s| s.contains("closed") || s.contains("inherited"))?; let screen = t.screen().text(); - assert_eq!(screen.trim(), "closed", "descriptor {fd} reached the child"); + t.send(Key::Enter)?; + assert!(t.wait_exit()?.success()); + assert!( + screen.contains("closed") && !screen.contains("inherited"), + "descriptor {fd} reached the child:\n{screen}" + ); drop(file); Ok(()) } diff --git a/crates/termlens/tests/process.rs b/crates/termlens/tests/process.rs index f364140..17a9ad8 100644 --- a/crates/termlens/tests/process.rs +++ b/crates/termlens/tests/process.rs @@ -96,59 +96,133 @@ fn a_relative_current_dir_is_the_test_process_s_and_a_relative_program_resolves_ /// pipe std reports a failed exec through, so the child aborted writing to /// it and `spawn` returned `Ok`. The usual culprit is the interpreter a /// script's `#!` line names. The sneakiest is the `\r` that CRLF line -/// endings leave on it, which the message shows escaped. +/// endings leave on it, which the message shows escaped. The messages keep +/// the prefix the skill's failure table promises. #[cfg(unix)] #[test] -fn a_script_whose_interpreter_is_missing_fails_to_spawn_and_names_it() -> termlens::Result<()> { +fn a_script_whose_interpreter_cannot_run_fails_to_spawn_and_names_it() -> termlens::Result<()> { + use std::os::unix::fs::PermissionsExt; + let scratch = std::path::Path::new(env!("CARGO_TARGET_TMPDIR")) .join(format!("exec-failure-{}", std::process::id())); std::fs::create_dir_all(&scratch)?; + let refused = |name: &str, script: &str, mode: u32| -> termlens::Result { + let path = scratch.join(name); + common::write_script(&path, script); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(mode))?; + match Terminal::builder() + .timeout(Duration::from_secs(10)) + .spawn(&path) + { + Ok(_) => panic!("{name}: spawned, and its interpreter cannot run"), + Err(Error::Spawn { reason, .. }) => Ok(reason), + Err(other) => panic!("{name}: not a spawn error: {other}"), + } + }; for (name, script, culprit) in [ ( "missing", "#!/nonexistent/interpreter\necho unreachable\n", - r#""/nonexistent/interpreter""#, + r#"its #! interpreter "/nonexistent/interpreter" does not exist"#, ), ( "crlf", "#!/bin/sh\r\necho unreachable\r\n", - r#""/bin/sh\r""#, + r#"its #! interpreter "/bin/sh\r" does not exist"#, ), ] { - let path = scratch.join(name); - common::write_script(&path, script); - let err = Terminal::builder() - .timeout(Duration::from_secs(10)) - .spawn(&path) - .expect_err("a script whose interpreter does not exist cannot run"); - let Error::Spawn { reason, .. } = &err else { - panic!("{name}: not a spawn error: {err}"); - }; + let reason = refused(name, script, 0o755)?; assert!( - reason.contains(culprit) && reason.contains("does not exist"), + reason.starts_with("cannot execute ") && reason.ends_with(culprit), "{name}: the message must name the interpreter: {reason}" ); } + // An interpreter that is there but is not a program is named as well. + // The kernel's EACCES alone would blame the script, which can be run. + let plain = scratch.join("plain-text"); + std::fs::write(&plain, "not a program\n")?; + std::fs::set_permissions(&plain, std::fs::Permissions::from_mode(0o644))?; + let reason = refused( + "plain-interpreter", + &format!("#!{}\n", plain.display()), + 0o755, + )?; + assert!( + reason.ends_with(&format!( + "its #! interpreter {plain:?} is not an executable file" + )), + "{reason}" + ); + // A script that may be run but not read cannot show its `#!` line, so + // the message names both suspects rather than guessing the loader. Root + // reads it anyway, and the interpreter is named outright. + let reason = refused("execute-only", "#!/nonexistent/interpreter\n", 0o111)?; + assert!(reason.contains("#! interpreter"), "{reason}"); let _ = std::fs::remove_dir_all(&scratch); Ok(()) } /// Any exec the kernel refuses is a spawn error carrying its reason, not /// only a missing interpreter. Every Unix refuses an argument this long, -/// with E2BIG. The error's command line holds the argument, so only the -/// reason is printed. +/// with E2BIG. #[cfg(unix)] #[test] fn an_argument_the_kernel_refuses_is_a_spawn_error_with_its_reason() { - let err = Terminal::builder() + let spawned = Terminal::builder() .timeout(Duration::from_secs(10)) .args(["-c", "true", &"x".repeat(2 << 20)]) - .spawn("sh") - .expect_err("a 2 MiB argument is past every Unix's limit"); - let Error::Spawn { reason, .. } = &err else { - panic!("not a spawn error"); - }; - assert!(reason.contains("Argument list too long"), "{reason}"); + .spawn("sh"); + // Matched, not unwrapped: the terminal and the error both carry the + // 2 MiB command line, which a failing assertion would print whole. + match spawned { + Ok(_) => panic!("a 2 MiB argument is past every Unix's limit, and it spawned"), + Err(Error::Spawn { reason, .. }) => { + assert!(reason.contains("Argument list too long"), "{reason}"); + } + Err(_) => panic!("not a spawn error"), + } +} + +/// A failure before the exec reads as std reports it, and does not blame a +/// program that is fine. A NUL in a variable is refused before anything is +/// forked. A working directory that cannot be entered fails in the child, +/// before the exec, and the directory is named. +#[cfg(unix)] +#[test] +fn a_failure_before_the_exec_does_not_blame_the_program() -> termlens::Result<()> { + use std::os::unix::fs::PermissionsExt; + + use termlens::TerminalBuilder; + + let reason = + |builder: TerminalBuilder| match builder.timeout(Duration::from_secs(10)).spawn("sh") { + Ok(_) => panic!("spawned"), + Err(Error::Spawn { reason, .. }) => reason, + Err(other) => panic!("not a spawn error: {other}"), + }; + let nul = reason(Terminal::builder().env("A", "a\0b")); + assert!( + nul.contains("nul byte") && !nul.contains("cannot execute"), + "{nul}" + ); + + let scratch = std::path::Path::new(env!("CARGO_TARGET_TMPDIR")) + .join(format!("unenterable-{}", std::process::id())); + std::fs::create_dir_all(&scratch)?; + std::fs::set_permissions(&scratch, std::fs::Permissions::from_mode(0o600))?; + // Root enters any directory, and then there is nothing to refuse. + let refused = std::fs::metadata(scratch.join("probe")) + .is_err_and(|e| e.kind() == std::io::ErrorKind::PermissionDenied); + let chdir = refused.then(|| reason(Terminal::builder().current_dir(&scratch))); + std::fs::set_permissions(&scratch, std::fs::Permissions::from_mode(0o700))?; + let _ = std::fs::remove_dir_all(&scratch); + if let Some(chdir) = chdir { + assert!( + chdir.starts_with("cannot enter ") && chdir.contains("unenterable-"), + "{chdir}" + ); + } + Ok(()) } #[test] diff --git a/crates/termlens/tests/queries.rs b/crates/termlens/tests/queries.rs index 234228d..dce92a8 100644 --- a/crates/termlens/tests/queries.rs +++ b/crates/termlens/tests/queries.rs @@ -858,6 +858,8 @@ fn xtgettcap_answers_what_it_knows_and_declines_the_rest() -> termlens::Result<( /// `TN` reports whatever `TERM` the child was actually given, so an /// application cannot get two different answers to "which terminal is this?". +/// Set twice, the later value is the one the child gets, and so the one `TN` +/// reports: it used to report the first. #[test] #[cfg_attr( windows, @@ -867,6 +869,7 @@ fn xtgettcap_tn_follows_the_configured_term() -> termlens::Result<()> { let mut t = common::spawn_emit( Terminal::builder() .size(120, 6) + .env("TERM", "vt100") .env("TERM", "xterm") .timeout(Duration::from_secs(10)), &[ diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 353bd7a..b4c5272 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -208,39 +208,74 @@ portable-pty opens the PTY, but on Unix it no longer starts the child. Its and one of them is the close-on-exec pipe through which std reports a failed `exec`. When the program exists but cannot be executed, the child's write into that pipe failed and std aborted the child. `spawn` then -returned `Ok` with a process already dead of SIGABRT (#563). The usual -causes are a missing `#!` interpreter (a `\r` left by CRLF line endings -counts), a missing dynamic loader, and an argument list over the kernel's -limit. +returned `Ok` with a process already dead of SIGABRT (#563; upstream, +wezterm/wezterm#7893). The usual causes are a missing `#!` interpreter (a +`\r` left by CRLF line endings counts), a missing dynamic loader, and an +argument list over the kernel's limit. -`src/spawn.rs` builds the command portable-pty would build and runs it with -`std::process::Command`: +`src/spawn.rs` builds the command portable-pty 0.9.0 would build and runs +it with `std::process::Command`: - **Resolution** finds the same file by the same rules. `./x` and `../x` are looked up in the working directory alone. Any other relative name is searched for along the child's `PATH`, each entry taken from the working directory. A candidate must pass `access(X_OK)` and must not be a - directory. `argv[0]` is the name as given. + directory. `argv[0]` is the name as given. A working directory that is + not a directory gives way to the child's `HOME`, as in portable-pty. - **The environment** is the same: the inherited one unless `env_clear`, plus the `SHELL` that portable-pty fills in from the login shell when the process has none, then termlens's pins and the test's own variables. - **The session** gets the same `pre_exec` steps. Six signals go back to their defaults and the mask is emptied. `setsid` gives the child a session of its own, and `TIOCSCTTY` makes the PTY its controlling - terminal. + terminal. portable-pty's umask step is left out, because it acts only on + a umask termlens never sets. - **Descriptors** above stdio are marked close-on-exec rather than closed. A successful exec closes exactly what portable-pty closed, and a failed - one still has std's pipe to report through. Every descriptor termlens - opens is close-on-exec too, so no other child of the test process - inherits one. + one still has std's pipe to report through. + +A unit test in `spawn.rs` starts one probe both ways and requires the same +answers about argv[0], directory, environment, descriptors, umask, process +group and controlling terminal. A portable-pty release that changes its +own spawn fails that test instead of drifting apart from this one. + +The error names the culprit where the kernel's errno would point at the +wrong file. `ENOENT` for a script that is plainly there means a missing +`#!` interpreter, which is named. For a binary it means a missing loader. +`EACCES` from an interpreter that cannot be run names the interpreter. A +failure before the exec, such as a NUL in an argument or a directory that +cannot be entered, reads as std reports it, with the directory named in the +second case. + +Reporting the exec has a price. std's `spawn` now returns only once the +exec has happened, so a spawn holds the lifecycle lock about a quarter of a +millisecond longer on Linux. On a machine whose security software inspects +every exec, it is held for the length of that inspection. The slave is opened by name (`tty_name`, `O_NOCTTY`), because portable-pty -does not expose its own handle. Three cases still go to portable-pty -unchanged. A working directory that is not a directory gets `$HOME` -substituted. A program that does not resolve gets portable-pty's diagnosis, -word for word, which is what termlens has always reported. A slave that -cannot be named or opened is the third. On Windows ConPTY reports a failed -`CreateProcess` itself, and portable-pty spawns the child as before. +does not expose its own handle. Four cases still go to portable-pty +unchanged: + +- a process whose children the kernel reaps, because SIGCHLD is ignored + or carries `SA_NOCLDWAIT`. std panics when it cannot reap a failed exec, + and every wait fails there anyway; +- a working directory that is not one, with no UTF-8 `HOME` in the child's + environment; portable-pty then reads the passwd entry; +- a program that does not resolve. portable-pty's diagnosis is what + termlens has always reported, word for word; +- a slave that cannot be named or opened. + +These are the old behaviour, not new risk. On Windows ConPTY reports a +failed `CreateProcess` itself, and portable-pty spawns the child as before. + +termlens's own descriptors are all close-on-exec, the second writer it +duplicates from the master included. One window remains, and it belongs to +the PTY layer. `openpty` creates the pair without close-on-exec, and +portable-pty marks it a moment later. A child that another thread of the +test process forks inside that window inherits the PTY. If it holds the +slave, that terminal reports no EOF until the child exits. It is measured +at a few in a thousand concurrent spawns, and only alongside processes the +test starts itself. ### Windows: ConPTY renders, and that decides what is claimed diff --git a/skills/termlens/SKILL.md b/skills/termlens/SKILL.md index e525da1..a832da3 100644 --- a/skills/termlens/SKILL.md +++ b/skills/termlens/SKILL.md @@ -424,7 +424,7 @@ included in `X`, and `terminal closed (EOF) while waiting for X` is | `input not receivable: the application has not enabled mouse tracking` | `click`/`drag`/`scroll` before the app enabled the mouse | `wait_until(|s| s.mouse_mode() != MouseMode::None)` first | | `input not receivable: mouse at (50, 2) is outside the 20x5 grid` | coordinates swapped or out of range | rule 6 | | `failed to spawn \`sh\`: \`sh\` is a bare program name and env_clear() removed PATH` | bare program name under `env_clear` | absolute path, `bin!`, or `.env("PATH", …)` | -| ``failed to spawn `./x`: cannot execute …: its #! interpreter "…" does not exist`` | the script's `#!` line names a file that is not there; a `\r` at the end of the quoted name means CRLF line endings | install the interpreter, point the `#!` line at one that exists, or save the script with LF endings | +| ``failed to spawn `./x`: cannot execute …: its #! interpreter "…" does not exist`` | the script's `#!` line names a file that is not there, or with `is not an executable file` one that cannot be run; a `\r` at the end of the quoted name means CRLF line endings | install the interpreter, point the `#!` line at one that exists, or save the script with LF endings | | `invalid terminal size: a terminal needs at least 2 columns and 2 rows` | geometry below 2x2 (past 1000 has its own message) | rule 5 | | `the terminal emulator failed and the screen stopped advancing` (`Error::Emulator`) | a bug in the emulation, not in your app | report it to termlens with the detail it names | From edef00d01d787b1d9c9f9b7eb8a7424fc69285c8 Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 14 Oct 2026 18:39:27 +0700 Subject: [PATCH 5/7] test: read the spawn probe through the end of its marker line 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> --- crates/termlens/src/spawn.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs index 3ada5e7..409a559 100644 --- a/crates/termlens/src/spawn.rs +++ b/crates/termlens/src/spawn.rs @@ -388,9 +388,14 @@ read _"#; drop(pair.slave); drop(lifecycle); + // Read up to the marker's line ending, not just the marker: a read + // that stops between the two leaves one side a line shorter. let mut seen = Vec::new(); let mut chunk = [0; 4096]; - while !String::from_utf8_lossy(&seen).contains("probe-finished") { + while !String::from_utf8_lossy(&seen) + .replace("\r\n", "\n") + .contains("probe-finished\n") + { let read = reader.read(&mut chunk).expect("the PTY reads"); assert!( read > 0, From 088aca68588ed54ec7cb0a0185413f5af9b8c58b Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:16:20 +0700 Subject: [PATCH 6/7] test: close the spawn probe's PTY under the lifecycle lock 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> --- crates/termlens/src/spawn.rs | 61 ++++++++++++++++++++++++++++++++++-- 1 file changed, 59 insertions(+), 2 deletions(-) diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs index 409a559..c1d6359 100644 --- a/crates/termlens/src/spawn.rs +++ b/crates/termlens/src/spawn.rs @@ -337,6 +337,9 @@ mod unix { mod tests { use std::ffi::OsString; use std::io::{Read, Write}; + use std::sync::{mpsc, Arc, Mutex, PoisonError}; + use std::thread; + use std::time::Duration; use portable_pty::{CommandBuilder, PtySize}; @@ -360,8 +363,45 @@ echo probe-finished read _"#; /// Run [`PROBE`] in a fresh PTY, through termlens's own spawn or through - /// the PTY layer's, and return what it printed. + /// the PTY layer's, and return what it printed. The probe runs on a + /// thread of its own under a deadline, so a probe that hangs fails here, + /// naming the step it hung in and what it had printed, instead of + /// stalling the suite until the CI job's limit. fn probe(through_termlens: bool) -> String { + let progress = Arc::new(Mutex::new(("starting", Vec::new()))); + let (done, outcome) = mpsc::channel(); + let shared = Arc::clone(&progress); + thread::spawn(move || { + let _ = done.send(run_probe(through_termlens, &shared)); + }); + let path = if through_termlens { + "termlens's spawn" + } else { + "the PTY layer's spawn" + }; + match outcome.recv_timeout(Duration::from_secs(60)) { + Ok(printed) => printed, + Err(mpsc::RecvTimeoutError::Timeout) => { + let (step, seen) = &*progress.lock().unwrap_or_else(PoisonError::into_inner); + panic!( + "the probe through {path} hung while {step}, having printed:\n{}", + String::from_utf8_lossy(seen) + ) + } + Err(mpsc::RecvTimeoutError::Disconnected) => { + panic!("the probe through {path} failed; its panic is above") + } + } + } + + type Progress = Arc)>>; + + fn step(progress: &Progress, now: &'static str) { + progress.lock().unwrap_or_else(PoisonError::into_inner).0 = now; + } + + fn run_probe(through_termlens: bool, progress: &Progress) -> String { + step(progress, "opening the PTY"); let (pair, lifecycle) = crate::terminal::open_pty(PtySize { rows: 80, cols: 300, @@ -378,6 +418,7 @@ read _"#; } let mut reader = pair.master.try_clone_reader().expect("a reader"); let mut writer = pair.master.take_writer().expect("a writer"); + step(progress, "spawning sh"); let mut child = if through_termlens { super::unix::spawn(pair.master.as_ref(), &cmd, false, &env) .expect("termlens's own spawn takes this command") @@ -390,6 +431,7 @@ read _"#; // Read up to the marker's line ending, not just the marker: a read // that stops between the two leaves one side a line shorter. + step(progress, "reading what sh printed"); let mut seen = Vec::new(); let mut chunk = [0; 4096]; while !String::from_utf8_lossy(&seen) @@ -403,11 +445,26 @@ read _"#; String::from_utf8_lossy(&seen) ); seen.extend_from_slice(&chunk[..read]); + progress + .lock() + .unwrap_or_else(PoisonError::into_inner) + .1 + .extend_from_slice(&chunk[..read]); } + step(progress, "answering sh's read"); writer.write_all(b"\n").expect("the PTY writes"); + step(progress, "waiting for sh to exit"); child.wait().expect("sh exits"); - let _teardown = crate::terminal::pty_lifecycle_guard(); + // Every handle on the master closes under the lifecycle lock, as a + // Terminal's do: on macOS the last close revokes the device, and a + // revoke landing after the next probe has opened the same, recycled + // device would hang up that probe's session. + step(progress, "closing the PTY"); + let teardown = crate::terminal::pty_lifecycle_guard(); + drop(writer); + drop(reader); drop(pair.master); + drop(teardown); String::from_utf8_lossy(&seen).replace("\r\n", "\n") } From b6c101ad4b62c24c8316effdcf426ca8d37f48ee Mon Sep 17 00:00:00 2001 From: Vyncint Ng <115854244+vyncint@users.noreply.github.com> Date: Wed, 14 Oct 2026 19:22:46 +0700 Subject: [PATCH 7/7] test: drain the spawn probe's terminal before waiting for sh 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> --- crates/termlens/src/spawn.rs | 109 ++++++++++++++++++++++++++--------- 1 file changed, 81 insertions(+), 28 deletions(-) diff --git a/crates/termlens/src/spawn.rs b/crates/termlens/src/spawn.rs index c1d6359..e349dea 100644 --- a/crates/termlens/src/spawn.rs +++ b/crates/termlens/src/spawn.rs @@ -364,40 +364,76 @@ read _"#; /// Run [`PROBE`] in a fresh PTY, through termlens's own spawn or through /// the PTY layer's, and return what it printed. The probe runs on a - /// thread of its own under a deadline, so a probe that hangs fails here, - /// naming the step it hung in and what it had printed, instead of - /// stalling the suite until the CI job's limit. - fn probe(through_termlens: bool) -> String { - let progress = Arc::new(Mutex::new(("starting", Vec::new()))); + /// thread of its own under a deadline, so a probe that hangs fails, + /// naming the step it hung in, what it had printed, and how the shell and + /// the terminal stood, instead of stalling the suite until the CI job's + /// limit. + fn probe(through_termlens: bool) -> Result { + let progress = Arc::new(Mutex::new(Probe::default())); let (done, outcome) = mpsc::channel(); let shared = Arc::clone(&progress); thread::spawn(move || { let _ = done.send(run_probe(through_termlens, &shared)); }); - let path = if through_termlens { - "termlens's spawn" - } else { - "the PTY layer's spawn" - }; match outcome.recv_timeout(Duration::from_secs(60)) { - Ok(printed) => printed, + Ok(printed) => Ok(printed), Err(mpsc::RecvTimeoutError::Timeout) => { - let (step, seen) = &*progress.lock().unwrap_or_else(PoisonError::into_inner); - panic!( - "the probe through {path} hung while {step}, having printed:\n{}", - String::from_utf8_lossy(seen) - ) - } - Err(mpsc::RecvTimeoutError::Disconnected) => { - panic!("the probe through {path} failed; its panic is above") + let state = progress.lock().unwrap_or_else(PoisonError::into_inner); + let shell = state.pid.map_or_else(String::new, |pid| { + run( + "ps", + &[ + "-o", + "pid=,stat=,pgid=,tpgid=,wchan=", + "-p", + &pid.to_string(), + ], + ) + }); + let tty = state.tty.as_ref().map_or_else(String::new, |tty| { + let flag = if cfg!(target_os = "macos") { + "-f" + } else { + "-F" + }; + run("stty", &[flag, &tty.to_string_lossy(), "-a"]) + }); + Err(format!( + "hung while {}, having printed:\n{}\nthe shell (ps): {shell}\nthe terminal \ + (stty): {tty}", + state.step, + String::from_utf8_lossy(&state.seen) + )) } + Err(mpsc::RecvTimeoutError::Disconnected) => Err("failed; its panic is above".into()), } } - type Progress = Arc)>>; + fn run(program: &str, args: &[&str]) -> String { + std::process::Command::new(program) + .args(args) + .output() + .map_or_else( + |error| error.to_string(), + |out| { + String::from_utf8_lossy(&out.stdout).into_owned() + + &String::from_utf8_lossy(&out.stderr) + }, + ) + } + + #[derive(Default)] + struct Probe { + step: &'static str, + seen: Vec, + pid: Option, + tty: Option, + } + + type Progress = Arc>; fn step(progress: &Progress, now: &'static str) { - progress.lock().unwrap_or_else(PoisonError::into_inner).0 = now; + progress.lock().unwrap_or_else(PoisonError::into_inner).step = now; } fn run_probe(through_termlens: bool, progress: &Progress) -> String { @@ -409,6 +445,7 @@ read _"#; pixel_height: 0, }) .expect("a PTY opens"); + progress.lock().unwrap_or_else(PoisonError::into_inner).tty = pair.master.tty_name(); let mut cmd = CommandBuilder::new("sh"); cmd.args(["-c", PROBE]); cmd.cwd(std::env::current_dir().expect("a working directory")); @@ -426,6 +463,7 @@ read _"#; } else { pair.slave.spawn_command(cmd).expect("sh starts") }; + progress.lock().unwrap_or_else(PoisonError::into_inner).pid = child.process_id(); drop(pair.slave); drop(lifecycle); @@ -448,11 +486,18 @@ read _"#; progress .lock() .unwrap_or_else(PoisonError::into_inner) - .1 + .seen .extend_from_slice(&chunk[..read]); } step(progress, "answering sh's read"); writer.write_all(b"\n").expect("the PTY writes"); + // Drained to the end, as a Terminal's reader drains: the terminal + // echoes the line, and on macOS a process exiting with output still + // queued waits for it to be read, so an unread echo is a shell that + // never finishes exiting. + step(progress, "draining the terminal"); + let mut rest = Vec::new(); + let _ = reader.read_to_end(&mut rest); step(progress, "waiting for sh to exit"); child.wait().expect("sh exits"); // Every handle on the master closes under the lifecycle lock, as a @@ -473,11 +518,19 @@ read _"#; /// changes its spawn fails here instead of drifting apart from this one. #[test] fn the_child_cannot_tell_which_spawn_started_it() { - let ours = probe(true); - assert!( - ours.contains("group-leader\nforeground\ncontrolling-tty\nprobe-finished"), - "the probe did not run to the end:\n{ours}" - ); - assert_eq!(ours, probe(false)); + match (probe(true), probe(false)) { + (Ok(ours), Ok(theirs)) => { + assert!( + ours.contains("group-leader\nforeground\ncontrolling-tty\nprobe-finished"), + "the probe did not run to the end:\n{ours}" + ); + assert_eq!(ours, theirs); + } + (ours, theirs) => panic!( + "through termlens's spawn: {}\n\nthrough the PTY layer's: {}", + ours.unwrap_or_else(|problem| problem), + theirs.unwrap_or_else(|problem| problem) + ), + } } }