mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-06-15 08:03:56 +00:00
fix(terminal): guard wrap count when resize pushes cursor to scrollback (#12935)
Found this issue when testing some search features; follow up for #12907. You can either reproduce using the PoC below with `libghostty-vt` or run Ghostty debug on macOS and follow these steps: 1. Open Ghostty and start search `0`. 2. Press `cmd+=` to increase font size. 3. You should see a panic crash. ### AI Disclosure As the commit suggests, Claude implemented the fix, the unit test, and PoC file. I reviewed it(seems reasonable to me, but I’m not a Zig professional) and tested it myself. ```zig // PoC: resize panic when shrinking both axes with the cursor near the top // of a fully-populated screen. // // Build (with libghostty-vt headers + dylib on the standard search paths): // zig run poc.zig -lghostty-vt // // Or point at a local build: // zig run poc.zig -I <prefix>/include -L <prefix>/lib -lghostty-vt // // At runtime the dylib must be discoverable (DYLD_LIBRARY_PATH on macOS, // LD_LIBRARY_PATH on Linux, or an rpath baked in at link time). // // Without the fix, this aborts with // reached unreachable code (assert in PageList.Pin.pageIterator) // at _terminal.PageList.resizeCols on a debug/safe build. On release it // silently iterates an empty (reversed) range. const std = @import("std"); const c = @cImport({ @cInclude("ghostty/vt.h"); }); pub fn main() !void { var term: c.GhosttyTerminal = null; const opts: c.GhosttyTerminalOptions = .{ .cols = 80, .rows = 24, .max_scrollback = 1000, }; if (c.ghostty_terminal_new(null, &term, opts) != c.GHOSTTY_SUCCESS) { return error.InitFailed; } defer c.ghostty_terminal_free(term); // Fill every one of the 24 active rows with non-blank content. This is // what makes the bug reachable: when rows shrink, resizeWithoutReflow // can only trim *blank* trailing rows, so non-blank rows are instead // pushed up into scrollback and the active-area top moves down. { var buf: [256]u8 = undefined; var i: usize = 0; while (i < 24) : (i += 1) { // "X" on each row; CR+LF between rows but not after the last so // we don't scroll the top row away. const line = if (i + 1 < 24) std.fmt.bufPrint(&buf, "X\r\n", .{}) catch unreachable else std.fmt.bufPrint(&buf, "X", .{}) catch unreachable; c.ghostty_terminal_vt_write(term, line.ptr, line.len); } } // CSI 1;1H -> park the cursor on the TOP row (1-based). The active area is // anchored to the bottom, so once we shrink rows this row falls above the // new active-area top, i.e. into scrollback. const move = "\x1b[1;1H"; c.ghostty_terminal_vt_write(term, move.ptr, move.len); // Shrink both axes. Columns must shrink to take resize()'s .lt branch, // which runs the row shrink first and then resizeCols with the original // (now out-of-active-area) cursor pin. Panics in // _terminal.PageList.resizeCols. _ = c.ghostty_terminal_resize(term, 79, 20, 8, 16); std.debug.print("survived resize (fix is present)\n", .{}); } ```
This commit is contained in:
@@ -1047,6 +1047,16 @@ fn resizeCols(
|
||||
const wrapped = wrapped: {
|
||||
var wrapped: usize = 0;
|
||||
|
||||
// If shrinking rows (in the .lt branch of resize, rows shrink
|
||||
// before we get here) pushed the cursor pin above the new active
|
||||
// area, there are no rows to count and iterating .left_up toward
|
||||
// the active-area top would be an invalid (reversed) range. The
|
||||
// preserved-cursor growth below already no-ops for a cursor that
|
||||
// isn't in the active area, so we just count zero here.
|
||||
if (active_pin) |ap| {
|
||||
if (p.before(ap)) break :wrapped 0;
|
||||
}
|
||||
|
||||
var row_it = p.rowIterator(.left_up, active_pin);
|
||||
while (row_it.next()) |next| {
|
||||
const row = next.rowAndCell().row;
|
||||
@@ -10859,6 +10869,58 @@ test "PageList resize less rows and cols cursor at bottom" {
|
||||
} }, s.pointFromPin(.active, cursor_pin.*).?);
|
||||
}
|
||||
|
||||
test "PageList resize less rows and cols cursor near top pushed to scrollback" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
var s = try init(alloc, 80, 24, null);
|
||||
defer s.deinit();
|
||||
|
||||
// Fill every active row with non-blank content so that shrinking rows
|
||||
// can't trim trailing blank lines and instead pushes the top rows into
|
||||
// scrollback.
|
||||
{
|
||||
var it = s.rowIterator(.right_down, .{ .active = .{} }, null);
|
||||
while (it.next()) |p| {
|
||||
const rac = p.rowAndCell();
|
||||
const cells = p.node.data.getCells(rac.row);
|
||||
for (cells, 0..) |*cell, x| cell.* = .{
|
||||
.content_tag = .codepoint,
|
||||
.content = .{ .codepoint = @intCast('A' + (x % 26)) },
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
// Cursor near the top of the active area. After we shrink rows the active
|
||||
// area top moves down past this pin, so it ends up in scrollback.
|
||||
const cursor_pin = try s.trackPin(s.pin(.{ .active = .{
|
||||
.x = 0,
|
||||
.y = 0,
|
||||
} }).?);
|
||||
defer s.untrackPin(cursor_pin);
|
||||
|
||||
// Shrink both axes with reflow. resizeWithoutReflow shrinks self.rows
|
||||
// first, leaving the cursor pin above the new active area, then resizeCols
|
||||
// walks .left_up from the cursor pin toward the active-area top.
|
||||
try s.resize(.{
|
||||
.cols = 79,
|
||||
.rows = 20,
|
||||
.reflow = true,
|
||||
.cursor = .{ .x = 0, .y = 0, .pin = cursor_pin },
|
||||
});
|
||||
try testing.expectEqual(@as(usize, 79), s.cols);
|
||||
try testing.expectEqual(@as(usize, 20), s.rows);
|
||||
|
||||
// The active area is anchored to the bottom, so shrinking rows pushed the
|
||||
// top-of-screen cursor into scrollback: it no longer resolves to an
|
||||
// active-area coordinate, but it remains a valid screen pin.
|
||||
try testing.expect(s.pointFromPin(.active, cursor_pin.*) == null);
|
||||
try testing.expect(s.pointFromPin(.screen, cursor_pin.*) != null);
|
||||
|
||||
// Integrity must hold after the resize.
|
||||
s.assertIntegrity();
|
||||
}
|
||||
|
||||
test "PageList resize (no reflow) more rows and less cols" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
Reference in New Issue
Block a user