superrob1500
/
mirror-ghostty
mirror of https://github.com/ghostty-org/ghostty
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix PageList Reflow OOM Conditions (#8277) 

Previously, when encountering an OOM when copying graphemes, hyperlinks,
or styles to a new page during reflow, the attempted resolution was to
copy the current row in to a new page and continue on- which works in
99% of cases, but isn't sound, since it's possible for a single row to
exceed the capacity on any of these.

This led to rare but real crashes like #8009.

I've added tests that produce all of the failure conditions, and
resolved them by changing the strategy from making a new page to
increasing the capacity of the current one.

There should probably be some level of abstraction added around this,
since multiple places in the code now do this sort of thing- attempt to
add some managed memory to a page, adjusting their capacity upwards as
necessary. But for now, I kept it all inline here.
pull/8288/head
Mitchell Hashimoto 2025-08-19 12:30:53 -07:00 committed by GitHub
parent b3f68f6653 a53ec1e567
commit 33b1c969d7
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 420 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

426
src/terminal/PageList.zig
View File

@ -1004,23 +1004,34 @@ const ReflowCursor = struct {
// Copy the graphemes
const cps = src_page.lookupGrapheme(cell).?;
// If our page can't support an additional cell with
// graphemes then we create a new page for this row.
// If our page can't support an additional cell
// with graphemes then we increase capacity.
if (self.page.graphemeCount() >= self.page.graphemeCapacity()) {
try self.moveLastRowToNewPage(list, cap);
} else {
// Attempt to allocate the space that would be required for
// these graphemes, and if it's not available, create a new
// page for this row.
if (self.page.grapheme_alloc.alloc(
u21,
self.page.memory,
cps.len,
)) |slice| {
self.page.grapheme_alloc.free(self.page.memory, slice);
} else |_| {
try self.moveLastRowToNewPage(list, cap);
try self.adjustCapacity(list, .{
.hyperlink_bytes = cap.grapheme_bytes * 2,
});
}
// Attempt to allocate the space that would be required
// for these graphemes, and if it's not available, then
// increase capacity.
if (self.page.grapheme_alloc.alloc(
u21,
self.page.memory,
cps.len,
)) |slice| {
self.page.grapheme_alloc.free(self.page.memory, slice);
} else |_| {
// Grow our capacity until we can
// definitely fit the extra bytes.
const required = cps.len * @sizeOf(u21);
var new_grapheme_capacity: usize = cap.grapheme_bytes;
while (new_grapheme_capacity - cap.grapheme_bytes < required) {
new_grapheme_capacity *= 2;
}
try self.adjustCapacity(list, .{
.grapheme_bytes = new_grapheme_capacity,
});
}
// This shouldn't fail since we made sure we have space above.
@ -1032,25 +1043,67 @@ const ReflowCursor = struct {
const src_id = src_page.lookupHyperlink(cell).?;
const src_link = src_page.hyperlink_set.get(src_page.memory, src_id);
// If our page can't support an additional cell with
// a hyperlink ID then we create a new page for this row.
// If our page can't support an additional cell
// with a hyperlink then we increase capacity.
if (self.page.hyperlinkCount() >= self.page.hyperlinkCapacity()) {
try self.moveLastRowToNewPage(list, cap);
try self.adjustCapacity(list, .{
.hyperlink_bytes = cap.hyperlink_bytes * 2,
});
}
// Ensure that the string alloc has sufficient capacity
// to dupe the link (and the ID if it's not implicit).
const additional_required_string_capacity =
src_link.uri.len +
switch (src_link.id) {
.explicit => |v| v.len,
.implicit => 0,
};
if (self.page.string_alloc.alloc(
u8,
self.page.memory,
additional_required_string_capacity,
)) |slice| {
// We have enough capacity, free the test alloc.
self.page.string_alloc.free(self.page.memory, slice);
} else |_| {
// Grow our capacity until we can
// definitely fit the extra bytes.
var new_string_capacity: usize = cap.string_bytes;
while (new_string_capacity - cap.string_bytes < additional_required_string_capacity) {
new_string_capacity *= 2;
}
try self.adjustCapacity(list, .{
.string_bytes = new_string_capacity,
});
}
const dst_id = self.page.hyperlink_set.addWithIdContext(
self.page.memory,
// We made sure there was enough capacity for this above.
try src_link.dupe(src_page, self.page),
src_id,
.{ .page = self.page },
) catch id: {
// We have no space for this link,
// so make a new page for this row.
try self.moveLastRowToNewPage(list, cap);
) catch |err| id: {
// If the add failed then either the set needs to grow
// or it needs to be rehashed. Either one of those can
// be accomplished by adjusting capacity, either with
// no actual change or with an increased hyperlink cap.
try self.adjustCapacity(list, switch (err) {
error.OutOfMemory => .{
.hyperlink_bytes = cap.hyperlink_bytes * 2,
},
error.NeedsRehash => .{},
});
break :id try self.page.hyperlink_set.addContext(
// We assume this one will succeed. We dupe the link
// again, and don't have to worry about the other one
// because adjusting the capacity naturally clears up
// any managed memory not associated with a cell yet.
break :id try self.page.hyperlink_set.addWithIdContext(
self.page.memory,
try src_link.dupe(src_page, self.page),
src_id,
.{ .page = self.page },
);
} orelse src_id;
@ -1075,14 +1128,23 @@ const ReflowCursor = struct {
self.page.memory,
style,
cell.style_id,
) catch id: {
// We have no space for this style,
// so make a new page for this row.
try self.moveLastRowToNewPage(list, cap);
) catch |err| id: {
// If the add failed then either the set needs to grow
// or it needs to be rehashed. Either one of those can
// be accomplished by adjusting capacity, either with
// no actual change or with an increased style cap.
try self.adjustCapacity(list, switch (err) {
error.OutOfMemory => .{
.styles = cap.styles * 2,
},
error.NeedsRehash => .{},
});
break :id try self.page.styles.add(
// We assume this one will succeed.
break :id try self.page.styles.addWithId(
self.page.memory,
style,
cell.style_id,
);
} orelse cell.style_id;
@ -1150,6 +1212,22 @@ const ReflowCursor = struct {
}
}
/// Adjust the capacity of the current page.
fn adjustCapacity(
self: *ReflowCursor,
list: *PageList,
adjustment: AdjustCapacity,
) !void {
const old_x = self.x;
const old_y = self.y;
self.* = .init(try list.adjustCapacity(
self.node,
adjustment,
));
self.cursorAbsolute(old_x, old_y);
}
/// True if this cursor is at the bottom of the page by capacity,
/// i.e. we can't scroll anymore.
fn bottom(self: *const ReflowCursor) bool {
@ -7029,6 +7107,7 @@ test "PageList resize reflow less cols wrap across page boundary cursor in secon
try testing.expect(!cells[3].hasText());
}
}
test "PageList resize reflow more cols cursor in wrapped row" {
const testing = std.testing;
const alloc = testing.allocator;
@ -7222,6 +7301,296 @@ test "PageList resize reflow more cols no reflow preserves semantic prompt" {
}
}
test "PageList resize reflow exceeds hyperlink memory forcing capacity increase" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try init(alloc, 2, 10, 0);
defer s.deinit();
try testing.expectEqual(@as(usize, 1), s.totalPages());
// Grow to the capacity of the first page and add
// one more row so that we have two pages total.
{
const page = &s.pages.first.?.data;
page.pauseIntegrityChecks(true);
for (page.size.rows..page.capacity.rows) |_| {
_ = try s.grow();
}
page.pauseIntegrityChecks(false);
try testing.expectEqual(@as(usize, 1), s.totalPages());
try s.growRows(1);
try testing.expectEqual(@as(usize, 2), s.totalPages());
// We now have two pages.
try std.testing.expect(s.pages.first.? != s.pages.last.?);
try std.testing.expectEqual(s.pages.last.?, s.pages.first.?.next);
}
// We use almost all string alloc capacity with a hyperlink in the final
// row of the first page, and do the same on the first row of the second
// page. We also mark the row as wrapped so that when we resize with more
// cols the row unwraps and we have a single row that requires almost two
// times the base string alloc capacity.
//
// This forces the reflow to increase capacity.
//
// +--+ = PAGE 0
// : :
// | X… <- where X is hyperlinked with almost all string cap.
// +--+
// +--+ = PAGE 1
// …X | <- X here also almost hits string cap with a hyperlink.
// +--+
// Almost hit string alloc cap in bottom right of first page.
// Mark the final row as wrapped.
{
const page = &s.pages.first.?.data;
const id = try page.insertHyperlink(.{
.id = .{ .implicit = 0 },
.uri = "a" ** (pagepkg.string_bytes_default - 1),
});
const rac = page.getRowAndCell(page.size.cols - 1, page.size.rows - 1);
rac.row.wrap = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
};
try page.setHyperlink(rac.row, rac.cell, id);
try std.testing.expectError(
error.StringsOutOfMemory,
page.insertHyperlink(.{
.id = .{ .implicit = 1 },
.uri = "AAAAAAAAAAAAAAAAAAAAAAAAAA",
}),
);
}
// Almost hit string alloc cap in top left of second page.
// Mark the first row as a wrap continuation.
{
const page = &s.pages.last.?.data;
const id = try page.insertHyperlink(.{
.id = .{ .implicit = 1 },
.uri = "a" ** (pagepkg.string_bytes_default - 1),
});
const rac = page.getRowAndCell(0, 0);
rac.row.wrap_continuation = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
};
try page.setHyperlink(rac.row, rac.cell, id);
try std.testing.expectError(
error.StringsOutOfMemory,
page.insertHyperlink(.{
.id = .{ .implicit = 2 },
.uri = "AAAAAAAAAAAAAAAAAAAAAAAAAA",
}),
);
}
// Resize to 1 column wider, unwrapping the row.
try s.resize(.{ .cols = s.cols + 1, .reflow = true });
}
test "PageList resize reflow exceeds grapheme memory forcing capacity increase" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try init(alloc, 2, 10, 0);
defer s.deinit();
try testing.expectEqual(@as(usize, 1), s.totalPages());
// Grow to the capacity of the first page and add
// one more row so that we have two pages total.
{
const page = &s.pages.first.?.data;
page.pauseIntegrityChecks(true);
for (page.size.rows..page.capacity.rows) |_| {
_ = try s.grow();
}
page.pauseIntegrityChecks(false);
try testing.expectEqual(@as(usize, 1), s.totalPages());
try s.growRows(1);
try testing.expectEqual(@as(usize, 2), s.totalPages());
// We now have two pages.
try std.testing.expect(s.pages.first.? != s.pages.last.?);
try std.testing.expectEqual(s.pages.last.?, s.pages.first.?.next);
}
// We use almost all grapheme alloc capacity with a grapheme in the final
// row of the first page, and do the same on the first row of the second
// page. We also mark the row as wrapped so that when we resize with more
// cols the row unwraps and we have a single row that requires almost two
// times the base grapheme alloc capacity.
//
// This forces the reflow to increase capacity.
//
// +--+ = PAGE 0
// : :
// | X… <- where X is a grapheme which uses almost all the capacity.
// +--+
// +--+ = PAGE 1
// …X | <- X here also almost hits grapheme cap.
// +--+
// Almost hit grapheme alloc cap in bottom right of first page.
// Mark the final row as wrapped.
{
const page = &s.pages.first.?.data;
const rac = page.getRowAndCell(page.size.cols - 1, page.size.rows - 1);
rac.row.wrap = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
};
try page.setGraphemes(
rac.row,
rac.cell,
&@as(
[
@divFloor(
pagepkg.grapheme_bytes_default - 1,
@sizeOf(u21),
)
]u21,
@splat('a'),
),
);
try std.testing.expectError(
error.OutOfMemory,
page.grapheme_alloc.alloc(
u21,
page.memory,
16,
),
);
}
// Almost hit grapheme alloc cap in top left of second page.
// Mark the first row as a wrap continuation.
{
const page = &s.pages.last.?.data;
const rac = page.getRowAndCell(0, 0);
rac.row.wrap = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
};
try page.setGraphemes(
rac.row,
rac.cell,
&@as(
[
@divFloor(
pagepkg.grapheme_bytes_default - 1,
@sizeOf(u21),
)
]u21,
@splat('a'),
),
);
try std.testing.expectError(
error.OutOfMemory,
page.grapheme_alloc.alloc(
u21,
page.memory,
16,
),
);
}
// Resize to 1 column wider, unwrapping the row.
try s.resize(.{ .cols = s.cols + 1, .reflow = true });
}
test "PageList resize reflow exceeds style memory forcing capacity increase" {
const testing = std.testing;
const alloc = testing.allocator;
var s = try init(alloc, pagepkg.std_capacity.styles - 1, 10, 0);
defer s.deinit();
try testing.expectEqual(@as(usize, 1), s.totalPages());
// Grow to the capacity of the first page and add
// one more row so that we have two pages total.
{
const page = &s.pages.first.?.data;
page.pauseIntegrityChecks(true);
for (page.size.rows..page.capacity.rows) |_| {
_ = try s.grow();
}
page.pauseIntegrityChecks(false);
try testing.expectEqual(@as(usize, 1), s.totalPages());
try s.growRows(1);
try testing.expectEqual(@as(usize, 2), s.totalPages());
// We now have two pages.
try std.testing.expect(s.pages.first.? != s.pages.last.?);
try std.testing.expectEqual(s.pages.last.?, s.pages.first.?.next);
}
// Give each cell in the final row of the first page a unique style.
// Mark the final row as wrapped.
{
const page = &s.pages.first.?.data;
for (0..s.cols) |x| {
const id = page.styles.add(
page.memory,
.{
.bg_color = .{ .rgb = .{
.r = @truncate(x),
.g = @truncate(x >> 8),
.b = @truncate(x >> 16),
} },
},
) catch break;
const rac = page.getRowAndCell(x, page.size.rows - 1);
rac.row.wrap = true;
rac.row.styled = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
.style_id = id,
};
}
}
// Do the same for the first row of the second page.
// Mark the first row as a wrap continuation.
{
const page = &s.pages.last.?.data;
for (0..s.cols) |x| {
const id = page.styles.add(
page.memory,
.{
.fg_color = .{ .rgb = .{
.r = @truncate(x),
.g = @truncate(x >> 8),
.b = @truncate(x >> 16),
} },
},
) catch break;
const rac = page.getRowAndCell(x, 0);
rac.row.wrap_continuation = true;
rac.row.styled = true;
rac.cell.* = .{
.content_tag = .codepoint,
.content = .{ .codepoint = 'X' },
.style_id = id,
};
}
}
// Resize to twice as wide, fully unwrapping the row.
try s.resize(.{ .cols = s.cols * 2, .reflow = true });
}
test "PageList resize reflow more cols unwrap wide spacer head" {
const testing = std.testing;
const alloc = testing.allocator;
@ -7767,6 +8136,7 @@ test "PageList resize reflow less cols wrapped rows with graphemes" {
try testing.expectEqual(@as(u21, 'A'), cps[0]);
}
}
test "PageList resize reflow less cols cursor in wrapped row" {
const testing = std.testing;
const alloc = testing.allocator;

33
src/terminal/hyperlink.zig
View File

@ -185,6 +185,25 @@ pub const PageEntry = struct {
other.uri.offset.ptr(other_base)[0..other.uri.len],
);
}
/// Free the memory for this entry from its page.
pub fn free(
self: *const PageEntry,
page: *Page,
) void {
const alloc = &page.string_alloc;
switch (self.id) {
.implicit => {},
.explicit => |v| alloc.free(
page.memory,
v.offset.ptr(page.memory)[0..v.len],
),
}
alloc.free(
page.memory,
self.uri.offset.ptr(page.memory)[0..self.uri.len],
);
}
};
/// The set of hyperlinks. This is ref-counted so that a set of cells
@ -215,19 +234,7 @@ pub const Set = RefCountedSet(
}
pub fn deleted(self: *const @This(), link: PageEntry) void {
const page = self.page.?;
const alloc = &page.string_alloc;
switch (link.id) {
.implicit => {},
.explicit => |v| alloc.free(
page.memory,
v.offset.ptr(page.memory)[0..v.len],
),
}
alloc.free(
page.memory,
link.uri.offset.ptr(page.memory)[0..link.uri.len],
);
link.free(self.page.?);
}
},
);

4
src/terminal/page.zig
View File

@ -34,7 +34,7 @@ const grapheme_chunk_len = 4;
const grapheme_chunk = grapheme_chunk_len * @sizeOf(u21);
const GraphemeAlloc = BitmapAllocator(grapheme_chunk);
const grapheme_count_default = GraphemeAlloc.bitmap_bit_size;
const grapheme_bytes_default = grapheme_count_default * grapheme_chunk;
pub const grapheme_bytes_default = grapheme_count_default * grapheme_chunk;
const GraphemeMap = AutoOffsetHashMap(Offset(Cell), Offset(u21).Slice);
/// The allocator used for shared utf8-encoded strings within a page.
@ -53,7 +53,7 @@ const string_chunk_len = 32;
const string_chunk = string_chunk_len * @sizeOf(u8);
const StringAlloc = BitmapAllocator(string_chunk);
const string_count_default = StringAlloc.bitmap_bit_size;
const string_bytes_default = string_count_default * string_chunk;
pub const string_bytes_default = string_count_default * string_chunk;
/// Default number of hyperlinks we support.
///