mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-08-26 17:11:40 +00:00
terminal: fix stale cursor style/hyperlink state after scroll clear (#13997)
Clearing the screen into scrollback and then printing could crash debug builds with a page integrity violation, or silently corrupt style/hyperlink reference counts in release builds. Found in #13991 via fuzzing. The cursor's style and hyperlink IDs are only valid on the page the cursor is on. When the scroll clear moved the start of the fresh screen onto a new page, the reset path in cursorReload updated the cursor's position directly instead of going through cursorChangePin, so the cursor kept IDs from its old page. On the new page those IDs pointed at entries that were dead or belonged to something else, and the next print used them. Fix this by making the reset path go through `cursorChangePin` like every other cross-page cursor move, which releases the style and hyperlink from the old page and recreates them on the new one.
This commit is contained in:
Mitchell Hashimoto
@@ -378,6 +378,16 @@ pub fn assertIntegrity(self: *const Screen) void {
|
||||
) orelse unreachable;
|
||||
assert(self.cursor.x == pt.active.x);
|
||||
assert(self.cursor.y == pt.active.y);
|
||||
|
||||
// The cursor style and hyperlink if non-zero must reference
|
||||
// real data in the page the pin is in.
|
||||
const page: *const Page = self.cursor.page_pin.node.page();
|
||||
if (self.cursor.style_id != style.default_id) {
|
||||
assert(page.styles.refCount(page.memory, self.cursor.style_id) > 0);
|
||||
}
|
||||
if (self.cursor.hyperlink_id != 0) {
|
||||
assert(page.hyperlink_set.refCount(page.memory, self.cursor.hyperlink_id) > 0);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -874,9 +884,31 @@ pub fn cursorReload(self: *Screen) void {
|
||||
.active,
|
||||
self.cursor.page_pin.*,
|
||||
) orelse reset: {
|
||||
// Our cached row/cell pointers may be invalid (that is often
|
||||
// the reason cursorReload is being called), so refresh them
|
||||
// from the pin first since cursorChangePin below marks the
|
||||
// old cursor row as dirty.
|
||||
const old_rac = self.cursor.page_pin.rowAndCell();
|
||||
self.cursor.page_row = old_rac.row;
|
||||
self.cursor.page_cell = old_rac.cell;
|
||||
|
||||
// The cursor style and hyperlink IDs are only valid within the
|
||||
// page that the pin points at, so the pin change must go through
|
||||
// cursorChangePin, which migrates them when the active top-left
|
||||
// is on a different page. Writing the pin directly here would
|
||||
// leave the cursor holding IDs that are dead or alias unrelated
|
||||
// entries on the new page.
|
||||
const pin = self.pages.pin(.{ .active = .{} }).?;
|
||||
self.cursor.page_pin.* = pin;
|
||||
break :reset self.pages.pointFromPin(.active, pin).?;
|
||||
self.cursor.x = 0; // Must be set before cursorChangePin
|
||||
self.cursor.y = 0;
|
||||
self.cursorChangePin(pin);
|
||||
|
||||
// cursorChangePin can trigger a page capacity adjustment which
|
||||
// moves the pin again, so we re-read it to derive our point.
|
||||
break :reset self.pages.pointFromPin(
|
||||
.active,
|
||||
self.cursor.page_pin.*,
|
||||
).?;
|
||||
};
|
||||
|
||||
self.cursor.x = @intCast(pt.active.x);
|
||||
@@ -884,20 +916,6 @@ pub fn cursorReload(self: *Screen) void {
|
||||
const page_rac = self.cursor.page_pin.rowAndCell();
|
||||
self.cursor.page_row = page_rac.row;
|
||||
self.cursor.page_cell = page_rac.cell;
|
||||
|
||||
// If we have a style, we need to ensure it is in the page because this
|
||||
// method may also be called after a page change.
|
||||
if (self.cursor.style_id != style.default_id) {
|
||||
self.manualStyleUpdate() catch |err| {
|
||||
// This failure should not happen because manualStyleUpdate
|
||||
// handles page splitting, overflow, and more. This should only
|
||||
// happen if we're out of RAM. In this case, we'll just degrade
|
||||
// gracefully back to the default style.
|
||||
log.err("failed to update style on cursor reload err={}", .{err});
|
||||
self.cursor.style = .{};
|
||||
self.cursor.style_id = 0;
|
||||
};
|
||||
}
|
||||
}
|
||||
|
||||
/// Scroll the active area and keep the cursor at the bottom of the screen.
|
||||
@@ -1487,6 +1505,10 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
|
||||
if (self.cursor.hyperlink != null) {
|
||||
const old_page: *Page = self.cursor.page_pin.node.page();
|
||||
old_page.hyperlink_set.release(old_page.memory, self.cursor.hyperlink_id);
|
||||
// Zero the ID, it is invalid now and style changes below may
|
||||
// run integrity checks. We still have self.cursor.hyperlink to
|
||||
// rebuild this later.
|
||||
self.cursor.hyperlink_id = 0;
|
||||
}
|
||||
|
||||
// Update our pin to the new page
|
||||
@@ -1508,8 +1530,9 @@ inline fn cursorChangePin(self: *Screen, new: Pin) void {
|
||||
|
||||
// On the new page, we need to migrate our hyperlink
|
||||
if (self.cursor.hyperlink) |link| {
|
||||
// So we don't attempt to free any memory in the replaced page.
|
||||
self.cursor.hyperlink_id = 0;
|
||||
// startHyperlink will try to free old hyperlinks, so set this
|
||||
// to null. We free it ourselves later since we're doing some
|
||||
// ref-counting shenanigans in this function.
|
||||
self.cursor.hyperlink = null;
|
||||
|
||||
// Re-add
|
||||
@@ -4139,6 +4162,82 @@ test "Screen write regrows compacted page capacity" {
|
||||
try testing.expect(page.graphemeCount() >= 1);
|
||||
}
|
||||
|
||||
// The cursor style and hyperlink IDs are only meaningful within the page
|
||||
// the cursor pin points at. scrollClear can move the active area onto a
|
||||
// later page while the cursor pin stays with its content on an earlier
|
||||
// page (now scrollback), so the reset in cursorReload must migrate both
|
||||
// references to the destination page. It previously replaced the pin
|
||||
// directly and then released the old style ID on the new page.
|
||||
test "Screen scrollClear across pages migrates cursor style and hyperlink" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
const io = testing.io;
|
||||
|
||||
var s = try init(io, alloc, .{
|
||||
.cols = 10,
|
||||
.rows = 10,
|
||||
.max_scrollback_bytes = std.math.maxInt(usize),
|
||||
});
|
||||
defer s.deinit();
|
||||
|
||||
// Fill the first page so the active area spans two pages.
|
||||
const first_page_size = s.pages.pages.first.?.capacity().rows;
|
||||
s.pages.pages.first.?.page().pauseIntegrityChecks(true);
|
||||
for (0..first_page_size - 5) |_| {
|
||||
try s.testWriteString("\n");
|
||||
}
|
||||
s.pages.pages.first.?.page().pauseIntegrityChecks(false);
|
||||
try s.testWriteString("1\n2\n3\n4\n5\n6\n7\n8\n9\n10");
|
||||
try testing.expect(s.pages.pages.first != s.pages.pages.last);
|
||||
|
||||
// Move the cursor to the top of the active area, which is on the
|
||||
// first page, and give it a style and a hyperlink there.
|
||||
s.cursorAbsolute(0, 0);
|
||||
try testing.expect(s.cursor.page_pin.node == s.pages.pages.first.?);
|
||||
try s.setAttribute(.{ .bold = {} });
|
||||
try s.startHyperlink("https://example.com/", null);
|
||||
|
||||
const old_page: *Page = s.cursor.page_pin.node.page();
|
||||
const old_style_id = s.cursor.style_id;
|
||||
const old_hyperlink_id = s.cursor.hyperlink_id;
|
||||
try testing.expect(old_style_id != style.default_id);
|
||||
try testing.expect(old_hyperlink_id != 0);
|
||||
|
||||
// All ten active rows are non-empty, so this moves the active area
|
||||
// fully onto the second page while the cursor pin stays with its
|
||||
// old row, which is now scrollback.
|
||||
try s.scrollClear();
|
||||
|
||||
// The cursor was moved to the new active top-left on the second
|
||||
// page with its style and hyperlink references rebuilt there.
|
||||
const new_page: *Page = s.cursor.page_pin.node.page();
|
||||
try testing.expect(new_page != old_page);
|
||||
try testing.expect(s.cursor.style_id != style.default_id);
|
||||
try testing.expect(s.cursor.hyperlink_id != 0);
|
||||
try testing.expect(new_page.styles.refCount(
|
||||
new_page.memory,
|
||||
s.cursor.style_id,
|
||||
) > 0);
|
||||
try testing.expect(new_page.hyperlink_set.refCount(
|
||||
new_page.memory,
|
||||
s.cursor.hyperlink_id,
|
||||
) > 0);
|
||||
|
||||
// The cursor's references on the old page were released. Nothing
|
||||
// else referenced either entry, so both are dead there now.
|
||||
try testing.expectEqual(0, old_page.styles.refCount(
|
||||
old_page.memory,
|
||||
old_style_id,
|
||||
));
|
||||
try testing.expectEqual(0, old_page.hyperlink_set.refCount(
|
||||
old_page.memory,
|
||||
old_hyperlink_id,
|
||||
));
|
||||
|
||||
// Printing attaches the migrated style and hyperlink to a cell.
|
||||
try s.testWriteString("B");
|
||||
}
|
||||
|
||||
test "Screen cursorCopy hyperlink deref new page" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
@@ -7421,6 +7421,85 @@ test "Terminal: print wide char at right edge with hyperlink" {
|
||||
}
|
||||
}
|
||||
|
||||
// A cursor style or hyperlink ID is an index into a set stored in the
|
||||
// page memory of the page the cursor pin points at. When scrollClear
|
||||
// (here via ED 22, kitty's scroll_complete) pushes the active area onto
|
||||
// a later page while the cursor pin is still on an earlier one,
|
||||
// cursorReload must migrate the cursor's style and hyperlink references
|
||||
// to the destination page. It previously replaced the pin directly,
|
||||
// leaving the cursor holding an ID that was dead or aliased an unrelated
|
||||
// entry on the new page, and the next print attached a live cell to it.
|
||||
// Found via fuzzing.
|
||||
test "Terminal: scrollClear across pages keeps cursor hyperlink refs page-local" {
|
||||
const alloc = testing.allocator;
|
||||
|
||||
// Minimized from a 774-byte AFL fuzz input. Reading it:
|
||||
//
|
||||
// A print, so REP has something to repeat
|
||||
// ESC [ 48111 b REP, filling the page and spilling onto a second
|
||||
// ESC ] 8 ; ; 0x93 OSC 8; the C1 byte terminates the OSC and makes
|
||||
// the URI non-empty, so a hyperlink starts
|
||||
// ESC [ 11 A CUU, moving the cursor back onto the first page
|
||||
// ESC [ 22 J ED 22, i.e. scroll_complete -> Screen.scrollClear
|
||||
// B print, which attaches the cursor hyperlink
|
||||
// ESC ] 8 ; ; ESC OSC 8 with an empty URI, ending the hyperlink
|
||||
//
|
||||
// The grid must be wide enough to fill a page from a single REP, so
|
||||
// this does not reproduce at 80x24.
|
||||
const input = "A\x1b[48111b\x1b]8;;\x93\x1b[11A\x1b[22JB\x1b]8;;\x1b";
|
||||
|
||||
var t = try init(testing.io, alloc, .{ .cols = 200, .rows = 50 });
|
||||
defer t.deinit(alloc);
|
||||
|
||||
{
|
||||
var s = t.vtStream();
|
||||
defer s.deinit();
|
||||
s.nextSlice(input);
|
||||
}
|
||||
|
||||
// With slow runtime safety on, the page integrity checks during the
|
||||
// stream above already catch the bug. Verify the ref counts explicitly
|
||||
// as well so this test is meaningful with runtime safety off: every
|
||||
// cell holding a hyperlink ID owns a reference, so a count below the
|
||||
// number of holding cells means a live cell points at an entry that
|
||||
// was already freed.
|
||||
var node_ = t.screens.active.pages.pages.first;
|
||||
while (node_) |node| : (node_ = node.next) {
|
||||
const page = node.page();
|
||||
const cap = page.hyperlink_set.layout.cap;
|
||||
if (cap == 0) continue;
|
||||
|
||||
const holders = try alloc.alloc(u32, cap);
|
||||
defer alloc.free(holders);
|
||||
@memset(holders, 0);
|
||||
|
||||
for (page.rows.ptr(page.memory)[0..page.size.rows]) |*row| {
|
||||
if (!row.hyperlink) continue;
|
||||
for (row.cells.ptr(page.memory)[0..page.size.cols]) |*cell| {
|
||||
if (!cell.hyperlink) continue;
|
||||
const id = page.lookupHyperlink(cell) orelse continue;
|
||||
if (id < cap) holders[id] += 1;
|
||||
}
|
||||
}
|
||||
|
||||
for (holders, 0..) |held, id| {
|
||||
if (held == 0) continue;
|
||||
const refs = page.hyperlink_set.refCount(page.memory, @intCast(id));
|
||||
try testing.expect(refs >= held);
|
||||
}
|
||||
}
|
||||
|
||||
// If the cursor still has an active hyperlink, its own extra
|
||||
// reference must live on the cursor's page.
|
||||
const cursor = &t.screens.active.cursor;
|
||||
if (cursor.hyperlink_id != 0) {
|
||||
const page = cursor.page_pin.node.page();
|
||||
try testing.expect(
|
||||
page.hyperlink_set.refCount(page.memory, cursor.hyperlink_id) > 0,
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: linefeed and carriage return" {
|
||||
var t = try init(testing.io, testing.allocator, .{ .cols = 80, .rows = 80 });
|
||||
defer t.deinit(testing.allocator);
|
||||
|
||||
Reference in New Issue
Block a user