Fix Row::clear_wide panic on truncated wide character#30
Open
Junyi-99 wants to merge 2 commits into
Open
Conversation
When Row::resize shrinks a row past the continuation cell of a wide
character, the row's last cell is left flagged as is_wide() with no
valid continuation. The next CSI K (erase in line) walks the row via
Row::erase -> Row::clear_wide, which indexes cells[col + 1] without
a bounds check and panics:
thread '...' panicked at src/row.rs:
index out of bounds: the len is N but the index is N
The minimal repro from doy#28:
let mut parser = vt100::Parser::new(2, 4, 0);
parser.process("你".as_bytes());
parser.screen_mut().set_size(2, 1);
parser.process(b"\x1b[K");
Two changes, both defensive — either alone prevents the panic, both
together cover any other path that could leave an orphaned wide cell:
1. Row::resize: when shrinking, clear a wide char whose continuation
was just truncated. Mirrors what Row::truncate already does.
2. Row::clear_wide: bounds-check before accessing col + 1; if the
continuation cell is gone, clear the wide cell in place rather
than indexing past the end.
Adds a regression test (the issue's minimal repro) to tests/basic.rs.
Closes doy#28.
There was a problem hiding this comment.
Pull request overview
This PR fixes a panic in wide-character handling when resizing truncates a wide character before an erase operation.
Changes:
- Clears dangling wide cells during
Row::resizewhen shrinking. - Adds bounds handling in
Row::clear_widefor orphaned wide cells. - Adds a regression test for the resize + erase repro from issue #28.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/row.rs |
Updates resize and wide-cell clearing logic to avoid out-of-bounds access. |
tests/basic.rs |
Adds a regression test covering the reported panic scenario. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4 tasks
clear_wide was hardened to handle an orphaned wide cell (wide flag set
with no continuation), but its caller erase still derived the logical
last column as cols() - if wide { 2 } else { 1 } from the pre-erase wide
flag. For such a cell in a one-column row that is 1 - 2, which underflows
and panics. Use saturating_sub; behaviour is unchanged for every row with
two or more columns.
Add an in-crate unit test for the one-column orphaned-wide case.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4 tasks
aannoo
added a commit
to aannoo/hcom
that referenced
this pull request
Jul 6, 2026
* fix: contain vt100 panics instead of letting them kill the PTY wrapper (#73) vt100 0.16.2 has a real upstream bug (doy/vt100-rust#28, open PR #30): Grid::set_size shrinks rows via Row::resize, which doesn't clean up a double-width character's "wide" flag when its continuation cell gets truncated away. The orphaned wide cell then panics the next time it's erased (Row::clear_wide indexes one past the row's new length) — this matches the exact panic signature and line from the reported incidents. hcom's existing global panic hook logs the panic but doesn't stop it from unwinding, so it killed the whole PTY wrapper thread and dropped the wrapped agent session (observed as repeated "stopped by pty: closed" on real Codex sessions). Wrap the two vt100 entry points (Parser::process, Screen::set_size) in catch_unwind, and rebuild the parser from scratch on a caught panic rather than continuing to use a parser that may have panicked mid-mutation and be left in an inconsistent state. This drops the current screen contents on the rare occasions it triggers, but keeps the PTY wrapper and wrapped agent process alive; the next output chunk repopulates the screen. Includes a regression test reproducing the exact upstream repro (wide CJK char, downward resize, erase-in-line) to confirm the panic no longer escapes process()/resize(). This is a containment fix, not a root-cause fix — the actual vt100 bug still exists upstream (unmerged as of this writing). Vendoring vt100 with the upstream PR #30 patch would fix the root cause and is a reasonable follow-up, but is a separate, heavier change (pinned git dependency) that this PR intentionally doesn't take on. * fix: patch vt100 to the actual upstream fix for the row-erase panic (#73) Pin vt100 to Junyi-99/vt100-rust@131a75e (the commit backing the still-open doy/vt100-rust#30) instead of relying solely on the catch_unwind containment added in the previous commit. That patch fixes the real bug: Row::clear_wide now bounds-checks before indexing the continuation cell, and Row::resize clears a wide flag left dangling by a downward resize, instead of the previous behavior of orphaning it and panicking on the next erase. Verified independently (outside hcom, against a plain clone of the patched fork) that the standalone repro from the previous commit's regression test no longer panics with this patch applied. The catch_unwind containment stays as defense-in-depth for any other vt100 edge case, not just this one. This dependency pin is a stopgap until vt100 ships a release containing the fix (or the PR merges) — at that point this patch entry should be removed and the crates.io version requirement bumped instead. --------- Co-authored-by: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #28.
Summary
Row::clear_wideindexescells[col + 1]without a bounds check. After a shrinkingRow::resizetruncates the continuation cell of a wide character, the row's last cell is left flaggedis_wide()with no valid neighbor, and the nextCSI Kpanics with an OOB index.Minimal repro (from #28):
Changes
Two defensive fixes — either alone stops the panic for the repro above. Both together harden against any other path that could leave an orphaned wide cell.
Row::resize: when shrinking, clear a wide char whose continuation cell was just truncated. This mirrors whatRow::truncatealready does, and is the actual root cause for the repro.Row::clear_wide: bounds-checkcol + 1before accessing. If the continuation is gone, clear the wide cell in place rather than indexing past the end.A regression test (the issue's repro) is added to
tests/basic.rs.Test plan
cargo test— all existing suites pass; newresize_truncating_wide_char_then_erase_line_does_not_panicpasses.Happy to split the two fixes into separate commits or to drop (2) if you'd prefer to fix only the root cause — let me know.