From 3dbf8182161d6b40e5c52b4847514ede0e3291c7 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 26 Apr 2026 03:04:14 +0900 Subject: [PATCH 1/3] fix: restore terminal mouse tracking state on PTY session disconnect (#189) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After a PTY session disconnects (normal exit, Ctrl+C, network drop, or panic), remote interactive programs (vim, tmux, htop) may have enabled mouse tracking on the local terminal. Without cleanup, the terminal remains in tracking mode and prints raw SGR escape sequences on mouse movement. All cleanup paths now emit the complete set of mouse-tracking-off sequences (modes 1000, 1002, 1003, 1006, 1015) together with cursor-show and alternate-screen-exit: - TerminalStateGuard::restore_terminal_state() (Drop path) - force_terminal_cleanup() (last-resort call sites) - TerminalGuard::restore_terminal() in interactive_signal.rs (panic hook) now delegates to force_terminal_cleanup() instead of carrying its own incomplete cleanup — centralising logic and fixing the gap. --- CHANGELOG.md | 5 ++++ src/commands/interactive_signal.rs | 21 ++++++----------- src/pty/terminal.rs | 37 +++++++++++++++++++++++------- 3 files changed, 41 insertions(+), 22 deletions(-) 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..8a89e863 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,22 @@ 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. pub fn force_terminal_cleanup() { let _guard = TERMINAL_MUTEX.lock().unwrap(); + + // 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); From 932960fd74131efbe9cbf15eeaaf2db921ca9f76 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 26 Apr 2026 03:10:40 +0900 Subject: [PATCH 2/3] fix: make force_terminal_cleanup safe to call from panic hook Use try_lock() in force_terminal_cleanup() so the panic hook path (TerminalGuard::restore_terminal -> force_terminal_cleanup) cannot deadlock if the panicking thread already holds TERMINAL_MUTEX, and so a poisoned mutex from a previous panic does not cause a secondary panic via unwrap(). The underlying stdout writes and disable_raw_mode are individually safe; the lock only serializes concurrent teardown. --- src/pty/terminal.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index 8a89e863..94eb70b7 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -207,8 +207,22 @@ impl Drop for TerminalStateGuard { /// 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. 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. From 5876e9419c3871d12299b5c9b2f1753051665234 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Sun, 26 Apr 2026 03:19:24 +0900 Subject: [PATCH 3/3] test(pty): add force_terminal_cleanup tests and clarify docstring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add four unit tests to src/pty/terminal.rs covering the safety properties of force_terminal_cleanup(): - idempotency: calling twice does not panic - poisoned-mutex resilience: try_lock().ok() yields None instead of panicking when the mutex is poisoned by a previous panic - held-mutex resilience: try_lock().ok() returns immediately rather than blocking when the lock is already held on this thread Tests use local Mutex instances (never the global TERMINAL_MUTEX) so the global state is not disturbed for other tests in the same process. A comment block explains what cannot be tested in a non-TTY environment and why. Also extend the force_terminal_cleanup() docstring to make explicit that "unsynchronized" means without the lock, not skipped — addressing the LOW-severity documentation finding from the security review. --- src/pty/terminal.rs | 100 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index 94eb70b7..38bb2b26 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -217,6 +217,11 @@ impl Drop for TerminalStateGuard { /// 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() { // 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 @@ -319,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)); + } +}