mirror of
https://github.com/ghostty-org/ghostty.git
synced 2026-08-25 16:41:44 +00:00
terminal: reload cell pointers when print grows a page (#13960)
Fixes #11261. `Terminal.print`'s grapheme path caches a `*Cell` for the previous cell and keeps using it after writing other cells. Writing the wide spacer tail calls `printCell`, which can grow the page to make room for the cursor hyperlink. Growing clones the page and frees the old one, so the cached pointer dangles and the following `appendGrapheme` writes into freed memory. The second test case in the issue reproduces it. Rather than recomputing `prev` on every use, which is too expensive for this path, the fix records the cursor page identity before the spacer write and reloads the cell only if the page actually changed. Node pointer plus serial is used because nodes are pooled and a replacement can land on the same address. Nothing changes when the page does not grow. Three other pointers in the same function were held across an operation that can replace a page, so they are now read through the cursor or a freshly resolved pin: the grapheme move after a wrap, the grapheme append loop, and `printCell`'s assert on a failed hyperlink write. Tests: - `Terminal: VS16 widening when the spacer tail grows the page` fills the page hyperlink map so the spacer tail is what forces growth. It crashes without the fix. - `Terminal: grapheme transfer when widening wraps to the next line` covers the wrap path where the previous cell already holds grapheme data, which had no test before. `zig build test` passes.
This commit is contained in:
@@ -1323,14 +1323,21 @@ pub fn print(self: *Terminal, c: u21) !void {
|
||||
const old_rac = old_pin.rowAndCell();
|
||||
|
||||
if (new_pin.node == old_pin.node) {
|
||||
new_pin.node.page().moveGrapheme(prev.cell, new_rac.cell);
|
||||
prev.cell.content_tag = .codepoint;
|
||||
new_pin.node.page().moveGrapheme(old_rac.cell, new_rac.cell);
|
||||
old_rac.cell.content_tag = .codepoint;
|
||||
new_rac.cell.content_tag = .codepoint_grapheme;
|
||||
new_rac.row.grapheme = true;
|
||||
} else {
|
||||
const cps = old_pin.node.page().lookupGrapheme(old_rac.cell).?;
|
||||
for (cps) |cp| {
|
||||
try self.screens.active.appendGrapheme(new_rac.cell, cp);
|
||||
// appendGrapheme can grow the cursor
|
||||
// page, so read the destination from
|
||||
// the cursor each time rather than
|
||||
// holding a pointer across the call.
|
||||
try self.screens.active.appendGrapheme(
|
||||
self.screens.active.cursor.page_cell,
|
||||
cp,
|
||||
);
|
||||
}
|
||||
old_pin.node.page().clearGrapheme(old_rac.cell);
|
||||
}
|
||||
@@ -1340,7 +1347,7 @@ pub fn print(self: *Terminal, c: u21) !void {
|
||||
|
||||
// Point prev.cell to our new previous cell that
|
||||
// we'll be appending graphemes to
|
||||
prev.cell = new_rac.cell;
|
||||
prev.cell = self.screens.active.cursor.page_cell;
|
||||
} else {
|
||||
self.printCell(
|
||||
0,
|
||||
@@ -1359,8 +1366,30 @@ pub fn print(self: *Terminal, c: u21) !void {
|
||||
|
||||
// Write our spacer, since prev.cell is now wide
|
||||
self.screens.active.cursorRight(1);
|
||||
|
||||
// Writing the spacer can grow the page to make room for
|
||||
// the cursor hyperlink. Growing replaces the page, which
|
||||
// invalidates `prev.cell`. Record the page identity first
|
||||
// so the common case where nothing grows stays free.
|
||||
//
|
||||
// A pointer comparison alone isn't enough: pages are
|
||||
// pooled, so a replacement can reuse the same address.
|
||||
// The serial makes the pair a unique identity.
|
||||
const spacer_node = self.screens.active.cursor.page_pin.node;
|
||||
const spacer_serial = spacer_node.serial;
|
||||
|
||||
self.printCell(0, .spacer_tail);
|
||||
|
||||
if (self.screens.active.cursor.page_pin.node != spacer_node or
|
||||
self.screens.active.cursor.page_pin.node.serial != spacer_serial)
|
||||
{
|
||||
@branchHint(.unlikely);
|
||||
|
||||
// The cursor is on the spacer tail we just wrote, so
|
||||
// the wide cell we append to is the one to its left.
|
||||
prev.cell = self.screens.active.cursorCellLeft(1);
|
||||
}
|
||||
|
||||
// Move the cursor again so we're beyond our spacer
|
||||
if (self.screens.active.cursor.x == right_limit - 1) {
|
||||
self.screens.active.cursor.pending_wrap = true;
|
||||
@@ -1704,7 +1733,11 @@ fn printCell(
|
||||
self.screens.active.cursorSetHyperlink() catch |err| {
|
||||
@branchHint(.unlikely);
|
||||
log.warn("error reallocating for more hyperlink space, ignoring hyperlink err={}", .{err});
|
||||
assert(!cell.hyperlink);
|
||||
|
||||
// A partially successful grow can replace the page even when the
|
||||
// call fails, so `cell` may be stale here. The cursor pointers are
|
||||
// always reloaded, so read the cell through the cursor.
|
||||
assert(!self.screens.active.cursor.page_cell.hyperlink);
|
||||
};
|
||||
} else if (had_hyperlink) {
|
||||
// If the previous cell had a hyperlink then we need to clear it.
|
||||
@@ -6132,6 +6165,106 @@ test "Terminal: VS16 to make wide character on next line with hyperlink" {
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: VS16 widening when the spacer tail grows the page" {
|
||||
// Regression test for a stale cell pointer in print's grapheme `.wide`
|
||||
// path: writing the spacer tail can grow the page to fit the hyperlink,
|
||||
// which replaces the page and invalidates the pointer to the wide cell.
|
||||
var t = try init(testing.io, testing.allocator, .{ .rows = 10, .cols = 20 });
|
||||
defer t.deinit(testing.allocator);
|
||||
|
||||
t.modes.set(.grapheme_cluster, true);
|
||||
try t.screens.active.startHyperlink("http://example.com", null);
|
||||
|
||||
// Fill the page hyperlink map until a single slot is left. The '#' below
|
||||
// takes that slot so the spacer tail is what forces the page to grow.
|
||||
while (true) {
|
||||
const page = t.screens.active.cursor.page_pin.node.page();
|
||||
const map = page.hyperlink_map.map(page.memory);
|
||||
if (map.maxLoad() - map.count() == 1) break;
|
||||
try t.print('x');
|
||||
}
|
||||
|
||||
const x = t.screens.active.cursor.x;
|
||||
const y = t.screens.active.cursor.y;
|
||||
try t.print('#');
|
||||
|
||||
// Without the fix this crashed appending to a freed page.
|
||||
try t.print(0xFE0F);
|
||||
|
||||
{
|
||||
// '#' is wide and carries the VS16 grapheme.
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .active = .{
|
||||
.x = x,
|
||||
.y = y,
|
||||
} }).?;
|
||||
const cell = list_cell.cell;
|
||||
try testing.expectEqual(@as(u21, '#'), cell.content.codepoint.data);
|
||||
try testing.expectEqual(Cell.Wide.wide, cell.wide);
|
||||
try testing.expect(cell.hasGrapheme());
|
||||
try testing.expectEqualSlices(
|
||||
u21,
|
||||
&.{0xFE0F},
|
||||
list_cell.node.page().lookupGrapheme(cell).?,
|
||||
);
|
||||
}
|
||||
{
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .active = .{
|
||||
.x = x + 1,
|
||||
.y = y,
|
||||
} }).?;
|
||||
try testing.expectEqual(Cell.Wide.spacer_tail, list_cell.cell.wide);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: grapheme transfer when widening wraps to the next line" {
|
||||
// Covers print's grapheme `.wide` path where the previous cell already
|
||||
// holds grapheme data and has to be moved to the wrapped row.
|
||||
var t = try init(testing.io, testing.allocator, .{ .rows = 5, .cols = 3 });
|
||||
defer t.deinit(testing.allocator);
|
||||
|
||||
t.modes.set(.grapheme_cluster, true);
|
||||
t.cursorRight(2);
|
||||
|
||||
// A narrow emoji, then ZWJ, then a second emoji. The ZWJ attaches
|
||||
// without changing the width, so the cell has grapheme data by the time
|
||||
// the second emoji widens it.
|
||||
try t.print(0x263A);
|
||||
try t.print(0x200D);
|
||||
try t.print(0x2764);
|
||||
|
||||
{
|
||||
// The old cell becomes a spacer head on the wrapped row.
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .screen = .{
|
||||
.x = 2,
|
||||
.y = 0,
|
||||
} }).?;
|
||||
try testing.expectEqual(Cell.Wide.spacer_head, list_cell.cell.wide);
|
||||
try testing.expect(list_cell.row.wrap);
|
||||
}
|
||||
{
|
||||
// The grapheme moved with the base codepoint.
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .screen = .{
|
||||
.x = 0,
|
||||
.y = 1,
|
||||
} }).?;
|
||||
const cell = list_cell.cell;
|
||||
try testing.expectEqual(@as(u21, 0x263A), cell.content.codepoint.data);
|
||||
try testing.expectEqual(Cell.Wide.wide, cell.wide);
|
||||
try testing.expectEqualSlices(
|
||||
u21,
|
||||
&.{ 0x200D, 0x2764 },
|
||||
list_cell.node.page().lookupGrapheme(cell).?,
|
||||
);
|
||||
}
|
||||
{
|
||||
const list_cell = t.screens.active.pages.getCell(.{ .screen = .{
|
||||
.x = 1,
|
||||
.y = 1,
|
||||
} }).?;
|
||||
try testing.expectEqual(Cell.Wide.spacer_tail, list_cell.cell.wide);
|
||||
}
|
||||
}
|
||||
|
||||
test "Terminal: VS16 to make wide character with pending wrap" {
|
||||
var t = try init(testing.io, testing.allocator, .{ .rows = 5, .cols = 3 });
|
||||
defer t.deinit(testing.allocator);
|
||||
|
||||
Reference in New Issue
Block a user