mirror of
https://github.com/ghostty-org/ghostty.git
synced 2025-07-20 10:46:07 +03:00
Fix large OSC 8 links causing memory corruption (#5666)
Fixes #5635 ### Changes: - Added handling to `Screen.adjustCapacity` which properly returns an error if there's no room for the cursor hyperlink. Note the TODO, in the future we should probably make it handle this by increasing the capacity further. - Added handling to `Screen.cursorSetHyperlink` which ensures there will be sufficient capacity to add the redundant cursor hyperlink to the string alloc after adjusting the capacity. ***Future work:** Aside from separating the cursor's managed memory from the page memory, which is a big thing we desperately need, we should probably also make the page string alloc intern the strings it gets to deduplicate.* - Fixed a slight bug in the OSC parser which was causing the hyperlink command to be issued with an empty (0-length) URI when the buffer capacity was exceeded during an OSC 8 sequence. ***Future work:** We should consider increasing the `MAX_BUF` since some specs call for a max size of 4096 (such as various Kitty protocols) -- otherwise we should switch all the OSCs that can take arbitrary data like this to use the allocator instead of the fixed buffer.* - Fixed a problem where when cloning content across pages (as happened every time we had to adjust page capacities) hyperlinks would not be properly looked up, often leading to many many redundant copies of a given URI being stored, exploding string memory.
This commit is contained in:
Mitchell Hashimoto
@ -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;
|
||||
|
||||
@ -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 {
|
||||
|
||||
@ -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" {
|
||||
|
||||
@ -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;
|
||||
|
||||
@ -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.
|
||||
|
||||
Reference in New Issue
Block a user