From b86011424996860cd40155b445e3cfb25b292aae Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 16:21:02 +0900 Subject: [PATCH 1/6] feat: preserve outer TUI keyboard state Capture Kitty keyboard flags, xterm modifyOtherKeys, and alternate-screen ownership before starting the PTY input task. Preserve unrelated input bytes and restore the captured state across normal and forced cleanup while retaining the legacy baseline fallback. Track remote screen transitions so an unchanged outer alternate buffer is not cleared during teardown. Refs #236 --- CHANGELOG.md | 1 + docs/architecture/interactive-mode.md | 9 +- src/pty/mod.rs | 2 + src/pty/session/mod.rs | 3 +- src/pty/session/raw_input_task.rs | 90 +++++ src/pty/session/session_manager.rs | 81 +---- src/pty/terminal.rs | 172 ++++----- src/pty/terminal_protocol.rs | 492 ++++++++++++++++++++++++++ src/pty/terminal_screen.rs | 128 +++++++ 9 files changed, 829 insertions(+), 149 deletions(-) create mode 100644 src/pty/session/raw_input_task.rs create mode 100644 src/pty/terminal_protocol.rs create mode 100644 src/pty/terminal_screen.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index d993eb3f..96a0354a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Roughly double bssh-server single-connection SFTP write throughput by fixing channel fragmentation and the serial write path** (#187). Three independent bottlenecks identified in the issue's code audit are addressed. First, `build_russh_config` now advertises a 65535-byte `maximum_packet_size` (the russh cap; library default 32768) and an 8 MiB `window_size` (library default 2 MiB), both configurable via `server.maximum_packet_size` / `server.window_size` (env: `BSSH_MAX_PACKET_SIZE` / `BSSH_WINDOW_SIZE`), so a 256 KiB SFTP write is no longer chopped into 8 CHANNEL_DATA packets that each pay the full cipher/copy/scheduling cost. Second, the SFTP server loop in `bssh-russh-sftp` is restructured from a strict read-process-write-flush cycle into a reader task with a bounded read-ahead queue plus an in-order processor: responses are flushed once per burst instead of once per request, and consecutive queued `SSH_FXP_WRITE` requests to the same handle at sequential offsets are coalesced into a single handler call (bounded by `max_write_coalesce_len`, default 256 KiB) while every merged request id still receives its own status reply, preserving SFTP response ordering and error semantics. The bssh write/read handlers additionally track each open file's cursor and elide the per-chunk `seek` for sequential transfers (append handles are exempt since O_APPEND ignores the cursor). Third, the workspace gains a tuned `[profile.release]` (`lto = "fat"`, `codegen-units = 1`) and documented `RUSTFLAGS` target-CPU guidance for self-builds. Local before/after benchmark (loopback, OpenSSH sftp client, 1 GiB random file, 3-run averages, 20-core Linux box): upload 71 MiB/s to 437 MiB/s (6.2x), download 73 MiB/s to 282 MiB/s (3.9x); an ablation with the code changes but library-default channel sizing lands at 333 / 291 MiB/s, attributing most of the gain to the pipelined write path and the rest of the upload gain to the larger packets. Numbers on the reporter's NIC-limited Xeon will differ; the methodology is documented in the PR. Remaining plan items (per-packet copies inside russh, AES-CTR internals) live in upstream russh since #212 removed the fork and are out of scope here. ### Added +- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh now performs a 100 ms bounded query for raw Kitty progressive-enhancement flags, xterm `modifyOtherKeys`, and DEC private mode 1049, using primary device attributes as the unsupported-terminal sentinel. Strictly matched replies are consumed while every unrelated, malformed, partial, or concurrently typed byte is retained and forwarded through the normal PTY input path. Remote alternate-screen transitions are tracked across fragmented output so teardown restores the captured keyboard settings on the original screen without switching away from an outer TUI that still owns its alternate buffer; failed queries retain #234's ordinary-shell baseline fallback, and the same snapshot is reused by normal, forced, and repeated cleanup paths. - **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. ### Fixed diff --git a/docs/architecture/interactive-mode.md b/docs/architecture/interactive-mode.md index a489f72f..4782e13d 100644 --- a/docs/architecture/interactive-mode.md +++ b/docs/architecture/interactive-mode.md @@ -193,7 +193,14 @@ Run this check in Ghostty and at least one other Kitty-keyboard-compatible termi 3. At the restored local prompt, verify that Space, Enter, arrow keys, and modified keys behave normally and do not print CSI-u fragments such as `;1:3u`, `:1C`, or `:3C`. 4. Repeat with a Kitty-keyboard-aware full-screen application on the remote host and terminate the SSH transport before the application exits cleanly. -The cleanup deliberately restores the ordinary shell baseline. Exact preservation of an enhanced keyboard mode owned by an outer local TUI is not currently available because crossterm exposes protocol support detection but not the active flag value, while issuing a separate `/dev/tty` query would race bssh's PTY input reader. +Before starting the PTY input task, bssh spends at most 100 ms querying the active Kitty enhancement flags, xterm `modifyOtherKeys` value, and DEC private mode 1049. A primary-device-attributes query terminates the probe promptly on terminals that do not support the optional protocols. Only strict query replies are consumed; user input and malformed or unrelated escape sequences read during the probe are retained and forwarded to the remote session. Teardown tracks remote screen-buffer transitions and restores the captured keyboard values on the original main or alternate screen. If any required screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234. + +To validate outer-TUI preservation in Ghostty and Kitty or WezTerm: + +1. Start a Kitty-keyboard-aware TUI that can launch a child command while remaining in its alternate screen, and record its active enhancement flags with `printf '\033[?u'`. +2. Launch `bssh` from that TUI, enable different remote flags with `printf '\033[=31u'`, and exit normally. +3. Verify the outer TUI receives the same keyboard encoding it used before launching bssh and that its alternate-screen contents were not cleared by an unnecessary `1049l`/`1049h` round trip. +4. Repeat after abrupt transport loss, local `~.`, and a remote `printf '\033[?1049l'` to exercise both unchanged-screen and screen-recovery cleanup paths. ### Performance Characteristics diff --git a/src/pty/mod.rs b/src/pty/mod.rs index e962befa..2f3df64c 100644 --- a/src/pty/mod.rs +++ b/src/pty/mod.rs @@ -29,6 +29,8 @@ use tokio::time::Duration; pub mod session; pub mod terminal; +mod terminal_protocol; +mod terminal_screen; pub use session::PtySession; pub use terminal::{TerminalState, TerminalStateGuard, force_terminal_cleanup}; diff --git a/src/pty/session/mod.rs b/src/pty/session/mod.rs index b0504c00..3eb12205 100644 --- a/src/pty/session/mod.rs +++ b/src/pty/session/mod.rs @@ -18,7 +18,8 @@ mod constants; mod escape_filter; mod input; mod local_escape; -mod raw_input; +pub(crate) mod raw_input; +mod raw_input_task; mod session_manager; mod terminal_modes; diff --git a/src/pty/session/raw_input_task.rs b/src/pty/session/raw_input_task.rs new file mode 100644 index 00000000..b628fa3a --- /dev/null +++ b/src/pty/session/raw_input_task.rs @@ -0,0 +1,90 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Blocking local-terminal input forwarding for an active PTY session. + +use std::time::Duration; + +use tokio::sync::{mpsc, watch}; + +use super::constants::INPUT_POLL_TIMEOUT_MS; +use super::local_escape::{LocalAction, LocalEscapeDetector}; +use super::raw_input::RawInputReader; +use crate::pty::PtyMessage; + +pub(crate) fn run( + input_tx: mpsc::Sender, + cancel_rx: watch::Receiver, + pending_input: Vec, +) { + let mut reader = RawInputReader::new(); + let mut buffer = [0_u8; 1024]; + let mut escape_detector = LocalEscapeDetector::new(); + + if !pending_input.is_empty() && !forward_input(&input_tx, &mut escape_detector, &pending_input) + { + return; + } + + loop { + if *cancel_rx.borrow() { + break; + } + + match reader.poll(Duration::from_millis(INPUT_POLL_TIMEOUT_MS)) { + Ok(true) => match reader.read(&mut buffer) { + Ok(0) => { + tracing::debug!("EOF received on stdin"); + break; + } + Ok(read) => { + if !forward_input(&input_tx, &mut escape_detector, &buffer[..read]) { + break; + } + } + Err(error) => { + let _ = input_tx.try_send(PtyMessage::Error(format!("Input error: {error}"))); + break; + } + }, + Ok(false) => continue, + Err(error) => { + let _ = input_tx.try_send(PtyMessage::Error(format!("Poll error: {error}"))); + break; + } + } + } +} + +fn forward_input( + input_tx: &mpsc::Sender, + detector: &mut LocalEscapeDetector, + bytes: &[u8], +) -> bool { + if let Some(action) = detector.process(bytes) { + match action { + LocalAction::Disconnect => { + tracing::debug!("Disconnect escape sequence detected"); + let _ = input_tx.try_send(PtyMessage::Terminate); + false + } + LocalAction::Passthrough(data) => { + input_tx.try_send(PtyMessage::LocalInput(data)).is_ok() + } + } + } else { + let data = smallvec::SmallVec::from_slice(bytes); + input_tx.try_send(PtyMessage::LocalInput(data)).is_ok() + } +} diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index e1b5de34..4fe61dff 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -16,8 +16,7 @@ use super::constants::*; use super::escape_filter::EscapeSequenceFilter; -use super::local_escape::{LocalAction, LocalEscapeDetector}; -use super::raw_input::RawInputReader; +use super::raw_input_task; use super::terminal_modes::configure_terminal_modes; use crate::pty::{ PtyConfig, PtyMessage, PtyState, @@ -156,7 +155,9 @@ impl PtySession { } // Set up terminal state guard - self.terminal_guard = Some(TerminalStateGuard::new()?); + let mut terminal_guard = TerminalStateGuard::new()?; + let pending_input = terminal_guard.take_pending_input(); + self.terminal_guard = Some(terminal_guard); // Enable mouse support if requested if self.config.enable_mouse { @@ -226,70 +227,7 @@ impl PtySession { // NOTE: TerminalStateGuard has already called enable_raw_mode() at this point, // so stdin.read() will return raw bytes without line buffering let input_task = tokio::task::spawn_blocking(move || { - let mut reader = RawInputReader::new(); - let mut buffer = [0u8; 1024]; - let mut escape_detector = LocalEscapeDetector::new(); - - loop { - if *cancel_for_input.borrow() { - break; - } - - let poll_timeout = Duration::from_millis(INPUT_POLL_TIMEOUT_MS); - - match reader.poll(poll_timeout) { - Ok(true) => { - match reader.read(&mut buffer) { - Ok(0) => { - // EOF - user closed stdin - tracing::debug!("EOF received on stdin"); - break; - } - Ok(n) => { - // Check for local escape sequences (e.g., ~. for disconnect) - if let Some(action) = escape_detector.process(&buffer[..n]) { - match action { - LocalAction::Disconnect => { - tracing::debug!("Disconnect escape sequence detected"); - let _ = input_tx.try_send(PtyMessage::Terminate); - break; - } - LocalAction::Passthrough(data) => { - // Send filtered data - if input_tx - .try_send(PtyMessage::LocalInput(data)) - .is_err() - { - break; - } - } - } - } else { - // Pass raw bytes through as-is - // This includes arrow keys, function keys, terminal responses, etc. - let data = smallvec::SmallVec::from_slice(&buffer[..n]); - if input_tx.try_send(PtyMessage::LocalInput(data)).is_err() { - break; - } - } - } - Err(e) => { - let _ = input_tx - .try_send(PtyMessage::Error(format!("Input error: {e}"))); - break; - } - } - } - Ok(false) => { - // Timeout - continue polling - continue; - } - Err(e) => { - let _ = input_tx.try_send(PtyMessage::Error(format!("Poll error: {e}"))); - break; - } - } - } + raw_input_task::run(input_tx, cancel_for_input, pending_input); }); // We'll integrate channel reading into the main loop since russh Channel doesn't clone @@ -323,6 +261,9 @@ impl PtySession { tracing::error!("Failed to write to stdout: {e}"); should_terminate = true; } else { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(&filtered_data); + } let _ = io::stdout().flush(); } } @@ -336,6 +277,9 @@ impl PtySession { tracing::error!("Failed to write stderr to stdout: {e}"); should_terminate = true; } else { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(&filtered_data); + } let _ = io::stdout().flush(); } } @@ -387,6 +331,9 @@ impl PtySession { tracing::error!("Failed to write to stdout: {e}"); should_terminate = true; } else { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(&filtered_data); + } let _ = io::stdout().flush(); } } diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index dcc27b7c..d5493106 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -14,10 +14,9 @@ //! Terminal state management for PTY sessions. -use std::io::Write; use std::sync::{ Arc, LazyLock, Mutex, - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicI8, Ordering}, }; use anyhow::{Context, Result}; @@ -27,33 +26,17 @@ use crossterm::{ terminal::{disable_raw_mode, enable_raw_mode}, }; +use super::terminal_protocol::{ProtocolState, capture_protocol_state, write_terminal_cleanup}; +use super::terminal_screen::ScreenModeTracker; + /// Global terminal cleanup synchronization /// Ensures only one cleanup attempt happens even with multiple guards static TERMINAL_MUTEX: LazyLock> = LazyLock::new(|| Mutex::new(())); +static SAVED_PROTOCOL_STATE: LazyLock>> = + LazyLock::new(|| Mutex::new(None)); static RAW_MODE_ACTIVE: AtomicBool = AtomicBool::new(false); - -/// Best-effort reset for terminal modes that a remote PTY application can leak. -/// -/// Kitty keyboard mode stacks are independent for the main and alternate screens. -/// Reset the current screen first, leave the alternate screen, and then reset the -/// main screen. `CSI < 999 u` performs a large bounded pop of unbalanced pushes, -/// `CSI = 0 u` also handles implementations without a stack, and -/// `CSI > 4 ; 0 m` disables xterm's `modifyOtherKeys` fallback. -/// -/// This intentionally restores the ordinary shell baseline. Crossterm does not -/// expose the current enhancement flags, and querying `/dev/tty` here would race -/// the PTY input reader. An outer local TUI that starts bssh with enhanced keyboard -/// reporting enabled may therefore need to re-enable its mode after bssh exits. -const TERMINAL_CLEANUP_SEQUENCE: &[u8] = b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\x1b[?1049l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?25h"; - -/// Emit the shared terminal reset sequence to an injectable writer. -/// -/// Cleanup must never replace the original session error, so write and flush -/// failures are deliberately ignored. -fn write_terminal_cleanup(writer: &mut impl Write) { - let _ = writer.write_all(TERMINAL_CLEANUP_SEQUENCE); - let _ = writer.flush(); -} +static CURRENT_ALTERNATE_SCREEN: AtomicI8 = AtomicI8::new(-1); +static SCREEN_CHANGED: AtomicBool = AtomicBool::new(false); /// Terminal state information that needs to be preserved and restored #[derive(Debug, Clone)] @@ -66,6 +49,7 @@ pub struct TerminalState { pub was_alternate_screen: bool, /// Whether mouse reporting was enabled pub was_mouse_enabled: bool, + protocol_state: ProtocolState, } impl Default for TerminalState { @@ -75,6 +59,7 @@ impl Default for TerminalState { size: (80, 24), was_alternate_screen: false, was_mouse_enabled: false, + protocol_state: ProtocolState::default(), } } } @@ -88,12 +73,14 @@ pub struct TerminalStateGuard { is_raw_mode_active: Arc, // Simplified cleanup - just track if we need cleanup _needs_cleanup: bool, + pending_input: Vec, + screen_mode_tracker: ScreenModeTracker, } impl TerminalStateGuard { /// Create a new terminal state guard and enter raw mode pub fn new() -> Result { - let saved_state = Self::save_terminal_state()?; + let mut saved_state = Self::save_terminal_state()?; let is_raw_mode_active = Arc::new(AtomicBool::new(false)); // Enter raw mode with global synchronization @@ -104,14 +91,32 @@ impl TerminalStateGuard { is_raw_mode_active.store(true, Ordering::Relaxed); } + // This bounded query runs before the PTY input task exists, so terminal + // replies cannot race normal input forwarding. + let capture = capture_protocol_state(&mut std::io::stdout()); + saved_state.protocol_state = capture.state.clone(); + // Enable bracketed paste mode execute!(std::io::stdout(), EnableBracketedPaste) .with_context(|| "Failed to enable bracketed paste mode")?; + if let Ok(mut saved) = SAVED_PROTOCOL_STATE.try_lock() { + *saved = Some(saved_state.protocol_state.clone()); + } + CURRENT_ALTERNATE_SCREEN.store( + encode_screen_state(saved_state.protocol_state.current_alternate_screen()), + Ordering::SeqCst, + ); + SCREEN_CHANGED.store(false, Ordering::SeqCst); + let screen_mode_tracker = + ScreenModeTracker::new(saved_state.protocol_state.current_alternate_screen()); + Ok(Self { saved_state, is_raw_mode_active, _needs_cleanup: true, + pending_input: capture.pending_input, + screen_mode_tracker, }) } @@ -124,6 +129,8 @@ impl TerminalStateGuard { saved_state, is_raw_mode_active, _needs_cleanup: false, + pending_input: Vec::new(), + screen_mode_tracker: ScreenModeTracker::new(None), }) } @@ -159,6 +166,26 @@ impl TerminalStateGuard { &self.saved_state } + /// Take bytes read during the pre-input terminal query that were not replies. + pub(crate) fn take_pending_input(&mut self) -> Vec { + std::mem::take(&mut self.pending_input) + } + + /// Track remote screen-buffer selection so cleanup avoids disrupting an + /// outer TUI that remains on its alternate screen. + pub(crate) fn observe_remote_output(&mut self, bytes: &[u8]) { + if let Some(observation) = self.screen_mode_tracker.observe(bytes) { + self.saved_state + .protocol_state + .set_screen_observation(Some(observation.current), observation.changed); + CURRENT_ALTERNATE_SCREEN.store( + encode_screen_state(Some(observation.current)), + Ordering::SeqCst, + ); + SCREEN_CHANGED.store(observation.changed, Ordering::SeqCst); + } + } + /// Save current terminal state fn save_terminal_state() -> Result { let size = if let Some((terminal_size::Width(w), terminal_size::Height(h))) = @@ -176,6 +203,7 @@ impl TerminalStateGuard { size, was_alternate_screen: false, was_mouse_enabled: false, + protocol_state: ProtocolState::default(), }) } @@ -186,7 +214,12 @@ impl TerminalStateGuard { // Reset every terminal mode owned by the remote PTY through the same path // used by panic and last-resort cleanup. - write_terminal_cleanup(&mut std::io::stdout()); + write_terminal_cleanup(&mut std::io::stdout(), &self.saved_state.protocol_state); + CURRENT_ALTERNATE_SCREEN.store( + encode_screen_state(self.saved_state.protocol_state.initial_alternate_screen()), + Ordering::SeqCst, + ); + SCREEN_CHANGED.store(false, Ordering::SeqCst); // Exit raw mode if it's globally active if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { @@ -241,7 +274,19 @@ pub fn force_terminal_cleanup() { // operations below are individually safe. let _guard = TERMINAL_MUTEX.try_lock().ok(); - write_terminal_cleanup(&mut std::io::stdout()); + let state = SAVED_PROTOCOL_STATE + .try_lock() + .ok() + .and_then(|saved| saved.clone()) + .map(|mut state| { + state.set_screen_observation( + decode_screen_state(CURRENT_ALTERNATE_SCREEN.load(Ordering::SeqCst)), + SCREEN_CHANGED.load(Ordering::SeqCst), + ); + state + }) + .unwrap_or_default(); + write_terminal_cleanup(&mut std::io::stdout(), &state); if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { let _ = disable_raw_mode(); @@ -249,6 +294,22 @@ pub fn force_terminal_cleanup() { } } +fn encode_screen_state(state: Option) -> i8 { + match state { + Some(false) => 0, + Some(true) => 1, + None => -1, + } +} + +fn decode_screen_state(state: i8) -> Option { + match state { + 0 => Some(false), + 1 => Some(true), + _ => None, + } +} + /// Terminal operations for PTY sessions pub struct TerminalOps; @@ -350,65 +411,16 @@ mod tests { // must be carefully ordered or marked #[serial]. // // What *can* be reliably tested here: - // 1. Exact cleanup bytes and ordering through an injectable writer. - // 2. Idempotency: calling force_terminal_cleanup() twice does not panic. - // 3. Poisoned-mutex resilience: the `try_lock().ok()` pattern correctly + // 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. - // 4. Held-mutex resilience: `try_lock()` returns WouldBlock (not a + // 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. // ----------------------------------------------------------------------- - #[test] - fn test_terminal_cleanup_writes_exact_mode_reset_sequence() { - let mut output = Vec::new(); - - write_terminal_cleanup(&mut output); - - assert_eq!( - output, - b"\x1b[?2004l\ - \x1b[<999u\x1b[=0u\x1b[>4;0m\ - \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ - \x1b[?1049l\ - \x1b[<999u\x1b[=0u\x1b[>4;0m\ - \x1b[?25h" - ); - } - - #[test] - fn test_terminal_cleanup_writer_is_idempotent() { - let mut output = Vec::new(); - - write_terminal_cleanup(&mut output); - write_terminal_cleanup(&mut output); - - assert_eq!(output.len(), TERMINAL_CLEANUP_SEQUENCE.len() * 2); - assert_eq!( - &output[..TERMINAL_CLEANUP_SEQUENCE.len()], - &output[TERMINAL_CLEANUP_SEQUENCE.len()..] - ); - } - - #[test] - fn test_terminal_cleanup_ignores_writer_failures() { - struct FailingWriter; - - impl Write for FailingWriter { - fn write(&mut self, _buffer: &[u8]) -> std::io::Result { - Err(std::io::Error::other("intentional write failure")) - } - - fn flush(&mut self) -> std::io::Result<()> { - Err(std::io::Error::other("intentional flush failure")) - } - } - - write_terminal_cleanup(&mut FailingWriter); - } - /// Calling force_terminal_cleanup() twice in succession must not panic. /// /// In a non-TTY test environment RAW_MODE_ACTIVE is false (no test calls diff --git a/src/pty/terminal_protocol.rs b/src/pty/terminal_protocol.rs new file mode 100644 index 00000000..f48f8405 --- /dev/null +++ b/src/pty/terminal_protocol.rs @@ -0,0 +1,492 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Bounded capture and restoration of local enhanced-keyboard terminal modes. + +use std::io::Write; +use std::time::{Duration, Instant}; + +use super::session::raw_input::RawInputReader; + +const QUERY_SEQUENCE: &[u8] = b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c"; +const QUERY_TIMEOUT: Duration = Duration::from_millis(100); +const MAX_QUERY_INPUT: usize = 4096; +const MAX_CSI_SEQUENCE: usize = 64; + +const COMMON_RESET_PREFIX: &[u8] = + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l"; +const LEAVE_ALTERNATE_SCREEN: &[u8] = b"\x1b[?1049l"; +const ENTER_ALTERNATE_SCREEN: &[u8] = b"\x1b[?1049h"; +const KEYBOARD_RESET: &[u8] = b"\x1b[<999u\x1b[=0u\x1b[>4;0m"; +const SHOW_CURSOR: &[u8] = b"\x1b[?25h"; + +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub(crate) struct ProtocolState { + /// Raw Kitty progressive-enhancement flags, including flags unknown to crossterm. + kitty_flags: Option, + /// xterm's `modifyOtherKeys` resource value. + modify_other_keys: Option, + /// Whether DEC private mode 1049 was set before bssh took control. + alternate_screen: Option, + /// Last screen selection observed in remote output. + current_alternate_screen: Option, + /// Whether remote output switched away from the initial screen at any point. + screen_changed: bool, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ModifyOtherKeys { + Value(u32), + Disabled, +} + +impl ProtocolState { + pub(crate) fn set_screen_observation( + &mut self, + is_alternate: Option, + screen_changed: bool, + ) { + self.current_alternate_screen = is_alternate; + self.screen_changed = screen_changed; + } + + pub(crate) fn current_alternate_screen(&self) -> Option { + self.current_alternate_screen + } + + pub(crate) fn initial_alternate_screen(&self) -> Option { + self.alternate_screen + } +} + +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct ProtocolCapture { + pub(crate) state: ProtocolState, + pub(crate) pending_input: Vec, +} + +trait QueryInput { + fn poll(&self, timeout: Duration) -> std::io::Result; + fn read(&mut self, buffer: &mut [u8]) -> std::io::Result; +} + +impl QueryInput for RawInputReader { + fn poll(&self, timeout: Duration) -> std::io::Result { + RawInputReader::poll(self, timeout) + } + + fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { + RawInputReader::read(self, buffer) + } +} + +/// Capture terminal protocol state before the PTY input task starts. +/// +/// The primary-device-attributes request is a sentinel: terminals answer it even +/// when they do not implement the preceding optional queries. Bytes that are not +/// strict matches for one of our replies are returned for normal PTY forwarding. +pub(crate) fn capture_protocol_state(writer: &mut impl Write) -> ProtocolCapture { + let mut reader = RawInputReader::new(); + capture_with_io(writer, &mut reader, QUERY_TIMEOUT) +} + +fn capture_with_io( + writer: &mut impl Write, + reader: &mut impl QueryInput, + timeout: Duration, +) -> ProtocolCapture { + if writer + .write_all(QUERY_SEQUENCE) + .and_then(|()| writer.flush()) + .is_err() + { + return ProtocolCapture::default(); + } + + let deadline = Instant::now() + timeout; + let mut input = Vec::new(); + let mut buffer = [0_u8; 256]; + + while input.len() < MAX_QUERY_INPUT { + let Some(remaining) = deadline.checked_duration_since(Instant::now()) else { + break; + }; + match reader.poll(remaining) { + Ok(true) => {} + Ok(false) | Err(_) => break, + } + + let read_limit = buffer.len().min(MAX_QUERY_INPUT - input.len()); + match reader.read(&mut buffer[..read_limit]) { + Ok(0) | Err(_) => break, + Ok(read) => input.extend_from_slice(&buffer[..read]), + } + + if contains_primary_device_attributes(&input) { + break; + } + } + + parse_query_input(&input) +} + +fn contains_primary_device_attributes(input: &[u8]) -> bool { + csi_sequences(input).any(is_primary_device_attributes) +} + +fn parse_query_input(input: &[u8]) -> ProtocolCapture { + let mut capture = ProtocolCapture::default(); + let mut cursor = 0; + + for (start, end) in csi_ranges(input) { + capture + .pending_input + .extend_from_slice(&input[cursor..start]); + let sequence = &input[start..end]; + + if let Some(flags) = parse_decimal_reply(sequence, b"\x1b[?", b"u") { + capture.state.kitty_flags = Some(flags); + } else if let Some(value) = parse_decimal_reply(sequence, b"\x1b[>4;", b"m") { + capture.state.modify_other_keys = Some(ModifyOtherKeys::Value(value)); + } else if sequence == b"\x1b[>4n" { + capture.state.modify_other_keys = Some(ModifyOtherKeys::Disabled); + } else if let Some(mode) = parse_decimal_reply(sequence, b"\x1b[?1049;", b"$y") { + capture.state.alternate_screen = match mode { + 1 | 3 => Some(true), + 2 | 4 => Some(false), + _ => None, + }; + capture.state.current_alternate_screen = capture.state.alternate_screen; + } else if !is_primary_device_attributes(sequence) { + capture.pending_input.extend_from_slice(sequence); + } + + cursor = end; + } + capture.pending_input.extend_from_slice(&input[cursor..]); + capture +} + +fn parse_decimal_reply(sequence: &[u8], prefix: &[u8], suffix: &[u8]) -> Option { + let digits = sequence.strip_prefix(prefix)?.strip_suffix(suffix)?; + if digits.is_empty() || digits.len() > 10 || !digits.iter().all(u8::is_ascii_digit) { + return None; + } + std::str::from_utf8(digits).ok()?.parse().ok() +} + +fn is_primary_device_attributes(sequence: &[u8]) -> bool { + let Some(parameters) = sequence + .strip_prefix(b"\x1b[?") + .and_then(|value| value.strip_suffix(b"c")) + else { + return false; + }; + !parameters.is_empty() + && parameters + .iter() + .all(|byte| byte.is_ascii_digit() || *byte == b';') +} + +fn csi_sequences(input: &[u8]) -> impl Iterator { + csi_ranges(input).map(|(start, end)| &input[start..end]) +} + +fn csi_ranges(input: &[u8]) -> impl Iterator { + let mut offset = 0; + std::iter::from_fn(move || { + while offset + 1 < input.len() { + if input[offset] != b'\x1b' || input[offset + 1] != b'[' { + offset += 1; + continue; + } + + let start = offset; + let limit = input.len().min(start + MAX_CSI_SEQUENCE); + offset += 2; + while offset < limit { + if (0x40..=0x7e).contains(&input[offset]) { + offset += 1; + return Some((start, offset)); + } + offset += 1; + } + } + None + }) +} + +/// Reset remote-owned modes, then restore the captured local keyboard state. +pub(crate) fn write_terminal_cleanup(writer: &mut impl Write, state: &ProtocolState) { + let _ = (|| -> std::io::Result<()> { + writer.write_all(COMMON_RESET_PREFIX)?; + + match ( + state.alternate_screen, + state.current_alternate_screen, + state.screen_changed, + ) { + (Some(true), Some(true), false) | (Some(false), Some(false), false) => { + write_keyboard_restore(writer, state)?; + } + (Some(true), Some(false), _) => { + writer.write_all(ENTER_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + write_keyboard_restore(writer, state)?; + } + (Some(false), Some(true), _) => { + writer.write_all(LEAVE_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + write_keyboard_restore(writer, state)?; + } + (Some(true), Some(true), true) => { + writer.write_all(LEAVE_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + writer.write_all(ENTER_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + write_keyboard_restore(writer, state)?; + } + (Some(false), Some(false), true) => { + writer.write_all(ENTER_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + writer.write_all(LEAVE_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + write_keyboard_restore(writer, state)?; + } + _ => { + writer.write_all(LEAVE_ALTERNATE_SCREEN)?; + writer.write_all(KEYBOARD_RESET)?; + } + } + + writer.write_all(SHOW_CURSOR)?; + writer.flush() + })(); +} + +fn write_keyboard_restore(writer: &mut impl Write, state: &ProtocolState) -> std::io::Result<()> { + if let Some(flags) = state.kitty_flags { + write!(writer, "\x1b[={flags}u")?; + } + if let Some(value) = state.modify_other_keys { + match value { + ModifyOtherKeys::Value(value) => write!(writer, "\x1b[>4;{value}m")?, + ModifyOtherKeys::Disabled => writer.write_all(b"\x1b[>4n")?, + } + } + Ok(()) +} + +#[cfg(test)] +mod tests { + use std::collections::VecDeque; + + use super::*; + + #[derive(Default)] + struct FakeInput { + chunks: VecDeque>, + } + + impl FakeInput { + fn new(chunks: &[&[u8]]) -> Self { + Self { + chunks: chunks.iter().map(|chunk| chunk.to_vec()).collect(), + } + } + } + + impl QueryInput for FakeInput { + fn poll(&self, _timeout: Duration) -> std::io::Result { + Ok(!self.chunks.is_empty()) + } + + fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { + let Some(chunk) = self.chunks.pop_front() else { + return Ok(0); + }; + let read = chunk.len().min(buffer.len()); + buffer[..read].copy_from_slice(&chunk[..read]); + Ok(read) + } + } + + #[test] + fn captures_fragmented_replies_and_preserves_user_input() { + let mut output = Vec::new(); + let mut input = FakeInput::new(&[ + b"typed", + b"\x1b[?3", + b"1u\x1b[>4;2m\x1b[?1049;1$y\x1b[?1;2c", + ]); + + let capture = capture_with_io(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!(output, QUERY_SEQUENCE); + assert_eq!( + capture.state, + ProtocolState { + kitty_flags: Some(31), + modify_other_keys: Some(ModifyOtherKeys::Value(2)), + alternate_screen: Some(true), + current_alternate_screen: Some(true), + screen_changed: false, + } + ); + assert_eq!(capture.pending_input, b"typed"); + } + + #[test] + fn timeout_and_unsupported_terminal_fall_back_without_losing_input() { + let mut output = Vec::new(); + let mut input = FakeInput::new(&[b"abc\x1b[?1;2c"]); + + let capture = capture_with_io(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!(capture.state, ProtocolState::default()); + assert_eq!(capture.pending_input, b"abc"); + } + + #[test] + fn malformed_and_unrelated_sequences_are_preserved() { + let input = b"\x1b[?99999999999u\x1b[?x u\x1b[Aplain"; + + let capture = parse_query_input(input); + + assert_eq!(capture.state, ProtocolState::default()); + assert_eq!(capture.pending_input, input); + } + + #[test] + fn restores_main_screen_flags_exactly() { + let mut output = Vec::new(); + let state = ProtocolState { + kitty_flags: Some(31), + modify_other_keys: Some(ModifyOtherKeys::Value(2)), + alternate_screen: Some(false), + current_alternate_screen: Some(false), + screen_changed: false, + }; + + write_terminal_cleanup(&mut output, &state); + + assert_eq!( + output, + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ + \x1b[=31u\x1b[>4;2m\x1b[?25h" + ); + } + + #[test] + fn preserves_outer_alternate_screen_without_switching_buffers() { + let mut output = Vec::new(); + let state = ProtocolState { + kitty_flags: Some(7), + modify_other_keys: Some(ModifyOtherKeys::Value(1)), + alternate_screen: Some(true), + current_alternate_screen: Some(true), + screen_changed: false, + }; + + write_terminal_cleanup(&mut output, &state); + + assert_eq!( + output, + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ + \x1b[=7u\x1b[>4;1m\x1b[?25h" + ); + } + + #[test] + fn returns_to_outer_alternate_screen_when_remote_left_it() { + let mut output = Vec::new(); + let state = ProtocolState { + kitty_flags: Some(7), + modify_other_keys: Some(ModifyOtherKeys::Value(1)), + alternate_screen: Some(true), + current_alternate_screen: Some(false), + screen_changed: true, + }; + + write_terminal_cleanup(&mut output, &state); + + assert!(output.windows(8).any(|window| window == b"\x1b[?1049h")); + assert!(output.ends_with(b"\x1b[=7u\x1b[>4;1m\x1b[?25h")); + } + + #[test] + fn cleans_both_screens_after_remote_round_trip() { + let mut output = Vec::new(); + let state = ProtocolState { + kitty_flags: Some(7), + modify_other_keys: None, + alternate_screen: Some(true), + current_alternate_screen: Some(true), + screen_changed: true, + }; + + write_terminal_cleanup(&mut output, &state); + + assert!(output.windows(8).any(|window| window == b"\x1b[?1049l")); + assert!(output.windows(8).any(|window| window == b"\x1b[?1049h")); + assert!(output.ends_with(b"\x1b[=7u\x1b[?25h")); + } + + #[test] + fn unknown_screen_state_uses_legacy_baseline() { + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &ProtocolState::default()); + + assert_eq!( + output, + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ + \x1b[?1049l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?25h" + ); + } + + #[test] + fn repeated_cleanup_keeps_restoring_the_captured_state() { + let state = ProtocolState { + kitty_flags: Some(31), + modify_other_keys: Some(ModifyOtherKeys::Value(2)), + alternate_screen: Some(false), + current_alternate_screen: Some(false), + screen_changed: false, + }; + let mut output = Vec::new(); + + for _ in 0..3 { + write_terminal_cleanup(&mut output, &state); + } + + assert_eq!( + output + .windows(b"\x1b[=31u".len()) + .filter(|window| *window == b"\x1b[=31u") + .count(), + 3 + ); + assert_eq!( + output + .windows(b"\x1b[>4;2m".len()) + .filter(|window| *window == b"\x1b[>4;2m") + .count(), + 3 + ); + } +} diff --git a/src/pty/terminal_screen.rs b/src/pty/terminal_screen.rs new file mode 100644 index 00000000..00bb390b --- /dev/null +++ b/src/pty/terminal_screen.rs @@ -0,0 +1,128 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Tracks remote alternate-screen transitions across fragmented PTY output. + +const MAX_CSI_SEQUENCE: usize = 64; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct ScreenObservation { + pub(crate) current: bool, + pub(crate) changed: bool, +} + +#[derive(Debug)] +pub(crate) struct ScreenModeTracker { + pending: Vec, + current: Option, + changed: bool, +} + +impl ScreenModeTracker { + pub(crate) fn new(initial: Option) -> Self { + Self { + pending: Vec::new(), + current: initial, + changed: false, + } + } + + pub(crate) fn observe(&mut self, input: &[u8]) -> Option { + self.pending.extend_from_slice(input); + let mut observed = false; + let mut consumed = 0; + + for (start, end) in csi_ranges(&self.pending) { + if let Some(is_alternate) = parse_screen_selection(&self.pending[start..end]) { + if self.current.is_some_and(|current| current != is_alternate) { + self.changed = true; + } + self.current = Some(is_alternate); + observed = true; + } + consumed = end; + } + + if consumed > 0 { + self.pending.drain(..consumed); + } + if self.pending.len() > MAX_CSI_SEQUENCE { + let keep_from = self.pending.len() - MAX_CSI_SEQUENCE; + self.pending.drain(..keep_from); + } + + observed.then(|| ScreenObservation { + current: self.current.unwrap_or(false), + changed: self.changed, + }) + } +} + +fn parse_screen_selection(sequence: &[u8]) -> Option { + let (parameters, is_alternate) = if let Some(value) = sequence + .strip_prefix(b"\x1b[?") + .and_then(|value| value.strip_suffix(b"h")) + { + (value, true) + } else { + (sequence.strip_prefix(b"\x1b[?")?.strip_suffix(b"l")?, false) + }; + + parameters + .split(|byte| *byte == b';') + .any(|mode| matches!(mode, b"47" | b"1047" | b"1049")) + .then_some(is_alternate) +} + +fn csi_ranges(input: &[u8]) -> impl Iterator { + let mut offset = 0; + std::iter::from_fn(move || { + while offset + 1 < input.len() { + if input[offset] != b'\x1b' || input[offset + 1] != b'[' { + offset += 1; + continue; + } + let start = offset; + let limit = input.len().min(start + MAX_CSI_SEQUENCE); + offset += 2; + while offset < limit { + if (0x40..=0x7e).contains(&input[offset]) { + offset += 1; + return Some((start, offset)); + } + offset += 1; + } + } + None + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tracks_fragmented_transitions_and_round_trips() { + let mut tracker = ScreenModeTracker::new(Some(true)); + + assert_eq!(tracker.observe(b"text\x1b[?10"), None); + assert_eq!( + tracker.observe(b"49l\x1b[?1049h"), + Some(ScreenObservation { + current: true, + changed: true, + }) + ); + } +} From 959d8bd209a977ac62ed6fe55709bda31b10104c Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 16:22:17 +0900 Subject: [PATCH 2/6] fix: preserve cleanup snapshot on setup failure Publish the captured terminal protocol state before bracketed-paste setup can fail, allowing outer forced cleanup to restore it even when guard construction returns an error. Keep the private protocol snapshot on TerminalStateGuard so the public TerminalState struct remains source-compatible. Refs #236 --- src/pty/terminal.rs | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index d5493106..a5f31126 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -49,7 +49,6 @@ pub struct TerminalState { pub was_alternate_screen: bool, /// Whether mouse reporting was enabled pub was_mouse_enabled: bool, - protocol_state: ProtocolState, } impl Default for TerminalState { @@ -59,7 +58,6 @@ impl Default for TerminalState { size: (80, 24), was_alternate_screen: false, was_mouse_enabled: false, - protocol_state: ProtocolState::default(), } } } @@ -75,12 +73,13 @@ pub struct TerminalStateGuard { _needs_cleanup: bool, pending_input: Vec, screen_mode_tracker: ScreenModeTracker, + protocol_state: ProtocolState, } impl TerminalStateGuard { /// Create a new terminal state guard and enter raw mode pub fn new() -> Result { - let mut saved_state = Self::save_terminal_state()?; + let saved_state = Self::save_terminal_state()?; let is_raw_mode_active = Arc::new(AtomicBool::new(false)); // Enter raw mode with global synchronization @@ -94,22 +93,24 @@ impl TerminalStateGuard { // This bounded query runs before the PTY input task exists, so terminal // replies cannot race normal input forwarding. let capture = capture_protocol_state(&mut std::io::stdout()); - saved_state.protocol_state = capture.state.clone(); - - // Enable bracketed paste mode - execute!(std::io::stdout(), EnableBracketedPaste) - .with_context(|| "Failed to enable bracketed paste mode")?; + let protocol_state = capture.state.clone(); + // Publish the snapshot before any later fallible setup so the caller's + // forced cleanup can preserve it if construction returns an error. if let Ok(mut saved) = SAVED_PROTOCOL_STATE.try_lock() { - *saved = Some(saved_state.protocol_state.clone()); + *saved = Some(protocol_state.clone()); } CURRENT_ALTERNATE_SCREEN.store( - encode_screen_state(saved_state.protocol_state.current_alternate_screen()), + encode_screen_state(protocol_state.current_alternate_screen()), Ordering::SeqCst, ); SCREEN_CHANGED.store(false, Ordering::SeqCst); - let screen_mode_tracker = - ScreenModeTracker::new(saved_state.protocol_state.current_alternate_screen()); + + // Enable bracketed paste mode + execute!(std::io::stdout(), EnableBracketedPaste) + .with_context(|| "Failed to enable bracketed paste mode")?; + + let screen_mode_tracker = ScreenModeTracker::new(protocol_state.current_alternate_screen()); Ok(Self { saved_state, @@ -117,6 +118,7 @@ impl TerminalStateGuard { _needs_cleanup: true, pending_input: capture.pending_input, screen_mode_tracker, + protocol_state, }) } @@ -131,6 +133,7 @@ impl TerminalStateGuard { _needs_cleanup: false, pending_input: Vec::new(), screen_mode_tracker: ScreenModeTracker::new(None), + protocol_state: ProtocolState::default(), }) } @@ -175,8 +178,7 @@ impl TerminalStateGuard { /// outer TUI that remains on its alternate screen. pub(crate) fn observe_remote_output(&mut self, bytes: &[u8]) { if let Some(observation) = self.screen_mode_tracker.observe(bytes) { - self.saved_state - .protocol_state + self.protocol_state .set_screen_observation(Some(observation.current), observation.changed); CURRENT_ALTERNATE_SCREEN.store( encode_screen_state(Some(observation.current)), @@ -203,7 +205,6 @@ impl TerminalStateGuard { size, was_alternate_screen: false, was_mouse_enabled: false, - protocol_state: ProtocolState::default(), }) } @@ -214,9 +215,9 @@ impl TerminalStateGuard { // Reset every terminal mode owned by the remote PTY through the same path // used by panic and last-resort cleanup. - write_terminal_cleanup(&mut std::io::stdout(), &self.saved_state.protocol_state); + write_terminal_cleanup(&mut std::io::stdout(), &self.protocol_state); CURRENT_ALTERNATE_SCREEN.store( - encode_screen_state(self.saved_state.protocol_state.initial_alternate_screen()), + encode_screen_state(self.protocol_state.initial_alternate_screen()), Ordering::SeqCst, ); SCREEN_CHANGED.store(false, Ordering::SeqCst); From 685df173ad49058527ceb31a601a40531dc2baa7 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 16:39:14 +0900 Subject: [PATCH 3/6] fix: isolate two-screen keyboard query transactions Trust terminal query replies only before each transaction's first primary DA sentinel, preserve all untrusted bytes, and discard incomplete snapshots. Capture Kitty flags independently on both screen buffers with non-clearing DEC mode 47 transitions, normalize remote input without popping outer stacks, and restore both screens during teardown. Enforce single raw-terminal ownership so concurrent guards cannot race the global snapshot. Refs #236 --- CHANGELOG.md | 2 +- docs/architecture/interactive-mode.md | 2 +- src/pty/mod.rs | 4 +- src/pty/session/session_manager.rs | 9 - src/pty/terminal.rs | 93 ++--- src/pty/terminal_protocol.rs | 382 +++++++------------- src/pty/terminal_protocol_boundary_tests.rs | 46 +++ src/pty/terminal_protocol_restore.rs | 153 ++++++++ src/pty/terminal_screen.rs | 128 ------- 9 files changed, 380 insertions(+), 439 deletions(-) create mode 100644 src/pty/terminal_protocol_boundary_tests.rs create mode 100644 src/pty/terminal_protocol_restore.rs delete mode 100644 src/pty/terminal_screen.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 96a0354a..9d8f5488 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Roughly double bssh-server single-connection SFTP write throughput by fixing channel fragmentation and the serial write path** (#187). Three independent bottlenecks identified in the issue's code audit are addressed. First, `build_russh_config` now advertises a 65535-byte `maximum_packet_size` (the russh cap; library default 32768) and an 8 MiB `window_size` (library default 2 MiB), both configurable via `server.maximum_packet_size` / `server.window_size` (env: `BSSH_MAX_PACKET_SIZE` / `BSSH_WINDOW_SIZE`), so a 256 KiB SFTP write is no longer chopped into 8 CHANNEL_DATA packets that each pay the full cipher/copy/scheduling cost. Second, the SFTP server loop in `bssh-russh-sftp` is restructured from a strict read-process-write-flush cycle into a reader task with a bounded read-ahead queue plus an in-order processor: responses are flushed once per burst instead of once per request, and consecutive queued `SSH_FXP_WRITE` requests to the same handle at sequential offsets are coalesced into a single handler call (bounded by `max_write_coalesce_len`, default 256 KiB) while every merged request id still receives its own status reply, preserving SFTP response ordering and error semantics. The bssh write/read handlers additionally track each open file's cursor and elide the per-chunk `seek` for sequential transfers (append handles are exempt since O_APPEND ignores the cursor). Third, the workspace gains a tuned `[profile.release]` (`lto = "fat"`, `codegen-units = 1`) and documented `RUSTFLAGS` target-CPU guidance for self-builds. Local before/after benchmark (loopback, OpenSSH sftp client, 1 GiB random file, 3-run averages, 20-core Linux box): upload 71 MiB/s to 437 MiB/s (6.2x), download 73 MiB/s to 282 MiB/s (3.9x); an ablation with the code changes but library-default channel sizing lands at 333 / 291 MiB/s, attributing most of the gain to the pipelined write path and the rest of the upload gain to the larger packets. Numbers on the reporter's NIC-limited Xeon will differ; the methodology is documented in the PR. Remaining plan items (per-packet copies inside russh, AES-CTR internals) live in upstream russh since #212 removed the fork and are out of scope here. ### Added -- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh now performs a 100 ms bounded query for raw Kitty progressive-enhancement flags, xterm `modifyOtherKeys`, and DEC private mode 1049, using primary device attributes as the unsupported-terminal sentinel. Strictly matched replies are consumed while every unrelated, malformed, partial, or concurrently typed byte is retained and forwarded through the normal PTY input path. Remote alternate-screen transitions are tracked across fragmented output so teardown restores the captured keyboard settings on the original screen without switching away from an outer TUI that still owns its alternate buffer; failed queries retain #234's ordinary-shell baseline fallback, and the same snapshot is reused by normal, forced, and repeated cleanup paths. +- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value; failed or incomplete queries retain #234's ordinary-shell baseline fallback, the same snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token prevents concurrent raw-terminal guards from overwriting it. - **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. ### Fixed diff --git a/docs/architecture/interactive-mode.md b/docs/architecture/interactive-mode.md index 4782e13d..b7795a05 100644 --- a/docs/architecture/interactive-mode.md +++ b/docs/architecture/interactive-mode.md @@ -193,7 +193,7 @@ Run this check in Ghostty and at least one other Kitty-keyboard-compatible termi 3. At the restored local prompt, verify that Space, Enter, arrow keys, and modified keys behave normally and do not print CSI-u fragments such as `;1:3u`, `:1C`, or `:3C`. 4. Repeat with a Kitty-keyboard-aware full-screen application on the remote host and terminate the SSH transport before the application exits cleanly. -Before starting the PTY input task, bssh spends at most 100 ms querying the active Kitty enhancement flags, xterm `modifyOtherKeys` value, and DEC private mode 1049. A primary-device-attributes query terminates the probe promptly on terminals that do not support the optional protocols. Only strict query replies are consumed; user input and malformed or unrelated escape sequences read during the probe are retained and forwarded to the remote session. Teardown tracks remote screen-buffer transitions and restores the captured keyboard values on the original main or alternate screen. If any required screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234. +Before starting the PTY input task, bssh uses a DA-delimited query transaction of at most 100 ms per screen to capture the Kitty enhancement flags independently for the main and alternate screens, together with the global xterm `modifyOtherKeys` value and DEC private mode 1049. It uses DEC mode 47 to inspect the hidden buffer without the clear/initialize behavior of mode 1049, sets both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, and returns to the original screen before remote input starts. Only strict replies before the first primary DA in each transaction are consumed; post-DA bytes and user input, malformed replies, or unrelated escape sequences are retained in byte order and forwarded to the remote session. Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. If a DA transaction or the screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234 rather than claiming partial preservation. A single-owner RAII token rejects concurrent raw-terminal guards so they cannot race the process-global cleanup snapshot. To validate outer-TUI preservation in Ghostty and Kitty or WezTerm: diff --git a/src/pty/mod.rs b/src/pty/mod.rs index 2f3df64c..8b552ed4 100644 --- a/src/pty/mod.rs +++ b/src/pty/mod.rs @@ -30,7 +30,9 @@ use tokio::time::Duration; pub mod session; pub mod terminal; mod terminal_protocol; -mod terminal_screen; +#[cfg(test)] +mod terminal_protocol_boundary_tests; +mod terminal_protocol_restore; pub use session::PtySession; pub use terminal::{TerminalState, TerminalStateGuard, force_terminal_cleanup}; diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index 4fe61dff..66f4a95c 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -261,9 +261,6 @@ impl PtySession { tracing::error!("Failed to write to stdout: {e}"); should_terminate = true; } else { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(&filtered_data); - } let _ = io::stdout().flush(); } } @@ -277,9 +274,6 @@ impl PtySession { tracing::error!("Failed to write stderr to stdout: {e}"); should_terminate = true; } else { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(&filtered_data); - } let _ = io::stdout().flush(); } } @@ -331,9 +325,6 @@ impl PtySession { tracing::error!("Failed to write to stdout: {e}"); should_terminate = true; } else { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(&filtered_data); - } let _ = io::stdout().flush(); } } diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index a5f31126..e1fe5e0a 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -16,7 +16,7 @@ use std::sync::{ Arc, LazyLock, Mutex, - atomic::{AtomicBool, AtomicI8, Ordering}, + atomic::{AtomicBool, Ordering}, }; use anyhow::{Context, Result}; @@ -27,7 +27,6 @@ use crossterm::{ }; use super::terminal_protocol::{ProtocolState, capture_protocol_state, write_terminal_cleanup}; -use super::terminal_screen::ScreenModeTracker; /// Global terminal cleanup synchronization /// Ensures only one cleanup attempt happens even with multiple guards @@ -35,8 +34,27 @@ static TERMINAL_MUTEX: LazyLock> = LazyLock::new(|| Mutex::new(())); static SAVED_PROTOCOL_STATE: LazyLock>> = LazyLock::new(|| Mutex::new(None)); static RAW_MODE_ACTIVE: AtomicBool = AtomicBool::new(false); -static CURRENT_ALTERNATE_SCREEN: AtomicI8 = AtomicI8::new(-1); -static SCREEN_CHANGED: AtomicBool = AtomicBool::new(false); +static TERMINAL_OWNER_ACTIVE: AtomicBool = AtomicBool::new(false); + +struct TerminalOwner; + +impl TerminalOwner { + fn acquire() -> Result { + if TERMINAL_OWNER_ACTIVE + .compare_exchange(false, true, Ordering::SeqCst, Ordering::SeqCst) + .is_err() + { + anyhow::bail!("Another terminal state guard is already active"); + } + Ok(Self) + } +} + +impl Drop for TerminalOwner { + fn drop(&mut self) { + TERMINAL_OWNER_ACTIVE.store(false, Ordering::SeqCst); + } +} /// Terminal state information that needs to be preserved and restored #[derive(Debug, Clone)] @@ -72,8 +90,8 @@ pub struct TerminalStateGuard { // Simplified cleanup - just track if we need cleanup _needs_cleanup: bool, pending_input: Vec, - screen_mode_tracker: ScreenModeTracker, protocol_state: ProtocolState, + _owner: Option, } impl TerminalStateGuard { @@ -81,6 +99,7 @@ impl TerminalStateGuard { pub fn new() -> Result { let saved_state = Self::save_terminal_state()?; let is_raw_mode_active = Arc::new(AtomicBool::new(false)); + let owner = TerminalOwner::acquire()?; // Enter raw mode with global synchronization let _guard = TERMINAL_MUTEX.lock().unwrap(); @@ -100,25 +119,17 @@ impl TerminalStateGuard { if let Ok(mut saved) = SAVED_PROTOCOL_STATE.try_lock() { *saved = Some(protocol_state.clone()); } - CURRENT_ALTERNATE_SCREEN.store( - encode_screen_state(protocol_state.current_alternate_screen()), - Ordering::SeqCst, - ); - SCREEN_CHANGED.store(false, Ordering::SeqCst); - // Enable bracketed paste mode execute!(std::io::stdout(), EnableBracketedPaste) .with_context(|| "Failed to enable bracketed paste mode")?; - let screen_mode_tracker = ScreenModeTracker::new(protocol_state.current_alternate_screen()); - Ok(Self { saved_state, is_raw_mode_active, _needs_cleanup: true, pending_input: capture.pending_input, - screen_mode_tracker, protocol_state, + _owner: Some(owner), }) } @@ -132,8 +143,8 @@ impl TerminalStateGuard { is_raw_mode_active, _needs_cleanup: false, pending_input: Vec::new(), - screen_mode_tracker: ScreenModeTracker::new(None), protocol_state: ProtocolState::default(), + _owner: None, }) } @@ -174,20 +185,6 @@ impl TerminalStateGuard { std::mem::take(&mut self.pending_input) } - /// Track remote screen-buffer selection so cleanup avoids disrupting an - /// outer TUI that remains on its alternate screen. - pub(crate) fn observe_remote_output(&mut self, bytes: &[u8]) { - if let Some(observation) = self.screen_mode_tracker.observe(bytes) { - self.protocol_state - .set_screen_observation(Some(observation.current), observation.changed); - CURRENT_ALTERNATE_SCREEN.store( - encode_screen_state(Some(observation.current)), - Ordering::SeqCst, - ); - SCREEN_CHANGED.store(observation.changed, Ordering::SeqCst); - } - } - /// Save current terminal state fn save_terminal_state() -> Result { let size = if let Some((terminal_size::Width(w), terminal_size::Height(h))) = @@ -216,11 +213,6 @@ impl TerminalStateGuard { // Reset every terminal mode owned by the remote PTY through the same path // used by panic and last-resort cleanup. write_terminal_cleanup(&mut std::io::stdout(), &self.protocol_state); - CURRENT_ALTERNATE_SCREEN.store( - encode_screen_state(self.protocol_state.initial_alternate_screen()), - Ordering::SeqCst, - ); - SCREEN_CHANGED.store(false, Ordering::SeqCst); // Exit raw mode if it's globally active if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { @@ -279,13 +271,6 @@ pub fn force_terminal_cleanup() { .try_lock() .ok() .and_then(|saved| saved.clone()) - .map(|mut state| { - state.set_screen_observation( - decode_screen_state(CURRENT_ALTERNATE_SCREEN.load(Ordering::SeqCst)), - SCREEN_CHANGED.load(Ordering::SeqCst), - ); - state - }) .unwrap_or_default(); write_terminal_cleanup(&mut std::io::stdout(), &state); @@ -295,22 +280,6 @@ pub fn force_terminal_cleanup() { } } -fn encode_screen_state(state: Option) -> i8 { - match state { - Some(false) => 0, - Some(true) => 1, - None => -1, - } -} - -fn decode_screen_state(state: i8) -> Option { - match state { - 0 => Some(false), - 1 => Some(true), - _ => None, - } -} - /// Terminal operations for PTY sessions pub struct TerminalOps; @@ -483,4 +452,14 @@ mod tests { assert!(!state.was_mouse_enabled); assert_eq!(state.size, (80, 24)); } + + #[test] + fn test_terminal_owner_rejects_nesting_and_releases_on_drop() { + let owner = TerminalOwner::acquire().unwrap(); + assert!(TerminalOwner::acquire().is_err()); + + drop(owner); + + assert!(TerminalOwner::acquire().is_ok()); + } } diff --git a/src/pty/terminal_protocol.rs b/src/pty/terminal_protocol.rs index f48f8405..134017b4 100644 --- a/src/pty/terminal_protocol.rs +++ b/src/pty/terminal_protocol.rs @@ -18,64 +18,51 @@ use std::io::Write; use std::time::{Duration, Instant}; use super::session::raw_input::RawInputReader; +pub(crate) use super::terminal_protocol_restore::write_terminal_cleanup; -const QUERY_SEQUENCE: &[u8] = b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c"; +const INITIAL_QUERY: &[u8] = b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c"; +const HIDDEN_SCREEN_QUERY: &[u8] = b"\x1b[?u\x1b[c"; const QUERY_TIMEOUT: Duration = Duration::from_millis(100); const MAX_QUERY_INPUT: usize = 4096; const MAX_CSI_SEQUENCE: usize = 64; -const COMMON_RESET_PREFIX: &[u8] = - b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l"; -const LEAVE_ALTERNATE_SCREEN: &[u8] = b"\x1b[?1049l"; -const ENTER_ALTERNATE_SCREEN: &[u8] = b"\x1b[?1049h"; -const KEYBOARD_RESET: &[u8] = b"\x1b[<999u\x1b[=0u\x1b[>4;0m"; -const SHOW_CURSOR: &[u8] = b"\x1b[?25h"; +const SELECT_MAIN_SCREEN: &[u8] = b"\x1b[?47l"; +const SELECT_ALTERNATE_SCREEN: &[u8] = b"\x1b[?47h"; +const ENTRY_KEYBOARD_BASELINE: &[u8] = b"\x1b[=0u\x1b[>4;0m"; #[derive(Debug, Clone, Default, PartialEq, Eq)] pub(crate) struct ProtocolState { - /// Raw Kitty progressive-enhancement flags, including flags unknown to crossterm. - kitty_flags: Option, + /// Raw Kitty flags for the main screen, including flags unknown to crossterm. + pub(crate) main_kitty_flags: Option, + /// Raw Kitty flags for the alternate screen. + pub(crate) alternate_kitty_flags: Option, /// xterm's `modifyOtherKeys` resource value. - modify_other_keys: Option, + pub(crate) modify_other_keys: Option, /// Whether DEC private mode 1049 was set before bssh took control. - alternate_screen: Option, - /// Last screen selection observed in remote output. - current_alternate_screen: Option, - /// Whether remote output switched away from the initial screen at any point. - screen_changed: bool, + pub(crate) alternate_screen: Option, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum ModifyOtherKeys { +pub(crate) enum ModifyOtherKeys { Value(u32), Disabled, } -impl ProtocolState { - pub(crate) fn set_screen_observation( - &mut self, - is_alternate: Option, - screen_changed: bool, - ) { - self.current_alternate_screen = is_alternate; - self.screen_changed = screen_changed; - } - - pub(crate) fn current_alternate_screen(&self) -> Option { - self.current_alternate_screen - } - - pub(crate) fn initial_alternate_screen(&self) -> Option { - self.alternate_screen - } -} - #[derive(Debug, Default, PartialEq, Eq)] pub(crate) struct ProtocolCapture { pub(crate) state: ProtocolState, pub(crate) pending_input: Vec, } +#[derive(Debug, Default, PartialEq, Eq)] +pub(crate) struct QueryReplies { + kitty_flags: Option, + modify_other_keys: Option, + pub(crate) alternate_screen: Option, + pub(crate) pending_input: Vec, + pub(crate) completed: bool, +} + trait QueryInput { fn poll(&self, timeout: Duration) -> std::io::Result; fn read(&mut self, buffer: &mut [u8]) -> std::io::Result; @@ -98,20 +85,97 @@ impl QueryInput for RawInputReader { /// strict matches for one of our replies are returned for normal PTY forwarding. pub(crate) fn capture_protocol_state(writer: &mut impl Write) -> ProtocolCapture { let mut reader = RawInputReader::new(); - capture_with_io(writer, &mut reader, QUERY_TIMEOUT) + capture_all_screens(writer, &mut reader, QUERY_TIMEOUT) } -fn capture_with_io( +fn capture_all_screens( writer: &mut impl Write, reader: &mut impl QueryInput, timeout: Duration, ) -> ProtocolCapture { + let first = capture_transaction(writer, reader, timeout, INITIAL_QUERY); + let mut pending_input = first.pending_input; + let Some(initial_alternate) = first.alternate_screen else { + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let _ = writer.flush(); + return ProtocolCapture { + state: ProtocolState::default(), + pending_input, + }; + }; + if !first.completed { + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let _ = writer.flush(); + return ProtocolCapture { + state: ProtocolState::default(), + pending_input, + }; + } + + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let hidden_switch = if initial_alternate { + SELECT_MAIN_SCREEN + } else { + SELECT_ALTERNATE_SCREEN + }; + let return_switch = if initial_alternate { + SELECT_ALTERNATE_SCREEN + } else { + SELECT_MAIN_SCREEN + }; + if writer + .write_all(hidden_switch) + .and_then(|()| writer.flush()) + .is_err() + { + return ProtocolCapture { + state: ProtocolState::default(), + pending_input, + }; + } + + let hidden = capture_transaction(writer, reader, timeout, HIDDEN_SCREEN_QUERY); + pending_input.extend_from_slice(&hidden.pending_input); + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let _ = writer.write_all(return_switch); + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let _ = writer.flush(); + + if !hidden.completed { + return ProtocolCapture { + state: ProtocolState::default(), + pending_input, + }; + } + + let (main_kitty_flags, alternate_kitty_flags) = if initial_alternate { + (hidden.kitty_flags, first.kitty_flags) + } else { + (first.kitty_flags, hidden.kitty_flags) + }; + ProtocolCapture { + state: ProtocolState { + main_kitty_flags, + alternate_kitty_flags, + modify_other_keys: first.modify_other_keys, + alternate_screen: Some(initial_alternate), + }, + pending_input, + } +} + +fn capture_transaction( + writer: &mut impl Write, + reader: &mut impl QueryInput, + timeout: Duration, + query: &[u8], +) -> QueryReplies { if writer - .write_all(QUERY_SEQUENCE) + .write_all(query) .and_then(|()| writer.flush()) .is_err() { - return ProtocolCapture::default(); + return QueryReplies::default(); } let deadline = Instant::now() + timeout; @@ -145,30 +209,40 @@ fn contains_primary_device_attributes(input: &[u8]) -> bool { csi_sequences(input).any(is_primary_device_attributes) } -fn parse_query_input(input: &[u8]) -> ProtocolCapture { - let mut capture = ProtocolCapture::default(); +pub(crate) fn parse_query_input(input: &[u8]) -> QueryReplies { + let Some(sentinel_end) = csi_ranges(input) + .find_map(|(start, end)| is_primary_device_attributes(&input[start..end]).then_some(end)) + else { + return QueryReplies { + pending_input: input.to_vec(), + ..QueryReplies::default() + }; + }; + + let mut capture = QueryReplies::default(); let mut cursor = 0; - for (start, end) in csi_ranges(input) { + for (start, end) in csi_ranges(&input[..sentinel_end]) { capture .pending_input .extend_from_slice(&input[cursor..start]); let sequence = &input[start..end]; if let Some(flags) = parse_decimal_reply(sequence, b"\x1b[?", b"u") { - capture.state.kitty_flags = Some(flags); + capture.kitty_flags = Some(flags); } else if let Some(value) = parse_decimal_reply(sequence, b"\x1b[>4;", b"m") { - capture.state.modify_other_keys = Some(ModifyOtherKeys::Value(value)); + capture.modify_other_keys = Some(ModifyOtherKeys::Value(value)); } else if sequence == b"\x1b[>4n" { - capture.state.modify_other_keys = Some(ModifyOtherKeys::Disabled); + capture.modify_other_keys = Some(ModifyOtherKeys::Disabled); } else if let Some(mode) = parse_decimal_reply(sequence, b"\x1b[?1049;", b"$y") { - capture.state.alternate_screen = match mode { - 1 | 3 => Some(true), - 2 | 4 => Some(false), - _ => None, - }; - capture.state.current_alternate_screen = capture.state.alternate_screen; - } else if !is_primary_device_attributes(sequence) { + match mode { + 1 | 3 => capture.alternate_screen = Some(true), + 2 | 4 => capture.alternate_screen = Some(false), + _ => capture.pending_input.extend_from_slice(sequence), + } + } else if is_primary_device_attributes(sequence) { + capture.completed = true; + } else { capture.pending_input.extend_from_slice(sequence); } @@ -227,67 +301,6 @@ fn csi_ranges(input: &[u8]) -> impl Iterator { }) } -/// Reset remote-owned modes, then restore the captured local keyboard state. -pub(crate) fn write_terminal_cleanup(writer: &mut impl Write, state: &ProtocolState) { - let _ = (|| -> std::io::Result<()> { - writer.write_all(COMMON_RESET_PREFIX)?; - - match ( - state.alternate_screen, - state.current_alternate_screen, - state.screen_changed, - ) { - (Some(true), Some(true), false) | (Some(false), Some(false), false) => { - write_keyboard_restore(writer, state)?; - } - (Some(true), Some(false), _) => { - writer.write_all(ENTER_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - write_keyboard_restore(writer, state)?; - } - (Some(false), Some(true), _) => { - writer.write_all(LEAVE_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - write_keyboard_restore(writer, state)?; - } - (Some(true), Some(true), true) => { - writer.write_all(LEAVE_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - writer.write_all(ENTER_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - write_keyboard_restore(writer, state)?; - } - (Some(false), Some(false), true) => { - writer.write_all(ENTER_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - writer.write_all(LEAVE_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - write_keyboard_restore(writer, state)?; - } - _ => { - writer.write_all(LEAVE_ALTERNATE_SCREEN)?; - writer.write_all(KEYBOARD_RESET)?; - } - } - - writer.write_all(SHOW_CURSOR)?; - writer.flush() - })(); -} - -fn write_keyboard_restore(writer: &mut impl Write, state: &ProtocolState) -> std::io::Result<()> { - if let Some(flags) = state.kitty_flags { - write!(writer, "\x1b[={flags}u")?; - } - if let Some(value) = state.modify_other_keys { - match value { - ModifyOtherKeys::Value(value) => write!(writer, "\x1b[>4;{value}m")?, - ModifyOtherKeys::Disabled => writer.write_all(b"\x1b[>4n")?, - } - } - Ok(()) -} - #[cfg(test)] mod tests { use std::collections::VecDeque; @@ -329,19 +342,26 @@ mod tests { b"typed", b"\x1b[?3", b"1u\x1b[>4;2m\x1b[?1049;1$y\x1b[?1;2c", + b"\x1b[?5u\x1b[?1;2c", ]); - let capture = capture_with_io(&mut output, &mut input, Duration::from_secs(1)); + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); - assert_eq!(output, QUERY_SEQUENCE); + assert_eq!( + output, + b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c\ + \x1b[=0u\x1b[>4;0m\x1b[?47l\ + \x1b[?u\x1b[c\ + \x1b[=0u\x1b[>4;0m\x1b[?47h\ + \x1b[=0u\x1b[>4;0m" + ); assert_eq!( capture.state, ProtocolState { - kitty_flags: Some(31), + main_kitty_flags: Some(5), + alternate_kitty_flags: Some(31), modify_other_keys: Some(ModifyOtherKeys::Value(2)), alternate_screen: Some(true), - current_alternate_screen: Some(true), - screen_changed: false, } ); assert_eq!(capture.pending_input, b"typed"); @@ -352,7 +372,7 @@ mod tests { let mut output = Vec::new(); let mut input = FakeInput::new(&[b"abc\x1b[?1;2c"]); - let capture = capture_with_io(&mut output, &mut input, Duration::from_secs(1)); + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); assert_eq!(capture.state, ProtocolState::default()); assert_eq!(capture.pending_input, b"abc"); @@ -364,129 +384,7 @@ mod tests { let capture = parse_query_input(input); - assert_eq!(capture.state, ProtocolState::default()); + assert!(!capture.completed); assert_eq!(capture.pending_input, input); } - - #[test] - fn restores_main_screen_flags_exactly() { - let mut output = Vec::new(); - let state = ProtocolState { - kitty_flags: Some(31), - modify_other_keys: Some(ModifyOtherKeys::Value(2)), - alternate_screen: Some(false), - current_alternate_screen: Some(false), - screen_changed: false, - }; - - write_terminal_cleanup(&mut output, &state); - - assert_eq!( - output, - b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ - \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ - \x1b[=31u\x1b[>4;2m\x1b[?25h" - ); - } - - #[test] - fn preserves_outer_alternate_screen_without_switching_buffers() { - let mut output = Vec::new(); - let state = ProtocolState { - kitty_flags: Some(7), - modify_other_keys: Some(ModifyOtherKeys::Value(1)), - alternate_screen: Some(true), - current_alternate_screen: Some(true), - screen_changed: false, - }; - - write_terminal_cleanup(&mut output, &state); - - assert_eq!( - output, - b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ - \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ - \x1b[=7u\x1b[>4;1m\x1b[?25h" - ); - } - - #[test] - fn returns_to_outer_alternate_screen_when_remote_left_it() { - let mut output = Vec::new(); - let state = ProtocolState { - kitty_flags: Some(7), - modify_other_keys: Some(ModifyOtherKeys::Value(1)), - alternate_screen: Some(true), - current_alternate_screen: Some(false), - screen_changed: true, - }; - - write_terminal_cleanup(&mut output, &state); - - assert!(output.windows(8).any(|window| window == b"\x1b[?1049h")); - assert!(output.ends_with(b"\x1b[=7u\x1b[>4;1m\x1b[?25h")); - } - - #[test] - fn cleans_both_screens_after_remote_round_trip() { - let mut output = Vec::new(); - let state = ProtocolState { - kitty_flags: Some(7), - modify_other_keys: None, - alternate_screen: Some(true), - current_alternate_screen: Some(true), - screen_changed: true, - }; - - write_terminal_cleanup(&mut output, &state); - - assert!(output.windows(8).any(|window| window == b"\x1b[?1049l")); - assert!(output.windows(8).any(|window| window == b"\x1b[?1049h")); - assert!(output.ends_with(b"\x1b[=7u\x1b[?25h")); - } - - #[test] - fn unknown_screen_state_uses_legacy_baseline() { - let mut output = Vec::new(); - - write_terminal_cleanup(&mut output, &ProtocolState::default()); - - assert_eq!( - output, - b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ - \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ - \x1b[?1049l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?25h" - ); - } - - #[test] - fn repeated_cleanup_keeps_restoring_the_captured_state() { - let state = ProtocolState { - kitty_flags: Some(31), - modify_other_keys: Some(ModifyOtherKeys::Value(2)), - alternate_screen: Some(false), - current_alternate_screen: Some(false), - screen_changed: false, - }; - let mut output = Vec::new(); - - for _ in 0..3 { - write_terminal_cleanup(&mut output, &state); - } - - assert_eq!( - output - .windows(b"\x1b[=31u".len()) - .filter(|window| *window == b"\x1b[=31u") - .count(), - 3 - ); - assert_eq!( - output - .windows(b"\x1b[>4;2m".len()) - .filter(|window| *window == b"\x1b[>4;2m") - .count(), - 3 - ); - } } diff --git a/src/pty/terminal_protocol_boundary_tests.rs b/src/pty/terminal_protocol_boundary_tests.rs new file mode 100644 index 00000000..de86ed15 --- /dev/null +++ b/src/pty/terminal_protocol_boundary_tests.rs @@ -0,0 +1,46 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use super::terminal_protocol::parse_query_input; + +#[test] +fn preserves_reply_shaped_and_user_bytes_after_da_sentinel() { + let input = b"\x1b[?7u\x1b[?1049;2$y\x1b[?1;2c\x1b[?31uuser"; + + let capture = parse_query_input(input); + + assert!(capture.completed); + assert_eq!(capture.pending_input, b"\x1b[?31uuser"); +} + +#[test] +fn missing_da_discards_state_and_preserves_every_captured_byte() { + let input = b"typed\x1b[?31u\x1b[>4;2m\x1b[?1049;1$y"; + + let capture = parse_query_input(input); + + assert!(!capture.completed); + assert_eq!(capture.pending_input, input); +} + +#[test] +fn invalid_decrqm_mode_is_preserved_as_malformed_input() { + let input = b"\x1b[?1049;9$y\x1b[?1;2c"; + + let capture = parse_query_input(input); + + assert!(capture.completed); + assert!(capture.alternate_screen.is_none()); + assert_eq!(capture.pending_input, b"\x1b[?1049;9$y"); +} diff --git a/src/pty/terminal_protocol_restore.rs b/src/pty/terminal_protocol_restore.rs new file mode 100644 index 00000000..cf798127 --- /dev/null +++ b/src/pty/terminal_protocol_restore.rs @@ -0,0 +1,153 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::io::Write; + +use super::terminal_protocol::{ModifyOtherKeys, ProtocolState}; + +const COMMON_RESET_PREFIX: &[u8] = + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l"; +const SELECT_MAIN_SCREEN: &[u8] = b"\x1b[?47l"; +const SELECT_ALTERNATE_SCREEN: &[u8] = b"\x1b[?47h"; +const KEYBOARD_RESET: &[u8] = b"\x1b[<999u\x1b[=0u\x1b[>4;0m"; +const SHOW_CURSOR: &[u8] = b"\x1b[?25h"; + +/// Reset remote-owned modes on both screens and restore both captured states. +pub(crate) fn write_terminal_cleanup(writer: &mut impl Write, state: &ProtocolState) { + let _ = (|| -> std::io::Result<()> { + writer.write_all(COMMON_RESET_PREFIX)?; + let Some(initial_alternate) = state.alternate_screen else { + writer.write_all(b"\x1b[?1049l")?; + writer.write_all(KEYBOARD_RESET)?; + writer.write_all(SHOW_CURSOR)?; + return writer.flush(); + }; + + writer.write_all(screen_select(!initial_alternate))?; + writer.write_all(KEYBOARD_RESET)?; + write_kitty_restore(writer, kitty_flags(state, !initial_alternate))?; + + writer.write_all(screen_select(initial_alternate))?; + writer.write_all(KEYBOARD_RESET)?; + write_kitty_restore(writer, kitty_flags(state, initial_alternate))?; + write_modify_other_keys_restore(writer, state.modify_other_keys)?; + writer.write_all(SHOW_CURSOR)?; + writer.flush() + })(); +} + +fn screen_select(alternate: bool) -> &'static [u8] { + if alternate { + SELECT_ALTERNATE_SCREEN + } else { + SELECT_MAIN_SCREEN + } +} + +fn kitty_flags(state: &ProtocolState, alternate: bool) -> Option { + if alternate { + state.alternate_kitty_flags + } else { + state.main_kitty_flags + } +} + +fn write_kitty_restore(writer: &mut impl Write, flags: Option) -> std::io::Result<()> { + if let Some(flags) = flags { + write!(writer, "\x1b[={flags}u")?; + } + Ok(()) +} + +fn write_modify_other_keys_restore( + writer: &mut impl Write, + value: Option, +) -> std::io::Result<()> { + match value { + Some(ModifyOtherKeys::Value(value)) => write!(writer, "\x1b[>4;{value}m"), + Some(ModifyOtherKeys::Disabled) => writer.write_all(b"\x1b[>4n"), + None => Ok(()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn restores_both_screens_then_returns_to_initial_main() { + let state = ProtocolState { + main_kitty_flags: Some(31), + alternate_kitty_flags: Some(15), + modify_other_keys: Some(ModifyOtherKeys::Value(2)), + alternate_screen: Some(false), + }; + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &state); + + assert_eq!( + output, + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ + \x1b[?47h\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[=15u\ + \x1b[?47l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[=31u\x1b[>4;2m\x1b[?25h" + ); + } + + #[test] + fn restores_both_screens_then_returns_to_initial_alternate() { + let state = ProtocolState { + main_kitty_flags: Some(3), + alternate_kitty_flags: Some(7), + modify_other_keys: Some(ModifyOtherKeys::Value(1)), + alternate_screen: Some(true), + }; + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &state); + + assert_eq!( + output, + b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l\ + \x1b[?47l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[=3u\ + \x1b[?47h\x1b[<999u\x1b[=0u\x1b[>4;0m\ + \x1b[=7u\x1b[>4;1m\x1b[?25h" + ); + } + + #[test] + fn repeated_cleanup_restores_same_effective_state() { + let state = ProtocolState { + main_kitty_flags: Some(31), + alternate_kitty_flags: Some(15), + modify_other_keys: None, + alternate_screen: Some(false), + }; + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &state); + write_terminal_cleanup(&mut output, &state); + + assert_eq!( + output + .windows(b"\x1b[=31u".len()) + .filter(|window| *window == b"\x1b[=31u") + .count(), + 2 + ); + } +} diff --git a/src/pty/terminal_screen.rs b/src/pty/terminal_screen.rs deleted file mode 100644 index 00bb390b..00000000 --- a/src/pty/terminal_screen.rs +++ /dev/null @@ -1,128 +0,0 @@ -// Copyright 2025 Lablup Inc. and Jeongkyu Shin -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -//! Tracks remote alternate-screen transitions across fragmented PTY output. - -const MAX_CSI_SEQUENCE: usize = 64; - -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) struct ScreenObservation { - pub(crate) current: bool, - pub(crate) changed: bool, -} - -#[derive(Debug)] -pub(crate) struct ScreenModeTracker { - pending: Vec, - current: Option, - changed: bool, -} - -impl ScreenModeTracker { - pub(crate) fn new(initial: Option) -> Self { - Self { - pending: Vec::new(), - current: initial, - changed: false, - } - } - - pub(crate) fn observe(&mut self, input: &[u8]) -> Option { - self.pending.extend_from_slice(input); - let mut observed = false; - let mut consumed = 0; - - for (start, end) in csi_ranges(&self.pending) { - if let Some(is_alternate) = parse_screen_selection(&self.pending[start..end]) { - if self.current.is_some_and(|current| current != is_alternate) { - self.changed = true; - } - self.current = Some(is_alternate); - observed = true; - } - consumed = end; - } - - if consumed > 0 { - self.pending.drain(..consumed); - } - if self.pending.len() > MAX_CSI_SEQUENCE { - let keep_from = self.pending.len() - MAX_CSI_SEQUENCE; - self.pending.drain(..keep_from); - } - - observed.then(|| ScreenObservation { - current: self.current.unwrap_or(false), - changed: self.changed, - }) - } -} - -fn parse_screen_selection(sequence: &[u8]) -> Option { - let (parameters, is_alternate) = if let Some(value) = sequence - .strip_prefix(b"\x1b[?") - .and_then(|value| value.strip_suffix(b"h")) - { - (value, true) - } else { - (sequence.strip_prefix(b"\x1b[?")?.strip_suffix(b"l")?, false) - }; - - parameters - .split(|byte| *byte == b';') - .any(|mode| matches!(mode, b"47" | b"1047" | b"1049")) - .then_some(is_alternate) -} - -fn csi_ranges(input: &[u8]) -> impl Iterator { - let mut offset = 0; - std::iter::from_fn(move || { - while offset + 1 < input.len() { - if input[offset] != b'\x1b' || input[offset + 1] != b'[' { - offset += 1; - continue; - } - let start = offset; - let limit = input.len().min(start + MAX_CSI_SEQUENCE); - offset += 2; - while offset < limit { - if (0x40..=0x7e).contains(&input[offset]) { - offset += 1; - return Some((start, offset)); - } - offset += 1; - } - } - None - }) -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn tracks_fragmented_transitions_and_round_trips() { - let mut tracker = ScreenModeTracker::new(Some(true)); - - assert_eq!(tracker.observe(b"text\x1b[?10"), None); - assert_eq!( - tracker.observe(b"49l\x1b[?1049h"), - Some(ScreenObservation { - current: true, - changed: true, - }) - ); - } -} From 49dd539a47c197af693061197c0c034055a32b41 Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 16:55:37 +0900 Subject: [PATCH 4/6] fix: harden terminal protocol state restoration --- CHANGELOG.md | 2 +- docs/architecture/interactive-mode.md | 8 +- src/pty/mod.rs | 1 + src/pty/session/session_manager.rs | 9 ++ src/pty/terminal.rs | 123 +++++++++------ src/pty/terminal_protocol.rs | 111 +++---------- src/pty/terminal_protocol_restore.rs | 66 +++++++- src/pty/terminal_protocol_tests.rs | 218 ++++++++++++++++++++++++++ src/pty/terminal_screen.rs | 107 +++++++++++++ 9 files changed, 499 insertions(+), 146 deletions(-) create mode 100644 src/pty/terminal_protocol_tests.rs create mode 100644 src/pty/terminal_screen.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d8f5488..2ef55b24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Roughly double bssh-server single-connection SFTP write throughput by fixing channel fragmentation and the serial write path** (#187). Three independent bottlenecks identified in the issue's code audit are addressed. First, `build_russh_config` now advertises a 65535-byte `maximum_packet_size` (the russh cap; library default 32768) and an 8 MiB `window_size` (library default 2 MiB), both configurable via `server.maximum_packet_size` / `server.window_size` (env: `BSSH_MAX_PACKET_SIZE` / `BSSH_WINDOW_SIZE`), so a 256 KiB SFTP write is no longer chopped into 8 CHANNEL_DATA packets that each pay the full cipher/copy/scheduling cost. Second, the SFTP server loop in `bssh-russh-sftp` is restructured from a strict read-process-write-flush cycle into a reader task with a bounded read-ahead queue plus an in-order processor: responses are flushed once per burst instead of once per request, and consecutive queued `SSH_FXP_WRITE` requests to the same handle at sequential offsets are coalesced into a single handler call (bounded by `max_write_coalesce_len`, default 256 KiB) while every merged request id still receives its own status reply, preserving SFTP response ordering and error semantics. The bssh write/read handlers additionally track each open file's cursor and elide the per-chunk `seek` for sequential transfers (append handles are exempt since O_APPEND ignores the cursor). Third, the workspace gains a tuned `[profile.release]` (`lto = "fat"`, `codegen-units = 1`) and documented `RUSTFLAGS` target-CPU guidance for self-builds. Local before/after benchmark (loopback, OpenSSH sftp client, 1 GiB random file, 3-run averages, 20-core Linux box): upload 71 MiB/s to 437 MiB/s (6.2x), download 73 MiB/s to 282 MiB/s (3.9x); an ablation with the code changes but library-default channel sizing lands at 333 / 291 MiB/s, attributing most of the gain to the pipelined write path and the rest of the upload gain to the larger packets. Numbers on the reporter's NIC-limited Xeon will differ; the methodology is documented in the PR. Remaining plan items (per-packet copies inside russh, AES-CTR internals) live in upstream russh since #212 removed the fork and are out of scope here. ### Added -- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value; failed or incomplete queries retain #234's ordinary-shell baseline fallback, the same snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token prevents concurrent raw-terminal guards from overwriting it. +- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and the screen initially selected according to DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value. Fragmented remote 1049 transitions are tracked so a session that began on main can safely use `1049l` to restore the remote-saved cursor after a disconnect; when a session began in an alternate screen, cleanup intentionally uses non-clearing mode 47 instead of a destructive `1049h`, because the terminal protocol cannot restore the 1049 bit and its single cursor-save slot without clearing or overwriting outer state. Failed or incomplete queries retain #234's ordinary-shell baseline fallback, hidden-screen setup failures make a best-effort return to the initial screen, the same guaranteed snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token covers both raw and deferred-raw guards. - **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. ### Fixed diff --git a/docs/architecture/interactive-mode.md b/docs/architecture/interactive-mode.md index b7795a05..00442952 100644 --- a/docs/architecture/interactive-mode.md +++ b/docs/architecture/interactive-mode.md @@ -193,14 +193,18 @@ Run this check in Ghostty and at least one other Kitty-keyboard-compatible termi 3. At the restored local prompt, verify that Space, Enter, arrow keys, and modified keys behave normally and do not print CSI-u fragments such as `;1:3u`, `:1C`, or `:3C`. 4. Repeat with a Kitty-keyboard-aware full-screen application on the remote host and terminate the SSH transport before the application exits cleanly. -Before starting the PTY input task, bssh uses a DA-delimited query transaction of at most 100 ms per screen to capture the Kitty enhancement flags independently for the main and alternate screens, together with the global xterm `modifyOtherKeys` value and DEC private mode 1049. It uses DEC mode 47 to inspect the hidden buffer without the clear/initialize behavior of mode 1049, sets both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, and returns to the original screen before remote input starts. Only strict replies before the first primary DA in each transaction are consumed; post-DA bytes and user input, malformed replies, or unrelated escape sequences are retained in byte order and forwarded to the remote session. Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. If a DA transaction or the screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234 rather than claiming partial preservation. A single-owner RAII token rejects concurrent raw-terminal guards so they cannot race the process-global cleanup snapshot. +Before starting the PTY input task, bssh uses a DA-delimited query transaction of at most 100 ms per screen to capture the Kitty enhancement flags independently for the main and alternate screens, together with the global xterm `modifyOtherKeys` value and the screen initially selected according to DEC private mode 1049. It uses DEC mode 47 to inspect the hidden buffer without the clear/initialize behavior of mode 1049, sets both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, and returns to the original screen before remote input starts. Only strict replies before the first primary DA in each transaction are consumed; post-DA bytes and user input, malformed replies, or unrelated escape sequences are retained in byte order and forwarded to the remote session. A failed hidden-screen write or flush makes a best-effort mode-47 return before capture falls back. + +Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. It also tracks fragmented remote `1049h`/`1049l` transitions. If bssh began on main and the connection dies in a remote-owned 1049 alternate screen, cleanup uses `1049l` to restore the remote-saved cursor before restoring both screens' keyboard state. The reverse correction is intentionally non-destructive: if bssh began inside an outer alternate screen and the remote reset 1049, cleanup selects the original buffer with mode 47 instead of forcing `1049h`. Generic terminal protocols expose only one cursor-save slot, and `1049h` clears the alternate buffer, so the 1049 mode bit, saved cursor, and existing outer contents cannot all be reconstructed exactly in that case. Keyboard state and screen contents take priority over claiming an impossible exact mode-bit restoration. + +If a DA transaction or the screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234 rather than claiming partial preservation. The cleanup snapshot is published with poison recovery and reused by normal, forced, and repeated cleanup paths. A single-owner RAII token covers raw and deferred-raw guards alike so they cannot race the process-global snapshot, while a deferred guard that never enters raw mode performs no terminal cleanup when dropped. To validate outer-TUI preservation in Ghostty and Kitty or WezTerm: 1. Start a Kitty-keyboard-aware TUI that can launch a child command while remaining in its alternate screen, and record its active enhancement flags with `printf '\033[?u'`. 2. Launch `bssh` from that TUI, enable different remote flags with `printf '\033[=31u'`, and exit normally. 3. Verify the outer TUI receives the same keyboard encoding it used before launching bssh and that its alternate-screen contents were not cleared by an unnecessary `1049l`/`1049h` round trip. -4. Repeat after abrupt transport loss, local `~.`, and a remote `printf '\033[?1049l'` to exercise both unchanged-screen and screen-recovery cleanup paths. +4. Repeat after abrupt transport loss and local `~.`. From an ordinary main-screen shell, disconnect after remote `printf '\033[?1049h'` and verify bssh safely returns with `1049l`; from an outer alternate-screen TUI, use remote `printf '\033[?1049l'` and verify cleanup returns with mode 47 without clearing the outer buffer. ### Performance Characteristics diff --git a/src/pty/mod.rs b/src/pty/mod.rs index 8b552ed4..fdaec89e 100644 --- a/src/pty/mod.rs +++ b/src/pty/mod.rs @@ -33,6 +33,7 @@ mod terminal_protocol; #[cfg(test)] mod terminal_protocol_boundary_tests; mod terminal_protocol_restore; +mod terminal_screen; pub use session::PtySession; pub use terminal::{TerminalState, TerminalStateGuard, force_terminal_cleanup}; diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index 66f4a95c..a46c9c6e 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -252,6 +252,9 @@ impl PtySession { match msg { Some(ChannelMsg::Data { ref data }) => { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(data); + } // Filter terminal escape sequence responses before display // This prevents raw XTGETTCAP, DA1/DA2/DA3 responses from appearing // on screen when running applications like Neovim @@ -267,6 +270,9 @@ impl PtySession { } Some(ChannelMsg::ExtendedData { ref data, ext }) => { if ext == 1 { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(data); + } // stderr - also filter escape sequences let filtered_data = self.escape_filter.filter(data); if !filtered_data.is_empty() { @@ -316,6 +322,9 @@ impl PtySession { } } Some(PtyMessage::RemoteOutput(data)) => { + if let Some(guard) = self.terminal_guard.as_mut() { + guard.observe_remote_output(&data); + } // Apply escape filter for consistency with SSH channel data // This path may receive data from other sources that could // contain terminal responses that shouldn't be displayed diff --git a/src/pty/terminal.rs b/src/pty/terminal.rs index e1fe5e0a..2106d2f8 100644 --- a/src/pty/terminal.rs +++ b/src/pty/terminal.rs @@ -15,7 +15,7 @@ //! Terminal state management for PTY sessions. use std::sync::{ - Arc, LazyLock, Mutex, + Arc, LazyLock, Mutex, MutexGuard, PoisonError, TryLockError, atomic::{AtomicBool, Ordering}, }; @@ -27,6 +27,7 @@ use crossterm::{ }; use super::terminal_protocol::{ProtocolState, capture_protocol_state, write_terminal_cleanup}; +use super::terminal_screen::ScreenModeTracker; /// Global terminal cleanup synchronization /// Ensures only one cleanup attempt happens even with multiple guards @@ -36,6 +37,14 @@ static SAVED_PROTOCOL_STATE: LazyLock>> = static RAW_MODE_ACTIVE: AtomicBool = AtomicBool::new(false); static TERMINAL_OWNER_ACTIVE: AtomicBool = AtomicBool::new(false); +fn lock_recover(mutex: &Mutex) -> MutexGuard<'_, T> { + mutex.lock().unwrap_or_else(PoisonError::into_inner) +} + +fn publish_protocol_state(state: &ProtocolState) { + *lock_recover(&SAVED_PROTOCOL_STATE) = Some(state.clone()); +} + struct TerminalOwner; impl TerminalOwner { @@ -87,19 +96,20 @@ impl Default for TerminalState { pub struct TerminalStateGuard { saved_state: TerminalState, is_raw_mode_active: Arc, - // Simplified cleanup - just track if we need cleanup - _needs_cleanup: bool, + needs_cleanup: AtomicBool, + protocol_cleanup_needed: bool, pending_input: Vec, protocol_state: ProtocolState, - _owner: Option, + screen_mode_tracker: ScreenModeTracker, + _owner: TerminalOwner, } impl TerminalStateGuard { /// Create a new terminal state guard and enter raw mode pub fn new() -> Result { + let owner = TerminalOwner::acquire()?; let saved_state = Self::save_terminal_state()?; let is_raw_mode_active = Arc::new(AtomicBool::new(false)); - let owner = TerminalOwner::acquire()?; // Enter raw mode with global synchronization let _guard = TERMINAL_MUTEX.lock().unwrap(); @@ -116,9 +126,7 @@ impl TerminalStateGuard { // Publish the snapshot before any later fallible setup so the caller's // forced cleanup can preserve it if construction returns an error. - if let Ok(mut saved) = SAVED_PROTOCOL_STATE.try_lock() { - *saved = Some(protocol_state.clone()); - } + publish_protocol_state(&protocol_state); // Enable bracketed paste mode execute!(std::io::stdout(), EnableBracketedPaste) .with_context(|| "Failed to enable bracketed paste mode")?; @@ -126,25 +134,30 @@ impl TerminalStateGuard { Ok(Self { saved_state, is_raw_mode_active, - _needs_cleanup: true, + needs_cleanup: AtomicBool::new(true), + protocol_cleanup_needed: true, pending_input: capture.pending_input, protocol_state, - _owner: Some(owner), + screen_mode_tracker: ScreenModeTracker::new(), + _owner: owner, }) } /// Create a terminal state guard without entering raw mode pub fn new_without_raw_mode() -> Result { + let owner = TerminalOwner::acquire()?; let saved_state = Self::save_terminal_state()?; let is_raw_mode_active = Arc::new(AtomicBool::new(false)); Ok(Self { saved_state, is_raw_mode_active, - _needs_cleanup: false, + needs_cleanup: AtomicBool::new(false), + protocol_cleanup_needed: false, pending_input: Vec::new(), protocol_state: ProtocolState::default(), - _owner: None, + screen_mode_tracker: ScreenModeTracker::new(), + _owner: owner, }) } @@ -156,6 +169,7 @@ impl TerminalStateGuard { RAW_MODE_ACTIVE.store(true, Ordering::SeqCst); self.is_raw_mode_active.store(true, Ordering::Relaxed); } + self.needs_cleanup.store(true, Ordering::Release); Ok(()) } @@ -167,6 +181,9 @@ impl TerminalStateGuard { RAW_MODE_ACTIVE.store(false, Ordering::SeqCst); self.is_raw_mode_active.store(false, Ordering::Relaxed); } + if !self.protocol_cleanup_needed { + self.needs_cleanup.store(false, Ordering::Release); + } Ok(()) } @@ -185,6 +202,15 @@ impl TerminalStateGuard { std::mem::take(&mut self.pending_input) } + /// Observe remote DEC 1049 transitions so safe teardown can leave a + /// remote-owned alternate screen when bssh originally started on main. + pub(crate) fn observe_remote_output(&mut self, output: &[u8]) { + if let Some(current) = self.screen_mode_tracker.observe(output) { + self.protocol_state.current_1049 = Some(current); + publish_protocol_state(&self.protocol_state); + } + } + /// Save current terminal state fn save_terminal_state() -> Result { let size = if let Some((terminal_size::Width(w), terminal_size::Height(h))) = @@ -212,7 +238,9 @@ impl TerminalStateGuard { // Reset every terminal mode owned by the remote PTY through the same path // used by panic and last-resort cleanup. - write_terminal_cleanup(&mut std::io::stdout(), &self.protocol_state); + if self.protocol_cleanup_needed { + write_terminal_cleanup(&mut std::io::stdout(), &self.protocol_state); + } // Exit raw mode if it's globally active if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { @@ -227,6 +255,7 @@ impl TerminalStateGuard { if self.is_raw_mode_active.load(Ordering::Relaxed) { self.is_raw_mode_active.store(false, Ordering::Relaxed); } + self.needs_cleanup.store(false, Ordering::Release); Ok(()) } @@ -234,7 +263,9 @@ impl TerminalStateGuard { impl Drop for TerminalStateGuard { fn drop(&mut self) { - if let Err(e) = self.restore_terminal_state() { + if self.needs_cleanup.load(Ordering::Acquire) + && let Err(e) = self.restore_terminal_state() + { eprintln!("Warning: Failed to restore terminal state: {e}"); } } @@ -267,11 +298,12 @@ pub fn force_terminal_cleanup() { // operations below are individually safe. let _guard = TERMINAL_MUTEX.try_lock().ok(); - let state = SAVED_PROTOCOL_STATE - .try_lock() - .ok() - .and_then(|saved| saved.clone()) - .unwrap_or_default(); + let state = match SAVED_PROTOCOL_STATE.try_lock() { + Ok(saved) => saved.clone(), + Err(TryLockError::Poisoned(error)) => error.into_inner().clone(), + Err(TryLockError::WouldBlock) => None, + } + .unwrap_or_default(); write_terminal_cleanup(&mut std::io::stdout(), &state); if RAW_MODE_ACTIVE.load(Ordering::SeqCst) { @@ -366,30 +398,8 @@ impl TerminalOps { 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. - // ----------------------------------------------------------------------- + // TTY mutations are not observable under the test harness, so these tests + // cover idempotency and synchronization behavior without enabling raw mode. /// Calling force_terminal_cleanup() twice in succession must not panic. /// @@ -454,12 +464,31 @@ mod tests { } #[test] - fn test_terminal_owner_rejects_nesting_and_releases_on_drop() { - let owner = TerminalOwner::acquire().unwrap(); - assert!(TerminalOwner::acquire().is_err()); + #[serial_test::serial(terminal_owner)] + fn test_terminal_owner_applies_to_non_raw_guard_and_releases_on_drop() { + let guard = TerminalStateGuard::new_without_raw_mode().unwrap(); + assert!(!guard.needs_cleanup.load(Ordering::Acquire)); + assert!(TerminalStateGuard::new_without_raw_mode().is_err()); - drop(owner); + drop(guard); assert!(TerminalOwner::acquire().is_ok()); } + + #[test] + fn test_lock_recover_allows_guaranteed_publication_after_poison() { + let saved = Mutex::new(None); + let _ = std::panic::catch_unwind(|| { + let _guard = saved.lock().unwrap(); + panic!("intentional poison"); + }); + let expected = ProtocolState { + main_kitty_flags: Some(7), + ..ProtocolState::default() + }; + + *lock_recover(&saved) = Some(expected.clone()); + + assert_eq!(*lock_recover(&saved), Some(expected)); + } } diff --git a/src/pty/terminal_protocol.rs b/src/pty/terminal_protocol.rs index 134017b4..ac8d6f34 100644 --- a/src/pty/terminal_protocol.rs +++ b/src/pty/terminal_protocol.rs @@ -38,8 +38,13 @@ pub(crate) struct ProtocolState { pub(crate) alternate_kitty_flags: Option, /// xterm's `modifyOtherKeys` resource value. pub(crate) modify_other_keys: Option, - /// Whether DEC private mode 1049 was set before bssh took control. - pub(crate) alternate_screen: Option, + /// Screen selected when bssh took control, as reported by DEC mode 1049. + /// + /// This is a buffer selector, not a promise that the 1049 mode bit and its + /// single cursor-save slot can both be restored without clearing content. + pub(crate) initial_screen_alternate: Option, + /// Last observed DECSET/DECRST 1049 state from the remote session. + pub(crate) current_1049: Option, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -128,6 +133,7 @@ fn capture_all_screens( .and_then(|()| writer.flush()) .is_err() { + return_to_initial_screen(writer, return_switch); return ProtocolCapture { state: ProtocolState::default(), pending_input, @@ -137,9 +143,7 @@ fn capture_all_screens( let hidden = capture_transaction(writer, reader, timeout, HIDDEN_SCREEN_QUERY); pending_input.extend_from_slice(&hidden.pending_input); let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); - let _ = writer.write_all(return_switch); - let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); - let _ = writer.flush(); + return_to_initial_screen(writer, return_switch); if !hidden.completed { return ProtocolCapture { @@ -158,12 +162,19 @@ fn capture_all_screens( main_kitty_flags, alternate_kitty_flags, modify_other_keys: first.modify_other_keys, - alternate_screen: Some(initial_alternate), + initial_screen_alternate: Some(initial_alternate), + current_1049: Some(initial_alternate), }, pending_input, } } +fn return_to_initial_screen(writer: &mut impl Write, return_switch: &[u8]) { + let _ = writer.write_all(return_switch); + let _ = writer.write_all(ENTRY_KEYBOARD_BASELINE); + let _ = writer.flush(); +} + fn capture_transaction( writer: &mut impl Write, reader: &mut impl QueryInput, @@ -302,89 +313,5 @@ fn csi_ranges(input: &[u8]) -> impl Iterator { } #[cfg(test)] -mod tests { - use std::collections::VecDeque; - - use super::*; - - #[derive(Default)] - struct FakeInput { - chunks: VecDeque>, - } - - impl FakeInput { - fn new(chunks: &[&[u8]]) -> Self { - Self { - chunks: chunks.iter().map(|chunk| chunk.to_vec()).collect(), - } - } - } - - impl QueryInput for FakeInput { - fn poll(&self, _timeout: Duration) -> std::io::Result { - Ok(!self.chunks.is_empty()) - } - - fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { - let Some(chunk) = self.chunks.pop_front() else { - return Ok(0); - }; - let read = chunk.len().min(buffer.len()); - buffer[..read].copy_from_slice(&chunk[..read]); - Ok(read) - } - } - - #[test] - fn captures_fragmented_replies_and_preserves_user_input() { - let mut output = Vec::new(); - let mut input = FakeInput::new(&[ - b"typed", - b"\x1b[?3", - b"1u\x1b[>4;2m\x1b[?1049;1$y\x1b[?1;2c", - b"\x1b[?5u\x1b[?1;2c", - ]); - - let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); - - assert_eq!( - output, - b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c\ - \x1b[=0u\x1b[>4;0m\x1b[?47l\ - \x1b[?u\x1b[c\ - \x1b[=0u\x1b[>4;0m\x1b[?47h\ - \x1b[=0u\x1b[>4;0m" - ); - assert_eq!( - capture.state, - ProtocolState { - main_kitty_flags: Some(5), - alternate_kitty_flags: Some(31), - modify_other_keys: Some(ModifyOtherKeys::Value(2)), - alternate_screen: Some(true), - } - ); - assert_eq!(capture.pending_input, b"typed"); - } - - #[test] - fn timeout_and_unsupported_terminal_fall_back_without_losing_input() { - let mut output = Vec::new(); - let mut input = FakeInput::new(&[b"abc\x1b[?1;2c"]); - - let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); - - assert_eq!(capture.state, ProtocolState::default()); - assert_eq!(capture.pending_input, b"abc"); - } - - #[test] - fn malformed_and_unrelated_sequences_are_preserved() { - let input = b"\x1b[?99999999999u\x1b[?x u\x1b[Aplain"; - - let capture = parse_query_input(input); - - assert!(!capture.completed); - assert_eq!(capture.pending_input, input); - } -} +#[path = "terminal_protocol_tests.rs"] +mod tests; diff --git a/src/pty/terminal_protocol_restore.rs b/src/pty/terminal_protocol_restore.rs index cf798127..c826d352 100644 --- a/src/pty/terminal_protocol_restore.rs +++ b/src/pty/terminal_protocol_restore.rs @@ -20,6 +20,7 @@ const COMMON_RESET_PREFIX: &[u8] = b"\x1b[?2004l\x1b[<999u\x1b[=0u\x1b[>4;0m\x1b[?1000l\x1b[?1002l\x1b[?1003l\x1b[?1006l\x1b[?1015l"; const SELECT_MAIN_SCREEN: &[u8] = b"\x1b[?47l"; const SELECT_ALTERNATE_SCREEN: &[u8] = b"\x1b[?47h"; +const LEAVE_1049_SCREEN: &[u8] = b"\x1b[?1049l"; const KEYBOARD_RESET: &[u8] = b"\x1b[<999u\x1b[=0u\x1b[>4;0m"; const SHOW_CURSOR: &[u8] = b"\x1b[?25h"; @@ -27,13 +28,26 @@ const SHOW_CURSOR: &[u8] = b"\x1b[?25h"; pub(crate) fn write_terminal_cleanup(writer: &mut impl Write, state: &ProtocolState) { let _ = (|| -> std::io::Result<()> { writer.write_all(COMMON_RESET_PREFIX)?; - let Some(initial_alternate) = state.alternate_screen else { + let Some(initial_alternate) = state.initial_screen_alternate else { writer.write_all(b"\x1b[?1049l")?; writer.write_all(KEYBOARD_RESET)?; writer.write_all(SHOW_CURSOR)?; return writer.flush(); }; + // If bssh started on the main screen and the remote died inside a + // 1049 alternate screen, DECRST 1049 is both safe and useful: it + // restores the main screen and the cursor saved by the remote. + // + // The inverse is deliberately not attempted. DECSET 1049 clears the + // alternate buffer and overwrites the terminal's single saved-cursor + // slot, so forcing it when bssh started in an alternate screen would + // destroy exactly the outer state this cleanup is preserving. Mode 47 + // below selects that buffer without clearing it. + if !initial_alternate && state.current_1049 == Some(true) { + writer.write_all(LEAVE_1049_SCREEN)?; + } + writer.write_all(screen_select(!initial_alternate))?; writer.write_all(KEYBOARD_RESET)?; write_kitty_restore(writer, kitty_flags(state, !initial_alternate))?; @@ -91,7 +105,8 @@ mod tests { main_kitty_flags: Some(31), alternate_kitty_flags: Some(15), modify_other_keys: Some(ModifyOtherKeys::Value(2)), - alternate_screen: Some(false), + initial_screen_alternate: Some(false), + current_1049: Some(false), }; let mut output = Vec::new(); @@ -113,7 +128,8 @@ mod tests { main_kitty_flags: Some(3), alternate_kitty_flags: Some(7), modify_other_keys: Some(ModifyOtherKeys::Value(1)), - alternate_screen: Some(true), + initial_screen_alternate: Some(true), + current_1049: Some(true), }; let mut output = Vec::new(); @@ -135,7 +151,8 @@ mod tests { main_kitty_flags: Some(31), alternate_kitty_flags: Some(15), modify_other_keys: None, - alternate_screen: Some(false), + initial_screen_alternate: Some(false), + current_1049: Some(false), }; let mut output = Vec::new(); @@ -150,4 +167,45 @@ mod tests { 2 ); } + + #[test] + fn leaves_remote_1049_screen_when_initial_screen_was_main() { + let state = ProtocolState { + main_kitty_flags: Some(3), + alternate_kitty_flags: Some(7), + modify_other_keys: None, + initial_screen_alternate: Some(false), + current_1049: Some(true), + }; + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &state); + + let leave = output + .windows(LEAVE_1049_SCREEN.len()) + .position(|window| window == LEAVE_1049_SCREEN) + .expect("cleanup should leave the remote's 1049 screen"); + let select_hidden = output + .windows(SELECT_ALTERNATE_SCREEN.len()) + .position(|window| window == SELECT_ALTERNATE_SCREEN) + .expect("cleanup should still restore hidden-screen keyboard state"); + assert!(leave < select_hidden); + } + + #[test] + fn does_not_clear_outer_alternate_screen_to_force_1049_bit() { + let state = ProtocolState { + main_kitty_flags: Some(3), + alternate_kitty_flags: Some(7), + modify_other_keys: None, + initial_screen_alternate: Some(true), + current_1049: Some(false), + }; + let mut output = Vec::new(); + + write_terminal_cleanup(&mut output, &state); + + assert!(!output.windows(8).any(|window| window == b"\x1b[?1049h")); + assert!(output.ends_with(b"\x1b[?25h")); + } } diff --git a/src/pty/terminal_protocol_tests.rs b/src/pty/terminal_protocol_tests.rs new file mode 100644 index 00000000..e08af2ef --- /dev/null +++ b/src/pty/terminal_protocol_tests.rs @@ -0,0 +1,218 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use std::{collections::VecDeque, io::Write}; + +use super::*; + +#[derive(Default)] +struct FakeInput { + chunks: VecDeque>, +} + +impl FakeInput { + fn new(chunks: &[&[u8]]) -> Self { + Self { + chunks: chunks.iter().map(|chunk| chunk.to_vec()).collect(), + } + } +} + +impl QueryInput for FakeInput { + fn poll(&self, _timeout: Duration) -> std::io::Result { + Ok(!self.chunks.is_empty()) + } + + fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { + let Some(chunk) = self.chunks.pop_front() else { + return Ok(0); + }; + let read = chunk.len().min(buffer.len()); + buffer[..read].copy_from_slice(&chunk[..read]); + Ok(read) + } +} + +struct FailOnceWriter { + output: Vec, + writes: usize, + fail_at: usize, +} + +impl Write for FailOnceWriter { + fn write(&mut self, buffer: &[u8]) -> std::io::Result { + self.writes += 1; + if self.writes == self.fail_at { + return Err(std::io::Error::other("injected write failure")); + } + self.output.extend_from_slice(buffer); + Ok(buffer.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + Ok(()) + } +} + +struct FailOnceFlushWriter { + output: Vec, + flushes: usize, + fail_at: usize, +} + +impl Write for FailOnceFlushWriter { + fn write(&mut self, buffer: &[u8]) -> std::io::Result { + self.output.extend_from_slice(buffer); + Ok(buffer.len()) + } + + fn flush(&mut self) -> std::io::Result<()> { + self.flushes += 1; + if self.flushes == self.fail_at { + return Err(std::io::Error::other("injected flush failure")); + } + Ok(()) + } +} + +struct NoReplyInput; + +impl QueryInput for NoReplyInput { + fn poll(&self, timeout: Duration) -> std::io::Result { + std::thread::sleep(timeout); + Ok(false) + } + + fn read(&mut self, _buffer: &mut [u8]) -> std::io::Result { + unreachable!("poll never reports readable input") + } +} + +#[test] +fn captures_fragmented_replies_and_preserves_user_input() { + let mut output = Vec::new(); + let mut input = FakeInput::new(&[ + b"typed", + b"\x1b[?3", + b"1u\x1b[>4;2m\x1b[?1049;1$y\x1b[?1;2c", + b"\x1b[?5u\x1b[?1;2c", + ]); + + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!( + output, + b"\x1b[?u\x1b[?4m\x1b[?1049$p\x1b[c\ + \x1b[=0u\x1b[>4;0m\x1b[?47l\ + \x1b[?u\x1b[c\ + \x1b[=0u\x1b[>4;0m\x1b[?47h\ + \x1b[=0u\x1b[>4;0m" + ); + assert_eq!( + capture.state, + ProtocolState { + main_kitty_flags: Some(5), + alternate_kitty_flags: Some(31), + modify_other_keys: Some(ModifyOtherKeys::Value(2)), + initial_screen_alternate: Some(true), + current_1049: Some(true), + } + ); + assert_eq!(capture.pending_input, b"typed"); +} + +#[test] +fn timeout_and_unsupported_terminal_fall_back_without_losing_input() { + let mut output = Vec::new(); + let mut input = FakeInput::new(&[b"abc\x1b[?1;2c"]); + + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!(capture.state, ProtocolState::default()); + assert_eq!(capture.pending_input, b"abc"); +} + +#[test] +fn malformed_and_unrelated_sequences_are_preserved() { + let input = b"\x1b[?99999999999u\x1b[?x u\x1b[Aplain"; + + let capture = parse_query_input(input); + + assert!(!capture.completed); + assert_eq!(capture.pending_input, input); +} + +#[test] +fn hidden_switch_write_failure_best_effort_returns_to_initial_screen() { + let mut output = FailOnceWriter { + output: Vec::new(), + writes: 0, + fail_at: 3, + }; + let mut input = FakeInput::new(&[b"\x1b[?7u\x1b[?1049;2$y\x1b[?1;2c"]); + + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!(capture.state, ProtocolState::default()); + assert!(output.output.ends_with(b"\x1b[?47l\x1b[=0u\x1b[>4;0m")); +} + +#[test] +fn hidden_switch_flush_failure_best_effort_returns_to_initial_screen() { + let mut output = FailOnceFlushWriter { + output: Vec::new(), + flushes: 0, + fail_at: 3, + }; + let mut input = FakeInput::new(&[b"\x1b[?7u\x1b[?1049;2$y\x1b[?1;2c"]); + + let capture = capture_all_screens(&mut output, &mut input, Duration::from_secs(1)); + + assert_eq!(capture.state, ProtocolState::default()); + assert!(output.output.ends_with(b"\x1b[?47l\x1b[=0u\x1b[>4;0m")); +} + +#[test] +fn no_da_reply_waits_for_the_bounded_timeout() { + let timeout = Duration::from_millis(15); + let started = Instant::now(); + let mut output = Vec::new(); + let mut input = NoReplyInput; + + let capture = capture_all_screens(&mut output, &mut input, timeout); + + assert!(started.elapsed() >= Duration::from_millis(10)); + assert_eq!(capture.state, ProtocolState::default()); + assert!(capture.pending_input.is_empty()); +} + +#[test] +fn captures_xtqmodkeys_disabled_reply() { + let capture = parse_query_input(b"\x1b[>4n\x1b[?1;2c"); + + assert!(capture.completed); + assert_eq!(capture.modify_other_keys, Some(ModifyOtherKeys::Disabled)); + assert!(capture.pending_input.is_empty()); +} + +#[test] +fn accepts_permanently_set_and_reset_decrqm_statuses() { + let set = parse_query_input(b"\x1b[?1049;3$y\x1b[?1;2c"); + let reset = parse_query_input(b"\x1b[?1049;4$y\x1b[?1;2c"); + + assert_eq!(set.alternate_screen, Some(true)); + assert_eq!(reset.alternate_screen, Some(false)); + assert!(set.pending_input.is_empty()); + assert!(reset.pending_input.is_empty()); +} diff --git a/src/pty/terminal_screen.rs b/src/pty/terminal_screen.rs new file mode 100644 index 00000000..ecfe130e --- /dev/null +++ b/src/pty/terminal_screen.rs @@ -0,0 +1,107 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Tracks remote DECSET/DECRST 1049 transitions across fragmented PTY output. + +const MAX_CSI_SEQUENCE: usize = 64; + +#[derive(Debug)] +pub(crate) struct ScreenModeTracker { + pending: Vec, +} + +impl ScreenModeTracker { + pub(crate) fn new() -> Self { + Self { + pending: Vec::new(), + } + } + + /// Return the last DEC 1049 state explicitly selected by this output. + pub(crate) fn observe(&mut self, input: &[u8]) -> Option { + self.pending.extend_from_slice(input); + let mut current = None; + let mut consumed = 0; + + for (start, end) in csi_ranges(&self.pending) { + if let Some(is_set) = parse_1049_selection(&self.pending[start..end]) { + current = Some(is_set); + } + consumed = end; + } + + if consumed > 0 { + self.pending.drain(..consumed); + } + if self.pending.len() > MAX_CSI_SEQUENCE { + let keep_from = self.pending.len() - MAX_CSI_SEQUENCE; + self.pending.drain(..keep_from); + } + + current + } +} + +fn parse_1049_selection(sequence: &[u8]) -> Option { + let (parameters, is_set) = if let Some(value) = sequence + .strip_prefix(b"\x1b[?") + .and_then(|value| value.strip_suffix(b"h")) + { + (value, true) + } else { + (sequence.strip_prefix(b"\x1b[?")?.strip_suffix(b"l")?, false) + }; + + parameters + .split(|byte| *byte == b';') + .any(|mode| mode == b"1049") + .then_some(is_set) +} + +fn csi_ranges(input: &[u8]) -> impl Iterator { + let mut offset = 0; + std::iter::from_fn(move || { + while offset + 1 < input.len() { + if input[offset] != b'\x1b' || input[offset + 1] != b'[' { + offset += 1; + continue; + } + let start = offset; + let limit = input.len().min(start + MAX_CSI_SEQUENCE); + offset += 2; + while offset < limit { + if (0x40..=0x7e).contains(&input[offset]) { + offset += 1; + return Some((start, offset)); + } + offset += 1; + } + } + None + }) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn tracks_fragmented_1049_transitions_and_last_value() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"text\x1b[?10"), None); + assert_eq!(tracker.observe(b"49h\x1b[?25;1049l"), Some(false)); + assert_eq!(tracker.observe(b"\x1b[?47h"), None); + } +} From 354f0514b44ab8f521a2cc6dd1f8ca88000f38ae Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 17:03:58 +0900 Subject: [PATCH 5/6] fix: track only delivered screen transitions --- CHANGELOG.md | 2 +- docs/architecture/interactive-mode.md | 2 +- src/pty/session/mod.rs | 1 + src/pty/session/output_delivery.rs | 110 ++++++++++++++ src/pty/session/session_manager.rs | 73 +++++----- src/pty/terminal_screen.rs | 197 ++++++++++++++++++++------ 6 files changed, 304 insertions(+), 81 deletions(-) create mode 100644 src/pty/session/output_delivery.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ef55b24..9dc03d0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Roughly double bssh-server single-connection SFTP write throughput by fixing channel fragmentation and the serial write path** (#187). Three independent bottlenecks identified in the issue's code audit are addressed. First, `build_russh_config` now advertises a 65535-byte `maximum_packet_size` (the russh cap; library default 32768) and an 8 MiB `window_size` (library default 2 MiB), both configurable via `server.maximum_packet_size` / `server.window_size` (env: `BSSH_MAX_PACKET_SIZE` / `BSSH_WINDOW_SIZE`), so a 256 KiB SFTP write is no longer chopped into 8 CHANNEL_DATA packets that each pay the full cipher/copy/scheduling cost. Second, the SFTP server loop in `bssh-russh-sftp` is restructured from a strict read-process-write-flush cycle into a reader task with a bounded read-ahead queue plus an in-order processor: responses are flushed once per burst instead of once per request, and consecutive queued `SSH_FXP_WRITE` requests to the same handle at sequential offsets are coalesced into a single handler call (bounded by `max_write_coalesce_len`, default 256 KiB) while every merged request id still receives its own status reply, preserving SFTP response ordering and error semantics. The bssh write/read handlers additionally track each open file's cursor and elide the per-chunk `seek` for sequential transfers (append handles are exempt since O_APPEND ignores the cursor). Third, the workspace gains a tuned `[profile.release]` (`lto = "fat"`, `codegen-units = 1`) and documented `RUSTFLAGS` target-CPU guidance for self-builds. Local before/after benchmark (loopback, OpenSSH sftp client, 1 GiB random file, 3-run averages, 20-core Linux box): upload 71 MiB/s to 437 MiB/s (6.2x), download 73 MiB/s to 282 MiB/s (3.9x); an ablation with the code changes but library-default channel sizing lands at 333 / 291 MiB/s, attributing most of the gain to the pipelined write path and the rest of the upload gain to the larger packets. Numbers on the reporter's NIC-limited Xeon will differ; the methodology is documented in the PR. Remaining plan items (per-packet copies inside russh, AES-CTR internals) live in upstream russh since #212 removed the fork and are out of scope here. ### Added -- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and the screen initially selected according to DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value. Fragmented remote 1049 transitions are tracked so a session that began on main can safely use `1049l` to restore the remote-saved cursor after a disconnect; when a session began in an alternate screen, cleanup intentionally uses non-clearing mode 47 instead of a destructive `1049h`, because the terminal protocol cannot restore the 1049 bit and its single cursor-save slot without clearing or overwriting outer state. Failed or incomplete queries retain #234's ordinary-shell baseline fallback, hidden-screen setup failures make a best-effort return to the initial screen, the same guaranteed snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token covers both raw and deferred-raw guards. +- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and the screen initially selected according to DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value. Fragmented remote 1049 transitions are tracked only after the bytes survive filtering and are successfully written; the streaming parser skips OSC/DCS/APC/PM/SOS payloads and validates CSI parameters so escape-looking data is not mistaken for an executed transition. A session that began on main can therefore safely use `1049l` to restore the remote-saved cursor after a disconnect; when a session began in an alternate screen, cleanup intentionally uses non-clearing mode 47 instead of a destructive `1049h`, because the terminal protocol cannot restore the 1049 bit and its single cursor-save slot without clearing or overwriting outer state. Failed or incomplete queries retain #234's ordinary-shell baseline fallback, hidden-screen setup failures make a best-effort return to the initial screen, the same guaranteed snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token covers both raw and deferred-raw guards. - **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. ### Fixed diff --git a/docs/architecture/interactive-mode.md b/docs/architecture/interactive-mode.md index 00442952..3a56b176 100644 --- a/docs/architecture/interactive-mode.md +++ b/docs/architecture/interactive-mode.md @@ -195,7 +195,7 @@ Run this check in Ghostty and at least one other Kitty-keyboard-compatible termi Before starting the PTY input task, bssh uses a DA-delimited query transaction of at most 100 ms per screen to capture the Kitty enhancement flags independently for the main and alternate screens, together with the global xterm `modifyOtherKeys` value and the screen initially selected according to DEC private mode 1049. It uses DEC mode 47 to inspect the hidden buffer without the clear/initialize behavior of mode 1049, sets both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, and returns to the original screen before remote input starts. Only strict replies before the first primary DA in each transaction are consumed; post-DA bytes and user input, malformed replies, or unrelated escape sequences are retained in byte order and forwarded to the remote session. A failed hidden-screen write or flush makes a best-effort mode-47 return before capture falls back. -Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. It also tracks fragmented remote `1049h`/`1049l` transitions. If bssh began on main and the connection dies in a remote-owned 1049 alternate screen, cleanup uses `1049l` to restore the remote-saved cursor before restoring both screens' keyboard state. The reverse correction is intentionally non-destructive: if bssh began inside an outer alternate screen and the remote reset 1049, cleanup selects the original buffer with mode 47 instead of forcing `1049h`. Generic terminal protocols expose only one cursor-save slot, and `1049h` clears the alternate buffer, so the 1049 mode bit, saved cursor, and existing outer contents cannot all be reconstructed exactly in that case. Keyboard state and screen contents take priority over claiming an impossible exact mode-bit restoration. +Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. It also tracks fragmented remote `1049h`/`1049l` transitions, but commits a transition only after the bytes survive response filtering and `write_all` succeeds. Its streaming parser skips OSC, DCS, APC, PM, and SOS payloads through fragmented BEL/ST terminators and accepts only validated private CSI parameters, preventing embedded escape-looking data from changing cleanup state. If bssh began on main and the connection dies in a remote-owned 1049 alternate screen, cleanup uses `1049l` to restore the remote-saved cursor before restoring both screens' keyboard state. The reverse correction is intentionally non-destructive: if bssh began inside an outer alternate screen and the remote reset 1049, cleanup selects the original buffer with mode 47 instead of forcing `1049h`. Generic terminal protocols expose only one cursor-save slot, and `1049h` clears the alternate buffer, so the 1049 mode bit, saved cursor, and existing outer contents cannot all be reconstructed exactly in that case. Keyboard state and screen contents take priority over claiming an impossible exact mode-bit restoration. If a DA transaction or the screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234 rather than claiming partial preservation. The cleanup snapshot is published with poison recovery and reused by normal, forced, and repeated cleanup paths. A single-owner RAII token covers raw and deferred-raw guards alike so they cannot race the process-global snapshot, while a deferred guard that never enters raw mode performs no terminal cleanup when dropped. diff --git a/src/pty/session/mod.rs b/src/pty/session/mod.rs index 3eb12205..36c0fc28 100644 --- a/src/pty/session/mod.rs +++ b/src/pty/session/mod.rs @@ -18,6 +18,7 @@ mod constants; mod escape_filter; mod input; mod local_escape; +mod output_delivery; pub(crate) mod raw_input; mod raw_input_task; mod session_manager; diff --git a/src/pty/session/output_delivery.rs b/src/pty/session/output_delivery.rs new file mode 100644 index 00000000..4c614f27 --- /dev/null +++ b/src/pty/session/output_delivery.rs @@ -0,0 +1,110 @@ +// Copyright 2025 Lablup Inc. and Jeongkyu Shin +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +//! Ordered filtering, terminal delivery, and observation of remote output. + +use std::io::{self, Write}; + +use super::escape_filter::EscapeSequenceFilter; + +/// Filter remote bytes, write the surviving bytes, then commit only bytes the +/// terminal actually accepted to the protocol observer. +pub(super) fn deliver_filtered_output( + filter: &mut EscapeSequenceFilter, + writer: &mut impl Write, + input: &[u8], + commit: impl FnOnce(&[u8]), +) -> io::Result<()> { + let output = filter.filter(input); + if output.is_empty() { + return Ok(()); + } + + writer.write_all(&output)?; + commit(&output); + let _ = writer.flush(); + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::pty::terminal_screen::ScreenModeTracker; + + struct FailedWriter; + + impl Write for FailedWriter { + fn write(&mut self, _buffer: &[u8]) -> io::Result { + Err(io::Error::other("injected delivery failure")) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } + } + + #[test] + fn failed_delivery_does_not_commit_screen_transition() { + let mut filter = EscapeSequenceFilter::new(); + let mut tracker = ScreenModeTracker::new(); + let mut committed = false; + + let result = + deliver_filtered_output(&mut filter, &mut FailedWriter, b"\x1b[?1049h", |output| { + committed = true; + tracker.observe(output); + }); + + assert!(result.is_err()); + assert!(!committed); + } + + #[test] + fn filtered_payload_does_not_commit_screen_transition() { + let mut filter = EscapeSequenceFilter::new(); + let mut tracker = ScreenModeTracker::new(); + let mut output = Vec::new(); + let mut committed = false; + + deliver_filtered_output( + &mut filter, + &mut output, + b"\x1b]10;payload\x1b[?1049h\x07", + |delivered| { + committed = true; + tracker.observe(delivered); + }, + ) + .unwrap(); + + assert!(output.is_empty()); + assert!(!committed); + } + + #[test] + fn successful_delivery_commits_screen_transition() { + let mut filter = EscapeSequenceFilter::new(); + let mut tracker = ScreenModeTracker::new(); + let mut output = Vec::new(); + let mut observed = None; + + deliver_filtered_output(&mut filter, &mut output, b"\x1b[?1049h", |delivered| { + observed = tracker.observe(delivered); + }) + .unwrap(); + + assert_eq!(output, b"\x1b[?1049h"); + assert_eq!(observed, Some(true)); + } +} diff --git a/src/pty/session/session_manager.rs b/src/pty/session/session_manager.rs index a46c9c6e..ae216992 100644 --- a/src/pty/session/session_manager.rs +++ b/src/pty/session/session_manager.rs @@ -16,6 +16,7 @@ use super::constants::*; use super::escape_filter::EscapeSequenceFilter; +use super::output_delivery::deliver_filtered_output; use super::raw_input_task; use super::terminal_modes::configure_terminal_modes; use crate::pty::{ @@ -252,36 +253,40 @@ impl PtySession { match msg { Some(ChannelMsg::Data { ref data }) => { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(data); - } // Filter terminal escape sequence responses before display // This prevents raw XTGETTCAP, DA1/DA2/DA3 responses from appearing // on screen when running applications like Neovim - let filtered_data = self.escape_filter.filter(data); - if !filtered_data.is_empty() { - if let Err(e) = io::stdout().write_all(&filtered_data) { - tracing::error!("Failed to write to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); - } + let terminal_guard = &mut self.terminal_guard; + if let Err(e) = deliver_filtered_output( + &mut self.escape_filter, + &mut io::stdout(), + data, + |delivered| { + if let Some(guard) = terminal_guard.as_mut() { + guard.observe_remote_output(delivered); + } + }, + ) { + tracing::error!("Failed to write to stdout: {e}"); + should_terminate = true; } } Some(ChannelMsg::ExtendedData { ref data, ext }) => { if ext == 1 { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(data); - } // stderr - also filter escape sequences - let filtered_data = self.escape_filter.filter(data); - if !filtered_data.is_empty() { - if let Err(e) = io::stdout().write_all(&filtered_data) { - tracing::error!("Failed to write stderr to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); - } + let terminal_guard = &mut self.terminal_guard; + if let Err(e) = deliver_filtered_output( + &mut self.escape_filter, + &mut io::stdout(), + data, + |delivered| { + if let Some(guard) = terminal_guard.as_mut() { + guard.observe_remote_output(delivered); + } + }, + ) { + tracing::error!("Failed to write stderr to stdout: {e}"); + should_terminate = true; } } } @@ -322,20 +327,22 @@ impl PtySession { } } Some(PtyMessage::RemoteOutput(data)) => { - if let Some(guard) = self.terminal_guard.as_mut() { - guard.observe_remote_output(&data); - } // Apply escape filter for consistency with SSH channel data // This path may receive data from other sources that could // contain terminal responses that shouldn't be displayed - let filtered_data = self.escape_filter.filter(&data); - if !filtered_data.is_empty() { - if let Err(e) = io::stdout().write_all(&filtered_data) { - tracing::error!("Failed to write to stdout: {e}"); - should_terminate = true; - } else { - let _ = io::stdout().flush(); - } + let terminal_guard = &mut self.terminal_guard; + if let Err(e) = deliver_filtered_output( + &mut self.escape_filter, + &mut io::stdout(), + &data, + |delivered| { + if let Some(guard) = terminal_guard.as_mut() { + guard.observe_remote_output(delivered); + } + }, + ) { + tracing::error!("Failed to write to stdout: {e}"); + should_terminate = true; } } Some(PtyMessage::Resize { width, height }) => { diff --git a/src/pty/terminal_screen.rs b/src/pty/terminal_screen.rs index ecfe130e..58b20614 100644 --- a/src/pty/terminal_screen.rs +++ b/src/pty/terminal_screen.rs @@ -12,56 +12,144 @@ // See the License for the specific language governing permissions and // limitations under the License. -//! Tracks remote DECSET/DECRST 1049 transitions across fragmented PTY output. +//! Tracks delivered DECSET/DECRST 1049 transitions across fragmented output. const MAX_CSI_SEQUENCE: usize = 64; +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ParserState { + Ground, + Escape, + Csi, + String, + StringEscape, +} + #[derive(Debug)] pub(crate) struct ScreenModeTracker { - pending: Vec, + state: ParserState, + csi: Vec, } impl ScreenModeTracker { pub(crate) fn new() -> Self { Self { - pending: Vec::new(), + state: ParserState::Ground, + csi: Vec::with_capacity(16), } } - /// Return the last DEC 1049 state explicitly selected by this output. + /// Return the last DEC 1049 state explicitly selected by delivered output. + /// + /// OSC, DCS, APC, PM, and SOS payloads are skipped until BEL or ST so an + /// escape-looking payload cannot be mistaken for an executed CSI command. pub(crate) fn observe(&mut self, input: &[u8]) -> Option { - self.pending.extend_from_slice(input); let mut current = None; - let mut consumed = 0; - for (start, end) in csi_ranges(&self.pending) { - if let Some(is_set) = parse_1049_selection(&self.pending[start..end]) { - current = Some(is_set); + for &byte in input { + match self.state { + ParserState::Ground => self.consume_ground(byte), + ParserState::Escape => self.consume_escape(byte), + ParserState::Csi => { + if let Some(is_set) = self.consume_csi(byte) { + current = Some(is_set); + } + } + ParserState::String => self.consume_string(byte), + ParserState::StringEscape => self.consume_string_escape(byte), } - consumed = end; } - if consumed > 0 { - self.pending.drain(..consumed); + current + } + + fn consume_ground(&mut self, byte: u8) { + match byte { + b'\x1b' => self.state = ParserState::Escape, + 0x9b => self.start_csi(), + 0x90 | 0x98 | 0x9d | 0x9e | 0x9f => self.state = ParserState::String, + _ => {} } - if self.pending.len() > MAX_CSI_SEQUENCE { - let keep_from = self.pending.len() - MAX_CSI_SEQUENCE; - self.pending.drain(..keep_from); + } + + fn consume_escape(&mut self, byte: u8) { + match byte { + b'[' => self.start_csi(), + // DCS, SOS, OSC, PM, and APC. + b'P' | b'X' | b']' | b'^' | b'_' => self.state = ParserState::String, + b'\x1b' => {} + _ => self.state = ParserState::Ground, } + } - current + fn start_csi(&mut self) { + self.csi.clear(); + self.state = ParserState::Csi; + } + + fn consume_csi(&mut self, byte: u8) -> Option { + match byte { + // CAN and SUB cancel a control sequence. + 0x18 | 0x1a | 0x9c => { + self.csi.clear(); + self.state = ParserState::Ground; + return None; + } + b'\x1b' => { + self.csi.clear(); + self.state = ParserState::Escape; + return None; + } + _ => self.csi.push(byte), + } + + if self.csi.len() > MAX_CSI_SEQUENCE { + self.csi.clear(); + self.state = ParserState::Ground; + return None; + } + if !(0x40..=0x7e).contains(&byte) { + return None; + } + + let selection = parse_1049_selection(&self.csi); + self.csi.clear(); + self.state = ParserState::Ground; + selection + } + + fn consume_string(&mut self, byte: u8) { + match byte { + b'\x07' | 0x9c => self.state = ParserState::Ground, + b'\x1b' => self.state = ParserState::StringEscape, + _ => {} + } + } + + fn consume_string_escape(&mut self, byte: u8) { + match byte { + b'\\' | b'\x07' | 0x9c => self.state = ParserState::Ground, + b'\x1b' => {} + _ => self.state = ParserState::String, + } } } fn parse_1049_selection(sequence: &[u8]) -> Option { - let (parameters, is_set) = if let Some(value) = sequence - .strip_prefix(b"\x1b[?") - .and_then(|value| value.strip_suffix(b"h")) - { - (value, true) - } else { - (sequence.strip_prefix(b"\x1b[?")?.strip_suffix(b"l")?, false) + let (&final_byte, body) = sequence.split_last()?; + let is_set = match final_byte { + b'h' => true, + b'l' => false, + _ => return None, }; + let parameters = body.strip_prefix(b"?")?; + if parameters.is_empty() + || !parameters + .iter() + .all(|byte| byte.is_ascii_digit() || *byte == b';') + { + return None; + } parameters .split(|byte| *byte == b';') @@ -69,29 +157,6 @@ fn parse_1049_selection(sequence: &[u8]) -> Option { .then_some(is_set) } -fn csi_ranges(input: &[u8]) -> impl Iterator { - let mut offset = 0; - std::iter::from_fn(move || { - while offset + 1 < input.len() { - if input[offset] != b'\x1b' || input[offset + 1] != b'[' { - offset += 1; - continue; - } - let start = offset; - let limit = input.len().min(start + MAX_CSI_SEQUENCE); - offset += 2; - while offset < limit { - if (0x40..=0x7e).contains(&input[offset]) { - offset += 1; - return Some((start, offset)); - } - offset += 1; - } - } - None - }) -} - #[cfg(test)] mod tests { use super::*; @@ -104,4 +169,44 @@ mod tests { assert_eq!(tracker.observe(b"49h\x1b[?25;1049l"), Some(false)); assert_eq!(tracker.observe(b"\x1b[?47h"), None); } + + #[test] + fn skips_osc_dcs_and_tmux_payload_false_positives() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"\x1b]0;title\x1b[?1049h\x07"), None); + assert_eq!(tracker.observe(b"\x1bPpayload\x1b[?1049l\x1b\\"), None); + assert_eq!(tracker.observe(b"\x1bPtmux;\x1b\x1b[?1049h\x1b\\"), None); + assert_eq!(tracker.observe(b"\x1b[?1049h"), Some(true)); + } + + #[test] + fn skips_apc_pm_and_sos_until_fragmented_st() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"\x1b_payload\x1b[?1049h\x1b"), None); + assert_eq!(tracker.observe(b"\\"), None); + assert_eq!(tracker.observe(b"\x1b^payload\x1b[?1049l\x1b\\"), None); + assert_eq!(tracker.observe(b"\x1bXpayload\x1b[?1049h\x07"), None); + assert_eq!(tracker.observe(b"\x1b[?1049l"), Some(false)); + } + + #[test] + fn rejects_invalid_csi_parameters() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"\x1b[?1049:1h"), None); + assert_eq!(tracker.observe(b"\x1b[?1049$h"), None); + assert_eq!(tracker.observe(b"\x1b[?x;1049h"), None); + assert_eq!(tracker.observe(b"\x1b[?25;1049h"), Some(true)); + } + + #[test] + fn supports_eight_bit_csi_strings_and_st() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"\x9dtitle\x9b?1049h\x9c"), None); + assert_eq!(tracker.observe(b"\x90payload\x9b?1049l\x9c"), None); + assert_eq!(tracker.observe(b"\x9b?1049h"), Some(true)); + } } From 64b3e8d17ec4b5914c9c5ff317bcfec3756f0fad Mon Sep 17 00:00:00 2001 From: Jeongkyu Shin Date: Wed, 29 Jul 2026 17:09:07 +0900 Subject: [PATCH 6/6] fix: respect terminal string terminators --- CHANGELOG.md | 2 +- docs/architecture/interactive-mode.md | 2 +- src/pty/terminal_screen.rs | 78 +++++++++++++++++---------- 3 files changed, 51 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9dc03d0a..e183583b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Roughly double bssh-server single-connection SFTP write throughput by fixing channel fragmentation and the serial write path** (#187). Three independent bottlenecks identified in the issue's code audit are addressed. First, `build_russh_config` now advertises a 65535-byte `maximum_packet_size` (the russh cap; library default 32768) and an 8 MiB `window_size` (library default 2 MiB), both configurable via `server.maximum_packet_size` / `server.window_size` (env: `BSSH_MAX_PACKET_SIZE` / `BSSH_WINDOW_SIZE`), so a 256 KiB SFTP write is no longer chopped into 8 CHANNEL_DATA packets that each pay the full cipher/copy/scheduling cost. Second, the SFTP server loop in `bssh-russh-sftp` is restructured from a strict read-process-write-flush cycle into a reader task with a bounded read-ahead queue plus an in-order processor: responses are flushed once per burst instead of once per request, and consecutive queued `SSH_FXP_WRITE` requests to the same handle at sequential offsets are coalesced into a single handler call (bounded by `max_write_coalesce_len`, default 256 KiB) while every merged request id still receives its own status reply, preserving SFTP response ordering and error semantics. The bssh write/read handlers additionally track each open file's cursor and elide the per-chunk `seek` for sequential transfers (append handles are exempt since O_APPEND ignores the cursor). Third, the workspace gains a tuned `[profile.release]` (`lto = "fat"`, `codegen-units = 1`) and documented `RUSTFLAGS` target-CPU guidance for self-builds. Local before/after benchmark (loopback, OpenSSH sftp client, 1 GiB random file, 3-run averages, 20-core Linux box): upload 71 MiB/s to 437 MiB/s (6.2x), download 73 MiB/s to 282 MiB/s (3.9x); an ablation with the code changes but library-default channel sizing lands at 333 / 291 MiB/s, attributing most of the gain to the pipelined write path and the rest of the upload gain to the larger packets. Numbers on the reporter's NIC-limited Xeon will differ; the methodology is documented in the PR. Remaining plan items (per-packet copies inside russh, AES-CTR internals) live in upstream russh since #212 removed the fork and are out of scope here. ### Added -- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and the screen initially selected according to DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value. Fragmented remote 1049 transitions are tracked only after the bytes survive filtering and are successfully written; the streaming parser skips OSC/DCS/APC/PM/SOS payloads and validates CSI parameters so escape-looking data is not mistaken for an executed transition. A session that began on main can therefore safely use `1049l` to restore the remote-saved cursor after a disconnect; when a session began in an alternate screen, cleanup intentionally uses non-clearing mode 47 instead of a destructive `1049h`, because the terminal protocol cannot restore the 1049 bit and its single cursor-save slot without clearing or overwriting outer state. Failed or incomplete queries retain #234's ordinary-shell baseline fallback, hidden-screen setup failures make a best-effort return to the initial screen, the same guaranteed snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token covers both raw and deferred-raw guards. +- **Preserve an outer TUI's enhanced-keyboard state across interactive PTY sessions** (#236). Before the local input task starts, bssh performs DA-delimited, 100 ms-per-screen bounded queries for raw Kitty progressive-enhancement flags on both main and alternate screens, plus xterm `modifyOtherKeys` and the screen initially selected according to DEC private mode 1049. It uses non-clearing DEC mode 47 transitions to inspect the hidden screen, then normalizes both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, so the remote shell is isolated from the outer TUI's protocol. Strictly matched replies before each transaction's first primary DA are consumed while every post-DA, unrelated, malformed, partial, or concurrently typed byte is retained in order and forwarded through the normal PTY input path. Teardown resets leaked stacks and restores each screen's captured Kitty flags plus the global xterm value. Fragmented remote 1049 transitions are tracked only after the bytes survive filtering and are successfully written; the streaming parser skips OSC/DCS/APC/PM/SOS payloads with their protocol-specific terminators and validates CSI parameters so escape-looking data is not mistaken for an executed transition. A session that began on main can therefore safely use `1049l` to restore the remote-saved cursor after a disconnect; when a session began in an alternate screen, cleanup intentionally uses non-clearing mode 47 instead of a destructive `1049h`, because the terminal protocol cannot restore the 1049 bit and its single cursor-save slot without clearing or overwriting outer state. Failed or incomplete queries retain #234's ordinary-shell baseline fallback, hidden-screen setup failures make a best-effort return to the initial screen, the same guaranteed snapshot is reused by normal, forced, and repeated cleanup paths, and an atomic RAII ownership token covers both raw and deferred-raw guards. - **Make server-side SSH compression configurable instead of hard-disabled** (#220). The #215 workaround forced the server to advertise only `none` compression for every deployment, with no way for operators to opt back in. A new `server.compression` option (YAML `server.compression: true`, env `BSSH_COMPRESSION`, builder `ServerConfigBuilder::compression`) now controls whether `build_russh_config` advertises russh's default compression list (`none`, `zlib`, `zlib@openssh.com`) or only `none`. The default stays off, so behavior is unchanged unless explicitly enabled, and enabling it logs a warning: russh's delayed-zlib (`zlib@openssh.com`) transport still desyncs a few packets after compression activates post-auth (reproduced on russh 0.61.1 and 0.62.1), dropping clients that negotiate it mid-session. Both settings are covered by unit tests, and the option plus the desync caveat are documented in the man page and config docs. ### Fixed diff --git a/docs/architecture/interactive-mode.md b/docs/architecture/interactive-mode.md index 3a56b176..ae393d72 100644 --- a/docs/architecture/interactive-mode.md +++ b/docs/architecture/interactive-mode.md @@ -195,7 +195,7 @@ Run this check in Ghostty and at least one other Kitty-keyboard-compatible termi Before starting the PTY input task, bssh uses a DA-delimited query transaction of at most 100 ms per screen to capture the Kitty enhancement flags independently for the main and alternate screens, together with the global xterm `modifyOtherKeys` value and the screen initially selected according to DEC private mode 1049. It uses DEC mode 47 to inspect the hidden buffer without the clear/initialize behavior of mode 1049, sets both screens to the legacy keyboard baseline without popping the outer application's Kitty stacks, and returns to the original screen before remote input starts. Only strict replies before the first primary DA in each transaction are consumed; post-DA bytes and user input, malformed replies, or unrelated escape sequences are retained in byte order and forwarded to the remote session. A failed hidden-screen write or flush makes a best-effort mode-47 return before capture falls back. -Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. It also tracks fragmented remote `1049h`/`1049l` transitions, but commits a transition only after the bytes survive response filtering and `write_all` succeeds. Its streaming parser skips OSC, DCS, APC, PM, and SOS payloads through fragmented BEL/ST terminators and accepts only validated private CSI parameters, preventing embedded escape-looking data from changing cleanup state. If bssh began on main and the connection dies in a remote-owned 1049 alternate screen, cleanup uses `1049l` to restore the remote-saved cursor before restoring both screens' keyboard state. The reverse correction is intentionally non-destructive: if bssh began inside an outer alternate screen and the remote reset 1049, cleanup selects the original buffer with mode 47 instead of forcing `1049h`. Generic terminal protocols expose only one cursor-save slot, and `1049h` clears the alternate buffer, so the 1049 mode bit, saved cursor, and existing outer contents cannot all be reconstructed exactly in that case. Keyboard state and screen contents take priority over claiming an impossible exact mode-bit restoration. +Teardown resets leaked stacks on both buffers and restores both captured Kitty values plus `modifyOtherKeys`. It also tracks fragmented remote `1049h`/`1049l` transitions, but commits a transition only after the bytes survive response filtering and `write_all` succeeds. Its streaming parser skips 7-bit OSC, DCS, APC, PM, and SOS payloads through fragmented terminators—BEL is accepted only for OSC compatibility, while every string type accepts `ESC \` ST—and accepts only validated private CSI parameters, preventing embedded escape-looking data from changing cleanup state. Raw 8-bit C1 controls are not interpreted because their byte values can also occur as UTF-8 continuation bytes. If bssh began on main and the connection dies in a remote-owned 1049 alternate screen, cleanup uses `1049l` to restore the remote-saved cursor before restoring both screens' keyboard state. The reverse correction is intentionally non-destructive: if bssh began inside an outer alternate screen and the remote reset 1049, cleanup selects the original buffer with mode 47 instead of forcing `1049h`. Generic terminal protocols expose only one cursor-save slot, and `1049h` clears the alternate buffer, so the 1049 mode bit, saved cursor, and existing outer contents cannot all be reconstructed exactly in that case. Keyboard state and screen contents take priority over claiming an impossible exact mode-bit restoration. If a DA transaction or the screen-state query is unsupported, malformed, or times out, cleanup falls back to the ordinary-shell baseline from #234 rather than claiming partial preservation. The cleanup snapshot is published with poison recovery and reused by normal, forced, and repeated cleanup paths. A single-owner RAII token covers raw and deferred-raw guards alike so they cannot race the process-global snapshot, while a deferred guard that never enters raw mode performs no terminal cleanup when dropped. diff --git a/src/pty/terminal_screen.rs b/src/pty/terminal_screen.rs index 58b20614..90651a56 100644 --- a/src/pty/terminal_screen.rs +++ b/src/pty/terminal_screen.rs @@ -21,8 +21,14 @@ enum ParserState { Ground, Escape, Csi, - String, - StringEscape, + String(StringKind), + StringEscape(StringKind), +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum StringKind { + Osc, + Other, } #[derive(Debug)] @@ -41,8 +47,9 @@ impl ScreenModeTracker { /// Return the last DEC 1049 state explicitly selected by delivered output. /// - /// OSC, DCS, APC, PM, and SOS payloads are skipped until BEL or ST so an - /// escape-looking payload cannot be mistaken for an executed CSI command. + /// OSC payloads are skipped until BEL or ST; DCS, APC, PM, and SOS payloads + /// require ST. This prevents escape-looking payload data from being + /// mistaken for an executed CSI command. pub(crate) fn observe(&mut self, input: &[u8]) -> Option { let mut current = None; @@ -55,8 +62,8 @@ impl ScreenModeTracker { current = Some(is_set); } } - ParserState::String => self.consume_string(byte), - ParserState::StringEscape => self.consume_string_escape(byte), + ParserState::String(kind) => self.consume_string(kind, byte), + ParserState::StringEscape(kind) => self.consume_string_escape(kind, byte), } } @@ -64,19 +71,19 @@ impl ScreenModeTracker { } fn consume_ground(&mut self, byte: u8) { - match byte { - b'\x1b' => self.state = ParserState::Escape, - 0x9b => self.start_csi(), - 0x90 | 0x98 | 0x9d | 0x9e | 0x9f => self.state = ParserState::String, - _ => {} + if byte == b'\x1b' { + self.state = ParserState::Escape; } } fn consume_escape(&mut self, byte: u8) { match byte { b'[' => self.start_csi(), - // DCS, SOS, OSC, PM, and APC. - b'P' | b'X' | b']' | b'^' | b'_' => self.state = ParserState::String, + b']' => self.state = ParserState::String(StringKind::Osc), + // DCS, SOS, PM, and APC. + b'P' | b'X' | b'^' | b'_' => { + self.state = ParserState::String(StringKind::Other); + } b'\x1b' => {} _ => self.state = ParserState::Ground, } @@ -90,7 +97,7 @@ impl ScreenModeTracker { fn consume_csi(&mut self, byte: u8) -> Option { match byte { // CAN and SUB cancel a control sequence. - 0x18 | 0x1a | 0x9c => { + 0x18 | 0x1a => { self.csi.clear(); self.state = ParserState::Ground; return None; @@ -118,19 +125,20 @@ impl ScreenModeTracker { selection } - fn consume_string(&mut self, byte: u8) { + fn consume_string(&mut self, kind: StringKind, byte: u8) { match byte { - b'\x07' | 0x9c => self.state = ParserState::Ground, - b'\x1b' => self.state = ParserState::StringEscape, + b'\x07' if kind == StringKind::Osc => self.state = ParserState::Ground, + b'\x1b' => self.state = ParserState::StringEscape(kind), _ => {} } } - fn consume_string_escape(&mut self, byte: u8) { + fn consume_string_escape(&mut self, kind: StringKind, byte: u8) { match byte { - b'\\' | b'\x07' | 0x9c => self.state = ParserState::Ground, + b'\\' => self.state = ParserState::Ground, + b'\x07' if kind == StringKind::Osc => self.state = ParserState::Ground, b'\x1b' => {} - _ => self.state = ParserState::String, + _ => self.state = ParserState::String(kind), } } } @@ -181,16 +189,29 @@ mod tests { } #[test] - fn skips_apc_pm_and_sos_until_fragmented_st() { + fn non_osc_strings_ignore_bel_and_embedded_1049_until_fragmented_st() { let mut tracker = ScreenModeTracker::new(); - assert_eq!(tracker.observe(b"\x1b_payload\x1b[?1049h\x1b"), None); - assert_eq!(tracker.observe(b"\\"), None); - assert_eq!(tracker.observe(b"\x1b^payload\x1b[?1049l\x1b\\"), None); - assert_eq!(tracker.observe(b"\x1bXpayload\x1b[?1049h\x07"), None); + for introducer in [ + b"\x1bP".as_slice(), + b"\x1b_".as_slice(), + b"\x1b^".as_slice(), + b"\x1bX".as_slice(), + ] { + assert_eq!(tracker.observe(introducer), None); + assert_eq!(tracker.observe(b"payload\x07\x1b[?1049h\x1b"), None); + assert_eq!(tracker.observe(b"\\"), None); + } assert_eq!(tracker.observe(b"\x1b[?1049l"), Some(false)); } + #[test] + fn osc_bel_terminates_before_following_1049() { + let mut tracker = ScreenModeTracker::new(); + + assert_eq!(tracker.observe(b"\x1b]0;title\x07\x1b[?1049h"), Some(true)); + } + #[test] fn rejects_invalid_csi_parameters() { let mut tracker = ScreenModeTracker::new(); @@ -202,11 +223,10 @@ mod tests { } #[test] - fn supports_eight_bit_csi_strings_and_st() { + fn does_not_treat_utf8_c1_continuation_as_csi() { let mut tracker = ScreenModeTracker::new(); - assert_eq!(tracker.observe(b"\x9dtitle\x9b?1049h\x9c"), None); - assert_eq!(tracker.observe(b"\x90payload\x9b?1049l\x9c"), None); - assert_eq!(tracker.observe(b"\x9b?1049h"), Some(true)); + assert_eq!(tracker.observe(b"\xc2\x9b?1049h"), None); + assert_eq!(tracker.observe(b"\x9b?1049h"), None); } }