fix: fix panics from vt100::Parser::new (#3933)

`vt100` has an [upstream
issue](https://github.com/doy/vt100-rust/issues/37) where panics can
occur if `Parser::new` is called with a `rows` or `cols` parameter less
than 2. This PR adds a wrapper function that clamps the values so they
are never less than 2.

Fixes #3930.

clippy.toml

@@ -1,8 +1,31 @@
-disallowed-methods = [
-    { path = "std::time::Duration::new", reason = "panics on overflow; use atuin_common::time::DurationExt::try_new, or Duration::from_secs when nanos are 0" },
-    { path = "time::Duration::new", reason = "panics on overflow; use atuin_common::time::DurationExt::try_new" },
-    { path = "std::time::Duration::from_secs_f64", reason = "panics on NaN/negative/overflow; use Duration::try_from_secs_f64" },
-    # https://github.com/time-rs/time/issues/802
-    { path = "time::OffsetDateTime::from_unix_timestamp_nanos", reason = "wraps out-of-range input to a valid-looking wrong date (time-rs/time#802); use atuin_common::time::OffsetDateTimeExt::from_unix_nanos" },
-    { path = "time::UtcDateTime::from_unix_timestamp_nanos", reason = "same truncation bug as OffsetDateTime (time-rs/time#802); range-check the full i128 before converting" },
-]
+[[disallowed-methods]]
+path = "std::time::Duration::new"
+reason = """panics on overflow; use atuin_common::time::DurationExt::try_new, or \
+    Duration::from_secs when nanos are 0"""
+
+[[disallowed-methods]]
+path = "time::Duration::new"
+reason = "panics on overflow; use atuin_common::time::DurationExt::try_new"
+
+[[disallowed-methods]]
+path = "std::time::Duration::from_secs_f64"
+reason = "panics on NaN/negative/overflow; use Duration::try_from_secs_f64"
+
+# https://github.com/time-rs/time/issues/802
+[[disallowed-methods]]
+path = "time::OffsetDateTime::from_unix_timestamp_nanos"
+reason = """wraps out-of-range input to a valid-looking wrong date (time-rs/time#802); use \
+    atuin_common::time::OffsetDateTimeExt::from_unix_nanos"""
+
+[[disallowed-methods]]
+path = "time::UtcDateTime::from_unix_timestamp_nanos"
+reason = """same truncation bug as OffsetDateTime (time-rs/time#802); range-check the full i128 \
+    before converting"""
+
+[[disallowed-methods]]
+path = "vt100::Parser::new"
+reason = "can panic; `use atuin_common::ansi::Vt100ParserExt as _` and use `Parser::new_safe`"
+
+[[disallowed-methods]]
+path = "vt100::Screen::set_size"
+reason = "can panic; `use atuin_common::ansi::Vt100ScreenExt as _` and use `Screen::set_size_safe`"

crates/atuin-ai/src/tools/mod.rs

@@ -6,7 +6,7 @@ use std::{
 };
 
 use atuin_client::history::AuthorPattern;
-use atuin_common::ansi;
+use atuin_common::ansi::{self, Vt100ParserExt as _};
 use atuin_common::filter::OrFilter;
 use atuin_common::time::UtcOffsetExt;
 use enum_dispatch::enum_dispatch;
@@ -875,7 +875,7 @@ pub(crate) async fn execute_shell_command_streaming(
     let stderr = child.stderr.take().expect("stderr was piped");
 
     // VT100 emulator for the live preview (viewport-sized)
-    let mut parser = vt100::Parser::new(PREVIEW_HEIGHT, PREVIEW_WIDTH, 0);
+    let mut parser = vt100::Parser::new_safe(PREVIEW_HEIGHT, PREVIEW_WIDTH, 0);
 
     let mut stdout_reader = tokio::io::BufReader::new(stdout);
     let mut stderr_reader = tokio::io::BufReader::new(stderr);

crates/atuin-common/src/ansi.rs

@@ -6,6 +6,50 @@ use std::num::NonZeroU16;
 /// Arbitrary upper bound on the emulated screen height, in rows. Mitigates OOMs with long output.
 const MAX_ROWS: usize = 16_384;
 
+/// Extension trait for [`vt100::Parser`].
+pub trait Vt100ParserExt: Sized {
+    fn new_safe(rows: u16, cols: u16, scrollback_len: usize) -> Self;
+}
+
+/// Extension trait for [`vt100::Screen`].
+pub trait Vt100ScreenExt: Sized {
+    fn set_size_safe(&mut self, rows: u16, cols: u16);
+}
+
+impl Vt100ParserExt for vt100::Parser {
+    /// Create a new [`vt100::Parser`].
+    ///
+    /// Prefer this function over calling [`vt100::Parser::new`] directly. [`vt100`] has [a bug]
+    /// that can cause panics when `rows` or `cols` is less than 2. This function avoids that
+    /// case by clamping `rows` and `cols` to 2 if either is less than that.
+    ///
+    /// [a bug]: https://github.com/doy/vt100-rust/issues/37
+    fn new_safe(rows: u16, cols: u16, scrollback_len: usize) -> Self {
+        #[allow(
+            clippy::disallowed_methods,
+            reason = "this is the one safe wrapper around the disallowed function"
+        )]
+        Self::new(rows.max(2), cols.max(2), scrollback_len)
+    }
+}
+
+impl Vt100ScreenExt for vt100::Screen {
+    /// Set the size of a [`vt100::Screen`].
+    ///
+    /// Prefer this function over calling [`vt100::Screen::set_size`] directly. [`vt100`] has
+    /// [a bug] that can cause panics when `rows` or `cols` is less than 2. This method avoids
+    /// that case by clamping `rows` and `cols` to 2 if either is less than that.
+    ///
+    /// [a bug]: https://github.com/doy/vt100-rust/issues/37
+    fn set_size_safe(&mut self, rows: u16, cols: u16) {
+        #[allow(
+            clippy::disallowed_methods,
+            reason = "this is the one safe wrapper around the disallowed method"
+        )]
+        self.set_size(rows.max(2), cols.max(2))
+    }
+}
+
 /// Render ANSI-encoded terminal output to plain text, as it would appear on a `cols`-wide terminal.
 ///
 /// Uses [`vt100::Parser`] under the hood meaning that backspaces, ANSI codes, etc. are gracefully
