From 09c76d95c7d235db0953623f980a7c47bfc94568 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Sun, 9 Feb 2025 22:48:38 -0500 Subject: [PATCH 1/6] fix(terminal): handle errors in Screen.adjustCapacity Previously assumed adjusted capacities would always fit the cursor style and hyperlink, this is not necessarily the case and led to memory corruption in scenarios with large hyperlinks. --- src/terminal/Screen.zig | 68 ++++++++++++++++++++++++++++++++++------- 1 file changed, 57 insertions(+), 11 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index a838e0e10..046ecb1b4 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -474,26 +474,40 @@ 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.err( + "(Screen.adjustCapacity) Failed to add cursor style back to page, err={}", + .{err}, + ); + + 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.err( + "(Screen.adjustCapacity) Failed to add cursor hyperlink back to page, err={}", + .{err}, + ); + }; // Remove our old link link.deinit(self.alloc); @@ -1003,8 +1017,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 +2001,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 From 03fd9a970bf1d1760aef500eda4f83c0f13f5962 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Feb 2025 11:49:05 -0500 Subject: [PATCH 2/6] fix(terminal): properly invalidate over-sized OSCs The `self.complete = false` here is important so we don't accidentally dispatch, for example, a hyperlink command with an empty URI. --- src/terminal/osc.zig | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 10ba5b5e7..90dd079a0 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; } From 1c524238c837cbd0d550a2e74af6df6e6235794a Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Feb 2025 13:27:26 -0500 Subject: [PATCH 3/6] test(terminal/osc): fix command longer than buffer test Ensure that the state is invalidated properly, this previously wasn't the case but was fixed in 03fd9a97 --- src/terminal/osc.zig | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/terminal/osc.zig b/src/terminal/osc.zig index 90dd079a0..faf376d13 100644 --- a/src/terminal/osc.zig +++ b/src/terminal/osc.zig @@ -1660,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" { From e540a79032c4c7c691e1299b3e0a6b4fced10013 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Feb 2025 14:19:26 -0500 Subject: [PATCH 4/6] test(terminal/screen): OOM handling in adjustCapacity Adds tests for the adjustCapacity changes from 09c76d95 Fixes a small oversight in that change as well (resetting cursor style). --- src/terminal/Screen.zig | 114 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 112 insertions(+), 2 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 046ecb1b4..b11c207a5 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -483,11 +483,13 @@ pub fn adjustCapacity( self.cursor.style, ) catch |err| id: { // TODO: Should we increase the capacity further in this case? - log.err( + 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; }; } @@ -503,7 +505,7 @@ pub fn adjustCapacity( // Re-add self.startHyperlinkOnce(link.*) catch |err| { // TODO: Should we increase the capacity further in this case? - log.err( + log.warn( "(Screen.adjustCapacity) Failed to add cursor hyperlink back to page, err={}", .{err}, ); @@ -2928,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; @@ -2946,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); @@ -2961,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.* = .{ @@ -2970,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; @@ -8805,6 +8819,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; From a1b682d0da53fbdc1d9e40b758ab61f744b3897d Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Feb 2025 15:45:53 -0500 Subject: [PATCH 5/6] fix(terminal): properly lookup hyperlinks when cloning rows across pages Before this we were doing bad things with the memory, looking at `PageEntry`s for links not actually in the page we were reading the strings from. --- src/terminal/hyperlink.zig | 14 ++++++++++++-- src/terminal/page.zig | 6 +----- src/terminal/ref_counted_set.zig | 6 ++++++ 3 files changed, 19 insertions(+), 7 deletions(-) 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/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. From dd8c795ec605f808aaf82d8fa551844cf09178fc Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Mon, 10 Feb 2025 15:48:06 -0500 Subject: [PATCH 6/6] test(terminal/screen): cursorSetHyperlink OOM handling edge case Tests handling introduced in 09c76d95 which ensures sufficient space for the cursor hyperlink uri in the string alloc when adjusting capacity. --- src/terminal/Screen.zig | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index b11c207a5..98894a83d 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -8785,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;