diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d398fe8..e77524b3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ All notable changes to bssh will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed +- **PTY session mouse tracking leak**: after a PTY session disconnects (normal exit, Ctrl+C, network drop, or panic), the local terminal no longer prints raw SGR mouse escape sequences when the mouse is moved. All cleanup paths (`TerminalStateGuard::Drop`, `force_terminal_cleanup`, and the panic hook via `TerminalGuard`) now emit the full set of mouse-tracking-off sequences (modes 1000, 1002, 1003, 1006, 1015) plus cursor-show and alternate-screen-exit on teardown. (#189) + ## [2.1.1] - 2026-04-17 ### Fixed diff --git a/src/commands/interactive_signal.rs b/src/commands/interactive_signal.rs index 78b086a7..b5627c63 100644 --- a/src/commands/interactive_signal.rs +++ b/src/commands/interactive_signal.rs @@ -133,21 +133,14 @@ impl TerminalGuard { } } - /// Restore terminal to normal mode + /// Restore terminal to normal mode. + /// + /// Delegates to `force_terminal_cleanup()` from `pty::terminal` so that the + /// panic-hook path shares exactly the same cleanup logic (mouse tracking reset, + /// alternate-screen restore, cursor show, raw-mode off) as every other cleanup + /// path. No bespoke logic is duplicated here. pub fn restore_terminal() -> Result<()> { - use crossterm::{execute, terminal}; - use std::io; - - // Disable raw mode if it was enabled - let _ = terminal::disable_raw_mode(); - - // Show cursor - let _ = execute!( - io::stdout(), - crossterm::cursor::Show, - terminal::LeaveAlternateScreen - ); - + crate::pty::terminal::force_terminal_cleanup(); Ok(()) } } diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index db94d853..38bb2b26 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -14,6 +14,12 @@ //! Terminal state management for PTY sessions. +use std::io::Write; +use std::sync::{ + Arc, Mutex, + atomic::{AtomicBool, Ordering}, +}; + use anyhow::{Context, Result}; use crossterm::{ event::{DisableBracketedPaste, EnableBracketedPaste}, @@ -21,10 +27,6 @@ use crossterm::{ terminal::{disable_raw_mode, enable_raw_mode}, }; use once_cell::sync::Lazy; -use std::sync::{ - Arc, Mutex, - atomic::{AtomicBool, Ordering}, -}; /// Global terminal cleanup synchronization /// Ensures only one cleanup attempt happens even with multiple guards @@ -165,6 +167,15 @@ impl TerminalStateGuard { eprintln!("Warning: Failed to disable bracketed paste mode during cleanup: {e}"); } + // Best-effort: disable all mouse tracking modes that a remote program may have + // enabled. Each write is independent so one failure does not abort the rest. + // Modes: 1000 (X11), 1002 (button-event), 1003 (any-event), 1006 (SGR), + // 1015 (urxvt), plus restore cursor visibility and alternate screen. + let _ = std::io::stdout().write_all( + b"\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\x1b[?1049l\x1b[?25h", + ); + let _ = std::io::stdout().flush(); + // Exit raw mode if it's globally active if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { if let Err(e) = disable_raw_mode() { @@ -179,9 +190,6 @@ impl TerminalStateGuard { self.is_raw_mode_active.store(false, Ordering::Relaxed); } - // TODO: Restore other terminal settings if needed - // For now, just exiting raw mode is sufficient - Ok(()) } } @@ -194,9 +202,41 @@ impl Drop for TerminalStateGuard { } } -/// Force terminal cleanup - can be called from anywhere to ensure terminal is restored +/// Force terminal cleanup - can be called from anywhere to ensure terminal is restored. +/// +/// This is a best-effort, infallible cleanup that disables mouse tracking, resets +/// alternate screen and cursor visibility, and exits raw mode. Each operation is +/// performed independently so a failure in one does not prevent the rest. +/// +/// # Panic-safety +/// +/// This function is safe to call from a panic hook. It uses `try_lock()` rather than +/// `lock()` so it never deadlocks if the panicking thread already holds +/// `TERMINAL_MUTEX` (re-entrant acquisition of `std::sync::Mutex` on the same thread +/// would otherwise deadlock), and it tolerates a poisoned mutex without secondary +/// panics. The underlying operations (stdout writes, `disable_raw_mode`) are +/// individually safe to run without the mutex; the lock only serializes concurrent +/// teardown attempts. +/// +/// When `try_lock()` fails — whether because the mutex is already held or because it +/// is poisoned — the cleanup body **always executes** regardless. The word +/// "unsynchronized" in the inline comment means the cleanup runs without holding the +/// lock; it does **not** mean the cleanup is skipped. pub fn force_terminal_cleanup() { - let _guard = TERMINAL_MUTEX.lock().unwrap(); + // Acquire the mutex if we can, but never block or panic on it. If the mutex is + // already held by this thread (re-entrant via panic hook) or poisoned by a + // previous panic, fall through and run the cleanup unsynchronized — the + // operations below are individually safe. + let _guard = TERMINAL_MUTEX.try_lock().ok(); + + // Best-effort: disable all mouse tracking modes, restore cursor, and leave alternate + // screen. Written as a single atomic blob to minimize partial-state risk. + // Modes: 1000 (X11), 1002 (button-event), 1003 (any-event), 1006 (SGR), + // 1015 (urxvt); then restore cursor visibility and normal screen buffer. + let _ = std::io::stdout() + .write_all(b"\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\x1b[?1049l\x1b[?25h"); + let _ = std::io::stdout().flush(); + if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { let _ = disable_raw_mode(); RAW_MODE_ACTIVE.store(false, Ordering::SeqCst); @@ -284,3 +324,98 @@ impl TerminalOps { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + // ----------------------------------------------------------------------- + // force_terminal_cleanup tests + // + // Terminal-state mutation (raw mode, alternate screen) cannot be unit- + // tested safely inside `cargo test` because: + // • the test runner does not allocate a real TTY — crossterm operations + // that require a TTY (e.g. enable_raw_mode) will fail or behave + // unexpectedly; + // • the escape-sequence writes go to cargo's captured stdout, which is + // harmless but not observable in a meaningful way; + // • the global statics (TERMINAL_MUTEX, RAW_MODE_ACTIVE) are shared + // across all tests in the same process, so tests that mutate them + // must be carefully ordered or marked #[serial]. + // + // What *can* be reliably tested here: + // 1. Idempotency: calling force_terminal_cleanup() twice does not panic. + // 2. Poisoned-mutex resilience: the `try_lock().ok()` pattern correctly + // yields None (rather than panicking) when the mutex is poisoned. + // Verified using a local Mutex so the global TERMINAL_MUTEX is never + // poisoned, keeping other tests unaffected. + // 3. Held-mutex resilience: `try_lock()` returns WouldBlock (not a + // deadlock) when a lock is already held. Verified using a local Mutex + // for the same isolation reason. + // + // Manual reproduction of the actual terminal fix (mouse tracking escape + // sequences) requires a real TTY (vim/tmux) and cannot be automated here. + // ----------------------------------------------------------------------- + + /// Calling force_terminal_cleanup() twice in succession must not panic. + /// + /// In a non-TTY test environment RAW_MODE_ACTIVE is false (no test calls + /// enable_raw_mode), so disable_raw_mode() is never invoked. The escape- + /// sequence writes succeed silently against cargo's stdout pipe. + #[test] + fn test_force_terminal_cleanup_idempotent() { + force_terminal_cleanup(); + force_terminal_cleanup(); + // Reaching here without a panic is the assertion. + } + + /// When a Mutex is poisoned, try_lock() returns Err(TryLockError::Poisoned) + /// and .ok() converts it to None — no secondary panic occurs. This mirrors + /// the exact pattern used inside force_terminal_cleanup() for TERMINAL_MUTEX. + /// + /// We verify the property on a local Mutex so we never poison the global + /// TERMINAL_MUTEX (which would break other tests in this process). + #[test] + fn test_try_lock_ok_survives_poisoned_mutex() { + let m = Mutex::new(()); + + // Poison the mutex by panicking while holding the lock. + let _ = std::panic::catch_unwind(|| { + let _guard = m.lock().unwrap(); + panic!("intentional poison"); + }); + + assert!(m.is_poisoned(), "mutex should be poisoned after the above"); + + // try_lock().ok() must yield None without panicking — the same + // guarantee force_terminal_cleanup() relies on for TERMINAL_MUTEX. + let guard = m.try_lock().ok(); + assert!(guard.is_none(), "expected None for a poisoned mutex"); + // Reaching here without a panic confirms resilience. + } + + /// When a Mutex is currently held, try_lock() returns + /// Err(TryLockError::WouldBlock) and .ok() yields None immediately — + /// no blocking or deadlock. This mirrors the re-entrant panic-hook + /// scenario that force_terminal_cleanup() is designed to survive. + #[test] + fn test_try_lock_ok_does_not_block_when_held() { + let m = Mutex::new(()); + let _held = m.lock().unwrap(); // hold the lock on this thread + + // On std::sync::Mutex a second try_lock from the same thread is + // Err(WouldBlock) (not a deadlock), and .ok() converts it to None. + let guard = m.try_lock().ok(); + assert!(guard.is_none(), "expected None when lock is already held"); + // Reaching here without blocking confirms the non-deadlock guarantee. + } + + #[test] + fn test_terminal_state_default() { + let state = TerminalState::default(); + assert!(!state.was_raw_mode); + assert!(!state.was_alternate_screen); + assert!(!state.was_mouse_enabled); + assert_eq!(state.size, (80, 24)); + } +}