diff --git a/crates/tty7-cli/src/server.rs b/crates/tty7-cli/src/server.rs index db7a1d71..1f582dc5 100644 --- a/crates/tty7-cli/src/server.rs +++ b/crates/tty7-cli/src/server.rs @@ -187,11 +187,12 @@ fn detach(cmd: &mut Command) { fn detach(cmd: &mut Command) { use std::os::windows::process::CommandExt as _; - const DETACHED_PROCESS: u32 = 0x0000_0008; - const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; - const CREATE_NO_WINDOW: u32 = 0x0800_0000; - - cmd.creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW); + // The daemon's flags, not a second opinion. `CREATE_NEW_PROCESS_GROUP` used + // to be in here too, and it disables Ctrl+C for everything in the new group: + // the server, every pane shell it spawns, and everything those shells run + // (#451, #314). `DETACHED_PROCESS` alone already leaves the server without a + // console for a control event to arrive on. + cmd.creation_flags(spawn::DAEMON_CREATION_FLAGS); } #[cfg(not(any(unix, windows)))] diff --git a/crates/tty7-core/src/daemon/pane.rs b/crates/tty7-core/src/daemon/pane.rs index 8bb38dfc..2e37489d 100644 --- a/crates/tty7-core/src/daemon/pane.rs +++ b/crates/tty7-core/src/daemon/pane.rs @@ -1222,6 +1222,13 @@ impl DaemonPane { ) -> anyhow::Result> { let pty_size = pty_size(size); + // Before the shell exists, not after: a ConPTY child inherits the + // "ignore Ctrl+C" state of whoever created it, and the daemon can be + // carrying it from its own launch. Leaving it on costs the pane every + // Ctrl+C it will ever be sent (#451, #314). + #[cfg(windows)] + crate::daemon::winproc::allow_ctrl_c_in_children(); + let pair = native_pty_system().openpty(pty_size)?; let spawn = build_spawn_config(id, cwd, shell, workspace.as_deref())?; diff --git a/crates/tty7-core/src/daemon/spawn.rs b/crates/tty7-core/src/daemon/spawn.rs index ae97bdde..e5295825 100644 --- a/crates/tty7-core/src/daemon/spawn.rs +++ b/crates/tty7-core/src/daemon/spawn.rs @@ -734,15 +734,64 @@ fn detach(cmd: &mut Command) { } } +/// How the daemon is created: no console of its own, and no window. +/// +/// Deliberately without `CREATE_NEW_PROCESS_GROUP`. That flag disables Ctrl+C +/// for the whole new group, and Windows hands the resulting "ignore Ctrl+C" +/// process state down to every descendant — which for the daemon means every +/// ConPTY shell it spawns, and everything those shells run. It bought nothing +/// either: `DETACHED_PROCESS` already leaves the daemon with no console, so no +/// console control event could reach it in the first place. Keeping it cost +/// every pane its Ctrl+C (#451, #314). +/// +/// Public because `tty7-cli` launches the headless `tty7-server` the same way, +/// and that server spawns panes too: the two detach paths agreeing is the whole +/// point of the constant. +#[cfg(windows)] +pub const DAEMON_CREATION_FLAGS: u32 = DETACHED_PROCESS | CREATE_NO_WINDOW; + +#[cfg(windows)] +const DETACHED_PROCESS: u32 = 0x0000_0008; +#[cfg(windows)] +const CREATE_NO_WINDOW: u32 = 0x0800_0000; +/// Named only so the tests can say which bit must stay out of the flags above. +#[cfg(all(windows, test))] +const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; + #[cfg(windows)] fn detach(cmd: &mut Command) { use std::os::windows::process::CommandExt; - const DETACHED_PROCESS: u32 = 0x0000_0008; - const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; - const CREATE_NO_WINDOW: u32 = 0x0800_0000; + cmd.creation_flags(DAEMON_CREATION_FLAGS); +} - cmd.creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW); +#[cfg(all(test, windows))] +mod ctrl_c_flag_tests { + use super::*; + + /// `CREATE_NEW_PROCESS_GROUP` disables Ctrl+C for every process in the new + /// group, and the daemon's group is every pane shell it ever spawns. Both + /// spawn paths — the ordinary one and the Redirection Trust detour — have to + /// stay clear of it or #451 and #314 come straight back. + #[test] + fn the_daemon_is_never_created_into_a_ctrl_c_free_process_group() { + assert_eq!( + DAEMON_CREATION_FLAGS & CREATE_NEW_PROCESS_GROUP, + 0, + "the daemon must not disable Ctrl+C for everything it spawns" + ); + assert_eq!( + DAEMON_CREATION_FLAGS, + DETACHED_PROCESS | CREATE_NO_WINDOW, + "the daemon still wants no console and no window" + ); + + assert_eq!( + windows::SPAWN_FLAGS & CREATE_NEW_PROCESS_GROUP, + 0, + "the clean-parent spawn path must not reintroduce the group either" + ); + } } #[cfg(all(test, windows))] diff --git a/crates/tty7-core/src/daemon/spawn/windows.rs b/crates/tty7-core/src/daemon/spawn/windows.rs index 6afa7e84..0314fcb9 100644 --- a/crates/tty7-core/src/daemon/spawn/windows.rs +++ b/crates/tty7-core/src/daemon/spawn/windows.rs @@ -7,14 +7,20 @@ use std::ptr; use windows_sys::Win32::Foundation::{CloseHandle, HANDLE}; use windows_sys::Win32::System::Threading::{ - CREATE_NEW_PROCESS_GROUP, CREATE_NO_WINDOW, CreateProcessW, DETACHED_PROCESS, - DeleteProcThreadAttributeList, EXTENDED_STARTUPINFO_PRESENT, GetCurrentProcess, + CreateProcessW, DeleteProcThreadAttributeList, EXTENDED_STARTUPINFO_PRESENT, GetCurrentProcess, GetProcessMitigationPolicy, InitializeProcThreadAttributeList, LPPROC_THREAD_ATTRIBUTE_LIST, OpenProcess, PROC_THREAD_ATTRIBUTE_PARENT_PROCESS, PROCESS_CREATE_PROCESS, PROCESS_INFORMATION, ProcessRedirectionTrustPolicy, STARTUPINFOEXW, UpdateProcThreadAttribute, }; use windows_sys::Win32::UI::WindowsAndMessaging::{GetShellWindow, GetWindowThreadProcessId}; +/// The ordinary daemon flags plus the attribute list this path exists for. +/// +/// Derived from the shared constant rather than spelled out again: the two +/// spawn paths differing here is how one of them could quietly go back to +/// creating the daemon into a process group where Ctrl+C is disabled. +pub(super) const SPAWN_FLAGS: u32 = super::DAEMON_CREATION_FLAGS | EXTENDED_STARTUPINFO_PRESENT; + /// Returns whether the current process has the enforcing Redirection Trust bit. /// /// Query failures deliberately fall back to the ordinary spawn path. The @@ -249,10 +255,7 @@ pub(super) fn spawn_detached_with_parent( startup.lpAttributeList = attributes.as_mut_ptr(); let mut process_info: PROCESS_INFORMATION = unsafe { std::mem::zeroed() }; - let flags = DETACHED_PROCESS - | CREATE_NEW_PROCESS_GROUP - | CREATE_NO_WINDOW - | EXTENDED_STARTUPINFO_PRESENT; + let flags = SPAWN_FLAGS; // SAFETY: Both strings are NUL-terminated, the command line is writable, // and every pointer, handle, and output structure remains alive throughout // CreateProcessW. Handle inheritance is intentionally disabled. diff --git a/crates/tty7-core/src/daemon/winproc.rs b/crates/tty7-core/src/daemon/winproc.rs index 0f4c05e6..fa0e5cf7 100644 --- a/crates/tty7-core/src/daemon/winproc.rs +++ b/crates/tty7-core/src/daemon/winproc.rs @@ -250,6 +250,191 @@ pub(crate) fn path_is_under(path: &std::path::Path, dir: &std::path::Path) -> bo path.starts_with(&dir) } +/// Makes Ctrl+C reach the processes this one goes on to spawn. +/// +/// Windows keeps an "ignore Ctrl+C" bit per process, and a child inherits it at +/// creation. Anything that sets it on the daemon — being created with +/// `CREATE_NEW_PROCESS_GROUP`, or simply being launched from a shell that +/// already had it — silently disarms Ctrl+C in every pane, because the pane's +/// shell and everything it runs are all descendants. The pane still writes 0x03 +/// into the pty and conhost still turns it into a keypress; what never happens +/// is the CTRL_C_EVENT, so `go run` and `npm install` keep going (#451, #314). +/// +/// A `NULL` routine with `FALSE` clears the bit, and it is what descendants +/// created afterwards inherit. Cheap and idempotent, so the pane spawn path +/// calls it rather than relying on a single well-placed call at startup. +pub(crate) fn allow_ctrl_c_in_children() { + // SAFETY: A NULL handler routine is the documented spelling of "stop + // ignoring Ctrl+C"; there is no handler list entry to keep alive. + unsafe { + windows_sys::Win32::System::Console::SetConsoleCtrlHandler(None, 0); + } +} + +#[cfg(test)] +mod ctrl_c_tests { + use std::io::{Read as _, Write as _}; + use std::os::windows::process::CommandExt as _; + use std::process::Command; + use std::time::{Duration, Instant}; + + use portable_pty::{CommandBuilder, PtySize, native_pty_system}; + + const RESULT_ENV: &str = "TTY7_CTRL_C_INHERITED_IGNORE_RESULT"; + const TEST_NAME: &str = + "daemon::winproc::ctrl_c_tests::a_pane_survives_an_inherited_ignore_ctrl_c"; + + const DETACHED_PROCESS: u32 = 0x0000_0008; + const CREATE_NEW_PROCESS_GROUP: u32 = 0x0000_0200; + const CREATE_NO_WINDOW: u32 = 0x0800_0000; + + /// The reported bug (#451, #314): nothing in a tty7 pane could be + /// interrupted, so `go run` and `npm install` ran on after Ctrl+C. + /// + /// The keystroke path was never at fault — the pane does write 0x03 into the + /// pty, and conhost does turn it into a keypress. What never happened was + /// the CTRL_C_EVENT, because the daemon carried Windows' "ignore Ctrl+C" + /// process state and every ConPTY child inherits it. + /// + /// So the state has to be *inherited* here rather than switched on in + /// place — that is the shape the daemon was in, and it is the only version a + /// single `SetConsoleCtrlHandler(NULL, FALSE)` has to be able to undo. An + /// intermediate process created exactly as the daemon used to be supplies + /// it, and then runs both arms: a pane spawned without the fix must hang, + /// and a pane spawned after it must not. Testing only the second arm would + /// pass on its own on a machine whose runner never had the bit set. + #[test] + fn a_pane_survives_an_inherited_ignore_ctrl_c() { + if let Some(result) = std::env::var_os(RESULT_ENV) { + let unfixed = interruptible(); + super::allow_ctrl_c_in_children(); + let fixed = interruptible(); + std::fs::write(result, format!("{unfixed} {fixed}")).expect("write the arms' verdict"); + return; + } + + let result = + std::env::temp_dir().join(format!("tty7-pane-ctrl-c-{}.txt", std::process::id())); + let _ = std::fs::remove_file(&result); + let status = Command::new(std::env::current_exe().expect("locate the test executable")) + .args(["--exact", TEST_NAME, "--nocapture"]) + .env(RESULT_ENV, &result) + // Exactly how the daemon used to be created. CREATE_NEW_PROCESS_GROUP + // is what leaves the inherited ignore behind. + .creation_flags(DETACHED_PROCESS | CREATE_NEW_PROCESS_GROUP | CREATE_NO_WINDOW) + .stdin(std::process::Stdio::null()) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .expect("run the arms in a process that inherits the ignore"); + assert!(status.success(), "the inner test process must finish"); + + let verdict = std::fs::read_to_string(&result).unwrap_or_else(|_| "MISSING".into()); + let _ = std::fs::remove_file(&result); + assert_eq!( + verdict.trim(), + "false true", + "a pane spawned under an inherited ignore must be interruptible only after \ + allow_ctrl_c_in_children (got `{}`); `true true` means the ignore never took \ + hold and the test proves nothing, `false false` is the bug in #451 and #314", + verdict.trim() + ); + } + + /// Runs a shell in a pane the way `DaemonPane::spawn` does, interrupts a + /// command that would otherwise never end, and reports whether the shell + /// came back. + /// + /// The observable is the shell, not the interrupted command: after the ^C, + /// `cmd` gets its prompt back and acts on the `exit` typed behind it. If the + /// ^C is swallowed, `ping` still owns the console, the `exit` is never read, + /// and the shell outlives the deadline. Nothing here reads a message, so it + /// holds on a non-English Windows too. + fn interruptible() -> bool { + let pty = native_pty_system() + .openpty(PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("open a pty the way a pane does"); + let mut child = pty + .slave + .spawn_command(CommandBuilder::new("cmd.exe")) + .expect("spawn the shell"); + drop(pty.slave); + + // ConPTY stalls its client once the output pipe fills, so this has to + // keep running whatever else happens. The byte count it keeps is how the + // test paces itself instead of guessing at sleeps. + let mut reader = pty.master.try_clone_reader().expect("clone the reader"); + let written = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); + let counter = std::sync::Arc::clone(&written); + std::thread::spawn(move || { + let mut buf = [0u8; 4096]; + while let Ok(n) = reader.read(&mut buf) { + if n == 0 { + break; + } + counter.fetch_add(n, std::sync::atomic::Ordering::Relaxed); + } + }); + let bytes = || written.load(std::sync::atomic::Ordering::Relaxed); + // Waits for `rounds` separate batches of output. Two of them means the + // running command is producing its own lines on its own schedule, which + // is the only reliable sign that it — not the shell — holds the console. + // Interrupting before that reaches nobody, and would look like this bug + // on a slow machine for a reason that has nothing to do with the fix. + let settle = |rounds: usize| { + let give_up = Instant::now() + Duration::from_secs(20); + for _ in 0..rounds { + let before = bytes(); + while bytes() == before && Instant::now() < give_up { + std::thread::sleep(Duration::from_millis(50)); + } + std::thread::sleep(Duration::from_millis(300)); + } + }; + + let mut writer = pty.master.take_writer().expect("take the writer"); + let mut type_in = |keys: &[u8]| { + writer.write_all(keys).expect("write to the pty"); + writer.flush().expect("flush the pty"); + }; + settle(1); // the shell's banner and first prompt + // Runs until interrupted, and holds the console while it does. + type_in(b"ping -t 127.0.0.1\r"); + settle(3); // the echoed line, then replies on ping's own clock + type_in(&[0x03]); + std::thread::sleep(Duration::from_millis(500)); + type_in(b"exit\r"); + + let deadline = Instant::now() + Duration::from_secs(8); + loop { + match child.try_wait().expect("poll the shell") { + Some(_) => return true, + None if Instant::now() >= deadline => { + // Killing the shell leaves the `ping` it never interrupted + // running forever, so take the subtree with it. Read the + // subtree while the shell is still alive: once it is gone + // its pid can be handed to somebody else, and a snapshot + // taken after that names a stranger's children. + let doomed = child + .process_id() + .map(|shell| super::descendants(&super::snapshot(), shell)) + .unwrap_or_default(); + let _ = child.kill(); + let _ = child.wait(); + super::terminate_and_wait_all(&doomed, Instant::now() + Duration::from_secs(2)); + return false; + } + None => std::thread::sleep(Duration::from_millis(100)), + } + } + } +} + #[cfg(test)] mod tests { use super::*;