fix-deviant(terminal): no panics on input-reachable terminal states
Intentional deviation from Ghostty 1.3.1 (DEVIANT_FIXES.md entry 10). Two terminal inputs reach states that Zig handles only by assertion (Debug/ReleaseSafe abort; ReleaseFast continues): - Toggling mode 2027 around a BOM before a wide character hits `assert(!b)` in print_grapheme. This repository's release profile keeps debug assertions, so terminal output could crash a release build. The assertion is removed and the path behaves like Zig ReleaseFast. - A grapheme (e.g. a Devanagari conjunct) at the right margin on the bottom row with scrolling margins returned InvalidCoordinate and left a grapheme flag without data, panicking later in the formatter. The grapheme now moves from the cell that actually holds it and no cell is flagged without data. tools/difftest gains a Rust-only panic hunt (`difftest_fuzz panics`, lenient driver): ~60k debug-build cases now run without any panic, and 3000 Zig differential cases show 0 divergences. Test Plan: - cargo test -p ghostty-terminal difftest_mode_2027_toggle_with_bom_does_not_panic difftest_grapheme_wrap_scrolling_margins_keeps_grapheme - cargo run -p difftest --bin difftest_fuzz -- panics
This commit is contained in:
1 parent
7b59457cb8
commit
325288b7ee
5 files changed
+104
-7
No files matched your search
@@ -1078,6 +1078,31 @@ mod tests {
|
|||||||
assert!(!screen_row_wrap(&t, 1));
|
assert!(!screen_row_wrap(&t, 1));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn difftest_grapheme_wrap_scrolling_margins_keeps_grapheme() {
|
||||||
|
// Same Devanagari conjunct at the right margin, but on the bottom row
|
||||||
|
// so the wrap scrolls the margin region and the grapheme data travels
|
||||||
|
// with its row. Zig asserts/faults (`moveGrapheme` on a blank cell);
|
||||||
|
// Rust must neither error nor leave a grapheme flag without data.
|
||||||
|
let mut t = term(255, 5);
|
||||||
|
t.vt_stream()
|
||||||
|
.next_slice("\x1b[?2027h\x1b[?69h\x1b[;9s\x1b[24H\t\u{915}\u{94d}\u{937}".as_bytes())
|
||||||
|
.unwrap();
|
||||||
|
let text = t.plain_string().unwrap();
|
||||||
|
assert!(text.contains('\u{915}'));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn difftest_mode_2027_toggle_with_bom_does_not_panic() {
|
||||||
|
// Zig asserts `!graphemeBreak` while replaying an existing grapheme
|
||||||
|
// (aborts in safe builds, ignored in ReleaseFast). It must never panic
|
||||||
|
// here: this repository's release profile enables debug assertions.
|
||||||
|
let mut t = term(20, 8);
|
||||||
|
t.vt_stream()
|
||||||
|
.next_slice(b"\x1b[?2027la\xef\xbb\xbf\x1b[?2027h\xe4\xb8\xad")
|
||||||
|
.unwrap();
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn difftest_grapheme_wrap_with_margins_moves_prev_cell() {
|
fn difftest_grapheme_wrap_with_margins_moves_prev_cell() {
|
||||||
// Devanagari conjunct widening the cell at the last column while a
|
// Devanagari conjunct widening the cell at the last column while a
|
||||||
|
|||||||
@@ -820,8 +820,12 @@ impl Terminal {
|
|||||||
.expect("prev cell");
|
.expect("prev cell");
|
||||||
let cps = page.lookup_grapheme(idx).expect("grapheme data");
|
let cps = page.lookup_grapheme(idx).expect("grapheme data");
|
||||||
for &cp2 in cps {
|
for &cp2 in cps {
|
||||||
let b = ghostty_unicode::grapheme_break(cp1, cp2, &mut state);
|
// Zig asserts `!graphemeBreak(cp1, cp2)` here, but that
|
||||||
debug_assert!(!b);
|
// is reachable from input (mode 2027 toggled after the
|
||||||
|
// grapheme was built, e.g. with a BOM), so it must not
|
||||||
|
// panic. ReleaseFast ignores the result; we do too. This
|
||||||
|
// only advances `state`.
|
||||||
|
let _ = ghostty_unicode::grapheme_break(cp1, cp2, &mut state);
|
||||||
cp1 = cp2;
|
cp1 = cp2;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -943,18 +947,31 @@ impl Terminal {
|
|||||||
let new_idx = page.cell_index(new_x, new_y).expect("new cell");
|
let new_idx = page.cell_index(new_x, new_y).expect("new cell");
|
||||||
// Zig moves from `prev.cell`, NOT from `old_pin`
|
// Zig moves from `prev.cell`, NOT from `old_pin`
|
||||||
// (they differ when margins keep the row from
|
// (they differ when margins keep the row from
|
||||||
// scrolling the cell out of `old_pin`).
|
// scrolling the cell out of `old_pin`). When
|
||||||
let old_idx = prev_idx;
|
// the wrap DID scroll the margin region, the
|
||||||
|
// data travelled with its row to `old_pin` and
|
||||||
|
// `prev.cell` is blank: Zig then asserts
|
||||||
|
// (page.zig moveGrapheme) or faults in
|
||||||
|
// ReleaseFast. Move from where the data really
|
||||||
|
// is, and never flag a cell without data.
|
||||||
|
let old_idx = if page.cells[prev_idx].has_grapheme() {
|
||||||
|
prev_idx
|
||||||
|
} else {
|
||||||
|
page.cell_index(old_x, old_y).expect("old cell")
|
||||||
|
};
|
||||||
|
if page.cells[old_idx].has_grapheme() {
|
||||||
page.move_grapheme(old_idx, new_idx)?;
|
page.move_grapheme(old_idx, new_idx)?;
|
||||||
page.cells[old_idx].set_content_tag(ContentTag::Codepoint);
|
page.cells[old_idx].set_content_tag(ContentTag::Codepoint);
|
||||||
page.cells[new_idx].set_content_tag(ContentTag::CodepointGrapheme);
|
page.cells[new_idx]
|
||||||
|
.set_content_tag(ContentTag::CodepointGrapheme);
|
||||||
page.rows[new_y].set_grapheme(true);
|
page.rows[new_y].set_grapheme(true);
|
||||||
|
}
|
||||||
page.update_row_grapheme_flag(old_y);
|
page.update_row_grapheme_flag(old_y);
|
||||||
} else {
|
} else {
|
||||||
let cps: Vec<u32> = {
|
let cps: Vec<u32> = {
|
||||||
let page = self.screen().page(old_pin.node);
|
let page = self.screen().page(old_pin.node);
|
||||||
let old_idx = page.cell_index(old_x, old_y).expect("old cell");
|
let old_idx = page.cell_index(old_x, old_y).expect("old cell");
|
||||||
page.lookup_grapheme(old_idx).expect("grapheme").to_vec()
|
page.lookup_grapheme(old_idx).unwrap_or_default().to_vec()
|
||||||
};
|
};
|
||||||
for cp in cps {
|
for cp in cps {
|
||||||
let x = self.screen().cursor.x;
|
let x = self.screen().cursor.x;
|
||||||
|
|||||||
@@ -35,7 +35,7 @@ when this file was written.
|
|||||||
| 7 | Core: selection replacement | `Surface.zig:2310-2322` (`setSelection`) | `select()` untracks the previous selection's pins, then `sel.eql(prev)` compares through those released pins (stale pointers). | The comparison happens before the old selection is replaced. (Rust handles are checked, so the naive port panicked; the e2e tests found it.) | 2f58f02 | `select_all_twice_with_copy_on_select_does_not_panic` (`ghostty-core/tests/e2e_bindings.rs`), `drag_selects_and_copy_on_select_fills_selection_clipboard` (`e2e_mouse.rs`). Dedicated eql-before-replace test: unverified |
|
| 7 | Core: selection replacement | `Surface.zig:2310-2322` (`setSelection`) | `select()` untracks the previous selection's pins, then `sel.eql(prev)` compares through those released pins (stale pointers). | The comparison happens before the old selection is replaced. (Rust handles are checked, so the naive port panicked; the e2e tests found it.) | 2f58f02 | `select_all_twice_with_copy_on_select_does_not_panic` (`ghostty-core/tests/e2e_bindings.rs`), `drag_selects_and_copy_on_select_fills_selection_clipboard` (`e2e_mouse.rs`). Dedicated eql-before-replace test: unverified |
|
||||||
| 8 | Datastruct: `lru` eviction report | `datastruct/lru.zig:163-166` | `entry.data.key` is overwritten with the new key before `entry.data` is copied out as `evicted`, so the evicted key is wrong. | `GetOrPutResult.evicted` carries the old key and old value. | 5b8c92b | `get_or_put` checks only `evicted.value`; the key is not asserted (`ghostty-datastruct/src/lru.rs`) |
|
| 8 | Datastruct: `lru` eviction report | `datastruct/lru.zig:163-166` | `entry.data.key` is overwritten with the new key before `entry.data` is copied out as `evicted`, so the evicted key is wrong. | `GetOrPutResult.evicted` carries the old key and old value. | 5b8c92b | `get_or_put` checks only `evicted.value`; the key is not asserted (`ghostty-datastruct/src/lru.rs`) |
|
||||||
| 9 | Terminal: stale cell pointer in grapheme attach | `terminal/Terminal.zig:482` and `:539` | `prev.cell` survives a page reallocation (`increaseCapacity` in `printCell` of the spacer tail), so `appendGrapheme` writes into freed memory and the grapheme (for example U+FE0F) is lost. | Cells are addressed by index; the grapheme is kept. | 4eb03f6 | `difftest_upstream_uaf_grapheme_survives_page_growth` (`ghostty-terminal/src/stream_readonly.rs`); the difftest harness stores such mismatches as `suspect*` |
|
| 9 | Terminal: stale cell pointer in grapheme attach | `terminal/Terminal.zig:482` and `:539` | `prev.cell` survives a page reallocation (`increaseCapacity` in `printCell` of the spacer tail), so `appendGrapheme` writes into freed memory and the grapheme (for example U+FE0F) is lost. | Cells are addressed by index; the grapheme is kept. | 4eb03f6 | `difftest_upstream_uaf_grapheme_survives_page_growth` (`ghostty-terminal/src/stream_readonly.rs`); the difftest harness stores such mismatches as `suspect*` |
|
||||||
| 10 | Terminal: asserts and segfaults reachable from input | `Terminal.zig:375` (`assert(!graphemeBreak)` after mode 2027 with U+2028/U+2060/BOM), `terminal/page.zig:1012` (`assert(y < size.rows)`; Devanagari conjuncts at the right edge with margins), `Terminal.zig:834` (`assert(!cell.hyperlink)`; REP with a huge count while a hyperlinked page grows) | Zig aborts or segfaults (roughly 0.1-0.4 % of generated difftest cases). | Rust handles the input without panicking. Zig-aborting cases are excluded from the differential comparison, not treated as divergences. | 4eb03f6 | `difftest_grapheme_wrap_with_margins_moves_prev_cell` and the harness (`tools/difftest`, `ZIG CRASH` handling) |
|
| 10 | Terminal: asserts and segfaults reachable from input | `Terminal.zig:375` (`assert(!unicode.graphemeBreak(cp1, cp2, &state))` after mode 2027 is toggled with U+2028/U+2060/BOM), `terminal/page.zig:1012` (`assert(y < size.rows)`; Devanagari conjunct at the right margin, including the bottom-row case where the margin wrap scrolls and `moveGrapheme(prev.cell, ...)` runs on a blank cell), `Terminal.zig:834` (`assert(!cell.hyperlink)`; REP with a huge count while a hyperlinked page grows) | Zig aborts (ReleaseSafe) or faults (ReleaseFast) in the margin case; the first and third abort only with assertions on (roughly 0.1-0.4 % of generated difftest cases). | No panic and no error. The grapheme replay ignores the break result like ReleaseFast (the assertion is dropped, because this repository's `release` profile enables debug assertions, so input could crash a release build). The margin case moves the grapheme from the cell that actually holds it (`prev.cell`, else the cell above) and never flags a cell without data. Verified with a Rust-only panic hunt (`difftest_fuzz panics`, debug build, about 60k cases, 0 panics) and the Zig differential run (0 divergences). Zig-aborting cases are excluded from the differential comparison. | 4eb03f6 (REP, first margin case); Z21 (uncommitted): assertion and bottom-row margin case | `difftest_grapheme_wrap_with_margins_moves_prev_cell`, `difftest_grapheme_wrap_scrolling_margins_keeps_grapheme`, `difftest_mode_2027_toggle_with_bom_does_not_panic`, `tools/difftest` (`ZIG CRASH` handling, `panics` mode) |
|
||||||
| 11 | VT: tmux control-mode ids | `terminal/tmux/control.zig:199,230,270,308` (`parseInt(usize) catch unreachable`) | An id that overflows `usize` hits `unreachable` (crash/UB). | The line is treated as a failed match. | b2b248e | none |
|
| 11 | VT: tmux control-mode ids | `terminal/tmux/control.zig:199,230,270,308` (`parseInt(usize) catch unreachable`) | An id that overflows `usize` hits `unreachable` (crash/UB). | The line is treated as a failed match. | b2b248e | none |
|
||||||
| 12 | VT: SGR separator bitset | `terminal/sgr.zig:228,514` (`params_sep.isSet(idx)` past `MAX_PARAMS`) | Asserts / out-of-range in `StaticBitSet` for long parameter lists. | Bounds-tolerant `sep_is_set`. | b2b248e | `sgr_underline_colon_with_trailing_separator_and_short_slice` (`ghostty-vt/src/sgr.rs`); whether Zig's path is reachable: unverified |
|
| 12 | VT: SGR separator bitset | `terminal/sgr.zig:228,514` (`params_sep.isSet(idx)` past `MAX_PARAMS`) | Asserts / out-of-range in `StaticBitSet` for long parameter lists. | Bounds-tolerant `sep_is_set`. | b2b248e | `sgr_underline_colon_with_trailing_separator_and_short_slice` (`ghostty-vt/src/sgr.rs`); whether Zig's path is reachable: unverified |
|
||||||
| 13 | OS: `NOTIFY_SOCKET` | `os/systemd.zig:100-116` | A leading `@` (abstract socket) is accepted by the check, then copied literally into the address, which cannot work. | A leading `@` becomes an abstract-namespace address. | f8c5ce0 | none (the notify tests use a path socket) |
|
| 13 | OS: `NOTIFY_SOCKET` | `os/systemd.zig:100-116` | A leading `@` (abstract socket) is accepted by the check, then copied literally into the address, which cannot work. | A leading `@` becomes an abstract-namespace address. | f8c5ce0 | none (the notify tests use a path socket) |
|
||||||
|
|||||||
@@ -4,6 +4,7 @@
|
|||||||
//! difftest_fuzz run [--seed N] [--cases N] [--jobs N] [--tokens N]
|
//! difftest_fuzz run [--seed N] [--cases N] [--jobs N] [--tokens N]
|
||||||
//! [--zig PATH] [--work DIR] [--size CxR] [--sb BYTES]
|
//! [--zig PATH] [--work DIR] [--size CxR] [--sb BYTES]
|
||||||
//! difftest_fuzz minz|minr <cols> <rows> <sb> <input-file> # minimise an abort
|
//! difftest_fuzz minz|minr <cols> <rows> <sb> <input-file> # minimise an abort
|
||||||
|
//! difftest_fuzz panics [--seed N] [--cases N] [--tokens N] # Rust only, no Zig
|
||||||
//! difftest_fuzz min <cols> <rows> <sb> <input-file> # minimise a divergence
|
//! difftest_fuzz min <cols> <rows> <sb> <input-file> # minimise a divergence
|
||||||
//! difftest_fuzz check <cols> <rows> <sb> <input-file> # compare one input
|
//! difftest_fuzz check <cols> <rows> <sb> <input-file> # compare one input
|
||||||
//! ```
|
//! ```
|
||||||
@@ -168,6 +169,10 @@ fn main() {
|
|||||||
let work = root.join("target/difftest-work");
|
let work = root.join("target/difftest-work");
|
||||||
std::fs::create_dir_all(&work).expect("mkdir");
|
std::fs::create_dir_all(&work).expect("mkdir");
|
||||||
let ctx = Arc::new(Ctx { zig, work });
|
let ctx = Arc::new(Ctx { zig, work });
|
||||||
|
if a.first().map(String::as_str) == Some("panics") {
|
||||||
|
panics(&a, &ctx);
|
||||||
|
return;
|
||||||
|
}
|
||||||
std::panic::set_hook(Box::new(|_| {}));
|
std::panic::set_hook(Box::new(|_| {}));
|
||||||
match a.first().map(String::as_str) {
|
match a.first().map(String::as_str) {
|
||||||
Some("check" | "min" | "minz" | "minr") => {
|
Some("check" | "min" | "minz" | "minr") => {
|
||||||
@@ -305,3 +310,43 @@ fn run(ctx: &Arc<Ctx>, a: &[String]) {
|
|||||||
std::process::exit(1);
|
std::process::exit(1);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
fn lenient_panics(input: &[u8], c: u16, r: u16, sb: usize) -> bool {
|
||||||
|
let input = input.to_vec();
|
||||||
|
std::panic::catch_unwind(move || ghostty_difftest::drive_lenient(&input, c, r, sb)).is_err()
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Rust-only panic hunt (run it with a debug build so assertions are on):
|
||||||
|
/// untrusted terminal input must never panic, even where Zig aborts.
|
||||||
|
fn panics(a: &[String], ctx: &Ctx) {
|
||||||
|
let seed: u64 = arg(a, "--seed", 1);
|
||||||
|
let cases: usize = arg(a, "--cases", 1000);
|
||||||
|
let tokens: usize = arg(a, "--tokens", 60);
|
||||||
|
let mut found = 0;
|
||||||
|
for i in 0..cases {
|
||||||
|
let mut rng = Rng(seed.wrapping_mul(0x1_0000_0001).wrapping_add(i as u64));
|
||||||
|
let (c, r, sb) = config(&mut rng);
|
||||||
|
let n = 1 + rng.below(tokens);
|
||||||
|
let input = generate(&mut rng, n);
|
||||||
|
if lenient_panics(&input, c, r, sb) {
|
||||||
|
found += 1;
|
||||||
|
let mut min = input.clone();
|
||||||
|
let mut i = 0;
|
||||||
|
while i < min.len() {
|
||||||
|
let mut cand = min.clone();
|
||||||
|
cand.remove(i);
|
||||||
|
if lenient_panics(&cand, c, r, sb) {
|
||||||
|
min = cand;
|
||||||
|
} else {
|
||||||
|
i += 1;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
let _ = ctx;
|
||||||
|
eprintln!("case {i}: {c}x{r} sb={sb} minimised={min:02x?}");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
println!("seed={seed} cases={cases} rust panics={found}");
|
||||||
|
if found > 0 {
|
||||||
|
std::process::exit(1);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -200,6 +200,16 @@ pub fn dump(input: &[u8], cols: u16, rows: u16, max_scrollback: usize) -> String
|
|||||||
dump_terminal(&t)
|
dump_terminal(&t)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Feed `input` and ignore stream errors (they are reported to the caller as
|
||||||
|
/// `Err`, not a crash). Used by the Rust-only panic hunt.
|
||||||
|
pub fn drive_lenient(input: &[u8], cols: u16, rows: u16, max_scrollback: usize) -> String {
|
||||||
|
let mut opts = Options::new(cols, rows);
|
||||||
|
opts.max_scrollback = max_scrollback;
|
||||||
|
let mut t = Terminal::new(opts).expect("terminal");
|
||||||
|
let _ = t.vt_stream().next_slice(input);
|
||||||
|
dump_terminal(&t)
|
||||||
|
}
|
||||||
|
|
||||||
/// Canonical dump of an already-driven terminal.
|
/// Canonical dump of an already-driven terminal.
|
||||||
pub fn dump_terminal(t: &Terminal) -> String {
|
pub fn dump_terminal(t: &Terminal) -> String {
|
||||||
let mut w = String::new();
|
let mut w = String::new();
|
||||||
|
|||||||
Reference in new issue
Block a user