@@ -18,14 +62,6 @@ pub fn to_plain_text(input: impl AsRef<[u8]>, cols: NonZeroU16) -> String {
         return String::new();
     }
 
-    // vt100 assumes at least two columns when a double-width character
-    // arrives (`size.cols - width` underflows, panicking in debug builds),
-    // so never emulate a one-column screen.
-    //
-    // TODO: Remove this once upstream issue is fixed:
-    // https://github.com/doy/vt100-rust/issues/37
-    let cols = cols.get().max(2);
-
     let mut newlines = 0usize;
     let normalized: Vec<u8> = onlcr(bytes)
         .inspect(|&b| {
@@ -41,13 +77,13 @@ pub fn to_plain_text(input: impl AsRef<[u8]>, cols: NonZeroU16) -> String {
     // We then add the bytes/cols case for the extra rows created due to soft-wrapping.
     // Note this overshoots, but that's OK, we'll clean up later.
     let newline_rows = newlines + 1;
-    let wrapped_rows = normalized.len() / cols as usize;
+    let wrapped_rows = normalized.len() / usize::from(cols.get());
     let rows = newline_rows
         .saturating_add(wrapped_rows)
         .saturating_add(1)
-        .clamp(1, MAX_ROWS.min(u16::MAX as usize)) as u16;
+        .clamp(1, MAX_ROWS.min(usize::from(u16::MAX))) as u16;
 
-    let mut parser = vt100::Parser::new(rows, cols, 0);
+    let mut parser = vt100::Parser::new_safe(rows, cols.get(), 0);
     parser.process(&normalized);
 
     // The emulator renders onto a fixed grid, so `contents()` comes back with each row right-padded

crates/atuin-lab-share/src/lib.rs

@@ -1,4 +1,5 @@
 #![deny(unsafe_code)]
+#![allow(clippy::disallowed_methods)]
 
 //! Experimental terminal sharing for atuin (`atuin lab share`).
 //!

crates/atuin-pty-proxy/src/protocol.rs

@@ -521,7 +521,9 @@ mod tests {
 
     #[test]
     fn snapshot_roundtrip_through_vt100() {
-        let mut parser = vt100::Parser::new(4, 10, 0);
+        use atuin_common::ansi::Vt100ParserExt as _;
+
+        let mut parser = vt100::Parser::new_safe(4, 10, 0);
         parser.process(b"hi\r\nthere\x1b[1;3H");
         let snapshot = Snapshot::from_screen(parser.screen());
         assert_eq!((snapshot.rows, snapshot.cols), (4, 10));

crates/atuin-pty-proxy/src/screen.rs

@@ -12,6 +12,7 @@ use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::mpsc::{self, Receiver, SyncSender};
 use std::time::Duration;
 
+use atuin_common::ansi::{Vt100ParserExt as _, Vt100ScreenExt as _};
 use rand::RngCore;
 
 use crate::protocol;
@@ -254,11 +255,10 @@ struct ParserState {
 
 impl ParserState {
     fn new(rows: u16, cols: u16) -> Self {
-        // vt100 0.16 underflows (and panics in debug builds) on a 0-row or
-        // 0-column grid; clamp here and in the Resize arm.
-        let (rows, cols) = (rows.max(1), cols.max(1));
+        let parser = vt100::Parser::new_safe(rows, cols, 0);
+        let (rows, cols) = parser.screen().size();
         Self {
-            parser: vt100::Parser::new(rows, cols, 0),
+            parser,
             subscribers: Vec::new(),
             rows,
             cols,
@@ -285,7 +285,7 @@ impl ParserState {
         let caught =
             std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| op(&mut self.parser)));
         if caught.is_err() {
-            self.parser = vt100::Parser::new(self.rows, self.cols, 0);
+            self.parser = vt100::Parser::new_safe(self.rows, self.cols, 0);
         }
     }
 
@@ -302,10 +302,10 @@ impl ParserState {
                 }
             }
             Msg::Resize { rows, cols } => {
-                let (rows, cols) = (rows.max(1), cols.max(1));
+                self.vt100_guarded(|parser| parser.screen_mut().set_size_safe(rows, cols));
+                let (rows, cols) = self.parser.screen().size();
                 self.rows = rows;
                 self.cols = cols;
-                self.vt100_guarded(|parser| parser.screen_mut().set_size(rows, cols));
                 let frame = protocol::resize_frame(rows, cols);
                 self.fan_out(&frame);
             }
@@ -721,6 +721,7 @@ fn peer_uid_matches(stream: &UnixStream) -> bool {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use rstest::rstest;
 
     #[test]
     fn zero_dimension_screens_do_not_panic() {
@@ -740,18 +741,18 @@ mod tests {
         state.handle(Msg::ScreenRequest(reply_tx));
         let blob = reply_rx.recv().unwrap();
         let snapshot = protocol::Snapshot::decode(&blob).unwrap();
-        assert_eq!((snapshot.rows, snapshot.cols), (1, 1));
+
+        // Dimensions need to be clamped to (2, 2) to avoid an upstream vt100 issue.
+        assert_eq!((snapshot.rows, snapshot.cols), (2, 2));
     }
 
-    /// vt100 0.16 panics in `col_wrap` when text wraps on a 1-row grid.
-    /// The guard must absorb it: the parser thread is the sole owner of the
-    /// screen model, so an unwinding one leaves every connection thread with
-    /// nothing to serve — no snapshots, no subscribers, and `--active` gone
-    /// for the shell's remaining lifetime. Raw-byte fan-out and the snapshot
-    /// service must both survive the panic.
-    #[test]
-    fn vt100_wrap_panic_does_not_kill_the_parser() {
-        let mut state = ParserState::new(1, 3);
+    /// Make sure <https://github.com/doy/vt100-rust/issues/37> doesn't kill our parser.
+    #[rstest]
+    fn vt100_wrap_panic_does_not_kill_the_parser(
+        #[values(0, 1, 2, 3)] rows: u16,
+        #[values(0, 1, 2, 3)] cols: u16,
+    ) {
+        let mut state = ParserState::new(rows, cols);
         let (frames_tx, frames_rx) = mpsc::sync_channel(SUBSCRIBER_QUEUE_FRAMES);
         state.handle(Msg::Subscribe {
             id: 1,
@@ -772,7 +773,10 @@ mod tests {
         state.handle(Msg::ScreenRequest(reply_tx));
         let blob = reply_rx.recv().expect("parser still answering");
         let snapshot = protocol::Snapshot::decode(&blob).unwrap();
-        assert_eq!((snapshot.rows, snapshot.cols), (1, 3));
+
+        // To avoid the upstream vt100 issue, the snapshot size needs to be clamped so neither
+        // dimension is less than 2.
+        assert_eq!((snapshot.rows, snapshot.cols), (rows.max(2), cols.max(2)));
     }
 
     #[test]
@@ -795,7 +799,9 @@ mod tests {
         let mut cursor = std::io::Cursor::new(resize);
         let (frame_type, payload) = protocol::read_frame(&mut cursor).unwrap().unwrap();
         assert_eq!(frame_type, protocol::FRAME_RESIZE);
-        assert_eq!(protocol::decode_resize(&payload), Some((1, 1)));
+
+        // Due to an upstream issue in vt100, each dimension is clamped to 2.
+        assert_eq!(protocol::decode_resize(&payload), Some((2, 2)));
     }
 
     #[test]