From 4ee35bec7a1b2a2d447e3d04ec8bbc08985eea71 Mon Sep 17 00:00:00 2001 From: can1357 Date: Sat, 30 May 2026 20:55:55 +0200 Subject: [PATCH] fix: corrected child-session behavior to DetachSession for non-term stdin - Renamed non-terminal stdin pipeline tests and updated non-terminal expectations to DetachSession behavior. - Updated child_session_action behavior so non-terminal, non-pipeline stdin now yields DetachSession instead of None. - Updated execute_external_command to skip process_group for detached children and move take_foreground into its foreground arm. - Added async regression coverage for detached pipeline stages and logged the fix in the natives CHANGELOG. --- crates/brush-core-vendored/src/commands.rs | 84 +++++++----- crates/pi-natives/src/shell.rs | 13 +- crates/pi-shell/src/shell.rs | 151 +++++++++++++++++++-- packages/natives/CHANGELOG.md | 4 + 4 files changed, 199 insertions(+), 53 deletions(-) diff --git a/crates/brush-core-vendored/src/commands.rs b/crates/brush-core-vendored/src/commands.rs index ac97d0011..0cdf8d406 100644 --- a/crates/brush-core-vendored/src/commands.rs +++ b/crates/brush-core-vendored/src/commands.rs @@ -617,37 +617,46 @@ pub(crate) fn execute_external_command( // Set up process group/session state. + // + // A child we are about to `setsid()` (`DetachSession`) must NOT also be + // handed a `process_group(...)`. For a would-be new-group leader it would + // duplicate the group `setsid` already creates; for a pipeline stage joining + // an established group it is a cross-session `setpgid` that fails with EPERM + // now that the leader (and every prior stage) has moved into its own session. + // In both cases `setsid` alone gives the child its own session and process + // group. See `child_session_action` for the decision rationale. let command_leads_session = new_pg && matches!(session_action, ChildSessionAction::TakeForeground) && context.shell.options().external_cmd_leads_session; - if new_pg { - match session_action { - ChildSessionAction::DetachSession => { - // `detach_session()` calls `setsid()`, which creates a fresh session - // and process group; requesting `process_group(0)` as well would - // conflict with that setup. - } - ChildSessionAction::TakeForeground if command_leads_session => { - // Don't set process_group(0) - setsid() in pre_exec will handle it. - cmd.lead_session(); - } - ChildSessionAction::TakeForeground | ChildSessionAction::None => { - // Normal case: create new process group in current session. + match session_action { + ChildSessionAction::DetachSession => { + // setsid() creates the fresh session + process group; no process_group(). + cmd.detach_session(); + } + ChildSessionAction::TakeForeground if command_leads_session => { + // Don't set process_group(0) - setsid() in pre_exec will handle it. + cmd.lead_session(); + } + ChildSessionAction::TakeForeground => { + // Foreground a child that is not leading its own session: create/join + // the process group in the current session, then grab the terminal. + if new_pg { cmd.process_group(0); + } else if let Some(pgid) = process_group_id { + cmd.process_group(pgid); + } + cmd.take_foreground(); + } + ChildSessionAction::None => { + // Normal case: create a new process group in the current session, or + // join an established one (later pipeline stages). + if new_pg { + cmd.process_group(0); + } else if let Some(pgid) = process_group_id { + cmd.process_group(pgid); } } - } else if let Some(pgid) = process_group_id { - // We need to join an established process group. - cmd.process_group(pgid); - } - - // See `child_session_action` for the decision rationale and call-out about - // pipeline groups. - match session_action { - ChildSessionAction::DetachSession => cmd.detach_session(), - ChildSessionAction::TakeForeground if !command_leads_session => cmd.take_foreground(), - ChildSessionAction::TakeForeground | ChildSessionAction::None => {} } // When tracing is enabled, report. @@ -964,18 +973,27 @@ pub enum ChildSessionAction { /// child inherited the host's controlling tty, and any `/dev/tty` open or /// `tcsetpgrp` call from the child could SIGTTIN/SIGTTOU and stop the host. /// -/// `detach_session()` is unsafe for any member of a multi-command pipeline: -/// for the first stage it puts the process-group leader in a different session, -/// causing later stages' `setpgid()` to fail with EPERM; for later stages it -/// either fails with EPERM or moves the child into a fresh session, breaking the -/// pipeline's shared process group and job-control signal propagation. Pipeline -/// stages therefore keep their pre-fix behavior (no detach). +/// A child whose stdin is **not** a terminal therefore always detaches, even +/// when it is a stage of a multi-command pipeline. An interactive program in a +/// pipeline (`zsh -i ... | awk`) would otherwise open `/dev/tty`, `tcsetpgrp` +/// itself to the foreground, and leave the host stopped on its next tty read. +/// `setsid()` puts each stage in its own session with no controlling tty, so it +/// cannot reach `/dev/tty` at all. The historical EPERM hazard — a later stage +/// `setpgid()`-joining a leader that already moved to a new session — is avoided +/// in `execute_external_command`, which skips `process_group(...)` entirely for +/// detached children; pipeline stages no longer share one process group, which +/// the embedded host does not rely on (it cancels via the descendant tree, and +/// pipes are session-independent). +/// +/// `in_pipeline_group` is no longer consulted: a pipeline stage that legitimately +/// needs the shared tty group has terminal stdin and is handled by the +/// `child_stdin_is_terminal` arm before pipeline membership would ever matter. /// /// Foregrounding remains gated on `new_pg && child_stdin_is_terminal`. pub fn child_session_action( new_pg: bool, child_stdin_is_terminal: bool, - in_pipeline_group: bool, + _in_pipeline_group: bool, ) -> ChildSessionAction { if new_pg && child_stdin_is_terminal { return ChildSessionAction::TakeForeground; @@ -985,9 +1003,5 @@ pub fn child_session_action( return ChildSessionAction::None; } - if in_pipeline_group { - return ChildSessionAction::None; - } - ChildSessionAction::DetachSession } diff --git a/crates/pi-natives/src/shell.rs b/crates/pi-natives/src/shell.rs index 3fa494ab2..719c0cf34 100644 --- a/crates/pi-natives/src/shell.rs +++ b/crates/pi-natives/src/shell.rs @@ -356,9 +356,11 @@ mod tests { } #[test] - fn non_terminal_stdin_leading_new_pgroup_detaches_unless_pipeline() { + fn non_terminal_stdin_detaches_regardless_of_pipeline() { assert_eq!(child_session_action(true, false, false), ChildSessionAction::DetachSession); - assert_eq!(child_session_action(true, false, true), ChildSessionAction::None); + // A leading-new-pgroup stage of a pipeline still detaches: setsid keeps + // it off the host's controlling tty. + assert_eq!(child_session_action(true, false, true), ChildSessionAction::DetachSession); } #[test] @@ -377,8 +379,11 @@ mod tests { } #[test] - fn pipeline_stage_does_not_detach() { - assert_eq!(child_session_action(false, false, true), ChildSessionAction::None); + fn pipeline_stage_with_non_terminal_stdin_detaches() { + // Regression: an interactive child inside a pipeline (`zsh -i | awk`) + // must not stay in the host session and seize its tty. Pre-fix this + // returned `None`, leaving the stage attached and able to SIGTTIN the host. + assert_eq!(child_session_action(false, false, true), ChildSessionAction::DetachSession); } } diff --git a/crates/pi-shell/src/shell.rs b/crates/pi-shell/src/shell.rs index 919f0ec6d..f844fc83f 100644 --- a/crates/pi-shell/src/shell.rs +++ b/crates/pi-shell/src/shell.rs @@ -1634,13 +1634,16 @@ mod tests { assert_eq!(child_session_action(true, true, true), ChildSessionAction::TakeForeground,); } - /// Brush leading a new pgroup with non-terminal stdin detaches only when - /// it is not part of a multi-command pipeline. Pipeline leaders must stay - /// in the parent session so later stages can join their process group. + /// Brush leading a new pgroup with non-terminal stdin always detaches — + /// including the first stage of a pipeline. `setsid()` keeps the child + /// off the host's controlling tty; the spawn path skips + /// `process_group(...)` for detached children, so later stages no + /// longer try to `setpgid`-join a leader that has moved sessions (the + /// historical EPERM hazard). #[test] - fn non_terminal_stdin_leading_new_pgroup_detaches_unless_pipeline() { + fn non_terminal_stdin_detaches_regardless_of_pipeline() { assert_eq!(child_session_action(true, false, false), ChildSessionAction::DetachSession,); - assert_eq!(child_session_action(true, false, true), ChildSessionAction::None,); + assert_eq!(child_session_action(true, false, true), ChildSessionAction::DetachSession,); } /// Non-interactive brush, terminal stdin, no pipeline: nothing to do. @@ -1665,16 +1668,16 @@ mod tests { assert_eq!(child_session_action(false, false, false), ChildSessionAction::DetachSession,); } - /// **Pipeline carve-out.** Non-interactive brush, non-terminal stdin - /// (pipe), and a multi-command pipeline: MUST NOT detach. For the first - /// external stage, `setsid()` puts the process-group leader into a - /// different session, so later stages fail to join its group with - /// EPERM. For later stages, `setsid()` would either fail with EPERM or - /// move the child into a new session, breaking the pipeline's shared - /// process group and job-control signal propagation. + /// **Pipeline tty-safety.** Non-interactive brush, non-terminal stdin + /// (pipe), and a multi-command pipeline: detach. An interactive child in + /// a pipeline (`zsh -i ... | awk`) would otherwise open `/dev/tty`, + /// `tcsetpgrp` itself to the foreground, and leave the host stopped on + /// its next tty read (`suspended (tty input)`). Each stage gets its own + /// session instead; the embedded host cancels via the descendant tree, + /// not a shared pgroup, and pipes are session-independent. #[test] - fn pipeline_stage_does_not_detach() { - assert_eq!(child_session_action(false, false, true), ChildSessionAction::None,); + fn pipeline_stage_with_non_terminal_stdin_detaches() { + assert_eq!(child_session_action(false, false, true), ChildSessionAction::DetachSession,); } } @@ -1796,6 +1799,126 @@ mod tests { ); } + /// Regression for the `suspended (tty input)` bug: an **interactive child + /// inside a pipeline** (`zsh -i ... | awk`) used to stay in the host + /// session, open `/dev/tty`, `tcsetpgrp` itself to the foreground, and + /// leave the embedded host (OMP) stopped on its next tty read. The earlier + /// embedded-host fix carved pipelines out of `detach_session` because a + /// later stage that `setpgid`-joined a detached leader failed with EPERM. + /// + /// This test boots a real embedded `BrushShell` and runs a two-stage + /// pipeline whose first stage prints its PID then sleeps (forwarded to us + /// by `cat`). It asserts two contracts at once: + /// 1. the first stage runs in its **own session** (`getsid == own pid`), + /// so it can never reach the host's controlling tty — guards the + /// decision; and + /// 2. the pipeline still exits **successfully**, proving the second stage + /// spawned without the cross-session `setpgid` EPERM — guards the + /// wiring that skips `process_group(...)` for detached children. + #[cfg(unix)] + #[tokio::test(flavor = "multi_thread")] + async fn embedded_pipeline_stage_runs_in_its_own_session() { + use std::io::Read as _; + + // SAFETY: `getsid(0)` only queries the current process session; checked below. + let host_sid = unsafe { libc::getsid(0) }; + assert!(host_sid > 0, "getsid(0) failed: {}", std::io::Error::last_os_error()); + + let config = ShellConfig { session_env: None, snapshot_path: None, minimizer: None }; + let mut session = create_session(&config).await.expect("create_session"); + + let (mut reader, writer) = pipe_to_files("e2e-pipe").expect("pipe"); + let stdout_file = OpenFile::from(writer.try_clone().expect("clone")); + let stderr_file = OpenFile::from(writer); + + let mut params = session.shell.default_exec_params(); + params.set_fd(OpenFiles::STDIN_FD, null_file().expect("null stdin")); + params.set_fd(OpenFiles::STDOUT_FD, stdout_file); + params.set_fd(OpenFiles::STDERR_FD, stderr_file); + + let (pid_tx, pid_rx) = tokio::sync::oneshot::channel::(); + let reader_handle = tokio::task::spawn_blocking(move || { + let mut buf = Vec::new(); + let mut chunk = [0u8; 64]; + let mut pid_tx = Some(pid_tx); + while let Ok(n) = reader.read(&mut chunk) + && n > 0 + { + buf.extend_from_slice(&chunk[..n]); + if pid_tx.is_some() + && let Some(line_end) = buf.iter().position(|&byte| byte == b'\n') + && let Ok(line) = std::str::from_utf8(&buf[..line_end]) + && let Ok(pid) = line.trim().parse::() + { + let _ = pid_tx + .take() + .expect("pid sender should be present") + .send(pid); + } + } + buf + }); + + let shell_handle = tokio::spawn(async move { + let source_info = SourceInfo::from("pi-natives:test"); + // First stage prints its own PID and sleeps; `cat` forwards the PID + // line to our reader and exits on EOF. The first stage leads the + // pipeline's process group, the second (`cat`) is the join-or-detach + // stage that would EPERM without the wiring fix. + let exec = session + .shell + .run_string( + "/bin/sh -c 'printf \"%d\\n\" \"$$\"; sleep 1' | /bin/cat", + &source_info, + ¶ms, + ) + .await + .expect("run_string"); + drop(params); + (session, exec) + }); + + let child_pid = time::timeout(Duration::from_secs(5), pid_rx) + .await + .expect("timed out waiting for first-stage PID") + .expect("reader closed pid channel without sending"); + assert!(child_pid > 0, "got non-positive child pid: {child_pid}"); + + // SAFETY: `child_pid` is a live positive PID (still in `sleep`); the return + // value is checked. + let child_sid = unsafe { libc::getsid(child_pid) }; + assert!( + child_sid > 0, + "getsid({child_pid}) failed: {} (child may have already exited)", + std::io::Error::last_os_error(), + ); + + let (_session, exec) = time::timeout(Duration::from_secs(5), shell_handle) + .await + .expect("shell timed out") + .expect("shell task panicked"); + // Guards the wiring: the second stage spawned without a cross-session + // `setpgid` EPERM, so the whole pipeline succeeded. + assert!( + matches!(exec.exit_code, ExecutionExitCode::Success), + "pipeline did not succeed (second stage may have hit setpgid EPERM): {}", + exit_code(&exec), + ); + let _ = time::timeout(Duration::from_secs(2), reader_handle).await; + + // Guards the decision: a pipeline stage must not share the host session, + // or it could seize the controlling tty and SIGTTIN the host. + assert_ne!( + child_sid, host_sid, + "pipeline stage PID {child_pid} inherited host session {host_sid}; it could seize the \ + controlling tty — the pipeline tty-suspend bug is back", + ); + assert_eq!( + child_sid, child_pid, + "pipeline stage PID {child_pid} should be its own session leader after setsid", + ); + } + #[tokio::test] async fn abort_state_signals_cancel_token() { let abort_state = ShellAbortState::default(); diff --git a/packages/natives/CHANGELOG.md b/packages/natives/CHANGELOG.md index 9f91f7173..15cf957c9 100644 --- a/packages/natives/CHANGELOG.md +++ b/packages/natives/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed an interactive shell inside a **pipeline** (`zsh -i ... | awk`, `time zsh -i | cat`, etc.) suspending the embedded host with `suspended (tty input)`. The earlier embedded-host fix `setsid`-detached external children so they could not seize the host's controlling tty, but carved pipeline stages out because a later stage that `setpgid`-joined a detached leader failed with EPERM — leaving every pipeline stage in the host session, where an interactive child opened `/dev/tty`, `tcsetpgrp`'d itself to the foreground, and stopped the host (OMP) on its next tty read. `pi_shell` now detaches pipeline stages too: `child_session_action` returns `DetachSession` for any non-terminal-stdin child regardless of pipeline membership, and `execute_external_command` skips `process_group(...)` entirely for detached children so no cross-session `setpgid` is attempted. Pipeline stages no longer share one process group, which the embedded host does not rely on (cancellation walks the descendant tree and pipes are session-independent). + ## [15.6.0] - 2026-05-30 ### Changed