diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 9c89b02b1..e5b4ed234 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -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; diff --git a/src/terminal/Terminal.zig b/src/terminal/Terminal.zig index 561728f5f..1940bc4bc 100644 --- a/src/terminal/Terminal.zig +++ b/src/terminal/Terminal.zig @@ -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);