From 33cda4dc5dbfd0478f6891fb4b53844a4fbee17c Mon Sep 17 00:00:00 2001 From: Arnesh Date: Sat, 22 Aug 2026 00:19:58 +0530 Subject: [PATCH] terminal: reload cell pointers when print grows a page Terminal.print's grapheme path holds a raw pointer to the previous cell while it writes other cells. Writing the wide spacer tail can grow the page to fit the cursor hyperlink, and growing replaces the page, so the pointer is left dangling and the following appendGrapheme writes into freed memory. Record the cursor page identity (node pointer plus serial, since pooled nodes can reuse an address) before the spacer write and reload the cell only when the page actually changed, so the common path costs nothing. The same function had three more pointers held across an operation that can replace a page: the grapheme move after a wrap, the grapheme append loop, and printCell's assert on a failed hyperlink write. Those now read through the cursor or a freshly resolved pin. Fixes #11261 --- src/terminal/Terminal.zig | 143 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 138 insertions(+), 5 deletions(-) diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 21c087446..b7176bba2 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -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);