From c93f4917bfd64caffd4108d046fbb555d3261cea Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Tue, 6 Oct 2026 15:13:10 -0700 Subject: [PATCH 1/9] Run mxc_pty unit tests in CI and make the pty tests reliable The mxc_pty unit tests ran on no CI lane: the macOS job skipped the module, the Linux job selects lib tests by module and never matched it, and Windows compiles only a stub. Give the Linux lane an explicit step and drop the macOS skip. Enabling them exposed three test defects. native_bridge_cancellation_does_not_inject_eof backgrounded its reader, so the parent shell's exit orphaned the reader's process group. Reading a controlling terminal from an orphaned group fails with EIO, and the `;` separator ran the marker printf regardless, so the test reported an injected end-of-input that never happened. A probe confirmed the marker was already in the pipe before the bridge was torn down. Keep the reader in the foreground process group and wait for it to block in `read`, so only the bridge can supply end-of-input. native_bridge_finishes_when_descendant_holds_terminal left its `sleep` running, which the runner reported as an orphan process. Kill the process group once the assertions are done. assert_canonical_eof_after_input read to end-of-input on the test thread, so a reader that never observes EOF hung the whole suite instead of failing. Read on a worker and report what was collected. Also give reader_from_file the same Darwin end-of-input probe that take_native_stdio already applies, so the boxed reader and the native output bridge agree on how end of input is detected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/workflows/Build.Linux.Job.yml | 8 ++ .github/workflows/Build.MacOS.Job.yml | 7 +- src/mxc-sdk/src/core/mxc_pty/mod.rs | 109 ++++++++++++++++++++++++-- 3 files changed, 112 insertions(+), 12 deletions(-) diff --git a/.github/workflows/Build.Linux.Job.yml b/.github/workflows/Build.Linux.Job.yml index 2a4c6feb3d..1916ef3eb3 100644 --- a/.github/workflows/Build.Linux.Job.yml +++ b/.github/workflows/Build.Linux.Job.yml @@ -131,6 +131,14 @@ jobs: cargo test --locked --release --target "${{ matrix.target }}" -p mxc-sdk -p mxc_ffi --lib state_aware cargo test --locked --release --target "${{ matrix.target }}" -p mxc-sdk --test state_aware + # This lane selects lib tests by module, so mxc_pty needs an explicit + # step; the macOS lane picks it up via the package-wide run. The tests + # drive /dev/ptmx directly and need no sandbox backend installed. + - name: Test unix pty bridge + working-directory: src + run: cargo test --locked --release --target ${{ matrix.target }} + -p mxc-sdk --lib 'core_modules::mxc_pty::' + # These need no LXC installed: the engine tests inject their probes, and # the dispatch and FFI pins are refused before a container is created. - name: Test LXC dispatch and discovery diff --git a/.github/workflows/Build.MacOS.Job.yml b/.github/workflows/Build.MacOS.Job.yml index 995735246b..4366b2f74b 100644 --- a/.github/workflows/Build.MacOS.Job.yml +++ b/.github/workflows/Build.MacOS.Job.yml @@ -44,17 +44,14 @@ jobs: run: cargo build --locked --release --target aarch64-apple-darwin -p mxc_darwin -p mxc-sdk -p unix_test_proxy -p mxc_ffi - # mxc-sdk includes the Seatbelt executor characterization tests. Its - # consolidated mxc_pty module was not part of this pre-consolidation - # macOS lane, so keep those host-sensitive unit tests out of this job. + # mxc-sdk includes the Seatbelt executor characterization tests. # mxc-exec-mac (built above via -p mxc_darwin) is what they drive; the # tests skip via has_platform_exec() if it is missing. sandbox-exec needs # no elevation, so they run in this standard macOS job. - name: Test run: | cargo test --locked --release --target aarch64-apple-darwin \ - -p mxc_darwin -p mxc-sdk -- \ - --skip 'core_modules::mxc_pty::tests::' + -p mxc_darwin -p mxc-sdk - name: Test state-aware SDK boundaries shell: bash diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index b920fb793d..b54f0e5145 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -106,6 +106,11 @@ pub struct PtySize { pub pixel_height: u16, } +/// Interval for the Darwin end-of-input probe. Darwin may not wake `poll` when +/// the last terminal writer closes, so readers retry to observe `EIO`. +#[cfg(target_os = "macos")] +const DARWIN_EOF_PROBE_INTERVAL: Duration = Duration::from_millis(50); + /// Caller-owned primary side of a Unix pseudo-terminal. #[cfg(any(target_os = "linux", target_os = "macos"))] pub struct LivePty { @@ -468,7 +473,7 @@ impl LivePty { #[cfg(target_os = "macos")] let input_reader = InterruptibleReader::new_with_periodic_eof_probe( input_read_fd, - std::time::Duration::from_millis(50), + DARWIN_EOF_PROBE_INTERVAL, )?; let input_canceller = input_reader.canceller(); let input_shutdown = BridgeShutdown::new()?; @@ -675,7 +680,13 @@ impl LivePty { } fn reader_from_file(reader: std::fs::File) -> std::io::Result<(PtyReader, PtyReadCanceller)> { + #[cfg(target_os = "linux")] let reader = InterruptibleReader::new(reader.into())?; + #[cfg(target_os = "macos")] + let reader = InterruptibleReader::new_with_periodic_eof_probe( + reader.into(), + DARWIN_EOF_PROBE_INTERVAL, + )?; let canceller = PtyReadCanceller(reader.canceller()); Ok((PtyReader(reader), canceller)) } @@ -1746,7 +1757,7 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let mut output: Box = if native { + let mut output: Box = if native { let stdio = terminal.take_native_stdio().expect("take native stdio"); let mut output = std::fs::File::from(stdio.stdout); let mut ready = [0_u8; 5]; @@ -1783,8 +1794,36 @@ mod tests { terminal.finish_native_bridge(); drop(terminal); - let mut text = String::new(); - output.read_to_string(&mut text).expect("read output"); + // Read on a worker so a reader that never observes end-of-input fails + // with what it collected instead of hanging the whole suite. + const EOF_TIMEOUT: Duration = Duration::from_secs(10); + let collected = Arc::new(Mutex::new(Vec::new())); + let worker = Arc::clone(&collected); + let (done_tx, done_rx) = std::sync::mpsc::channel(); + std::thread::spawn(move || { + let mut buffer = [0_u8; 1024]; + loop { + match output.read(&mut buffer) { + Ok(0) | Err(_) => break, + Ok(count) => worker + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .extend_from_slice(&buffer[..count]), + } + } + let _ = done_tx.send(()); + }); + let reached_eof = done_rx.recv_timeout(EOF_TIMEOUT).is_ok(); + let text = { + let bytes = collected + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); + String::from_utf8_lossy(&bytes).into_owned() + }; + assert!( + reached_eof, + "reader never observed EOF; collected: {text:?}" + ); assert!(text.contains("terminal-eof-observed"), "got: {text:?}"); } @@ -2029,6 +2068,7 @@ mod tests { command.arg("-c").arg("(sleep 30) & printf 'done\\n'"); let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); + let session = child.id() as libc::pid_t; drop(command); let stdio = terminal.take_native_stdio().expect("take native stdio"); drop(stdio.stdin); @@ -2040,6 +2080,10 @@ mod tests { start.elapsed() < Duration::from_secs(5), "native bridge did not cancel promptly" ); + // SAFETY: the child created its own session with its PID as the process-group ID. + unsafe { + libc::kill(-session, libc::SIGKILL); + } let mut output = std::fs::File::from(stdio.stdout); let mut text = String::new(); @@ -2047,14 +2091,62 @@ mod tests { assert!(text.contains("done"), "got: {text:?}"); } + /// Reads whatever is already buffered on `fd` without blocking. + #[cfg(any(target_os = "linux", target_os = "macos"))] + fn drain_available(fd: std::os::fd::RawFd) -> String { + // SAFETY: `fd` is owned by the caller and stays open across the call. + let previous = unsafe { libc::fcntl(fd, libc::F_GETFL) }; + if previous < 0 { + return String::new(); + } + // SAFETY: as above. + unsafe { libc::fcntl(fd, libc::F_SETFL, previous | libc::O_NONBLOCK) }; + let mut collected = Vec::new(); + let mut buffer = [0_u8; 4096]; + loop { + // SAFETY: `buffer` is a valid writable slice for the length passed. + let count = unsafe { libc::read(fd, buffer.as_mut_ptr().cast(), buffer.len()) }; + if count <= 0 { + break; + } + collected.extend_from_slice(&buffer[..count as usize]); + } + // SAFETY: restores the descriptor flags sampled above. + unsafe { libc::fcntl(fd, libc::F_SETFL, previous) }; + String::from_utf8_lossy(&collected).into_owned() + } + + /// Collects output until `marker` arrives. + #[cfg(any(target_os = "linux", target_os = "macos"))] + fn wait_for_marker(fd: std::os::fd::RawFd, marker: &str) -> String { + const MARKER_TIMEOUT: Duration = Duration::from_secs(10); + const MARKER_POLL: Duration = Duration::from_millis(10); + + let deadline = std::time::Instant::now() + MARKER_TIMEOUT; + let mut collected = String::new(); + while std::time::Instant::now() < deadline { + collected.push_str(&drain_available(fd)); + if collected.contains(marker) { + return collected; + } + std::thread::sleep(MARKER_POLL); + } + panic!("timed out waiting for {marker:?}; got: {collected:?}"); + } + #[cfg(any(target_os = "linux", target_os = "macos"))] #[test] fn native_bridge_cancellation_does_not_inject_eof() { let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); let mut command = Command::new("/bin/sh"); + // The reader has to stay in the foreground process group. A + // backgrounded reader is orphaned once the parent shell exits, and + // reading a controlling terminal from an orphaned group fails with + // EIO, which looks exactly like the injected end-of-input this test + // is meant to catch. command .arg("-c") - .arg("(read value; printf eof-injected) & printf 'done\\n'"); + .arg("printf 'done\\n'; read value; printf 'eof-injected'"); let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); let session = child.id() as libc::pid_t; @@ -2062,16 +2154,19 @@ mod tests { let stdio = terminal.take_native_stdio().expect("take native stdio"); let input = stdio.stdin; - assert!(child.wait().expect("wait child").success()); + // The shell is now blocked in `read`, so any end-of-input it observes + // has to come from the bridge. + let mut text = wait_for_marker(stdio.stdout.as_raw_fd(), "done"); + terminal.finish_native_bridge(); drop(input); // SAFETY: the child created its own session with its PID as the process-group ID. unsafe { libc::kill(-session, libc::SIGKILL); } + let _ = child.wait(); let mut output = std::fs::File::from(stdio.stdout); - let mut text = String::new(); output.read_to_string(&mut text).expect("read output"); assert!(text.contains("done"), "got: {text:?}"); assert!(!text.contains("eof-injected"), "got: {text:?}"); From f093e98b6f4dd40869376d5e9caaaf1247a0cfb0 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Mon, 12 Oct 2026 15:33:58 -0700 Subject: [PATCH 2/9] Resolve MacOS hangs and simply macos pty --- .github/workflows/Build.Linux.Job.yml | 2 + .github/workflows/Build.MacOS.Job.yml | 2 + docs/api-reference/dotnet/v1/api.md | 4 + docs/api-reference/node/v1/api.md | 4 + docs/api-reference/rust/v1/api.md | 4 + .../core/mxc_common/interruptible_reader.rs | 62 +-- src/mxc-sdk/src/core/mxc_pty/mod.rs | 411 ++++++++---------- 7 files changed, 205 insertions(+), 284 deletions(-) diff --git a/.github/workflows/Build.Linux.Job.yml b/.github/workflows/Build.Linux.Job.yml index bf4fde8f01..0d7ac962eb 100644 --- a/.github/workflows/Build.Linux.Job.yml +++ b/.github/workflows/Build.Linux.Job.yml @@ -19,6 +19,8 @@ jobs: target: aarch64-unknown-linux-gnu features: '' runs-on: ${{ matrix.runner }} + # A wedged PTY child would otherwise hold the runner for the 6-hour cap. + timeout-minutes: 45 defaults: run: working-directory: src/tools/lxc diff --git a/.github/workflows/Build.MacOS.Job.yml b/.github/workflows/Build.MacOS.Job.yml index 4366b2f74b..0a8e620231 100644 --- a/.github/workflows/Build.MacOS.Job.yml +++ b/.github/workflows/Build.MacOS.Job.yml @@ -7,6 +7,8 @@ jobs: build: name: arm64 runs-on: macos-15 + # A wedged PTY child would otherwise hold the runner for the 6-hour cap. + timeout-minutes: 45 defaults: run: working-directory: src diff --git a/docs/api-reference/dotnet/v1/api.md b/docs/api-reference/dotnet/v1/api.md index 101f01382d..2241c44089 100644 --- a/docs/api-reference/dotnet/v1/api.md +++ b/docs/api-reference/dotnet/v1/api.md @@ -24,6 +24,10 @@ IsolationSession-only. Seatbelt PTY rejects `guiAccess` and legacy give the caller explicit input, output, resize, and process ownership; there is no separate public attached-console launch API or API accepting ["MXC request JSON"](../../../schema.md#mxc-request-json). +Read the terminal output concurrently rather than only after waiting. On macOS +a workload cannot exit while its terminal output is still unread, so a caller +that waits first without reading can block indefinitely. + ## Discovery and validation | Operation | Use it to | diff --git a/docs/api-reference/node/v1/api.md b/docs/api-reference/node/v1/api.md index e60087d775..2227f9c1db 100644 --- a/docs/api-reference/node/v1/api.md +++ b/docs/api-reference/node/v1/api.md @@ -24,6 +24,10 @@ rejects `guiAccess` and legacy `launchMethod: "open"`. Terminal handles give the caller explicit input, output, resize, and process ownership; there is no separate attached-console launch API or API accepting ["MXC request JSON"](../../../schema.md#mxc-request-json). +Read the terminal output concurrently rather than only after waiting. On macOS +a workload cannot exit while its terminal output is still unread, so a caller +that waits first without reading can block indefinitely. + ## Discovery and validation | Operation | Use it to | diff --git a/docs/api-reference/rust/v1/api.md b/docs/api-reference/rust/v1/api.md index 90b6137035..15d6d0a3b2 100644 --- a/docs/api-reference/rust/v1/api.md +++ b/docs/api-reference/rust/v1/api.md @@ -26,6 +26,10 @@ IsolationSession-only. Seatbelt PTY rejects `guiAccess` and legacy caller explicit input, output, resize, and process ownership instead of attaching the workload to the host process's global console streams. +Read the terminal output concurrently rather than only after `wait`. On macOS +a workload cannot exit while its terminal output is still unread, so a caller +that waits first without reading can block indefinitely. + ## Discovery and validation | Operation | Use it to | diff --git a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs index a788618784..caefef5335 100644 --- a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs +++ b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs @@ -20,7 +20,6 @@ use std::io::{self, Read}; use std::os::fd::{AsFd, AsRawFd, FromRawFd, OwnedFd, RawFd}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; -use std::time::Duration; use crate::mxc_common::sandbox_process::StreamCloser; @@ -78,7 +77,6 @@ pub struct InterruptibleReader { /// Read end of the self-pipe; readable once cancellation writes its byte. wake_r: OwnedFd, state: Arc, - eof_probe_interval: Option, } impl InterruptibleReader { @@ -92,21 +90,6 @@ impl InterruptibleReader { /// Returns the underlying [`io::Error`] if the self-pipe cannot be created /// or descriptor flags cannot be applied. pub fn new(fd: OwnedFd) -> io::Result { - Self::new_with_eof_probe_interval(fd, None) - } - - /// Wrap an owned readable pipe and periodically probe it for EOF. - /// - /// This is needed for Darwin FIFOs, where `poll` may not wake when the - /// final writer closes even though a non-blocking `read` returns EOF. - pub fn new_with_periodic_eof_probe(fd: OwnedFd, interval: Duration) -> io::Result { - Self::new_with_eof_probe_interval(fd, Some(interval)) - } - - fn new_with_eof_probe_interval( - fd: OwnedFd, - eof_probe_interval: Option, - ) -> io::Result { set_cloexec(fd.as_raw_fd())?; set_nonblocking(fd.as_raw_fd())?; @@ -134,7 +117,6 @@ impl InterruptibleReader { cancelled: AtomicBool::new(false), wake_w, }), - eof_probe_interval, }) } @@ -194,13 +176,7 @@ impl Read for InterruptibleReader { ]; // SAFETY: `poll_fds` is a valid 2-element array of pollfds; both // fds are owned and live for the duration of the call. - let timeout = self - .eof_probe_interval - .map(|interval| { - interval.as_millis().clamp(1, libc::c_int::MAX as u128) as libc::c_int - }) - .unwrap_or(-1); - let rc = unsafe { libc::poll(poll_fds.as_mut_ptr(), 2, timeout) }; + let rc = unsafe { libc::poll(poll_fds.as_mut_ptr(), 2, -1) }; if rc < 0 { let err = io::Error::last_os_error(); if err.kind() == io::ErrorKind::Interrupted { @@ -208,13 +184,6 @@ impl Read for InterruptibleReader { } return Err(err); } - if rc == 0 { - match read_fd(self.fd.as_raw_fd(), buf) { - Ok(count) => return Ok(count), - Err(error) if error.kind() == io::ErrorKind::WouldBlock => continue, - Err(error) => return Err(error), - } - } // Cancellation wins over any pending data so a held-open pipe is // abandoned promptly. @@ -304,35 +273,6 @@ mod tests { assert_eq!(reader.read(&mut buf).expect("read eof"), 0); } - #[test] - fn periodic_probe_detects_writer_close_after_data() { - let mut fds = [0 as RawFd; 2]; - assert!(unsafe { libc::pipe(fds.as_mut_ptr()) } == 0, "pipe"); - let read_end = unsafe { OwnedFd::from_raw_fd(fds[0]) }; - let write_end = unsafe { OwnedFd::from_raw_fd(fds[1]) }; - let mut reader = - InterruptibleReader::new_with_periodic_eof_probe(read_end, Duration::from_millis(10)) - .expect("wrap reader"); - let mut writer = std::fs::File::from(write_end); - writer.write_all(b"hello").expect("write"); - - let mut buf = [0u8; 16]; - let n = reader.read(&mut buf).expect("read data"); - assert_eq!(&buf[..n], b"hello"); - - let closer = std::thread::spawn(move || { - std::thread::sleep(Duration::from_millis(50)); - drop(writer); - }); - let started = Instant::now(); - assert_eq!(reader.read(&mut buf).expect("read eof"), 0); - assert!( - started.elapsed() < Duration::from_secs(5), - "periodic EOF probe did not observe writer close" - ); - closer.join().expect("writer closer"); - } - #[test] fn zero_length_read_returns_ok_zero_without_blocking() { // The write end stays open, so a normal read would block; a zero-length diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index b54f0e5145..cb5f46d80c 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -106,11 +106,6 @@ pub struct PtySize { pub pixel_height: u16, } -/// Interval for the Darwin end-of-input probe. Darwin may not wake `poll` when -/// the last terminal writer closes, so readers retry to observe `EIO`. -#[cfg(target_os = "macos")] -const DARWIN_EOF_PROBE_INTERVAL: Duration = Duration::from_millis(50); - /// Caller-owned primary side of a Unix pseudo-terminal. #[cfg(any(target_os = "linux", target_os = "macos"))] pub struct LivePty { @@ -468,13 +463,7 @@ impl LivePty { let input_primary = self.primary.try_clone()?; let (input_read_fd, input_write_fd) = create_pipe()?; - #[cfg(target_os = "linux")] let input_reader = InterruptibleReader::new(input_read_fd)?; - #[cfg(target_os = "macos")] - let input_reader = InterruptibleReader::new_with_periodic_eof_probe( - input_read_fd, - DARWIN_EOF_PROBE_INTERVAL, - )?; let input_canceller = input_reader.canceller(); let input_shutdown = BridgeShutdown::new()?; @@ -511,17 +500,26 @@ impl LivePty { } } let mut reader = output_reader; - let mut writer = File::from(output_write_fd); + let mut writer = Some(File::from(output_write_fd)); let mut buffer = [0_u8; 8192]; loop { let count = match reader.read(&mut buffer) { Ok(0) | Err(_) => return, Ok(count) => count, }; - if write_all_until_shutdown(&mut writer, &buffer[..count], &output_shutdown) + let Some(sink) = writer.as_mut() else { + continue; + }; + if write_all_until_shutdown(sink, &buffer[..count], &output_shutdown) .is_err() { - return; + if output_shutdown.is_cancelled() { + return; + } + // The consumer closed its end. Keep draining the + // terminal anyway: on Darwin the child cannot exit + // while its output sits in the primary's queue. + writer = None; } } } @@ -680,13 +678,7 @@ impl LivePty { } fn reader_from_file(reader: std::fs::File) -> std::io::Result<(PtyReader, PtyReadCanceller)> { - #[cfg(target_os = "linux")] let reader = InterruptibleReader::new(reader.into())?; - #[cfg(target_os = "macos")] - let reader = InterruptibleReader::new_with_periodic_eof_probe( - reader.into(), - DARWIN_EOF_PROBE_INTERVAL, - )?; let canceller = PtyReadCanceller(reader.canceller()); Ok((PtyReader(reader), canceller)) } @@ -1122,90 +1114,12 @@ fn create_pipe() -> std::io::Result<(std::os::fd::OwnedFd, std::os::fd::OwnedFd) } #[cfg(target_os = "macos")] { - create_cloexec_fifo() - } -} - -#[cfg(target_os = "macos")] -fn create_cloexec_fifo() -> std::io::Result<(std::os::fd::OwnedFd, std::os::fd::OwnedFd)> { - use std::ffi::{CStr, CString}; - - struct FifoPath { - directory: CString, - fifo: CString, - created: bool, - } - - impl Drop for FifoPath { - fn drop(&mut self) { - if self.created { - // SAFETY: both paths remain valid NUL-terminated strings. - unsafe { - libc::unlink(self.fifo.as_ptr()); - libc::rmdir(self.directory.as_ptr()); - } - } - } - } - - let mut template = b"/tmp/mxc-pty-pipe.XXXXXX\0".to_vec(); - // SAFETY: `template` is writable and ends with six X bytes plus NUL as - // required by `mkdtemp`. - let directory = unsafe { libc::mkdtemp(template.as_mut_ptr().cast()) }; - if directory.is_null() { - return Err(std::io::Error::last_os_error()); - } - // SAFETY: successful `mkdtemp` returns a NUL-terminated pointer into - // `template`, which remains alive for this conversion. - let directory = unsafe { CStr::from_ptr(directory) }.to_owned(); - let mut fifo = directory.as_bytes().to_vec(); - fifo.extend_from_slice(b"/pipe"); - let fifo = CString::new(fifo).expect("temporary FIFO path contains no NUL"); - let mut path = FifoPath { - directory, - fifo, - created: true, - }; - - // SAFETY: `path.fifo` is a valid path in a private mode-0700 directory. - if unsafe { libc::mkfifo(path.fifo.as_ptr(), 0o600) } != 0 { - return Err(std::io::Error::last_os_error()); - } - let read = open_fifo_endpoint(&path.fifo, libc::O_RDONLY | libc::O_NONBLOCK)?; - let write = open_fifo_endpoint(&path.fifo, libc::O_WRONLY)?; - set_blocking(read.as_raw_fd())?; - - // SAFETY: open descriptors retain the FIFO after its directory entry is removed. - if unsafe { libc::unlink(path.fifo.as_ptr()) } != 0 { - return Err(std::io::Error::last_os_error()); - } - // SAFETY: the private directory is empty after unlinking the FIFO. - if unsafe { libc::rmdir(path.directory.as_ptr()) } != 0 { - return Err(std::io::Error::last_os_error()); - } - path.created = false; - Ok((read, write)) -} - -#[cfg(target_os = "macos")] -fn open_fifo_endpoint( - path: &std::ffi::CStr, - access: libc::c_int, -) -> std::io::Result { - use std::os::fd::FromRawFd; - - loop { - // SAFETY: `path` is a valid FIFO path and O_CLOEXEC is applied as part - // of descriptor creation. - let fd = unsafe { libc::open(path.as_ptr(), access | libc::O_CLOEXEC) }; - if fd >= 0 { - // SAFETY: `open` returned a new owned descriptor. - return Ok(unsafe { std::os::fd::OwnedFd::from_raw_fd(fd) }); - } - let error = std::io::Error::last_os_error(); - if error.kind() != std::io::ErrorKind::Interrupted { - return Err(error); - } + // Darwin has no `pipe2`, so close-on-exec is set immediately after + // creation. This matches what `std::process` does for its own stdio. + let (read, write) = nix::unistd::pipe().map_err(std::io::Error::from)?; + set_cloexec(read.as_raw_fd())?; + set_cloexec(write.as_raw_fd())?; + Ok((read, write)) } } @@ -1231,14 +1145,14 @@ fn set_nonblocking(fd: std::os::fd::RawFd) -> std::io::Result<()> { } #[cfg(target_os = "macos")] -fn set_blocking(fd: std::os::fd::RawFd) -> std::io::Result<()> { +fn set_cloexec(fd: std::os::fd::RawFd) -> std::io::Result<()> { // SAFETY: `fd` is a valid open descriptor; these commands only update its flags. - let flags = unsafe { libc::fcntl(fd, libc::F_GETFL) }; + let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) }; if flags < 0 { return Err(std::io::Error::last_os_error()); } - // SAFETY: `fd` remains open and clearing O_NONBLOCK is valid for F_SETFL. - if unsafe { libc::fcntl(fd, libc::F_SETFL, flags & !libc::O_NONBLOCK) } < 0 { + // SAFETY: `fd` remains open and `flags | FD_CLOEXEC` is valid for F_SETFD. + if unsafe { libc::fcntl(fd, libc::F_SETFD, flags | libc::FD_CLOEXEC) } < 0 { return Err(std::io::Error::last_os_error()); } Ok(()) @@ -1582,6 +1496,100 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos"))] static RUN_WITH_PTY_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + /// Reads a PTY-backed stream on a worker thread so the child is never + /// blocked by a full terminal output queue. + /// + /// Darwin blocks the final close of the secondary until the primary's + /// output queue drains, and `SIGKILL` does not release it. So a drain must + /// already be running before anything waits for the child. + #[cfg(any(target_os = "linux", target_os = "macos"))] + struct PtyDrain { + collected: Arc>>, + worker: Option>, + finished: std::sync::mpsc::Receiver<()>, + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + impl PtyDrain { + const TIMEOUT: Duration = Duration::from_secs(20); + + fn start(mut source: Box) -> Self { + let collected = Arc::new(Mutex::new(Vec::new())); + let sink = Arc::clone(&collected); + let (done, finished) = std::sync::mpsc::sync_channel(1); + let worker = std::thread::spawn(move || { + let mut buffer = [0u8; 8192]; + while let Ok(count) = source.read(&mut buffer) { + if count == 0 { + break; + } + sink.lock() + .unwrap_or_else(|e| e.into_inner()) + .extend_from_slice(&buffer[..count]); + } + let _ = done.send(()); + }); + Self { + collected, + worker: Some(worker), + finished, + } + } + + /// Everything drained so far, as lossy UTF-8. + fn snapshot(&self) -> String { + let bytes = self.collected.lock().unwrap_or_else(|e| e.into_inner()); + String::from_utf8_lossy(&bytes).into_owned() + } + + /// Blocks until `marker` appears in the drained output. + fn wait_for(&self, marker: &str) -> String { + let deadline = std::time::Instant::now() + Self::TIMEOUT; + loop { + let seen = self.snapshot(); + if seen.contains(marker) { + return seen; + } + assert!( + std::time::Instant::now() < deadline, + "terminal output never contained {marker:?}; got: {seen:?}" + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + + /// Waits for end of input and returns the full output. + fn finish(mut self) -> String { + self.finished + .recv_timeout(Self::TIMEOUT) + .unwrap_or_else(|_| panic!("terminal drain did not reach end of input")); + if let Some(worker) = self.worker.take() { + worker.join().expect("terminal drain thread"); + } + self.snapshot() + } + } + + /// Waits for `child` with a deadline instead of hanging the whole suite. + /// + /// Deliberately does not fall back to `kill`: a child stuck in the Darwin + /// terminal drain ignores `SIGKILL`. Unwinding drops `LivePty`, which + /// closes the primary and releases it. + #[cfg(any(target_os = "linux", target_os = "macos"))] + fn wait_for_exit(child: &mut std::process::Child) -> std::process::ExitStatus { + let deadline = std::time::Instant::now() + Duration::from_secs(20); + loop { + if let Some(status) = child.try_wait().expect("poll child") { + return status; + } + assert!( + std::time::Instant::now() < deadline, + "child did not exit within the test deadline" + ); + std::thread::sleep(Duration::from_millis(10)); + } + } + #[test] fn default_options() { let opts = PtyOptions::default(); @@ -1625,15 +1633,14 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let mut reader = terminal.try_clone_reader().expect("clone reader"); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); let mut writer = terminal.take_writer().expect("take writer"); writer.write_all(b"hello\r").expect("write input"); drop(writer); - let status = child.wait().expect("wait child"); - assert!(status.success()); - let mut output = String::new(); - reader.read_to_string(&mut output).expect("read output"); + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); assert!(output.contains("40 120"), "got: {output:?}"); assert!(output.contains("reply:hello"), "got: {output:?}"); } @@ -1649,7 +1656,7 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let mut reader = terminal.try_clone_reader().expect("clone reader"); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); let mut writer = terminal.take_writer().expect("take writer"); writer @@ -1657,22 +1664,9 @@ mod tests { .expect("write partial line"); drop(writer); - let deadline = std::time::Instant::now() + Duration::from_secs(5); - loop { - if let Some(status) = child.try_wait().expect("poll child") { - assert!(status.success()); - break; - } - if std::time::Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); - panic!("child did not observe canonical terminal EOF"); - } - std::thread::sleep(Duration::from_millis(10)); - } - - let mut output = String::new(); - reader.read_to_string(&mut output).expect("read output"); + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); assert!(output.contains("terminal-eof-observed"), "got: {output:?}"); } @@ -1685,7 +1679,7 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let _reader = terminal.try_clone_reader().expect("clone reader"); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); let started = std::time::Instant::now(); drop(terminal.take_writer().expect("take writer")); @@ -1696,6 +1690,8 @@ mod tests { let _ = child.kill(); let _ = child.wait(); + drop(terminal); + let _ = drain.finish(); } #[cfg(any(target_os = "linux", target_os = "macos"))] @@ -1707,6 +1703,7 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); let mut fill = terminal .access @@ -1732,7 +1729,13 @@ mod tests { Err(error) => panic!("fill PTY input queue: {error}"), } } + // Linux reports `EWOULDBLOCK` once the terminal input queue is full; + // Darwin's `ttyinput` silently discards the excess instead. Either way + // the queue is saturated, which is what the drop path must survive. + #[cfg(target_os = "linux")] assert!(queue_full, "PTY input queue did not reach backpressure"); + #[cfg(target_os = "macos")] + let _ = queue_full; drop(fill); let started = std::time::Instant::now(); @@ -1744,6 +1747,8 @@ mod tests { let _ = child.kill(); let _ = child.wait(); + drop(terminal); + let _ = drain.finish(); } #[cfg(any(target_os = "linux", target_os = "macos"))] @@ -1757,73 +1762,29 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let mut output: Box = if native { + let (mut writer, output): (Box, Box) = if native { let stdio = terminal.take_native_stdio().expect("take native stdio"); - let mut output = std::fs::File::from(stdio.stdout); - let mut ready = [0_u8; 5]; - output.read_exact(&mut ready).expect("read readiness"); - assert_eq!(&ready, b"ready"); - let mut writer = std::fs::File::from(stdio.stdin); - writer.write_all(input).expect("write terminal input"); - drop(writer); - Box::new(output) + ( + Box::new(std::fs::File::from(stdio.stdin)), + Box::new(std::fs::File::from(stdio.stdout)), + ) } else { - let mut output = terminal.try_clone_reader().expect("clone reader"); - let mut ready = [0_u8; 5]; - output.read_exact(&mut ready).expect("read readiness"); - assert_eq!(&ready, b"ready"); - let mut writer = terminal.take_writer().expect("take writer"); - writer.write_all(input).expect("write terminal input"); - drop(writer); - output + ( + Box::new(terminal.take_writer().expect("take writer")), + terminal.try_clone_reader().expect("clone reader"), + ) }; - let deadline = std::time::Instant::now() + Duration::from_secs(5); - loop { - if let Some(status) = child.try_wait().expect("poll child") { - assert!(status.success()); - break; - } - if std::time::Instant::now() >= deadline { - let _ = child.kill(); - let _ = child.wait(); - panic!("child did not observe canonical terminal EOF"); - } - std::thread::sleep(Duration::from_millis(10)); - } + let drain = PtyDrain::start(output); + drain.wait_for("ready"); + writer.write_all(input).expect("write terminal input"); + drop(writer); + + assert!(wait_for_exit(&mut child).success()); terminal.finish_native_bridge(); drop(terminal); - // Read on a worker so a reader that never observes end-of-input fails - // with what it collected instead of hanging the whole suite. - const EOF_TIMEOUT: Duration = Duration::from_secs(10); - let collected = Arc::new(Mutex::new(Vec::new())); - let worker = Arc::clone(&collected); - let (done_tx, done_rx) = std::sync::mpsc::channel(); - std::thread::spawn(move || { - let mut buffer = [0_u8; 1024]; - loop { - match output.read(&mut buffer) { - Ok(0) | Err(_) => break, - Ok(count) => worker - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .extend_from_slice(&buffer[..count]), - } - } - let _ = done_tx.send(()); - }); - let reached_eof = done_rx.recv_timeout(EOF_TIMEOUT).is_ok(); - let text = { - let bytes = collected - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - String::from_utf8_lossy(&bytes).into_owned() - }; - assert!( - reached_eof, - "reader never observed EOF; collected: {text:?}" - ); + let text = drain.finish(); assert!(text.contains("terminal-eof-observed"), "got: {text:?}"); } @@ -1890,29 +1851,31 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - if native { + let (writer, output): (Option>, Box) = if native { let stdio = terminal.take_native_stdio().expect("take native stdio"); - let mut output = std::fs::File::from(stdio.stdout); - let mut ready = [0_u8; 5]; - output.read_exact(&mut ready).expect("read readiness"); - assert_eq!(&ready, b"ready"); - drop(stdio.stdin); - std::thread::sleep(Duration::from_millis(200)); - assert!(child.try_wait().expect("poll child").is_none()); - let _ = child.kill(); - let _ = child.wait(); - terminal.finish_native_bridge(); + ( + Some(Box::new(std::fs::File::from(stdio.stdin))), + Box::new(std::fs::File::from(stdio.stdout)), + ) } else { - let mut output = terminal.try_clone_reader().expect("clone reader"); - let mut ready = [0_u8; 5]; - output.read_exact(&mut ready).expect("read readiness"); - assert_eq!(&ready, b"ready"); - drop(terminal.take_writer().expect("take writer")); - std::thread::sleep(Duration::from_millis(200)); - assert!(child.try_wait().expect("poll child").is_none()); - let _ = child.kill(); - let _ = child.wait(); + (None, terminal.try_clone_reader().expect("clone reader")) + }; + + let drain = PtyDrain::start(output); + drain.wait_for("ready"); + match writer { + Some(writer) => drop(writer), + None => drop(terminal.take_writer().expect("take writer")), } + + std::thread::sleep(Duration::from_millis(200)); + assert!(child.try_wait().expect("poll child").is_none()); + let _ = child.kill(); + let _ = child.wait(); + terminal.finish_native_bridge(); + drop(terminal); + let text = drain.finish(); + assert!(!text.contains("injected"), "got: {text:?}"); } #[cfg(any(target_os = "linux", target_os = "macos"))] @@ -1962,14 +1925,16 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let _reader = terminal.try_clone_reader().expect("clone reader"); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); let mut writer = terminal.take_writer().expect("take writer"); let mut input = b"x\n".repeat(128 * 1024); input.extend_from_slice(b"partial-line"); writer.write_all(&input).expect("write large partial input"); drop(writer); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let _ = drain.finish(); } #[cfg(any(target_os = "linux", target_os = "macos"))] @@ -1983,20 +1948,20 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let (mut reader, closer) = terminal + let (reader, closer) = terminal .try_clone_reader_with_canceller() .expect("clone reader"); - assert!(child.wait().expect("wait child").success()); - let reader_thread = std::thread::spawn(move || { - let mut output = String::new(); - reader.read_to_string(&mut output).expect("read output"); - output - }); - std::thread::sleep(Duration::from_millis(50)); - closer.close(); + // The drain must already be running: on Darwin the foreground shell + // cannot exit while its output sits in the terminal queue. + let drain = PtyDrain::start(reader); + assert!(wait_for_exit(&mut child).success()); + drain.wait_for("foreground-exited"); - let output = reader_thread.join().expect("reader thread"); + // The backgrounded `sleep` still holds the secondary open, so the read + // parks until it is cancelled. + closer.close(); + let output = drain.finish(); assert!(output.contains("foreground-exited"), "got: {output:?}"); } From d180374408933a69072b88ea896ae6e46cdd891d Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Mon, 12 Oct 2026 16:29:03 -0700 Subject: [PATCH 3/9] additional pty test coverage --- src/mxc-sdk/src/core/mxc_pty/mod.rs | 184 ++++++++++++++++++++++++++++ 1 file changed, 184 insertions(+) diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index cb5f46d80c..bed7076273 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -1670,6 +1670,146 @@ mod tests { assert!(output.contains("terminal-eof-observed"), "got: {output:?}"); } + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn close_writer_sends_canonical_terminal_eof() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command + .arg("-c") + .arg("cat >/dev/null; printf 'closed-writer-eof\\n'"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + terminal.close_writer(); + // Closing again is a no-op, and the writer can no longer be taken. + terminal.close_writer(); + assert_eq!( + terminal + .take_writer() + .err() + .expect("writer was already closed") + .kind(), + std::io::ErrorKind::BrokenPipe + ); + + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); + assert!(output.contains("closed-writer-eof"), "got: {output:?}"); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn close_writer_after_take_writer_leaves_the_taken_writer_usable() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command + .arg("-c") + .arg("read value; printf 'reply:%s\\n' \"$value\""); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + let mut writer = terminal.take_writer().expect("take writer"); + terminal.close_writer(); + writer.write_all(b"still-writable\r").expect("write input"); + drop(writer); + + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); + assert!(output.contains("reply:still-writable"), "got: {output:?}"); + } + + /// The drain-before-wait pattern every backend uses: nothing claimed a + /// reader, so the runner claims one and discards output while it waits. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn take_unclaimed_reader_drains_output_so_waiting_cannot_deadlock() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command + .arg("-c") + .arg("head -c 200000 /dev/zero; printf 'drained-to-exit\\n'"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + + terminal.close_writer(); + let (reader, canceller) = terminal + .take_unclaimed_reader() + .expect("take unclaimed reader") + .expect("reader was unclaimed"); + let drain = PtyDrain::start(reader); + assert!( + terminal + .take_unclaimed_reader() + .expect("second take") + .is_none(), + "the reader is claimed after the first take" + ); + + assert!(wait_for_exit(&mut child).success()); + let seen = drain.wait_for("drained-to-exit"); + assert!(seen.contains("drained-to-exit"), "got: {seen:?}"); + canceller.close(); + drop(terminal); + let _ = drain.finish(); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn take_unclaimed_reader_yields_nothing_once_a_reader_is_claimed() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg("printf claimed"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + assert!( + terminal + .take_unclaimed_reader() + .expect("take unclaimed reader") + .is_none(), + "a cloned reader belongs to the caller" + ); + + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); + assert!(output.contains("claimed"), "got: {output:?}"); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn take_unclaimed_reader_yields_nothing_after_native_stdio() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg("printf bridged"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let stdio = terminal.take_native_stdio().expect("take native stdio"); + + assert!( + terminal + .take_unclaimed_reader() + .expect("take unclaimed reader") + .is_none(), + "the native bridge owns terminal output" + ); + + assert!(wait_for_exit(&mut child).success()); + terminal.finish_native_bridge(); + drop(stdio); + } + #[cfg(any(target_os = "linux", target_os = "macos"))] #[test] fn dropping_untouched_writer_does_not_wait_for_an_input_reader() { @@ -2258,6 +2398,50 @@ mod tests { drop(stdio.stdout); } + /// macOS has no `F_SETPIPE_SZ`, so fill the output pipe by volume instead. + #[cfg(target_os = "macos")] + #[test] + fn native_bridge_cancels_when_the_output_pipe_fills() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg("head -c 1000000 /dev/zero"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let stdio = terminal.take_native_stdio().expect("take native stdio"); + drop(stdio.stdin); + + // Never read stdout: the pipe fills, the bridge parks in its + // cancellable write, and the child blocks on the terminal behind it. + std::thread::sleep(Duration::from_millis(250)); + assert!( + child.try_wait().expect("poll child").is_none(), + "child should still be blocked behind the full output pipe" + ); + + // Run the join off-thread so a bridge that ignores cancellation fails + // the test instead of hanging it. + let (done, finished) = std::sync::mpsc::sync_channel(1); + std::thread::scope(|scope| { + scope.spawn(|| { + terminal.finish_native_bridge(); + let _ = done.send(()); + }); + let cancelled = finished.recv_timeout(Duration::from_secs(5)).is_ok(); + if !cancelled { + // Drain so the parked bridge can finish and the scope can join. + let mut sink = std::fs::File::from(stdio.stdout.try_clone().expect("clone stdout")); + let mut scratch = [0u8; 8192]; + while sink.read(&mut scratch).unwrap_or(0) > 0 {} + } + assert!(cancelled, "full output bridge did not cancel promptly"); + }); + + drop(stdio.stdout); + drop(terminal); + let _ = wait_for_exit(&mut child); + } + #[cfg(any(target_os = "linux", target_os = "macos"))] #[test] fn echo_runs_under_pty() { From 277f3097df90b88d35b5c156141aae851fdea793 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Mon, 12 Oct 2026 17:02:51 -0700 Subject: [PATCH 4/9] add linux check for regression --- src/mxc-sdk/src/core/mxc_pty/mod.rs | 37 ++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index bed7076273..5765afbca1 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -2127,7 +2127,7 @@ mod tests { input.write_all(b"hello").expect("write partial input"); drop(input); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); terminal.finish_native_bridge(); let mut text = String::new(); output.read_to_string(&mut text).expect("read output"); @@ -2156,7 +2156,7 @@ mod tests { stdio.stdout }); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); terminal.finish_native_bridge(); let stdout = input_thread.join().expect("input thread"); let mut output = std::fs::File::from(stdout); @@ -2178,7 +2178,7 @@ mod tests { let stdio = terminal.take_native_stdio().expect("take native stdio"); drop(stdio.stdin); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); let start = std::time::Instant::now(); terminal.finish_native_bridge(); assert!( @@ -2297,7 +2297,7 @@ mod tests { output.read_to_end(&mut bytes).expect("read output"); bytes }); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); terminal.finish_native_bridge(); let output = output_thread.join().expect("output thread"); @@ -2325,7 +2325,7 @@ mod tests { let buffer = vec![b'x'; 1024 * 1024]; let _ = input.write_all(&buffer); }); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); let start = std::time::Instant::now(); terminal.finish_native_bridge(); @@ -2349,7 +2349,32 @@ mod tests { drop(stdio.stdin); drop(stdio.stdout); - assert!(child.wait().expect("wait child").success()); + assert!(wait_for_exit(&mut child).success()); + terminal.finish_native_bridge(); + } + + /// The closed-consumer case above drains the child's whole output in one + /// read, so it only catches a bridge that stops draining because Darwin + /// blocks the secondary's close. Gate a bulk write behind the close so the + /// child also blocks on a full terminal queue, which catches it anywhere. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn native_bridge_keeps_draining_after_the_output_consumer_closes() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg("read go; head -c 1000000 /dev/zero"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let stdio = terminal.take_native_stdio().expect("take native stdio"); + + // Close the consumer first so the echoed input already triggers EPIPE. + drop(stdio.stdout); + let mut input = std::fs::File::from(stdio.stdin); + input.write_all(b"go\n").expect("write input"); + drop(input); + + assert!(wait_for_exit(&mut child).success()); terminal.finish_native_bridge(); } From 7c8f3969e54a24da45046843547ae5f8c2fc5d76 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Tue, 13 Oct 2026 11:59:19 -0700 Subject: [PATCH 5/9] Close the macOS close-on-exec window and cover the PTY API macOS has no atomic close-on-exec pipe: there is no `pipe2`, and `socketpair` has no `SOCK_CLOEXEC`. Both ends are inheritable between `pipe(2)` and the `fcntl` that marks them, so a spawn on another thread during that window captures them and the child holds a live handle to another container's stdin or stdout. Linux has `pipe2`, so the wakeup self-pipe now uses it and the race is gone outright there. macOS closes the window from the other side: a new `inherited_descriptors` module drops every descriptor that is not marked close-on-exec, called from the `pre_exec` hook `LivePty` already installs. A full descriptor-table scan costs ~15ms per spawn, so the scrub walks `/dev/fd` instead. The libc `getdirentries` wrapper is unavailable once 64-bit inodes are in effect, so the syscall is issued directly, with the full scan kept as a fallback. The walk completes before anything is closed, since removing entries mid-walk shifts the directory offset and skips descriptors. Descriptors already marked close-on-exec are spared, including the pipe the Rust runtime uses to report a failed `exec`. Closing that one makes every failure look like a success, so a test pins the behaviour. Also adds coverage for the public surface the backends depend on, which had none: `size`, `is_canonical_mode`, SIGWINCH delivery on resize, `attach_forwarded` against a raw-mode terminal, and `RawSecondaryGuard`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../core/mxc_common/inherited_descriptors.rs | 286 ++++++++++++++++++ .../core/mxc_common/interruptible_reader.rs | 48 ++- src/mxc-sdk/src/core/mxc_common/mod.rs | 5 + src/mxc-sdk/src/core/mxc_pty/mod.rs | 257 ++++++++++++++++ 4 files changed, 581 insertions(+), 15 deletions(-) create mode 100644 src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs diff --git a/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs b/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs new file mode 100644 index 0000000000..330048e109 --- /dev/null +++ b/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs @@ -0,0 +1,286 @@ +//! Close-on-exec hygiene for descriptors inherited across `fork`. +//! +//! macOS has no atomic close-on-exec pipe: there is no `pipe2`, and +//! `socketpair` has no `SOCK_CLOEXEC`. Both ends are therefore inheritable +//! between `pipe(2)` and the `fcntl` that marks them, so a spawn running on +//! another thread during that window captures them. The child then holds a +//! live handle to another container's stdin or stdout. +//! +//! Only path-based `open(2)` is atomic here, which pipes cannot use. The +//! window is closed from the other side instead: between `fork` and `exec`, +//! drop every descriptor that is not marked close-on-exec. + +use std::os::fd::RawFd; + +/// Entries collected per enumeration pass before falling back to a full scan. +const MAX_TRACKED_DESCRIPTORS: usize = 1024; + +/// Close inherited descriptors that are not marked close-on-exec. +/// +/// Standard streams are left alone, as is every descriptor already marked +/// `FD_CLOEXEC` — including the pipe the Rust runtime uses to report a failed +/// `exec` back to the parent. +/// +/// # Safety +/// +/// Must be called only between `fork` and `exec`. It closes descriptors the +/// caller does not own, and is async-signal-safe only in that context. +pub unsafe fn close_unmarked_inherited_descriptors() { + // SAFETY: the caller guarantees the post-fork, pre-exec context. + if unsafe { close_enumerated() } { + return; + } + // SAFETY: as above. + unsafe { close_scanned() }; +} + +/// Close unmarked descriptors found by enumerating `/dev/fd`. +/// +/// Returns `false` if the directory could not be enumerated, or if it held +/// more entries than [`MAX_TRACKED_DESCRIPTORS`]. +/// +/// # Safety +/// +/// Must be called only between `fork` and `exec`. +unsafe fn close_enumerated() -> bool { + let mut found = [0 as RawFd; MAX_TRACKED_DESCRIPTORS]; + let Some(count) = collect_unmarked(&mut found) else { + return false; + }; + for fd in &found[..count] { + // SAFETY: `fd` was reported open and is not a standard stream. + unsafe { libc::close(*fd) }; + } + true +} + +/// Record the open descriptors that lack `FD_CLOEXEC` into `found`, returning +/// how many were stored. +/// +/// Standard streams are skipped, as is the handle used for the walk. Returns +/// `None` if `/dev/fd` could not be read or `found` was too small, which tells +/// the caller to fall back to a full scan. +/// +/// The directory is read to completion before anything is closed: removing +/// entries mid-walk would shift the directory offset and skip descriptors. +fn collect_unmarked(found: &mut [RawFd]) -> Option { + // `getdirentries64`. The libc wrapper is unavailable once 64-bit inodes + // are in effect, so the syscall is issued directly. + const SYS_GETDIRENTRIES64: libc::c_int = 344; + + // SAFETY: a NUL-terminated literal path with no output parameters. + let dir = unsafe { libc::open(c"/dev/fd".as_ptr(), libc::O_RDONLY | libc::O_CLOEXEC) }; + if dir < 0 { + return None; + } + + let mut count = 0usize; + let mut buffer = [0u8; 4096]; + let mut offset = 0i64; + let mut complete = true; + + 'walk: loop { + // SAFETY: `buffer` and `offset` are valid writable locals sized as + // passed, and `dir` is an open directory descriptor. + let read = unsafe { + libc::syscall( + SYS_GETDIRENTRIES64, + dir, + buffer.as_mut_ptr(), + buffer.len(), + &mut offset, + ) + }; + if read < 0 { + complete = false; + break; + } + if read == 0 { + break; + } + + let mut cursor = 0usize; + let limit = read as usize; + while cursor + DIRENT_NAME_OFFSET < limit { + let entry = &buffer[cursor..]; + let length = dirent_record_length(entry); + if length == 0 || cursor + length > limit { + complete = false; + break 'walk; + } + if let Some(fd) = dirent_descriptor(entry, length) { + // Skip the standard streams and the enumeration handle. + if fd > libc::STDERR_FILENO && fd != dir && is_unmarked(fd) { + if count == found.len() { + complete = false; + break 'walk; + } + found[count] = fd; + count += 1; + } + } + cursor += length; + } + } + + // SAFETY: `dir` is open and owned by this function. + unsafe { libc::close(dir) }; + complete.then_some(count) +} + +/// Close unmarked descriptors by scanning the whole descriptor table. +/// +/// Used when `/dev/fd` is unavailable. Correct but slow, since the table is +/// sized by `RLIMIT_NOFILE` rather than by the number of open descriptors. +/// +/// # Safety +/// +/// Must be called only between `fork` and `exec`. +unsafe fn close_scanned() { + // SAFETY: no arguments and no output parameters. + let limit = unsafe { libc::getdtablesize() }; + for fd in (libc::STDERR_FILENO + 1)..limit { + if is_unmarked(fd) { + // SAFETY: `fd` is open, since `F_GETFD` succeeded for it. + unsafe { libc::close(fd) }; + } + } +} + +/// Whether `fd` is open and lacks `FD_CLOEXEC`. +fn is_unmarked(fd: RawFd) -> bool { + // SAFETY: `F_GETFD` only reads descriptor flags and reports `EBADF` for + // descriptors that are not open. + let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) }; + flags >= 0 && flags & libc::FD_CLOEXEC == 0 +} + +/// Offset of `d_name` within `struct dirent` on macOS: `d_ino` and +/// `d_seekoff` (8 bytes each), then `d_reclen` and `d_namlen` (2 each), then +/// `d_type`. +const DIRENT_NAME_OFFSET: usize = 21; +/// Offset of the `d_reclen` field. +const DIRENT_RECORD_LENGTH_OFFSET: usize = 16; +/// Offset of the `d_namlen` field. +const DIRENT_NAME_LENGTH_OFFSET: usize = 18; + +/// Read `d_reclen` from a raw directory entry. +fn dirent_record_length(entry: &[u8]) -> usize { + let Some(field) = entry.get(DIRENT_RECORD_LENGTH_OFFSET..DIRENT_RECORD_LENGTH_OFFSET + 2) + else { + return 0; + }; + u16::from_ne_bytes([field[0], field[1]]) as usize +} + +/// Parse a `/dev/fd` entry name as a descriptor number. +fn dirent_descriptor(entry: &[u8], record_length: usize) -> Option { + let field = entry.get(DIRENT_NAME_LENGTH_OFFSET..DIRENT_NAME_LENGTH_OFFSET + 2)?; + let name_length = u16::from_ne_bytes([field[0], field[1]]) as usize; + if name_length == 0 || DIRENT_NAME_OFFSET + name_length > record_length { + return None; + } + let name = entry.get(DIRENT_NAME_OFFSET..DIRENT_NAME_OFFSET + name_length)?; + + let mut value: RawFd = 0; + for byte in name { + if !byte.is_ascii_digit() { + return None; + } + value = value + .checked_mul(10)? + .checked_add(RawFd::from(byte - b'0'))?; + } + Some(value) +} + +#[cfg(test)] +mod tests { + use super::*; + use std::os::fd::AsRawFd; + + /// Build a `dirent` image the way the kernel lays one out. + fn dirent(name: &str) -> Vec { + let mut entry = vec![0u8; DIRENT_NAME_OFFSET]; + entry.extend_from_slice(name.as_bytes()); + entry.push(0); + let record_length = entry.len(); + entry[DIRENT_RECORD_LENGTH_OFFSET..DIRENT_RECORD_LENGTH_OFFSET + 2] + .copy_from_slice(&(record_length as u16).to_ne_bytes()); + entry[DIRENT_NAME_LENGTH_OFFSET..DIRENT_NAME_LENGTH_OFFSET + 2] + .copy_from_slice(&(name.len() as u16).to_ne_bytes()); + entry + } + + #[test] + fn dirent_descriptor_parses_a_numeric_name() { + let entry = dirent("42"); + let length = dirent_record_length(&entry); + assert_eq!(length, entry.len()); + assert_eq!(dirent_descriptor(&entry, length), Some(42)); + } + + #[test] + fn dirent_descriptor_rejects_a_non_numeric_name() { + let entry = dirent("stdin"); + let length = dirent_record_length(&entry); + assert_eq!(dirent_descriptor(&entry, length), None); + } + + #[test] + fn dirent_descriptor_rejects_a_truncated_record() { + let entry = dirent("7"); + // A record that claims to end before its own name. + assert_eq!(dirent_descriptor(&entry, DIRENT_NAME_OFFSET), None); + } + + #[test] + fn is_unmarked_distinguishes_close_on_exec() { + let marked = std::fs::File::open("/dev/null").expect("open /dev/null"); + assert!(!is_unmarked(marked.as_raw_fd()), "Rust opens files CLOEXEC"); + + // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. + unsafe { libc::fcntl(marked.as_raw_fd(), libc::F_SETFD, 0) }; + assert!(is_unmarked(marked.as_raw_fd())); + } + + #[test] + fn is_unmarked_rejects_a_closed_descriptor() { + let fd = { + let file = std::fs::File::open("/dev/null").expect("open /dev/null"); + file.as_raw_fd() + }; + assert!(!is_unmarked(fd)); + } + + #[test] + fn collect_unmarked_finds_an_unmarked_descriptor_and_spares_the_rest() { + let exposed = std::fs::File::open("/dev/null").expect("open /dev/null"); + let marked = std::fs::File::open("/dev/null").expect("open /dev/null"); + // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. + unsafe { libc::fcntl(exposed.as_raw_fd(), libc::F_SETFD, 0) }; + + let mut found = [0 as RawFd; MAX_TRACKED_DESCRIPTORS]; + let count = collect_unmarked(&mut found).expect("/dev/fd walk succeeds"); + let found = &found[..count]; + + assert!( + found.contains(&exposed.as_raw_fd()), + "walk missed fd {}: {found:?}", + exposed.as_raw_fd() + ); + assert!(!found.contains(&marked.as_raw_fd())); + assert!(!found.iter().any(|fd| *fd <= libc::STDERR_FILENO)); + } + + #[test] + fn collect_unmarked_reports_an_undersized_buffer() { + let exposed = std::fs::File::open("/dev/null").expect("open /dev/null"); + // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. + unsafe { libc::fcntl(exposed.as_raw_fd(), libc::F_SETFD, 0) }; + + // Too small to hold the descriptor just exposed, so the caller must be + // told to fall back rather than silently skipping it. + assert_eq!(collect_unmarked(&mut []), None); + } +} diff --git a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs index caefef5335..faab5ed205 100644 --- a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs +++ b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs @@ -17,7 +17,7 @@ //! and sets a flag so later reads short-circuit to EOF. use std::io::{self, Read}; -use std::os::fd::{AsFd, AsRawFd, FromRawFd, OwnedFd, RawFd}; +use std::os::fd::{AsFd, AsRawFd, OwnedFd, RawFd}; use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::Arc; @@ -95,19 +95,7 @@ impl InterruptibleReader { // Self-pipe for wakeups: the write end is non-blocking so `cancel` // never stalls; the read end stays blocking but is only ever polled. - let mut fds = [0 as RawFd; 2]; - // SAFETY: `fds` is a valid 2-element array for `pipe` to fill. - if unsafe { libc::pipe(fds.as_mut_ptr()) } < 0 { - return Err(io::Error::last_os_error()); - } - // SAFETY: `pipe` succeeded, so both fds are freshly owned by us. - let wake_r = unsafe { OwnedFd::from_raw_fd(fds[0]) }; - let wake_w = unsafe { OwnedFd::from_raw_fd(fds[1]) }; - // `pipe(2)` doesn't set close-on-exec, so mark both ends `FD_CLOEXEC` — - // otherwise they leak into any process this thread later forks+execs - // (e.g. another sandbox child). - set_cloexec(wake_r.as_raw_fd())?; - set_cloexec(wake_w.as_raw_fd())?; + let (wake_r, wake_w) = new_wake_pipe()?; set_nonblocking(wake_w.as_raw_fd())?; Ok(Self { @@ -125,7 +113,6 @@ impl InterruptibleReader { pub fn canceller(&self) -> ReadCanceller { ReadCanceller(Arc::clone(&self.state)) } - pub fn try_clone_owned_fd(&self) -> io::Result { self.fd.as_fd().try_clone_to_owned() } @@ -229,6 +216,36 @@ fn set_nonblocking(fd: RawFd) -> io::Result<()> { Ok(()) } +/// Create the wakeup self-pipe with both ends marked close-on-exec. +/// +/// Linux gets this atomically from `pipe2`. macOS has no such variant, so the +/// ends are inheritable between creation and the `fcntl` that marks them; a +/// spawn racing on another thread can capture them. `LivePty` closes anything +/// that slips through from its `pre_exec` hook. +fn new_wake_pipe() -> io::Result<(OwnedFd, OwnedFd)> { + #[cfg(target_os = "linux")] + { + nix::unistd::pipe2(nix::fcntl::OFlag::O_CLOEXEC).map_err(io::Error::from) + } + #[cfg(not(target_os = "linux"))] + { + use std::os::fd::FromRawFd; + + let mut fds = [0 as RawFd; 2]; + // SAFETY: `fds` is a valid 2-element array for `pipe` to fill. + if unsafe { libc::pipe(fds.as_mut_ptr()) } < 0 { + return Err(io::Error::last_os_error()); + } + // SAFETY: `pipe` succeeded, so both fds are freshly owned by us. + let read = unsafe { OwnedFd::from_raw_fd(fds[0]) }; + // SAFETY: as above, for the write end. + let write = unsafe { OwnedFd::from_raw_fd(fds[1]) }; + set_cloexec(read.as_raw_fd())?; + set_cloexec(write.as_raw_fd())?; + Ok((read, write)) + } +} + /// Add `FD_CLOEXEC` to `fd`'s descriptor flags so it doesn't leak across `exec`. fn set_cloexec(fd: RawFd) -> io::Result<()> { // SAFETY: `fd` is a valid open fd; `fcntl` with these commands only reads @@ -247,6 +264,7 @@ fn set_cloexec(fd: RawFd) -> io::Result<()> { mod tests { use super::*; use std::io::Write; + use std::os::fd::FromRawFd; use std::time::{Duration, Instant}; /// Build an `InterruptibleReader` over a fresh pipe, returning it plus the diff --git a/src/mxc-sdk/src/core/mxc_common/mod.rs b/src/mxc-sdk/src/core/mxc_common/mod.rs index 0fd5d59089..e9365b5218 100644 --- a/src/mxc-sdk/src/core/mxc_common/mod.rs +++ b/src/mxc-sdk/src/core/mxc_common/mod.rs @@ -72,6 +72,11 @@ pub mod system_dir; #[cfg(unix)] pub mod interruptible_reader; +// macOS-only: no atomic close-on-exec pipe exists, so inherited descriptors +// are scrubbed between `fork` and `exec` instead. +#[cfg(target_os = "macos")] +pub mod inherited_descriptors; + // Linux-only: used by the backends that realise filesystem policy as bind // mounts, which must resolve a destination before mounting over it. #[cfg(target_os = "linux")] diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index 5765afbca1..b361c39661 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -377,6 +377,12 @@ impl LivePty { unsafe { use std::os::unix::process::CommandExt; command.pre_exec(move || { + // Drop anything captured through the macOS close-on-exec + // window before handing the process to the workload. + #[cfg(target_os = "macos")] + crate::core_modules::mxc_common::inherited_descriptors:: + close_unmarked_inherited_descriptors(); + nix::unistd::setsid().map_err(std::io::Error::from)?; let _ = libc::ioctl(0, libc::TIOCSCTTY as _, 0); @@ -2467,6 +2473,257 @@ mod tests { let _ = wait_for_exit(&mut child); } + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn size_reports_the_last_applied_dimensions() { + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg("true"); + let initial = PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }; + let terminal = LivePty::attach(&mut command, initial, &[]).expect("attach PTY"); + assert_eq!(terminal.size(), initial); + + let resized = PtySize { + rows: 40, + cols: 120, + pixel_width: 0, + pixel_height: 0, + }; + terminal.resize(resized).expect("resize PTY"); + assert_eq!(terminal.size(), resized); + + // A rejected resize must not disturb the recorded size. + assert_eq!( + terminal + .resize(PtySize { + rows: 0, + cols: 0, + pixel_width: 0, + pixel_height: 0, + }) + .expect_err("zero dimensions are rejected") + .kind(), + std::io::ErrorKind::InvalidInput + ); + assert_eq!(terminal.size(), resized); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn is_canonical_mode_follows_the_terminal_discipline() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command + .arg("-c") + .arg("printf 'ready\\n'; read go; stty -icanon; printf 'canonical-off\\n'; read done"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let mut writer = terminal.take_writer().expect("take writer"); + + drain.wait_for("ready"); + assert!(terminal.is_canonical_mode().expect("read discipline")); + + writer.write_all(b"go\r").expect("write input"); + drain.wait_for("canonical-off"); + assert!(!terminal.is_canonical_mode().expect("read discipline")); + + drop(writer); + let _ = child.kill(); + let _ = wait_for_exit(&mut child); + drop(terminal); + let _ = drain.finish(); + } + + /// `resize` must notify the foreground process group, not just set the + /// kernel's window size. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn resize_signals_the_foreground_process_group() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg( + "trap 'stty size; exit 0' WINCH; printf 'ready\\n'; \ + i=0; while [ $i -lt 200 ]; do sleep 0.1; i=$((i+1)); done", + ); + let terminal = LivePty::attach( + &mut command, + PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }, + &[], + ) + .expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + drain.wait_for("ready"); + terminal + .resize(PtySize { + rows: 40, + cols: 120, + pixel_width: 0, + pixel_height: 0, + }) + .expect("resize PTY"); + + let seen = drain.wait_for("40 120"); + assert!(seen.contains("40 120"), "got: {seen:?}"); + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let _ = drain.finish(); + } + + /// A forwarding process switches the terminal to raw mode, so the live + /// discipline no longer reports an EOF character. `attach_forwarded` + /// captures it up front and still delivers it. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn forwarded_eof_is_delivered_through_a_raw_mode_terminal() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg( + "stty raw -echo; printf 'ready\\r\\n'; \ + head -c 1 | od -An -tx1 | tr -d ' \\n'; printf '\\r\\n'", + ); + let terminal = LivePty::attach_forwarded(&mut command, PtySize::default(), &[]) + .expect("attach forwarded PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + drain.wait_for("ready"); + drop(terminal.take_writer().expect("take writer")); + + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); + assert!(output.contains("04"), "expected ^D byte; got: {output:?}"); + } + + /// The contrast to the test above: the default discipline reads the live + /// termios, which reports no EOF character once the terminal is raw. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn current_canonical_eof_is_skipped_for_a_raw_mode_terminal() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let mut command = Command::new("/bin/sh"); + command + .arg("-c") + .arg("stty raw -echo; printf 'ready\\r\\n'; head -c 1 >/dev/null"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + drain.wait_for("ready"); + drop(terminal.take_writer().expect("take writer")); + + std::thread::sleep(Duration::from_secs(1)); + assert!( + child.try_wait().expect("poll child").is_none(), + "no EOF byte should reach a raw-mode terminal" + ); + + let _ = child.kill(); + let _ = wait_for_exit(&mut child); + drop(terminal); + let _ = drain.finish(); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn raw_secondary_guard_sets_raw_mode_and_restores_on_drop() { + use nix::sys::termios::{self, LocalFlags}; + + let (primary, secondary) = open_pty(None).expect("open PTY pair"); + let before = termios::tcgetattr(&secondary).expect("read termios"); + assert!(before.local_flags.contains(LocalFlags::ICANON)); + + { + let _raw = RawSecondaryGuard::install(secondary.as_raw_fd()).expect("install raw mode"); + let during = termios::tcgetattr(&secondary).expect("read termios"); + assert!(!during.local_flags.contains(LocalFlags::ICANON)); + assert!(!during.local_flags.contains(LocalFlags::ECHO)); + } + + let after = termios::tcgetattr(&secondary).expect("read termios"); + // PENDIN is kernel-managed status, not settable state. + let settable = |flags: LocalFlags| flags - LocalFlags::PENDIN; + assert_eq!(settable(after.local_flags), settable(before.local_flags)); + assert_eq!(after.input_flags, before.input_flags); + assert_eq!(after.output_flags, before.output_flags); + drop(primary); + } + + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[test] + fn raw_secondary_guard_declines_a_non_tty() { + let (read, write) = create_pipe().expect("create pipe"); + assert!(RawSecondaryGuard::install(read.as_raw_fd()).is_none()); + drop((read, write)); + } + + /// A descriptor caught in the macOS close-on-exec window must not reach + /// the workload. + #[cfg(target_os = "macos")] + #[test] + fn attach_closes_descriptors_captured_through_the_cloexec_window() { + use std::os::fd::{FromRawFd, OwnedFd, RawFd}; + + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + // A pipe left unmarked, exactly as the window leaves one. + let mut fds = [0 as RawFd; 2]; + // SAFETY: `fds` is a valid 2-element array for `pipe` to fill. + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0); + // SAFETY: `pipe` succeeded, so both fds are freshly owned here. + let read = unsafe { OwnedFd::from_raw_fd(fds[0]) }; + // SAFETY: as above, for the write end. + let write = unsafe { OwnedFd::from_raw_fd(fds[1]) }; + let target = write.as_raw_fd(); + + let mut command = Command::new("/bin/sh"); + command.arg("-c").arg(format!( + "if echo leaked >&{target} 2>/dev/null; then echo LEAKED; else echo CLOSED; fi" + )); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let mut child = command.spawn().expect("spawn child"); + drop(command); + let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + + assert!(wait_for_exit(&mut child).success()); + drop(terminal); + let output = drain.finish(); + assert!(output.contains("CLOSED"), "fd {target} leaked: {output:?}"); + drop((read, write)); + } + + /// The scrub must spare the pipe the Rust runtime uses to report a failed + /// `exec`; closing it would make every failure look like a success. + #[cfg(target_os = "macos")] + #[test] + fn attach_still_reports_a_failed_exec() { + let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + + let mut command = Command::new("/nonexistent/mxc-pty-missing-binary"); + let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); + let error = command + .spawn() + .expect_err("missing binary must fail to spawn"); + assert_eq!(error.kind(), std::io::ErrorKind::NotFound); + drop(terminal); + } + #[cfg(any(target_os = "linux", target_os = "macos"))] #[test] fn echo_runs_under_pty() { From 6a2c969e5fe43dace38b4ba3ef617056fd48e420 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Tue, 13 Oct 2026 12:21:02 -0700 Subject: [PATCH 6/9] add file-descriptor removal to non-pty exec too --- .../backends/seatbelt/common/seatbelt_runner.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs index b6f1e8dcaa..ddb4219b81 100644 --- a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs +++ b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs @@ -29,6 +29,7 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; +use crate::mxc_common::inherited_descriptors::close_unmarked_inherited_descriptors; use crate::mxc_common::interruptible_reader::{wrap_pipe, InterruptibleReader, ReadCanceller}; use crate::mxc_common::logger::Logger; use crate::mxc_common::models::{ExecutionRequest, LaunchMethod, ProxyAddress, ScriptResponse}; @@ -964,7 +965,8 @@ impl Drop for SeatbeltSandboxProcess { /// `pre_exec` runs between `fork()` and `exec()`. Session/PTY setup uses /// async-signal-safe syscalls before the pre-allocated `sandbox_init` call. /// `sandbox_init` is not formally async-signal-safe but is used in this -/// pattern by Chromium and other production macOS sandboxes. +/// pattern by Chromium and other production macOS sandboxes. A final hook +/// then drops descriptors that are not marked close-on-exec. fn build_sandbox_command( profile: &str, script_code: &str, @@ -1055,6 +1057,19 @@ fn build_sandbox_command( }); } + // Registered last so it runs immediately before `exec`: `sandbox_init` + // keeps whatever descriptors it needs, and nothing can expose a new one + // afterwards. Also covers runs without a PTY, which `LivePty` never sees. + // + // SAFETY: the closure runs after fork(), before exec(), and the scrub uses + // only async-signal-safe calls. + unsafe { + command.pre_exec(|| { + close_unmarked_inherited_descriptors(); + Ok(()) + }); + } + Ok((command, pty)) } From 3a53c53c49cfb54139358b985e14cd4b527034ea Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Wed, 14 Oct 2026 11:55:44 -0700 Subject: [PATCH 7/9] Removed fd deletion. Fixed PTY drai n --- .../seatbelt/common/seatbelt_runner.rs | 17 +- .../core/mxc_common/inherited_descriptors.rs | 286 ------------------ .../core/mxc_common/interruptible_reader.rs | 5 +- src/mxc-sdk/src/core/mxc_common/mod.rs | 5 - src/mxc-sdk/src/core/mxc_pty/mod.rs | 159 ++++++---- 5 files changed, 104 insertions(+), 368 deletions(-) delete mode 100644 src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs diff --git a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs index ddb4219b81..b6f1e8dcaa 100644 --- a/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs +++ b/src/mxc-sdk/src/backends/seatbelt/common/seatbelt_runner.rs @@ -29,7 +29,6 @@ use std::path::{Path, PathBuf}; use std::process::{Command, Stdio}; use std::time::{Duration, Instant}; -use crate::mxc_common::inherited_descriptors::close_unmarked_inherited_descriptors; use crate::mxc_common::interruptible_reader::{wrap_pipe, InterruptibleReader, ReadCanceller}; use crate::mxc_common::logger::Logger; use crate::mxc_common::models::{ExecutionRequest, LaunchMethod, ProxyAddress, ScriptResponse}; @@ -965,8 +964,7 @@ impl Drop for SeatbeltSandboxProcess { /// `pre_exec` runs between `fork()` and `exec()`. Session/PTY setup uses /// async-signal-safe syscalls before the pre-allocated `sandbox_init` call. /// `sandbox_init` is not formally async-signal-safe but is used in this -/// pattern by Chromium and other production macOS sandboxes. A final hook -/// then drops descriptors that are not marked close-on-exec. +/// pattern by Chromium and other production macOS sandboxes. fn build_sandbox_command( profile: &str, script_code: &str, @@ -1057,19 +1055,6 @@ fn build_sandbox_command( }); } - // Registered last so it runs immediately before `exec`: `sandbox_init` - // keeps whatever descriptors it needs, and nothing can expose a new one - // afterwards. Also covers runs without a PTY, which `LivePty` never sees. - // - // SAFETY: the closure runs after fork(), before exec(), and the scrub uses - // only async-signal-safe calls. - unsafe { - command.pre_exec(|| { - close_unmarked_inherited_descriptors(); - Ok(()) - }); - } - Ok((command, pty)) } diff --git a/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs b/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs deleted file mode 100644 index 330048e109..0000000000 --- a/src/mxc-sdk/src/core/mxc_common/inherited_descriptors.rs +++ /dev/null @@ -1,286 +0,0 @@ -//! Close-on-exec hygiene for descriptors inherited across `fork`. -//! -//! macOS has no atomic close-on-exec pipe: there is no `pipe2`, and -//! `socketpair` has no `SOCK_CLOEXEC`. Both ends are therefore inheritable -//! between `pipe(2)` and the `fcntl` that marks them, so a spawn running on -//! another thread during that window captures them. The child then holds a -//! live handle to another container's stdin or stdout. -//! -//! Only path-based `open(2)` is atomic here, which pipes cannot use. The -//! window is closed from the other side instead: between `fork` and `exec`, -//! drop every descriptor that is not marked close-on-exec. - -use std::os::fd::RawFd; - -/// Entries collected per enumeration pass before falling back to a full scan. -const MAX_TRACKED_DESCRIPTORS: usize = 1024; - -/// Close inherited descriptors that are not marked close-on-exec. -/// -/// Standard streams are left alone, as is every descriptor already marked -/// `FD_CLOEXEC` — including the pipe the Rust runtime uses to report a failed -/// `exec` back to the parent. -/// -/// # Safety -/// -/// Must be called only between `fork` and `exec`. It closes descriptors the -/// caller does not own, and is async-signal-safe only in that context. -pub unsafe fn close_unmarked_inherited_descriptors() { - // SAFETY: the caller guarantees the post-fork, pre-exec context. - if unsafe { close_enumerated() } { - return; - } - // SAFETY: as above. - unsafe { close_scanned() }; -} - -/// Close unmarked descriptors found by enumerating `/dev/fd`. -/// -/// Returns `false` if the directory could not be enumerated, or if it held -/// more entries than [`MAX_TRACKED_DESCRIPTORS`]. -/// -/// # Safety -/// -/// Must be called only between `fork` and `exec`. -unsafe fn close_enumerated() -> bool { - let mut found = [0 as RawFd; MAX_TRACKED_DESCRIPTORS]; - let Some(count) = collect_unmarked(&mut found) else { - return false; - }; - for fd in &found[..count] { - // SAFETY: `fd` was reported open and is not a standard stream. - unsafe { libc::close(*fd) }; - } - true -} - -/// Record the open descriptors that lack `FD_CLOEXEC` into `found`, returning -/// how many were stored. -/// -/// Standard streams are skipped, as is the handle used for the walk. Returns -/// `None` if `/dev/fd` could not be read or `found` was too small, which tells -/// the caller to fall back to a full scan. -/// -/// The directory is read to completion before anything is closed: removing -/// entries mid-walk would shift the directory offset and skip descriptors. -fn collect_unmarked(found: &mut [RawFd]) -> Option { - // `getdirentries64`. The libc wrapper is unavailable once 64-bit inodes - // are in effect, so the syscall is issued directly. - const SYS_GETDIRENTRIES64: libc::c_int = 344; - - // SAFETY: a NUL-terminated literal path with no output parameters. - let dir = unsafe { libc::open(c"/dev/fd".as_ptr(), libc::O_RDONLY | libc::O_CLOEXEC) }; - if dir < 0 { - return None; - } - - let mut count = 0usize; - let mut buffer = [0u8; 4096]; - let mut offset = 0i64; - let mut complete = true; - - 'walk: loop { - // SAFETY: `buffer` and `offset` are valid writable locals sized as - // passed, and `dir` is an open directory descriptor. - let read = unsafe { - libc::syscall( - SYS_GETDIRENTRIES64, - dir, - buffer.as_mut_ptr(), - buffer.len(), - &mut offset, - ) - }; - if read < 0 { - complete = false; - break; - } - if read == 0 { - break; - } - - let mut cursor = 0usize; - let limit = read as usize; - while cursor + DIRENT_NAME_OFFSET < limit { - let entry = &buffer[cursor..]; - let length = dirent_record_length(entry); - if length == 0 || cursor + length > limit { - complete = false; - break 'walk; - } - if let Some(fd) = dirent_descriptor(entry, length) { - // Skip the standard streams and the enumeration handle. - if fd > libc::STDERR_FILENO && fd != dir && is_unmarked(fd) { - if count == found.len() { - complete = false; - break 'walk; - } - found[count] = fd; - count += 1; - } - } - cursor += length; - } - } - - // SAFETY: `dir` is open and owned by this function. - unsafe { libc::close(dir) }; - complete.then_some(count) -} - -/// Close unmarked descriptors by scanning the whole descriptor table. -/// -/// Used when `/dev/fd` is unavailable. Correct but slow, since the table is -/// sized by `RLIMIT_NOFILE` rather than by the number of open descriptors. -/// -/// # Safety -/// -/// Must be called only between `fork` and `exec`. -unsafe fn close_scanned() { - // SAFETY: no arguments and no output parameters. - let limit = unsafe { libc::getdtablesize() }; - for fd in (libc::STDERR_FILENO + 1)..limit { - if is_unmarked(fd) { - // SAFETY: `fd` is open, since `F_GETFD` succeeded for it. - unsafe { libc::close(fd) }; - } - } -} - -/// Whether `fd` is open and lacks `FD_CLOEXEC`. -fn is_unmarked(fd: RawFd) -> bool { - // SAFETY: `F_GETFD` only reads descriptor flags and reports `EBADF` for - // descriptors that are not open. - let flags = unsafe { libc::fcntl(fd, libc::F_GETFD) }; - flags >= 0 && flags & libc::FD_CLOEXEC == 0 -} - -/// Offset of `d_name` within `struct dirent` on macOS: `d_ino` and -/// `d_seekoff` (8 bytes each), then `d_reclen` and `d_namlen` (2 each), then -/// `d_type`. -const DIRENT_NAME_OFFSET: usize = 21; -/// Offset of the `d_reclen` field. -const DIRENT_RECORD_LENGTH_OFFSET: usize = 16; -/// Offset of the `d_namlen` field. -const DIRENT_NAME_LENGTH_OFFSET: usize = 18; - -/// Read `d_reclen` from a raw directory entry. -fn dirent_record_length(entry: &[u8]) -> usize { - let Some(field) = entry.get(DIRENT_RECORD_LENGTH_OFFSET..DIRENT_RECORD_LENGTH_OFFSET + 2) - else { - return 0; - }; - u16::from_ne_bytes([field[0], field[1]]) as usize -} - -/// Parse a `/dev/fd` entry name as a descriptor number. -fn dirent_descriptor(entry: &[u8], record_length: usize) -> Option { - let field = entry.get(DIRENT_NAME_LENGTH_OFFSET..DIRENT_NAME_LENGTH_OFFSET + 2)?; - let name_length = u16::from_ne_bytes([field[0], field[1]]) as usize; - if name_length == 0 || DIRENT_NAME_OFFSET + name_length > record_length { - return None; - } - let name = entry.get(DIRENT_NAME_OFFSET..DIRENT_NAME_OFFSET + name_length)?; - - let mut value: RawFd = 0; - for byte in name { - if !byte.is_ascii_digit() { - return None; - } - value = value - .checked_mul(10)? - .checked_add(RawFd::from(byte - b'0'))?; - } - Some(value) -} - -#[cfg(test)] -mod tests { - use super::*; - use std::os::fd::AsRawFd; - - /// Build a `dirent` image the way the kernel lays one out. - fn dirent(name: &str) -> Vec { - let mut entry = vec![0u8; DIRENT_NAME_OFFSET]; - entry.extend_from_slice(name.as_bytes()); - entry.push(0); - let record_length = entry.len(); - entry[DIRENT_RECORD_LENGTH_OFFSET..DIRENT_RECORD_LENGTH_OFFSET + 2] - .copy_from_slice(&(record_length as u16).to_ne_bytes()); - entry[DIRENT_NAME_LENGTH_OFFSET..DIRENT_NAME_LENGTH_OFFSET + 2] - .copy_from_slice(&(name.len() as u16).to_ne_bytes()); - entry - } - - #[test] - fn dirent_descriptor_parses_a_numeric_name() { - let entry = dirent("42"); - let length = dirent_record_length(&entry); - assert_eq!(length, entry.len()); - assert_eq!(dirent_descriptor(&entry, length), Some(42)); - } - - #[test] - fn dirent_descriptor_rejects_a_non_numeric_name() { - let entry = dirent("stdin"); - let length = dirent_record_length(&entry); - assert_eq!(dirent_descriptor(&entry, length), None); - } - - #[test] - fn dirent_descriptor_rejects_a_truncated_record() { - let entry = dirent("7"); - // A record that claims to end before its own name. - assert_eq!(dirent_descriptor(&entry, DIRENT_NAME_OFFSET), None); - } - - #[test] - fn is_unmarked_distinguishes_close_on_exec() { - let marked = std::fs::File::open("/dev/null").expect("open /dev/null"); - assert!(!is_unmarked(marked.as_raw_fd()), "Rust opens files CLOEXEC"); - - // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. - unsafe { libc::fcntl(marked.as_raw_fd(), libc::F_SETFD, 0) }; - assert!(is_unmarked(marked.as_raw_fd())); - } - - #[test] - fn is_unmarked_rejects_a_closed_descriptor() { - let fd = { - let file = std::fs::File::open("/dev/null").expect("open /dev/null"); - file.as_raw_fd() - }; - assert!(!is_unmarked(fd)); - } - - #[test] - fn collect_unmarked_finds_an_unmarked_descriptor_and_spares_the_rest() { - let exposed = std::fs::File::open("/dev/null").expect("open /dev/null"); - let marked = std::fs::File::open("/dev/null").expect("open /dev/null"); - // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. - unsafe { libc::fcntl(exposed.as_raw_fd(), libc::F_SETFD, 0) }; - - let mut found = [0 as RawFd; MAX_TRACKED_DESCRIPTORS]; - let count = collect_unmarked(&mut found).expect("/dev/fd walk succeeds"); - let found = &found[..count]; - - assert!( - found.contains(&exposed.as_raw_fd()), - "walk missed fd {}: {found:?}", - exposed.as_raw_fd() - ); - assert!(!found.contains(&marked.as_raw_fd())); - assert!(!found.iter().any(|fd| *fd <= libc::STDERR_FILENO)); - } - - #[test] - fn collect_unmarked_reports_an_undersized_buffer() { - let exposed = std::fs::File::open("/dev/null").expect("open /dev/null"); - // SAFETY: `F_SETFD` only writes descriptor flags on an open fd. - unsafe { libc::fcntl(exposed.as_raw_fd(), libc::F_SETFD, 0) }; - - // Too small to hold the descriptor just exposed, so the caller must be - // told to fall back rather than silently skipping it. - assert_eq!(collect_unmarked(&mut []), None); - } -} diff --git a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs index faab5ed205..5887ae564b 100644 --- a/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs +++ b/src/mxc-sdk/src/core/mxc_common/interruptible_reader.rs @@ -220,8 +220,9 @@ fn set_nonblocking(fd: RawFd) -> io::Result<()> { /// /// Linux gets this atomically from `pipe2`. macOS has no such variant, so the /// ends are inheritable between creation and the `fcntl` that marks them; a -/// spawn racing on another thread can capture them. `LivePty` closes anything -/// that slips through from its `pre_exec` hook. +/// spawn racing on another thread can capture them. The platform offers no +/// atomic alternative, and Rust's own `Stdio::piped()` has the same window on +/// macOS. fn new_wake_pipe() -> io::Result<(OwnedFd, OwnedFd)> { #[cfg(target_os = "linux")] { diff --git a/src/mxc-sdk/src/core/mxc_common/mod.rs b/src/mxc-sdk/src/core/mxc_common/mod.rs index d2bae09498..96cf41da23 100644 --- a/src/mxc-sdk/src/core/mxc_common/mod.rs +++ b/src/mxc-sdk/src/core/mxc_common/mod.rs @@ -72,11 +72,6 @@ pub mod system_dir; #[cfg(unix)] pub mod interruptible_reader; -// macOS-only: no atomic close-on-exec pipe exists, so inherited descriptors -// are scrubbed between `fork` and `exec` instead. -#[cfg(target_os = "macos")] -pub mod inherited_descriptors; - // Linux-only: used by the backends that realise filesystem policy as bind // mounts, which must resolve a destination before mounting over it. #[cfg(target_os = "linux")] diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index b361c39661..52c18931a8 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -377,12 +377,6 @@ impl LivePty { unsafe { use std::os::unix::process::CommandExt; command.pre_exec(move || { - // Drop anything captured through the macOS close-on-exec - // window before handing the process to the workload. - #[cfg(target_os = "macos")] - crate::core_modules::mxc_common::inherited_descriptors:: - close_unmarked_inherited_descriptors(); - nix::unistd::setsid().map_err(std::io::Error::from)?; let _ = libc::ioctl(0, libc::TIOCSCTTY as _, 0); @@ -1120,8 +1114,12 @@ fn create_pipe() -> std::io::Result<(std::os::fd::OwnedFd, std::os::fd::OwnedFd) } #[cfg(target_os = "macos")] { - // Darwin has no `pipe2`, so close-on-exec is set immediately after - // creation. This matches what `std::process` does for its own stdio. + // macOS has no `pipe2`, and `O_CLOEXEC` applies only to `open`, so + // close-on-exec has to be set in a second call. A concurrent + // `fork`+`exec` in the same process can inherit the descriptors in + // between. The platform offers no atomic alternative, and Rust's own + // `Stdio::piped()` has the identical window on macOS, so this matches + // the standard library rather than adding a new class of exposure. let (read, write) = nix::unistd::pipe().map_err(std::io::Error::from)?; set_cloexec(read.as_raw_fd())?; set_cloexec(write.as_raw_fd())?; @@ -1513,13 +1511,27 @@ mod tests { collected: Arc>>, worker: Option>, finished: std::sync::mpsc::Receiver<()>, + canceller: Option, } #[cfg(any(target_os = "linux", target_os = "macos"))] impl PtyDrain { const TIMEOUT: Duration = Duration::from_secs(20); - fn start(mut source: Box) -> Self { + fn start(source: Box) -> Self { + Self::start_inner(source, None) + } + + /// Preferred over [`Self::start`]: the canceller lets [`Drop`] unblock + /// the worker so a failing test cannot leave it holding the primary. + fn start_cancellable(source: (Box, PtyReadCanceller)) -> Self { + Self::start_inner(source.0, Some(source.1)) + } + + fn start_inner( + mut source: Box, + canceller: Option, + ) -> Self { let collected = Arc::new(Mutex::new(Vec::new())); let sink = Arc::clone(&collected); let (done, finished) = std::sync::mpsc::sync_channel(1); @@ -1539,6 +1551,7 @@ mod tests { collected, worker: Some(worker), finished, + canceller, } } @@ -1576,6 +1589,21 @@ mod tests { } } + /// Joins the worker rather than detaching it. A detached worker still owns + /// the reader, so dropping `LivePty` would not close the final primary and + /// a failing test would hang instead of failing. + #[cfg(any(target_os = "linux", target_os = "macos"))] + impl Drop for PtyDrain { + fn drop(&mut self) { + if let Some(worker) = self.worker.take() { + if let Some(canceller) = &self.canceller { + canceller.close(); + } + let _ = worker.join(); + } + } + } + /// Waits for `child` with a deadline instead of hanging the whole suite. /// /// Deliberately does not fall back to `kill`: a child stuck in the Darwin @@ -1639,7 +1667,11 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut writer = terminal.take_writer().expect("take writer"); writer.write_all(b"hello\r").expect("write input"); drop(writer); @@ -1662,7 +1694,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut writer = terminal.take_writer().expect("take writer"); writer @@ -1687,7 +1723,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); terminal.close_writer(); // Closing again is a no-op, and the writer can no longer be taken. @@ -1718,7 +1758,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut writer = terminal.take_writer().expect("take writer"); terminal.close_writer(); @@ -1776,7 +1820,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); assert!( terminal @@ -1825,7 +1873,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let started = std::time::Instant::now(); drop(terminal.take_writer().expect("take writer")); @@ -1849,7 +1901,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut fill = terminal .access @@ -2071,7 +2127,11 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut writer = terminal.take_writer().expect("take writer"); let mut input = b"x\n".repeat(128 * 1024); input.extend_from_slice(b"partial-line"); @@ -2523,7 +2583,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); let mut writer = terminal.take_writer().expect("take writer"); drain.wait_for("ready"); @@ -2564,7 +2628,11 @@ mod tests { .expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); drain.wait_for("ready"); terminal @@ -2599,7 +2667,11 @@ mod tests { .expect("attach forwarded PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); drain.wait_for("ready"); drop(terminal.take_writer().expect("take writer")); @@ -2623,7 +2695,11 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); drain.wait_for("ready"); drop(terminal.take_writer().expect("take writer")); @@ -2673,44 +2749,9 @@ mod tests { drop((read, write)); } - /// A descriptor caught in the macOS close-on-exec window must not reach - /// the workload. - #[cfg(target_os = "macos")] - #[test] - fn attach_closes_descriptors_captured_through_the_cloexec_window() { - use std::os::fd::{FromRawFd, OwnedFd, RawFd}; - - let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - - // A pipe left unmarked, exactly as the window leaves one. - let mut fds = [0 as RawFd; 2]; - // SAFETY: `fds` is a valid 2-element array for `pipe` to fill. - assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0); - // SAFETY: `pipe` succeeded, so both fds are freshly owned here. - let read = unsafe { OwnedFd::from_raw_fd(fds[0]) }; - // SAFETY: as above, for the write end. - let write = unsafe { OwnedFd::from_raw_fd(fds[1]) }; - let target = write.as_raw_fd(); - - let mut command = Command::new("/bin/sh"); - command.arg("-c").arg(format!( - "if echo leaked >&{target} 2>/dev/null; then echo LEAKED; else echo CLOSED; fi" - )); - let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); - let mut child = command.spawn().expect("spawn child"); - drop(command); - let drain = PtyDrain::start(terminal.try_clone_reader().expect("clone reader")); - - assert!(wait_for_exit(&mut child).success()); - drop(terminal); - let output = drain.finish(); - assert!(output.contains("CLOSED"), "fd {target} leaked: {output:?}"); - drop((read, write)); - } - - /// The scrub must spare the pipe the Rust runtime uses to report a failed - /// `exec`; closing it would make every failure look like a success. - #[cfg(target_os = "macos")] + /// A failed `exec` must still surface as a spawn error after `attach` + /// has rewired stdio onto the terminal. + #[cfg(any(target_os = "linux", target_os = "macos"))] #[test] fn attach_still_reports_a_failed_exec() { let _guard = RUN_WITH_PTY_LOCK.lock().unwrap_or_else(|e| e.into_inner()); From 3e189ac8c1612faf426aa15218818eab203d03f5 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Wed, 14 Oct 2026 14:20:07 -0700 Subject: [PATCH 8/9] fix drain bugs --- src/mxc-sdk/src/core/mxc_pty/mod.rs | 114 ++++++++++++++++++++-------- 1 file changed, 81 insertions(+), 33 deletions(-) diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index 52c18931a8..89d050f81e 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -1115,11 +1115,14 @@ fn create_pipe() -> std::io::Result<(std::os::fd::OwnedFd, std::os::fd::OwnedFd) #[cfg(target_os = "macos")] { // macOS has no `pipe2`, and `O_CLOEXEC` applies only to `open`, so - // close-on-exec has to be set in a second call. A concurrent - // `fork`+`exec` in the same process can inherit the descriptors in - // between. The platform offers no atomic alternative, and Rust's own - // `Stdio::piped()` has the identical window on macOS, so this matches - // the standard library rather than adding a new class of exposure. + // close-on-exec must be set in a second call. A `fork`+`exec` racing + // on another thread can inherit the ends in between and hold them + // open, delaying EOF. + // + // Accepted rather than fixed: Rust's own `Stdio::piped()` has the same + // window on macOS, so the descriptors carrying a workload's streams + // already have it, and spawns owned by the embedding application are + // outside this crate's control either way. let (read, write) = nix::unistd::pipe().map_err(std::io::Error::from)?; set_cloexec(read.as_raw_fd())?; set_cloexec(write.as_raw_fd())?; @@ -1517,13 +1520,19 @@ mod tests { #[cfg(any(target_os = "linux", target_os = "macos"))] impl PtyDrain { const TIMEOUT: Duration = Duration::from_secs(20); + /// How long [`Drop`] waits for an uncancellable worker before leaving + /// it running. Only reached while unwinding. + const DROP_GRACE: Duration = Duration::from_secs(2); + /// Only for native-pipe readers, which have no canceller. Drains of a + /// PTY reader must use [`Self::start_cancellable`] so [`Drop`] can + /// unblock the worker. fn start(source: Box) -> Self { Self::start_inner(source, None) } - /// Preferred over [`Self::start`]: the canceller lets [`Drop`] unblock - /// the worker so a failing test cannot leave it holding the primary. + /// The canceller lets [`Drop`] unblock the worker, so a failing test + /// cannot leave it parked holding the primary. fn start_cancellable(source: (Box, PtyReadCanceller)) -> Self { Self::start_inner(source.0, Some(source.1)) } @@ -1577,6 +1586,13 @@ mod tests { } } + /// Cancels the read so a parked worker reaches end of input. + fn cancel(&self) { + if let Some(canceller) = &self.canceller { + canceller.close(); + } + } + /// Waits for end of input and returns the full output. fn finish(mut self) -> String { self.finished @@ -1589,16 +1605,27 @@ mod tests { } } - /// Joins the worker rather than detaching it. A detached worker still owns - /// the reader, so dropping `LivePty` would not close the final primary and - /// a failing test would hang instead of failing. + /// Releases the worker rather than detaching it. A detached PTY worker + /// still owns the reader, so dropping `LivePty` would not close the final + /// primary and a failing test would hang instead of failing. #[cfg(any(target_os = "linux", target_os = "macos"))] impl Drop for PtyDrain { fn drop(&mut self) { - if let Some(worker) = self.worker.take() { - if let Some(canceller) = &self.canceller { - canceller.close(); - } + let Some(worker) = self.worker.take() else { + return; + }; + if let Some(canceller) = &self.canceller { + // Cancelling unblocks the read, so the join cannot outlast it. + canceller.close(); + let _ = worker.join(); + return; + } + // A native-pipe drain has nothing to cancel: only the bridge closes + // the write end, and while unwinding it outlives this drop. Joining + // would hang, so wait briefly and otherwise leave the worker to + // finish on its own. It holds a pipe read end, not the primary, so + // dropping `LivePty` still closes the terminal. + if self.finished.recv_timeout(Self::DROP_GRACE).is_ok() { let _ = worker.join(); } } @@ -1790,11 +1817,12 @@ mod tests { drop(command); terminal.close_writer(); - let (reader, canceller) = terminal - .take_unclaimed_reader() - .expect("take unclaimed reader") - .expect("reader was unclaimed"); - let drain = PtyDrain::start(reader); + let drain = PtyDrain::start_cancellable( + terminal + .take_unclaimed_reader() + .expect("take unclaimed reader") + .expect("reader was unclaimed"), + ); assert!( terminal .take_unclaimed_reader() @@ -1806,7 +1834,7 @@ mod tests { assert!(wait_for_exit(&mut child).success()); let seen = drain.wait_for("drained-to-exit"); assert!(seen.contains("drained-to-exit"), "got: {seen:?}"); - canceller.close(); + drain.cancel(); drop(terminal); let _ = drain.finish(); } @@ -1964,20 +1992,23 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let (mut writer, output): (Box, Box) = if native { + let (mut writer, drain): (Box, PtyDrain) = if native { let stdio = terminal.take_native_stdio().expect("take native stdio"); ( Box::new(std::fs::File::from(stdio.stdin)), - Box::new(std::fs::File::from(stdio.stdout)), + PtyDrain::start(Box::new(std::fs::File::from(stdio.stdout))), ) } else { ( Box::new(terminal.take_writer().expect("take writer")), - terminal.try_clone_reader().expect("clone reader"), + PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ), ) }; - let drain = PtyDrain::start(output); drain.wait_for("ready"); writer.write_all(input).expect("write terminal input"); drop(writer); @@ -2053,17 +2084,23 @@ mod tests { let mut child = command.spawn().expect("spawn child"); drop(command); - let (writer, output): (Option>, Box) = if native { + let (writer, drain): (Option>, PtyDrain) = if native { let stdio = terminal.take_native_stdio().expect("take native stdio"); ( Some(Box::new(std::fs::File::from(stdio.stdin))), - Box::new(std::fs::File::from(stdio.stdout)), + PtyDrain::start(Box::new(std::fs::File::from(stdio.stdout))), ) } else { - (None, terminal.try_clone_reader().expect("clone reader")) + ( + None, + PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ), + ) }; - let drain = PtyDrain::start(output); drain.wait_for("ready"); match writer { Some(writer) => drop(writer), @@ -2154,19 +2191,20 @@ mod tests { let terminal = LivePty::attach(&mut command, PtySize::default(), &[]).expect("attach PTY"); let mut child = command.spawn().expect("spawn child"); drop(command); - let (reader, closer) = terminal - .try_clone_reader_with_canceller() - .expect("clone reader"); // The drain must already be running: on Darwin the foreground shell // cannot exit while its output sits in the terminal queue. - let drain = PtyDrain::start(reader); + let drain = PtyDrain::start_cancellable( + terminal + .try_clone_reader_with_canceller() + .expect("clone reader"), + ); assert!(wait_for_exit(&mut child).success()); drain.wait_for("foreground-exited"); // The backgrounded `sleep` still holds the secondary open, so the read // parks until it is cancelled. - closer.close(); + drain.cancel(); let output = drain.finish(); assert!(output.contains("foreground-exited"), "got: {output:?}"); } @@ -2331,6 +2369,16 @@ mod tests { terminal.finish_native_bridge(); drop(input); + + // If cancellation injected end of input, the shell leaves its `read`, + // prints the marker and exits. Still being blocked is the evidence, so + // check it before the kill can suppress it. + std::thread::sleep(Duration::from_millis(200)); + assert!( + child.try_wait().expect("poll child").is_none(), + "cancellation ended the shell's `read`" + ); + // SAFETY: the child created its own session with its PID as the process-group ID. unsafe { libc::kill(-session, libc::SIGKILL); From 7a9c8eb49338c0463b34ca955300c6c1b55fa326 Mon Sep 17 00:00:00 2001 From: Elliot Michlin <31219104+theelliotm@users.noreply.github.com> Date: Wed, 14 Oct 2026 16:28:48 -0700 Subject: [PATCH 9/9] Detect injected EOF by draining the terminal directly `try_wait()` alone conflated two states: a shell still blocked in `read`, and one that received EOF, wrote the marker into an unread terminal and wedged while exiting. Both report the child as running, and because `finish_native_bridge()` has stopped the output reader, the marker never reaches the pipe either. Watch the primary directly and keep draining it, so the marker is detected positively instead of inferred from the child still running. Reap on a deadline, still draining: a Darwin child stuck writing to a full terminal does not die on SIGKILL until the queue drains. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/mxc-sdk/src/core/mxc_pty/mod.rs | 41 +++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 11 deletions(-) diff --git a/src/mxc-sdk/src/core/mxc_pty/mod.rs b/src/mxc-sdk/src/core/mxc_pty/mod.rs index 89d050f81e..29d7e536f5 100644 --- a/src/mxc-sdk/src/core/mxc_pty/mod.rs +++ b/src/mxc-sdk/src/core/mxc_pty/mod.rs @@ -2370,23 +2370,42 @@ mod tests { terminal.finish_native_bridge(); drop(input); - // If cancellation injected end of input, the shell leaves its `read`, - // prints the marker and exits. Still being blocked is the evidence, so - // check it before the kill can suppress it. - std::thread::sleep(Duration::from_millis(200)); - assert!( - child.try_wait().expect("poll child").is_none(), - "cancellation ended the shell's `read`" - ); + // The bridge has stopped copying, so watch the primary directly and + // keep draining it. An undrained terminal would wedge the shell mid + // exit, which looks exactly like it never left its `read`. + let watch = std::time::Instant::now() + Duration::from_millis(500); + while std::time::Instant::now() < watch { + text.push_str(&drain_available(stdio.stdout.as_raw_fd())); + text.push_str(&drain_available(terminal.primary.as_raw_fd())); + assert!(!text.contains("eof-injected"), "got: {text:?}"); + assert!( + child.try_wait().expect("poll child").is_none(), + "cancellation ended the shell's `read`; got: {text:?}" + ); + std::thread::sleep(Duration::from_millis(10)); + } // SAFETY: the child created its own session with its PID as the process-group ID. unsafe { libc::kill(-session, libc::SIGKILL); } - let _ = child.wait(); - let mut output = std::fs::File::from(stdio.stdout); - output.read_to_string(&mut text).expect("read output"); + // Reap on a deadline, still draining: a Darwin child stuck writing to a + // full terminal does not die until the queue drains. + let deadline = std::time::Instant::now() + Duration::from_secs(10); + loop { + text.push_str(&drain_available(stdio.stdout.as_raw_fd())); + text.push_str(&drain_available(terminal.primary.as_raw_fd())); + if child.try_wait().expect("poll child").is_some() { + break; + } + assert!( + std::time::Instant::now() < deadline, + "the shell did not exit after SIGKILL; got: {text:?}" + ); + std::thread::sleep(Duration::from_millis(10)); + } + assert!(text.contains("done"), "got: {text:?}"); assert!(!text.contains("eof-injected"), "got: {text:?}"); }