From 57520a515e9a04aa52269a034c43249d0efd2aa6 Mon Sep 17 00:00:00 2001 From: petruha Date: Thu, 20 Aug 2026 05:23:00 +0400 Subject: [PATCH 1/5] fix(session): answer terminal status queries from the embedded emulator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An application inside the PTY that asks the terminal a question got no answer: vt100 implements no query at all, so `ESC [ 6 n` (cursor position report) landed in `unhandled_csi` and died there. The asker then blocked until its own timeout — atuin's history search fails outright on the first Up arrow with "The cursor position could not be read within a normal duration", which is exactly what issue #113 reports, and works over plain `ssh` because the real terminal replies. Collect the answers in the parser's callbacks and write them back into the PTY from `Session::drain`, so every session answers whether or not its tab is the visible one — unlike a clipboard write there is nothing for the frame to decide. Answered: DSR 6 (from our own grid, which mirrors the remote screen), DSR 5, and DA1, the last of which also ends the two-second stall every crossterm TUI took at startup probing for the kitty keyboard protocol. Private queries we don't actually speak stay unanswered, since silence is what tells the caller they are unsupported. The end-to-end test drives the whole loop through a real PTY: the child asks, reads six bytes back, and prints what it got. It fails without the write. Closes #113 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 15 ++++ src/session/mod.rs | 57 +++++++++++++++ src/session/parser.rs | 163 +++++++++++++++++++++++++++++++++++++++--- 3 files changed, 225 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57f70f8..8f3cec0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,21 @@ All notable changes to SSHub are documented in this file. ## [Unreleased] +### Fixed + +- **The embedded terminal answers cursor-position and status queries** (issue + #113, reported by [@ikerib](https://github.com/ikerib)) - an application + asking the terminal where the cursor is (`ESC [ 6 n`) got no answer from our + vt100 emulator and blocked until its own timeout, so atuin's history search + died on the first Up arrow with "The cursor position could not be read within + a normal duration" — over plain `ssh` the real terminal replies and it works. + The emulator now answers the cursor position report from its own grid, the + device status report, and the primary device attributes query, which also + ends the two-second stall every crossterm-based TUI took at startup while + probing for the kitty keyboard protocol. Queries we do not actually speak + (the kitty keyboard protocol itself, secondary device attributes) stay + unanswered on purpose: silence is what tells a caller they are unsupported. + ## [0.15.0] - 2026-08-18 ### Added diff --git a/src/session/mod.rs b/src/session/mod.rs index 24628bb..59fcc1a 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -522,6 +522,7 @@ impl Session { } } if had_bytes { + self.answer_terminal_queries(); self.maybe_send_pending_secret(); self.maybe_reveal(); } @@ -538,6 +539,23 @@ impl Session { self.maybe_detect_connected(); } + /// Write the emulator's answers to terminal status queries back into the + /// PTY. Driven from [`Self::drain`] rather than from the frame, because + /// unlike a clipboard write there is nothing to decide: an application + /// waiting on a reply blocks whether or not its tab is the visible one. + fn answer_terminal_queries(&mut self) { + let replies = self.parser.take_replies(); + if replies.is_empty() { + return; + } + if let Err(e) = self.runtime.write(&replies) { + // Prefixed `session:` so the event loop's connected-session filter + // keeps it — see [`keep_diagnostic`]. + self.diagnostics + .push(format!("session: terminal query reply failed: {e}")); + } + } + /// Throw away whatever the PTY asked us to copy, including the drop /// counters. Used for every session that is not the visible one this /// frame, so a background tab can neither relay now nor replay later. @@ -1327,6 +1345,45 @@ mod prompt_tests { ); } + #[test] + fn the_pty_child_receives_a_cursor_position_report() { + // End-to-end for #113: a child asks the terminal where the cursor is + // and *reads the answer back*. Nothing in the parser unit tests proves + // the reply actually leaves the process, and this is exactly the loop + // that hangs atuin — it asks, waits two seconds, and errors out. + // + // `stty raw -echo` so the reply arrives byte-for-byte instead of being + // line-buffered and echoed back at us; the cursor is still at the home + // position, so the answer is exactly the 6 bytes of `ESC [ 1 ; 1 R`, + // which is what lets `dd` stop without a timeout. `tr` strips the + // escape so the marker is printable on the grid. + let script = r"stty raw -echo; printf '\033[6n'; \ + r=$(dd bs=1 count=6 2>/dev/null | tr -d '\033['); \ + stty sane; printf 'CPR<%s>\r\n' $r"; + let config = SessionConfig { + argv: vec!["sh".into(), "-c".into(), script.into()], + display_name: "t".into(), + meta: SessionMeta::default(), + pending_secret: None, + key_push_identity: None, + host_name: "t".into(), + }; + let mut s = Session::spawn(config, 24, 80, None).unwrap(); + + for _ in 0..300 { + s.drain(); + if s.screen_tail_snippet().contains("CPR<") { + break; + } + std::thread::sleep(Duration::from_millis(10)); + } + assert!( + s.screen_tail_snippet().contains("CPR<1;1R>"), + "child never got its cursor position back, tail: {:?}", + s.screen_tail_snippet() + ); + } + /// Build a throwaway session for tests that only need a live `Session`. fn scratch_session() -> Session { let config = SessionConfig { diff --git a/src/session/parser.rs b/src/session/parser.rs index 88d8993..7af23a5 100644 --- a/src/session/parser.rs +++ b/src/session/parser.rs @@ -10,6 +10,12 @@ const CLIPBOARD_RELAY_MAX_BYTES: usize = 64 * 1024; /// copy loop can't grow the queue without bound; the excess is dropped. const CLIPBOARD_RELAY_MAX_QUEUED: usize = 8; +/// Largest queue of answers to terminal status queries we hold between drains +/// (1 KiB). Every answer is a dozen bytes at most, so this is far more than any +/// real application asks for in one frame — it exists so a remote stuck in a +/// query loop can't make us buffer unbounded input for it. +const REPLY_QUEUE_MAX_BYTES: usize = 1024; + /// Exact decoded byte length of a base64 payload, without decoding it. The /// payload is relayed verbatim, so a real decoder would be pure waste — this /// only exists to enforce the size cap and to size the "n bytes" notice. @@ -37,22 +43,26 @@ pub(crate) struct ClipboardDrops { pub(crate) queue_full: usize, } -/// Collects OSC 52 clipboard writes coming out of the PTY so the session can -/// re-emit them toward the real terminal. +/// Everything the emulator has to answer for, collected per drain: OSC 52 +/// clipboard writes headed for the real terminal, and answers to terminal +/// status queries headed back into the PTY. /// -/// Without this, `vt100` parses `ESC ] 52 ; c ; BEL` and hands it to -/// the default `Callbacks for ()` impl, which silently drops it — so anything -/// copying inside the PTY (herdr, tmux, neovim, lazygit…) appears to work but -/// never reaches the system clipboard. +/// Without this, `vt100` hands both to the default `Callbacks for ()` impl, +/// which silently drops them. A dropped clipboard write means anything copying +/// inside the PTY (herdr, tmux, neovim, lazygit…) appears to work but never +/// reaches the system clipboard; a dropped *query* means the application waits +/// for an answer that never comes. #[derive(Default)] -struct ClipboardRelay { +struct PtyCallbacks { /// Pending base64 payloads, in arrival order. pending: Vec, /// Writes rejected since the last drain, by reason. drops: ClipboardDrops, + /// Answers to terminal status queries, waiting to go back into the PTY. + replies: Vec, } -impl vt100::Callbacks for ClipboardRelay { +impl vt100::Callbacks for PtyCallbacks { fn copy_to_clipboard(&mut self, _: &mut vt100::Screen, _ty: &[u8], data: &[u8]) { // An empty payload is a clipboard *clear* on terminals that honour it. // We neither forward it nor count it as a drop: a remote must not be @@ -81,19 +91,70 @@ impl vt100::Callbacks for ClipboardRelay { // `paste_from_clipboard` is deliberately left as the no-op default: // answering `ESC]52;c;?BEL` would let any host we're SSH'd into *read* the // local clipboard, which is far worse than a write and buys us nothing. + + /// vt100 implements no terminal *query* at all, so every one of them lands + /// here — and an application that asks and hears nothing back blocks until + /// its own timeout expires. atuin's history search dies outright ("The + /// cursor position could not be read within a normal duration", #113), and + /// every crossterm-based TUI stalls two seconds at startup probing for the + /// kitty keyboard protocol. Answering is simply what the terminal on the + /// other side does when `ssh` runs without sshub in front of it. + fn unhandled_csi( + &mut self, + screen: &mut vt100::Screen, + i1: Option, + _i2: Option, + params: &[&[u16]], + c: char, + ) { + // Private sequences (`CSI ? … u`, the kitty keyboard protocol query; + // `CSI > c`, secondary device attributes) stay unanswered on purpose. + // We don't speak them, and claiming otherwise is worse than silence: + // crossterm reads a missing `?u` reply *plus* the DA1 answer below as + // a definitive "not supported", which is the truth. + if i1.is_some() { + return; + } + let param = params.first().and_then(|p| p.first().copied()).unwrap_or(0); + let reply = match (c, param) { + // DSR 6 — cursor position report. Our grid mirrors the remote + // screen, so its cursor *is* the answer. Reported 1-based. + ('n', 6) => { + let (row, col) = screen.cursor_position(); + format!("\x1b[{};{}R", row + 1, col + 1).into_bytes() + } + // DSR 5 — device status. Nothing can go wrong in an in-memory grid. + ('n', 5) => b"\x1b[0n".to_vec(), + // DA1 — device attributes. VT100 with the advanced video option is + // the honest floor for what vt100 emulates; callers only care that + // an answer arrives at all, not what it claims. + ('c', 0) => b"\x1b[?1;2c".to_vec(), + _ => return, + }; + if self.replies.len() + reply.len() <= REPLY_QUEUE_MAX_BYTES { + self.replies.extend_from_slice(&reply); + } + } } pub struct ParserState { - inner: vt100::Parser, + inner: vt100::Parser, } impl ParserState { pub fn new(rows: u16, cols: u16) -> Self { Self { - inner: vt100::Parser::new_with_callbacks(rows, cols, 10_000, ClipboardRelay::default()), + inner: vt100::Parser::new_with_callbacks(rows, cols, 10_000, PtyCallbacks::default()), } } + /// Take the answers to terminal queries seen since the last call. Unlike a + /// clipboard write these go straight back into the PTY, not to the host + /// terminal, and every session owes them whether or not it is on screen. + pub(crate) fn take_replies(&mut self) -> Vec { + std::mem::take(&mut self.inner.callbacks_mut().replies) + } + /// Take the drops recorded since the last call, resetting the counters. pub(crate) fn take_clipboard_drops(&mut self) -> ClipboardDrops { std::mem::take(&mut self.inner.callbacks_mut().drops) @@ -327,6 +388,88 @@ mod tests { assert_eq!(p.take_clipboard_writes().len(), 1); } + // ── Terminal status queries ─────────────────────────────────── + // + // vt100 implements none of these, so they arrive at `unhandled_csi`. An + // application that asks and hears nothing back hangs on its own timeout. + + #[test] + fn cursor_position_report_answers_the_grid_cursor() { + // `CSI 6 n` is what crossterm's `cursor::position()` writes, and what + // atuin's history search needs before it can draw (#113). The answer is + // 1-based, so a cursor parked after "hi" on the first row is (1, 3). + let mut p = parser_with(10, 80, b"hi\x1b[6n"); + assert_eq!(p.take_replies(), b"\x1b[1;3R".to_vec()); + } + + #[test] + fn cursor_position_report_follows_the_cursor() { + // Same query from elsewhere on the grid must not hand back a constant. + let mut p = parser_with(10, 80, b"\x1b[5;7H\x1b[6n"); + assert_eq!(p.take_replies(), b"\x1b[5;7R".to_vec()); + } + + #[test] + fn device_status_report_answers_ok() { + let mut p = parser_with(10, 80, b"\x1b[5n"); + assert_eq!(p.take_replies(), b"\x1b[0n".to_vec()); + } + + #[test] + fn device_attributes_are_answered_with_and_without_a_param() { + // crossterm probes kitty-keyboard support with `ESC[?u ESC[c` and waits + // two seconds for *either* reply. The DA1 answer is what ends that wait. + for query in [&b"\x1b[c"[..], &b"\x1b[0c"[..]] { + let mut p = parser_with(10, 80, query); + assert_eq!(p.take_replies(), b"\x1b[?1;2c".to_vec(), "query {query:?}"); + } + } + + #[test] + fn private_queries_stay_unanswered() { + // We don't speak the kitty keyboard protocol (`CSI ? u`) or secondary + // device attributes (`CSI > c`). Silence is the honest answer, and it's + // what makes crossterm conclude "unsupported" once DA1 arrives. + let mut p = parser_with(10, 80, b"\x1b[?u\x1b[>c\x1b[?6n"); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn unknown_dsr_parameters_are_not_answered() { + // Making something up for a query we don't recognise is worse than not + // replying: the application would parse our answer as the wrong event. + let mut p = parser_with(10, 80, b"\x1b[n\x1b[99n"); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn take_replies_drains() { + let mut p = parser_with(10, 80, b"\x1b[5n"); + assert_eq!(p.take_replies().len(), 4); + assert!(p.take_replies().is_empty()); + } + + #[test] + fn reply_queue_is_capped() { + // A remote spinning on `CSI 5 n` must not grow our buffer without + // bound between drains. + let mut stream = Vec::new(); + for _ in 0..1000 { + stream.extend_from_slice(b"\x1b[5n"); + } + let mut p = parser_with(10, 80, &stream); + assert_eq!(p.take_replies().len(), REPLY_QUEUE_MAX_BYTES); + } + + #[test] + fn queries_do_not_reach_the_grid() { + // Regression: the query must stay invisible. If it ever landed on a + // cell the user would see escape gibberish mid-session. + let mut p = parser_with(10, 80, b"before\x1b[6nafter"); + assert_eq!(p.screen().contents().trim(), "beforeafter"); + assert_eq!(p.take_replies(), b"\x1b[1;7R".to_vec()); + } + #[test] fn decoded_len_matches_real_decode() { // Exact decoded size without pulling in a base64 decoder — the payload From 2a34f6971e3ac18da1afc85ca4046241fdf94d46 Mon Sep 17 00:00:00 2001 From: petruha Date: Thu, 20 Aug 2026 05:44:17 +0400 Subject: [PATCH 2/5] fix(session): clamp the cursor report and rate-limit the answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review of the previous commit turned up two real defects in it. The cursor position report could name a column the terminal does not have. vt100 parks the cursor one past the right margin once a character lands in the last column, resolving the pending wrap only when the next one arrives (`col_inc` is unclamped, vt100-0.16.2 grid.rs), so `cursor_position()` returns `cols` on a `cols`-wide grid and we reported `cols + 1`. That is exactly the state a prompt filling the line leaves behind — the moment the #113 reporter presses Up — and whoever measures the room left from it underflows. Clamp to the screen size, and say out loud that origin mode stays wrong because vt100 handles DECOM itself and exposes no accessor. The answers also went out unthrottled through a blocking `write_all` on the PTY master, from the single-threaded frame loop. The remote picks how often it asks; one that asks in a loop and never reads its own stdin makes `ssh` stop reading ours, and ~32 KiB of replies then park every tab, input and rendering included — reachable in under a second at the old rate. A token bucket over reply bytes puts the sustained rate below what a person typing already writes into the same PTY, and an over-budget batch is dropped whole: half an escape sequence in the remote's input is worse than an unanswered query. Also swap the reply after the auto-typed secret. Both write to the same PTY, and a reply queued ahead of the password would be read as part of it by whatever asks for a line. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 11 ++++-- src/session/mod.rs | 84 +++++++++++++++++++++++++++++++++++++++---- src/session/parser.rs | 34 +++++++++++++++--- 3 files changed, 116 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f3cec0..5fa59ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,11 +13,16 @@ All notable changes to SSHub are documented in this file. died on the first Up arrow with "The cursor position could not be read within a normal duration" — over plain `ssh` the real terminal replies and it works. The emulator now answers the cursor position report from its own grid, the - device status report, and the primary device attributes query, which also - ends the two-second stall every crossterm-based TUI took at startup while - probing for the kitty keyboard protocol. Queries we do not actually speak + device status report, and the primary device attributes query — the last of + which also ends the two-second stall in any TUI that probes for the kitty + keyboard protocol (crossterm's `supports_keyboard_enhancement`), and the + middle one the same hang in probes that end on `ESC [ 5 n` to be sure + *something* answers. Queries we do not actually speak (the kitty keyboard protocol itself, secondary device attributes) stay unanswered on purpose: silence is what tells a caller they are unsupported. + The remote decides how often it asks, so the answers are rate-limited — a + burst costs nothing, but a host stuck in a query loop cannot flood the PTY it + is asking through. ## [0.15.0] - 2026-08-18 diff --git a/src/session/mod.rs b/src/session/mod.rs index 59fcc1a..46cddf4 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -202,6 +202,10 @@ pub struct Session { pub host_name: String, /// Optional PTY transcript writer; closed on session end. log: Option, + /// Remaining byte allowance for terminal-query answers, and when it was + /// last topped up. See [`REPLY_BURST_BYTES`]. + reply_tokens: usize, + reply_refilled_at: Instant, } impl Session { @@ -283,6 +287,8 @@ impl Session { host_name: config.host_name.clone(), config, log, + reply_tokens: REPLY_BURST_BYTES, + reply_refilled_at: Instant::now(), }) } @@ -522,8 +528,12 @@ impl Session { } } if had_bytes { - self.answer_terminal_queries(); + // Secret first: both write to the same PTY, and a reply queued + // ahead of the password would be read as part of it by whatever + // asks for a line. Answering after leaves the reply as harmless + // leftover input instead. self.maybe_send_pending_secret(); + self.answer_terminal_queries(); self.maybe_reveal(); } if had_stderr { @@ -548,12 +558,27 @@ impl Session { if replies.is_empty() { return; } - if let Err(e) = self.runtime.write(&replies) { - // Prefixed `session:` so the event loop's connected-session filter - // keeps it — see [`keep_diagnostic`]. - self.diagnostics - .push(format!("session: terminal query reply failed: {e}")); + let now = Instant::now(); + let refill = (now.duration_since(self.reply_refilled_at).as_millis() as usize) + .saturating_mul(REPLY_BYTES_PER_SEC) + / 1000; + if refill > 0 { + self.reply_tokens = self + .reply_tokens + .saturating_add(refill) + .min(REPLY_BURST_BYTES); + self.reply_refilled_at = now; } + // Whole batch or nothing: half an escape sequence in the remote's input + // is worse than a query left unanswered. + if replies.len() > self.reply_tokens { + return; + } + self.reply_tokens -= replies.len(); + // A failed write needs no diagnostic of its own: the application that + // asked reports its own timeout, which tells the user more than a line + // in the SSH log would. + let _ = self.runtime.write(&replies); } /// Throw away whatever the PTY asked us to copy, including the drop @@ -939,6 +964,24 @@ const CONNECTED_NEEDLES: &[&str] = &["authenticated to ", "authenticated ("]; /// dropped from the front so a long session can't grow it without bound. const DEBUG_LOG_CAP: usize = 64 * 1024; +/// Token bucket over the bytes of terminal-query answers we write back into the +/// PTY, as a burst allowance and a sustained refill rate. Real applications ask +/// a handful of times per keystroke, so neither is ever felt; the sustained rate +/// is a fraction of what a person typing already writes into the same PTY. +/// +/// It exists because the remote decides how often it asks, and the answer goes +/// out through a blocking `write_all` on the master from the single-threaded +/// frame loop. A remote that asks in a loop and never reads its own stdin makes +/// `ssh` stop reading ours, and an unthrottled reply stream would then fill the +/// master and park every tab, input and rendering included. +/// +/// ponytail: a token bucket, not a non-blocking writer — it turns a freeze a +/// hostile remote could reach in under a second into one needing hours of its +/// cooperation. Moving every PTY write onto its own thread is the real +/// ceiling-lifter, and where to go if keystrokes ever need the same guarantee. +const REPLY_BURST_BYTES: usize = 4096; +const REPLY_BYTES_PER_SEC: usize = 256; + /// Ordered (lowercase needle → plain-language reason) map for failed connects. /// First match wins, so keep more specific patterns before generic ones. const FAILURE_EXPLANATIONS: &[(&str, &str)] = &[ @@ -1384,6 +1427,35 @@ mod prompt_tests { ); } + #[test] + fn a_query_flood_is_capped_and_a_batch_is_never_truncated() { + // The remote decides how often it asks, and the answer leaves through a + // blocking write on the PTY master from the frame loop. Over budget the + // whole batch has to be dropped — sending the part that fits would put + // half an escape sequence into the remote's input. + let mut s = scratch_session(); + s.reply_tokens = 6; // room for exactly one `ESC [ 1 ; 1 R` + s.reply_refilled_at = Instant::now(); + + s.parser.process(b"\x1b[6n\x1b[6n"); // two answers, 12 bytes + let before = s.reply_tokens; + s.answer_terminal_queries(); + assert!( + s.reply_tokens >= before, + "an over-budget batch must not be charged, had {before} left {}", + s.reply_tokens + ); + + s.parser.process(b"\x1b[6n"); // one answer, inside the budget + let before = s.reply_tokens; + s.answer_terminal_queries(); + assert!( + s.reply_tokens < before, + "a batch inside the budget must be sent, had {before} left {}", + s.reply_tokens + ); + } + /// Build a throwaway session for tests that only need a live `Session`. fn scratch_session() -> Session { let config = SessionConfig { diff --git a/src/session/parser.rs b/src/session/parser.rs index 7af23a5..9709103 100644 --- a/src/session/parser.rs +++ b/src/session/parser.rs @@ -1,6 +1,8 @@ //! VT100 parser wrapper. Maintains an in-memory `vt100::Screen` that the -//! renderer reads via `tui-term`, and relays OSC 52 clipboard writes that -//! applications inside the PTY emit. +//! renderer reads via `tui-term`, relays OSC 52 clipboard writes that +//! applications inside the PTY emit, and answers the terminal status queries +//! they send — `vt100` implements none of the latter, and an application that +//! asks and hears nothing back blocks until its own timeout. /// Largest decoded payload we'll relay from the PTY to the host clipboard /// (64 KiB). Keeps a remote from flooding the clipboard with a huge write. @@ -118,10 +120,21 @@ impl vt100::Callbacks for PtyCallbacks { let param = params.first().and_then(|p| p.first().copied()).unwrap_or(0); let reply = match (c, param) { // DSR 6 — cursor position report. Our grid mirrors the remote - // screen, so its cursor *is* the answer. Reported 1-based. + // screen, so its cursor *is* the answer. Reported 1-based, and + // clamped: vt100 parks the cursor one column *past* the right + // margin after a character lands in the last column (the pending + // wrap is only resolved when the next one arrives), so a prompt + // that fills the line would otherwise be reported at column + // `cols + 1` — a real terminal answers `cols`, and code measuring + // the room left underflows on anything else. Origin mode is the + // one case we still get wrong: vt100 handles DECOM itself and + // exposes no accessor, so the row here is absolute where a + // conformant terminal would report it relative to the region. ('n', 6) => { let (row, col) = screen.cursor_position(); - format!("\x1b[{};{}R", row + 1, col + 1).into_bytes() + let (rows, cols) = screen.size(); + let (row, col) = ((row + 1).min(rows), (col + 1).min(cols)); + format!("\x1b[{row};{col}R").into_bytes() } // DSR 5 — device status. Nothing can go wrong in an in-memory grid. ('n', 5) => b"\x1b[0n".to_vec(), @@ -409,6 +422,19 @@ mod tests { assert_eq!(p.take_replies(), b"\x1b[5;7R".to_vec()); } + #[test] + fn cursor_position_report_is_clamped_at_the_right_margin() { + // vt100 parks the cursor at column `cols` (0-based, i.e. one past the + // last cell) once a character lands in the last column, resolving the + // wrap only when the next one arrives. Unclamped that reports column + // `cols + 1` — a column the terminal does not have. A prompt that + // fills the line and then asks where it is gets this every time, and + // whoever measures the room left from it underflows. + let mut p = parser_with(3, 10, b"0123456789\x1b[6n"); + assert_eq!(p.screen().cursor_position(), (0, 10), "vt100 behaviour"); + assert_eq!(p.take_replies(), b"\x1b[1;10R".to_vec()); + } + #[test] fn device_status_report_answers_ok() { let mut p = parser_with(10, 80, b"\x1b[5n"); From 211a7b15627abb37bc5315279f4d88a4981aa087 Mon Sep 17 00:00:00 2001 From: petruha Date: Thu, 20 Aug 2026 05:58:31 +0400 Subject: [PATCH 3/5] test(session): make the cursor-report test read the whole answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dd bs=1 count=6` was right only because the cursor happened to sit at the home position. A prelude that prints anything first makes the answer longer, and the fixed count then truncates it and leaves the tail in the input queue — the child reads a wrong position instead of the test failing. Read with `-icanon min 0 time 5` and a generous count instead: whatever arrived within half a second, no guess about its length. That makes the right margin testable end to end, which is where the review found the real bug: 80 characters on an 80-column grid leave vt100's cursor in pending wrap at column 80, and the child must be told `1;80R`, not the `1;81R` it got before the clamp. Also assert that discarding a background tab's clipboard write leaves its query answers alone. Both come out of the same `PtyCallbacks`, and gating answers on visibility the way the clipboard is gated would quietly bring #113 back for every tab the user is not looking at. The queue-cap assertion checked an arithmetic accident (1024 divides by the 4-byte DSR 5 answer); assert the bound it actually means. Co-Authored-By: Claude Opus 5 (1M context) --- src/session/mod.rs | 103 +++++++++++++++++++++++++----------------- src/session/parser.rs | 9 +++- 2 files changed, 70 insertions(+), 42 deletions(-) diff --git a/src/session/mod.rs b/src/session/mod.rs index 46cddf4..be49d54 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -575,9 +575,10 @@ impl Session { return; } self.reply_tokens -= replies.len(); - // A failed write needs no diagnostic of its own: the application that - // asked reports its own timeout, which tells the user more than a line - // in the SSH log would. + // Neither an over-budget batch nor a failed write says anything out + // loud. The application that asked reports its own timeout, which tells + // the user more than a line in the SSH log would — and a diagnostic here + // would be pushed on every frame for as long as the flood lasts. let _ = self.runtime.write(&replies); } @@ -1388,23 +1389,26 @@ mod prompt_tests { ); } - #[test] - fn the_pty_child_receives_a_cursor_position_report() { - // End-to-end for #113: a child asks the terminal where the cursor is - // and *reads the answer back*. Nothing in the parser unit tests proves - // the reply actually leaves the process, and this is exactly the loop - // that hangs atuin — it asks, waits two seconds, and errors out. - // - // `stty raw -echo` so the reply arrives byte-for-byte instead of being - // line-buffered and echoed back at us; the cursor is still at the home - // position, so the answer is exactly the 6 bytes of `ESC [ 1 ; 1 R`, - // which is what lets `dd` stop without a timeout. `tr` strips the - // escape so the marker is printable on the grid. - let script = r"stty raw -echo; printf '\033[6n'; \ - r=$(dd bs=1 count=6 2>/dev/null | tr -d '\033['); \ - stty sane; printf 'CPR<%s>\r\n' $r"; + /// End-to-end for #113: a child asks the terminal where the cursor is and + /// *reads the answer back*. Nothing in the parser unit tests proves the + /// reply actually leaves the process, and this is exactly the loop that + /// hangs atuin — it asks, waits two seconds, and errors out. + /// + /// `stty raw -echo` so the reply arrives byte-for-byte rather than + /// line-buffered and echoed back at us, and `-icanon min 0 time 5` so the + /// read returns after half a second with whatever arrived — reading a fixed + /// byte count instead would have to guess the answer's length, and would + /// silently truncate it (leaving the tail in the input queue) the moment the + /// cursor is anywhere but the home position. `tr` strips the escape so the + /// marker is printable on the grid. + fn asks_for_the_cursor_position(prelude: &str) -> Session { + let script = format!( + r"stty raw -echo -icanon min 0 time 5; {prelude} printf '\033[6n'; \ + r=$(dd bs=1 count=32 2>/dev/null | tr -d '\033['); \ + stty sane; printf 'CPR<%s>\r\n' $r" + ); let config = SessionConfig { - argv: vec!["sh".into(), "-c".into(), script.into()], + argv: vec!["sh".into(), "-c".into(), script], display_name: "t".into(), meta: SessionMeta::default(), pending_secret: None, @@ -1412,7 +1416,9 @@ mod prompt_tests { host_name: "t".into(), }; let mut s = Session::spawn(config, 24, 80, None).unwrap(); - + // Bounded poll: a child that never gets its answer must fail the assert, + // not park the test binary. `Drop for PtyRuntime` kills the process + // group on the way out, so a `dd` still blocked on the read is reaped. for _ in 0..300 { s.drain(); if s.screen_tail_snippet().contains("CPR<") { @@ -1420,6 +1426,12 @@ mod prompt_tests { } std::thread::sleep(Duration::from_millis(10)); } + s + } + + #[test] + fn the_pty_child_receives_a_cursor_position_report() { + let s = asks_for_the_cursor_position(""); assert!( s.screen_tail_snippet().contains("CPR<1;1R>"), "child never got its cursor position back, tail: {:?}", @@ -1428,31 +1440,40 @@ mod prompt_tests { } #[test] - fn a_query_flood_is_capped_and_a_batch_is_never_truncated() { - // The remote decides how often it asks, and the answer leaves through a - // blocking write on the PTY master from the frame loop. Over budget the - // whole batch has to be dropped — sending the part that fits would put - // half an escape sequence into the remote's input. - let mut s = scratch_session(); - s.reply_tokens = 6; // room for exactly one `ESC [ 1 ; 1 R` - s.reply_refilled_at = Instant::now(); - - s.parser.process(b"\x1b[6n\x1b[6n"); // two answers, 12 bytes - let before = s.reply_tokens; - s.answer_terminal_queries(); + fn the_report_the_child_receives_is_clamped_at_the_right_margin() { + // The grid is 80 wide, so 80 characters leave the cursor in vt100's + // pending-wrap state at column 80 (0-based) — one past the last cell. + // Unclamped the child would be told `1;81R`, a column its terminal does + // not have, and code measuring the room left from it underflows. This is + // the state a prompt that fills the line leaves behind, i.e. the moment + // #113's reporter presses Up. + let s = asks_for_the_cursor_position(r"printf '%080d' 0;"); assert!( - s.reply_tokens >= before, - "an over-budget batch must not be charged, had {before} left {}", - s.reply_tokens + s.screen_tail_snippet().contains("CPR<1;80R>"), + "child should be told column 80 of 80, tail: {:?}", + s.screen_tail_snippet() ); + } - s.parser.process(b"\x1b[6n"); // one answer, inside the budget - let before = s.reply_tokens; - s.answer_terminal_queries(); + #[test] + fn discarding_clipboard_writes_keeps_the_query_answers() { + // The clipboard relay is gated on visibility: a background tab discards + // instead of relaying. Query answers must never be gated the same way — + // the application waits for its reply whether or not the user is looking + // at its tab — so the discard path has to leave them alone. Both come + // out of the same `PtyCallbacks`, which is what makes this worth an + // assertion rather than a comment. + let mut s = scratch_session(); + s.parser.process(b"\x1b]52;c;R0VIRUlN\x07\x1b[5n"); + s.discard_clipboard_writes(); assert!( - s.reply_tokens < before, - "a batch inside the budget must be sent, had {before} left {}", - s.reply_tokens + s.parser.take_clipboard_writes().is_empty(), + "the clipboard write is the one that gets discarded" + ); + assert_eq!( + s.parser.take_replies(), + b"\x1b[0n".to_vec(), + "the query answer must survive the discard" ); } diff --git a/src/session/parser.rs b/src/session/parser.rs index 9709103..82d4308 100644 --- a/src/session/parser.rs +++ b/src/session/parser.rs @@ -484,7 +484,14 @@ mod tests { stream.extend_from_slice(b"\x1b[5n"); } let mut p = parser_with(10, 80, &stream); - assert_eq!(p.take_replies().len(), REPLY_QUEUE_MAX_BYTES); + // The invariant is the bound, not an exact number: only whole replies + // are queued, so where the cap lands depends on how long they are. + let queued = p.take_replies().len(); + assert!(queued <= REPLY_QUEUE_MAX_BYTES, "over the cap: {queued}"); + assert!( + queued > REPLY_QUEUE_MAX_BYTES - 4, + "cap not actually reached: {queued}" + ); } #[test] From b09148a8ae2e713a2abd9e3d22d4b1dc5309d6e5 Mon Sep 17 00:00:00 2001 From: petruha Date: Thu, 20 Aug 2026 06:21:06 +0400 Subject: [PATCH 4/5] fix(session): write to the PTY off the frame loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured, because the token bucket in 2a34f69 turned out to only postpone the freeze rather than remove it. Same hostile shape both times — a child in raw mode emitting `ESC [ 5 n` in a loop and never reading its own stdin, which is what a remote in a query loop does to the local `ssh` that feeds us: before drain() stopped returning at t≈76-80s and never came back (4096 burst + 256 B/s reaches the master's ~20 KiB limit at t≈64s, so the arithmetic matches the observation) after 9401 drains over 200s, worst single drain 98ms, ran to completion A control with the identical loop emitting plain text instead of queries completed in both builds, so the reply write was the only difference. `write_all` on the PTY master blocks once the child stops reading, and `Session::drain` runs for every session on the single frame loop — so that block parked every tab, input and rendering included, with no timeout and no recovery. The master fd is `dup`ed for the reader, so O_NONBLOCK is not available per-fd; the write moves to its own thread behind a bounded queue instead. One thread keeps keystrokes, the auto-typed secret, pastes and query answers in order, and a queue that fills means the far end is gone, which is when dropping is the honest outcome. The thread is not joined on drop: waiting for a blocked write would just move the freeze into session teardown. The token bucket stays, with its claim corrected. It no longer guards against the freeze — it keeps a query flood from filling the shared write queue and dropping the user's keystrokes instead. Co-Authored-By: Claude Opus 5 (1M context) --- src/session/mod.rs | 18 ++++++------- src/session/pty.rs | 66 ++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/session/mod.rs b/src/session/mod.rs index be49d54..627e7ce 100644 --- a/src/session/mod.rs +++ b/src/session/mod.rs @@ -970,16 +970,16 @@ const DEBUG_LOG_CAP: usize = 64 * 1024; /// a handful of times per keystroke, so neither is ever felt; the sustained rate /// is a fraction of what a person typing already writes into the same PTY. /// -/// It exists because the remote decides how often it asks, and the answer goes -/// out through a blocking `write_all` on the master from the single-threaded -/// frame loop. A remote that asks in a loop and never reads its own stdin makes -/// `ssh` stop reading ours, and an unthrottled reply stream would then fill the -/// master and park every tab, input and rendering included. +/// It exists because the remote decides how often it asks, and the write queue +/// it answers into is shared with the user's keystrokes. Unthrottled, a host in +/// a query loop fills the PTY write queue on its own and +/// the typing gets dropped instead. Keeping the answers to a trickle leaves the +/// queue for the person at the keyboard. /// -/// ponytail: a token bucket, not a non-blocking writer — it turns a freeze a -/// hostile remote could reach in under a second into one needing hours of its -/// cooperation. Moving every PTY write onto its own thread is the real -/// ceiling-lifter, and where to go if keystrokes ever need the same guarantee. +/// What it does *not* do is prevent the freeze: measured against the blocking +/// write this bucket only moved the wedge from under a second to ~80 s +/// (4096 + 256/s reaching the master's ~20 KiB limit at t≈64 s). That is what +/// the writer thread is for. const REPLY_BURST_BYTES: usize = 4096; const REPLY_BYTES_PER_SEC: usize = 256; diff --git a/src/session/pty.rs b/src/session/pty.rs index e618854..e01f6cf 100644 --- a/src/session/pty.rs +++ b/src/session/pty.rs @@ -8,7 +8,7 @@ use std::os::unix::ffi::OsStrExt; use std::os::unix::fs::OpenOptionsExt; use std::path::PathBuf; use std::sync::atomic::{AtomicBool, AtomicU64, Ordering}; -use std::sync::mpsc::{self, Receiver}; +use std::sync::mpsc::{self, Receiver, SyncSender, TrySendError}; use std::sync::Arc; use std::thread::{self, JoinHandle}; use std::time::Duration; @@ -18,6 +18,19 @@ use portable_pty::{native_pty_system, Child, CommandBuilder, MasterPty, PtySize} const READ_BUF: usize = 4096; +/// How many pending writes toward the PTY we hold before dropping them. +/// +/// The writer sits on its own thread for one reason: `write_all` on the master +/// blocks once the child stops reading its stdin. A remote stuck in a terminal +/// query loop that never reads its own input makes `ssh` stop reading ours, the +/// master fills after ~20 KiB, and doing that write on the frame loop parked +/// every tab, input and rendering included — measured, not theorised. Off the +/// frame loop the only cost of a wedged remote is that writes aimed at it pile +/// up, and this is where the pile stops: a session that cannot be written to is +/// one where dropping is the honest outcome. Human typing never queues this +/// deep, so a full queue always means the far end is gone. +const WRITE_QUEUE_LEN: usize = 64; + /// Env var carrying the stderr FIFO path into the `sh` wrapper. const STDERR_FIFO_ENV: &str = "SSHUB_STDERR_FIFO"; @@ -88,7 +101,9 @@ impl Drop for StderrFifo { pub struct PtyRuntime { master: Box, - writer: Box, + /// Sender into the writer thread. `None` only while dropping, which is what + /// tells the thread to exit. + write_tx: Option>>, rx: Receiver, /// Set when the reader has signalled EOF / child exit. Used so we don't /// keep spinning on a dead PTY. @@ -168,7 +183,24 @@ impl PtyRuntime { drop(pair.slave); let mut reader = pair.master.try_clone_reader().context("clone pty reader")?; - let writer = pair.master.take_writer().context("take pty writer")?; + let mut writer = pair.master.take_writer().context("take pty writer")?; + + // Everything written toward the child goes through here, in order: + // keystrokes, the auto-typed secret, pastes, and the emulator's answers + // to terminal queries. One thread keeps that ordering while taking the + // blocking write off the frame loop. It ends when the sender drops. + let (write_tx, write_rx) = mpsc::sync_channel::>(WRITE_QUEUE_LEN); + thread::Builder::new() + .name("sshub-pty-writer".into()) + .spawn(move || { + for chunk in write_rx { + if writer.write_all(&chunk).is_err() { + break; + } + writer.flush().ok(); + } + }) + .context("spawn pty writer thread")?; let (tx, rx) = mpsc::channel(); let stderr_tx = tx.clone(); @@ -238,7 +270,7 @@ impl PtyRuntime { Ok(Self { master: pair.master, - writer, + write_tx: Some(write_tx), rx, closed, reader_thread: Some(reader_thread), @@ -254,11 +286,23 @@ impl PtyRuntime { self.rx.try_recv().ok() } - /// Write bytes to the master side. Called for each forwarded keystroke. + /// Hand bytes to the writer thread. Never blocks: see [`WRITE_QUEUE_LEN`] + /// for why the write cannot happen on the caller's thread. The cost is that + /// a write failure is no longer reported synchronously — a full queue is, + /// and it is the only failure a caller can do anything about (nothing is + /// reaching the remote). pub fn write(&mut self, bytes: &[u8]) -> Result<()> { - self.writer.write_all(bytes)?; - self.writer.flush().ok(); - Ok(()) + let tx = self + .write_tx + .as_ref() + .ok_or_else(|| anyhow!("pty writer is gone"))?; + match tx.try_send(bytes.to_vec()) { + Ok(()) => Ok(()), + Err(TrySendError::Full(_)) => { + Err(anyhow!("pty write queue full — the remote is not reading")) + } + Err(TrySendError::Disconnected(_)) => Err(anyhow!("pty writer has exited")), + } } pub fn resize(&self, rows: u16, cols: u16) -> Result<()> { @@ -317,6 +361,12 @@ fn terminate_child_process(child: &mut dyn portable_pty::Child) { impl Drop for PtyRuntime { fn drop(&mut self) { self.terminate_child(); + // Closing the channel is what ends the writer thread; it is deliberately + // not joined. A write blocked on a child that stopped reading only + // returns once the slave closes, and while `terminate_child` above makes + // that happen, waiting on it here would move the freeze we just fixed + // into session teardown. The thread owns nothing but its own fd. + self.write_tx = None; self.stderr_stop.store(true, Ordering::Relaxed); if let Some(handle) = self.stderr_reader.take() { let _ = handle.join(); From 55b7df7e100f41fc8a19561a8de0b0c7b7461675 Mon Sep 17 00:00:00 2001 From: petruha Date: Thu, 20 Aug 2026 06:21:20 +0400 Subject: [PATCH 5/5] docs(changelog): record the PTY write freeze fix Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5fa59ee..594500f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,8 +21,18 @@ All notable changes to SSHub are documented in this file. (the kitty keyboard protocol itself, secondary device attributes) stay unanswered on purpose: silence is what tells a caller they are unsupported. The remote decides how often it asks, so the answers are rate-limited — a - burst costs nothing, but a host stuck in a query loop cannot flood the PTY it - is asking through. + burst costs nothing, but a host stuck in a query loop cannot crowd the user's + keystrokes out of the PTY write queue. + +- **A remote that stops reading its input can no longer freeze the app** - + writing to the PTY blocks once the child stops reading, and it was happening + on the frame loop, so a host in a terminal-query loop that never read its own + stdin parked every tab, input and rendering included, with no timeout and no + recovery. Measured: `drain()` stopped returning after ~80 s of such a flood + and never came back. PTY writes now go through their own thread behind a + bounded queue — keystrokes, the auto-typed secret, pastes and query answers + stay in order, and a queue that fills means nothing is reaching the remote + anyway. The same flood now runs indefinitely with a worst frame of 98 ms. ## [0.15.0] - 2026-08-18