Merge pull request #2198 from ghostty-org/decaln

DECALN must clear protected cells (fixes managed memory leak in page)
This commit is contained in:
Mitchell Hashimoto
2024-09-05 14:43:32 -07:00
committed by GitHub
2 changed files with 63 additions and 3 deletions

View File

@ -2272,8 +2272,13 @@ pub fn decaln(self: *Terminal) !void {
// Move our cursor to the top-left
self.setCursorPos(1, 1);
// Erase the display which will deallocate graphemes, styles, etc.
self.eraseDisplay(.complete, false);
// Use clearRows instead of eraseDisplay because we must NOT respect
// protected attributes here.
self.screen.clearRows(
.{ .active = .{} },
null,
false,
);
// Fill with Es by moving the cursor but reset it after.
while (true) {
@ -2285,7 +2290,9 @@ pub fn decaln(self: *Terminal) !void {
.content_tag = .codepoint,
.content = .{ .codepoint = 'E' },
.style_id = self.screen.cursor.style_id,
.protected = self.screen.cursor.protected,
// DECALN does not respect protected state. Verified with xterm.
.protected = false,
});
// If we have a ref-counted style, increase
@ -2298,6 +2305,9 @@ pub fn decaln(self: *Terminal) !void {
row.styled = true;
}
// We messed with the page so assert its integrity here.
page.assertIntegrity();
self.screen.cursorMarkDirty();
if (self.screen.cursor.y == self.rows - 1) break;
self.screen.cursorDown(1);
@ -8030,6 +8040,42 @@ test "Terminal: decaln preserves color" {
}
}
test "Terminal: DECALN resets graphemes with protected mode" {
const alloc = testing.allocator;
var t = try init(alloc, .{ .cols = 3, .rows = 3 });
defer t.deinit(alloc);
// Add protected mode. A previous version of DECALN accidentally preserved
// protected mode which left dangling managed memory.
t.setProtectedMode(.iso);
// This is: 👨👩👧 (which may or may not render correctly)
t.modes.set(.grapheme_cluster, true);
try t.print(0x1F468);
try t.print(0x200D);
try t.print(0x1F469);
try t.print(0x200D);
try t.print(0x1F467);
try t.decaln();
try testing.expectEqual(@as(usize, 0), t.screen.cursor.y);
try testing.expectEqual(@as(usize, 0), t.screen.cursor.x);
try testing.expect(t.screen.cursor.protected);
try testing.expect(t.screen.protected_mode == .iso);
for (0..t.rows) |y| try testing.expect(t.isDirty(.{ .active = .{
.x = 0,
.y = @intCast(y),
} }));
{
const str = try t.plainString(testing.allocator);
defer testing.allocator.free(str);
try testing.expectEqualStrings("EEE\nEEE\nEEE", str);
}
}
test "Terminal: insertBlanks" {
// NOTE: this is not verified with conformance tests, so these
// tests might actually be verifying wrong behavior.

View File

@ -289,6 +289,7 @@ pub const Page = struct {
UnmarkedGraphemeRow,
MissingGraphemeData,
InvalidGraphemeCount,
UnmarkedGraphemeCell,
MissingStyle,
UnmarkedStyleRow,
MismatchedStyleRef,
@ -368,6 +369,8 @@ pub const Page = struct {
var hyperlinks_seen = std.AutoHashMap(hyperlink.Id, usize).init(alloc);
defer hyperlinks_seen.deinit();
const grapheme_count = self.graphemeCount();
const rows = self.rows.ptr(self.memory)[0..self.size.rows];
for (rows, 0..) |*row, y| {
const graphemes_start = graphemes_seen;
@ -385,6 +388,17 @@ pub const Page = struct {
};
graphemes_seen += 1;
} else if (grapheme_count > 0) {
// It should not have grapheme data if it isn't marked.
// The grapheme_count check above is just an optimization
// to speed up integrity checks.
if (self.lookupGrapheme(cell) != null) {
log.warn(
"page integrity violation y={} x={} cell not marked as grapheme",
.{ y, x },
);
return IntegrityError.UnmarkedGraphemeCell;
}
}
if (cell.style_id != style.default_id) {