diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index a838e0e10..98894a83d 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -474,26 +474,42 @@ pub fn adjustCapacity( const new_node = try self.pages.adjustCapacity(node, adjustment); const new_page: *Page = &new_node.data; - // All additions below have unreachable catches because when - // we adjust cap we should have enough memory to fit the - // existing data. - - // Re-add the style + // Re-add the style, if the page somehow doesn't have enough + // memory to add it, we emit a warning and gracefully degrade + // to the default style for the cursor. if (self.cursor.style_id != 0) { self.cursor.style_id = new_page.styles.add( new_page.memory, self.cursor.style, - ) catch unreachable; + ) catch |err| id: { + // TODO: Should we increase the capacity further in this case? + log.warn( + "(Screen.adjustCapacity) Failed to add cursor style back to page, err={}", + .{err}, + ); + + // Reset the cursor style. + self.cursor.style = .{}; + break :id style.default_id; + }; } - // Re-add the hyperlink + // Re-add the hyperlink, if the page somehow doesn't have enough + // memory to add it, we emit a warning and gracefully degrade to + // no hyperlink. if (self.cursor.hyperlink) |link| { // So we don't attempt to free any memory in the replaced page. self.cursor.hyperlink_id = 0; self.cursor.hyperlink = null; // Re-add - self.startHyperlinkOnce(link.*) catch unreachable; + self.startHyperlinkOnce(link.*) catch |err| { + // TODO: Should we increase the capacity further in this case? + log.warn( + "(Screen.adjustCapacity) Failed to add cursor hyperlink back to page, err={}", + .{err}, + ); + }; // Remove our old link link.deinit(self.alloc); @@ -1003,8 +1019,9 @@ pub fn cursorCopy(self: *Screen, other: Cursor, opts: struct { /// Always use this to write to cursor.page_pin.*. /// /// This specifically handles the case when the new pin is on a different -/// page than the old AND we have a style set. In that case, we must release -/// our old style and upsert our new style since styles are stored per-page. +/// page than the old AND we have a style or hyperlink set. In that case, +/// we must release our old one and insert the new one, since styles are +/// stored per-page. fn cursorChangePin(self: *Screen, new: Pin) void { // Moving the cursor affects text run splitting (ligatures) so // we must mark the old and new page dirty. We do this as long @@ -1986,9 +2003,40 @@ pub fn cursorSetHyperlink(self: *Screen) !void { } else |err| switch (err) { // hyperlink_map is out of space, realloc the page to be larger error.HyperlinkMapOutOfMemory => { + const uri_size = if (self.cursor.hyperlink) |link| link.uri.len else 0; + + var string_bytes = page.capacity.string_bytes; + + // Attempt to allocate the space that would be required to + // insert a new copy of the cursor hyperlink uri in to the + // string alloc, since right now adjustCapacity always just + // adds an extra copy even if one already exists in the page. + // If this alloc fails then we know we also need to grow our + // string bytes. + // + // FIXME: This SUCKS + if (page.string_alloc.alloc( + u8, + page.memory, + uri_size, + )) |slice| { + // We don't bother freeing because we're + // about to free the entire page anyway. + _ = &slice; + } else |_| { + // We didn't have enough room, let's just double our + // string bytes until there's definitely enough room + // for our uri. + const before = string_bytes; + while (string_bytes - before < uri_size) string_bytes *= 2; + } + _ = try self.adjustCapacity( self.cursor.page_pin.node, - .{ .hyperlink_bytes = page.capacity.hyperlink_bytes * 2 }, + .{ + .hyperlink_bytes = page.capacity.hyperlink_bytes * 2, + .string_bytes = string_bytes, + }, ); // Retry @@ -2882,6 +2930,9 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .protected = self.cursor.protected, }; + // If we have a hyperlink, add it to the cell. + if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); + // If we have a ref-counted style, increase. if (self.cursor.style_id != style.default_id) { const page = self.cursor.page_pin.node.data; @@ -2900,6 +2951,9 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .protected = self.cursor.protected, }; + // If we have a hyperlink, add it to the cell. + if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); + self.cursor.page_row.wrap = true; try self.cursorDownOrScroll(); self.cursorHorizontalAbsolute(0); @@ -2915,6 +2969,9 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .protected = self.cursor.protected, }; + // If we have a hyperlink, add it to the cell. + if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); + // Write our tail self.cursorRight(1); self.cursor.page_cell.* = .{ @@ -2924,6 +2981,9 @@ pub fn testWriteString(self: *Screen, text: []const u8) !void { .protected = self.cursor.protected, }; + // If we have a hyperlink, add it to the cell. + if (self.cursor.hyperlink_id > 0) try self.cursorSetHyperlink(); + // If we have a ref-counted style, increase twice. if (self.cursor.style_id != style.default_id) { const page = self.cursor.page_pin.node.data; @@ -8725,6 +8785,40 @@ test "Screen: hyperlink cursor state on resize" { } } +test "Screen: cursorSetHyperlink OOM + URI too large for string alloc" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + // Start a hyperlink with a URI that just barely fits in the string alloc. + // This will ensure that additional string alloc space is needed for the + // redundant copy of the URI when the page is re-alloced. + const uri = "a" ** (pagepkg.std_capacity.string_bytes - 8); + try s.startHyperlink(uri, null); + + // Figure out how many cells should can have hyperlinks in this page, + // and write twice that number, to guarantee the capacity needs to be + // increased at some point. + const base_capacity = s.cursor.page_pin.node.data.hyperlinkCapacity(); + const base_string_bytes = s.cursor.page_pin.node.data.capacity.string_bytes; + for (0..base_capacity * 2) |_| { + try s.cursorSetHyperlink(); + if (s.cursor.x >= s.pages.cols - 1) { + try s.cursorDownOrScroll(); + s.cursorHorizontalAbsolute(0); + } else { + s.cursorRight(1); + } + } + + // Make sure the capacity really did increase. + try testing.expect(base_capacity < s.cursor.page_pin.node.data.hyperlinkCapacity()); + // And that our string_bytes increased as well. + try testing.expect(base_string_bytes < s.cursor.page_pin.node.data.capacity.string_bytes); +} + test "Screen: adjustCapacity cursor style ref count" { const testing = std.testing; const alloc = testing.allocator; @@ -8759,6 +8853,102 @@ test "Screen: adjustCapacity cursor style ref count" { } } +test "Screen: adjustCapacity cursor hyperlink exceeds string alloc size" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 0); + defer s.deinit(); + + // Start a hyperlink with a URI that just barely fits in the string alloc. + // This will ensure that the redundant copy added in `adjustCapacity` won't + // fit in the available string alloc space. + const uri = "a" ** (pagepkg.std_capacity.string_bytes - 8); + try s.startHyperlink(uri, null); + + // Write some characters with this so that the URI + // is copied to the new page when adjusting capacity. + try s.testWriteString("Hello"); + + // Adjust the capacity, right now this will cause a redundant copy of + // the URI to be added to the string alloc, but since there isn't room + // for this this will clear the cursor hyperlink. + _ = try s.adjustCapacity(s.cursor.page_pin.node, .{}); + + // The cursor hyperlink should have been cleared by the `adjustCapacity` + // call, because there isn't enough room to add the redundant URI string. + // + // This behavior will change, causing this test to fail, if any of these + // changes are made: + // + // - The string alloc is changed to intern strings. + // + // - The adjustCapacity function is changed to ensure the new + // capacity will fit the redundant copy of the hyperlink uri. + // + // - The cursor managed memory handling is reworked so that it + // doesn't reside in the pages anymore and doesn't need this + // accounting. + // + // In such a case, adjust this test accordingly. + try testing.expectEqual(null, s.cursor.hyperlink); + try testing.expectEqual(0, s.cursor.hyperlink_id); +} + +test "Screen: adjustCapacity cursor style exceeds style set capacity" { + const testing = std.testing; + const alloc = testing.allocator; + + var s = try init(alloc, 80, 24, 1000); + defer s.deinit(); + + const page = &s.cursor.page_pin.node.data; + + // We add unique styles to the page until no more will fit. + fill: for (0..255) |bg| { + for (0..255) |fg| { + const st: style.Style = .{ + .bg_color = .{ .palette = @intCast(bg) }, + .fg_color = .{ .palette = @intCast(fg) }, + }; + + s.cursor.style = st; + + // Try to insert the new style, if it doesn't fit then + // we succeeded in filling the style set, so we break. + s.cursor.style_id = page.styles.add( + page.memory, + s.cursor.style, + ) catch break :fill; + + try s.testWriteString("a"); + } + } + + // Adjust the capacity, this should cause the style set to reach the + // same state it was in to begin with, since it will clone the page + // in the same order as the styles were added to begin with, meaning + // the cursor style will not be able to be added to the set, which + // should, right now, result in the cursor style being cleared. + _ = try s.adjustCapacity(s.cursor.page_pin.node, .{}); + + // The cursor style should have been cleared by the `adjustCapacity`. + // + // This behavior will change, causing this test to fail, if either + // of these changes are made: + // + // - The adjustCapacity function is changed to ensure the + // new capacity will definitely fit the cursor style. + // + // - The cursor managed memory handling is reworked so that it + // doesn't reside in the pages anymore and doesn't need this + // accounting. + // + // In such a case, adjust this test accordingly. + try testing.expect(s.cursor.style.default()); + try testing.expectEqual(style.default_id, s.cursor.style_id); +} + test "Screen UTF8 cell map with newlines" { const testing = std.testing; const alloc = testing.allocator; diff --git a/src/terminal/hyperlink.zig b/src/terminal/hyperlink.zig index 1ab3c5ea7..bb9e78ca6 100644 --- a/src/terminal/hyperlink.zig +++ b/src/terminal/hyperlink.zig @@ -194,14 +194,24 @@ pub const Set = RefCountedSet( Id, size.CellCountInt, struct { + /// The page which holds the strings for items in this set. page: ?*Page = null, + /// The page which holds the strings for items + /// looked up with, e.g., `add` or `lookup`, + /// if different from the destination page. + src_page: ?*const Page = null, + pub fn hash(self: *const @This(), link: PageEntry) u64 { - return link.hash(self.page.?.memory); + return link.hash((self.src_page orelse self.page.?).memory); } pub fn eql(self: *const @This(), a: PageEntry, b: PageEntry) bool { - return a.eql(self.page.?.memory, &b, self.page.?.memory); + return a.eql( + (self.src_page orelse self.page.?).memory, + &b, + self.page.?.memory, + ); } pub fn deleted(self: *const @This(), link: PageEntry) void { diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 10ba5b5e7..faf376d13 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -272,6 +272,9 @@ pub const Parser = struct { // Maximum length of a single OSC command. This is the full OSC command // sequence length (excluding ESC ]). This is arbitrary, I couldn't find // any definitive resource on how long this should be. + // + // NOTE: This does mean certain OSC sequences such as OSC 8 (hyperlinks) + // won't work if their parameters are larger than fit in the buffer. const MAX_BUF = 2048; pub const State = enum { @@ -425,9 +428,23 @@ pub const Parser = struct { /// Consume the next character c and advance the parser state. pub fn next(self: *Parser, c: u8) void { - // If our buffer is full then we're invalid. + // If our buffer is full then we're invalid, so we set our state + // accordingly and indicate the sequence is incomplete so that we + // don't accidentally issue a command when ending. if (self.buf_idx >= self.buf.len) { + if (self.state != .invalid) { + log.warn( + "OSC sequence too long (> {d}), ignoring. state={}", + .{ self.buf.len, self.state }, + ); + } + self.state = .invalid; + + // We have to do this here because it will never reach the + // switch statement below, since our buf_idx will always be + // too high after this. + self.complete = false; return; } @@ -1643,10 +1660,11 @@ test "OSC: longer than buffer" { var p: Parser = .{}; - const input = "a" ** (Parser.MAX_BUF + 2); + const input = "0;" ++ "a" ** (Parser.MAX_BUF + 2); for (input) |ch| p.next(ch); try testing.expect(p.end(null) == null); + try testing.expect(p.complete == false); } test "OSC: report default foreground color" { diff --git a/src/terminal/page.zig b/src/terminal/page.zig index ae14b8c01..30f6658aa 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -821,11 +821,7 @@ pub const Page = struct { if (self.hyperlink_set.lookupContext( self.memory, other_link.*, - - // `lookupContext` uses the context for hashing, and - // that doesn't write to the page, so this constCast - // is completely safe. - .{ .page = @constCast(other) }, + .{ .page = self, .src_page = @constCast(other) }, )) |i| { self.hyperlink_set.use(self.memory, i); break :dst_id i; diff --git a/src/terminal/ref_counted_set.zig b/src/terminal/ref_counted_set.zig index 1a58a4e5b..b674295dc 100644 --- a/src/terminal/ref_counted_set.zig +++ b/src/terminal/ref_counted_set.zig @@ -38,8 +38,14 @@ const fastmem = @import("../fastmem.zig"); /// /// `Context` /// A type containing methods to define behaviors. +/// /// - `fn hash(*Context, T) u64` - Return a hash for an item. +/// /// - `fn eql(*Context, T, T) bool` - Check two items for equality. +/// The first of the two items passed in is guaranteed to be from +/// a value passed in to an `add` or `lookup` function, the second +/// is guaranteed to be a value already resident in the set. +/// /// - `fn deleted(*Context, T) void` - [OPTIONAL] Deletion callback. /// If present, called whenever an item is finally deleted. /// Useful if the item has memory that needs to be freed.