From 0a7da32c7161c183db0bd0bdebafb4163e7c5d51 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Nov 2025 15:29:35 -0700 Subject: [PATCH 1/6] fix: drop tmux control parsing immediately if broken --- src/terminal/tmux.zig | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/terminal/tmux.zig b/src/terminal/tmux.zig index 67c5a979c..54cd7cdd5 100644 --- a/src/terminal/tmux.zig +++ b/src/terminal/tmux.zig @@ -33,7 +33,8 @@ pub const Client = struct { idle, /// We experienced unexpected input and are in a broken state - /// so we cannot continue processing. + /// so we cannot continue processing. When this state is set, + /// the buffer has been deinited and must not be accessed. broken, /// Inside an active notification (started with '%'). @@ -44,11 +45,21 @@ pub const Client = struct { }; pub fn deinit(self: *Client) void { + // If we're in a broken state, we already deinited + // the buffer, so we don't need to do anything. + if (self.state == .broken) return; + self.buffer.deinit(); } // Handle a byte of input. pub fn put(self: *Client, byte: u8) !?Notification { + // If we're in a broken state, just do nothing. + // + // We have to do this check here before we check the buffer, because if + // we're in a broken state then we'd have already deinited the buffer. + if (self.state == .broken) return null; + if (self.buffer.written().len >= self.max_bytes) { self.broken(); return error.OutOfMemory; From 712cc9e55c4dfc006c8e9767c07aee3af96dcbb3 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Nov 2025 16:50:18 -0700 Subject: [PATCH 2/6] fix(shaper/coretext): handle non-monotonic runs by sorting --- src/font/shaper/coretext.zig | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index d73b191b8..45844d3e2 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -392,6 +392,12 @@ pub const Shaper = struct { self.cell_buf.clearRetainingCapacity(); try self.cell_buf.ensureTotalCapacity(self.alloc, line.getGlyphCount()); + // CoreText, despite our insistence with an enforced embedding level, + // may sometimes output runs that are non-monotonic. In order to fix + // this, we check the run status for each run and if any aren't ltr + // we set this to true, which indicates that we must sort our buffer. + var non_ltr: bool = false; + // CoreText may generate multiple runs even though our input to // CoreText is already split into runs by our own run iterator. // The runs as far as I can tell are always sequential to each @@ -401,6 +407,9 @@ pub const Shaper = struct { for (0..runs.getCount()) |i| { const ctrun = runs.getValueAtIndex(macos.text.Run, i); + const status = ctrun.getStatus(); + if (status.non_monotonic or status.right_to_left) non_ltr = true; + // Get our glyphs and positions const glyphs = ctrun.getGlyphsPtr() orelse try ctrun.getGlyphs(alloc); const advances = ctrun.getAdvancesPtr() orelse try ctrun.getAdvances(alloc); @@ -441,6 +450,25 @@ pub const Shaper = struct { } } + // If our buffer contains some non-ltr sections we need to sort it :/ + if (non_ltr) { + // This is EXCEPTIONALLY rare. Only happens for languages with + // complex shaping which we don't even really support properly + // right now, so are very unlikely to be used heavily by users + // of Ghostty. + @branchHint(.cold); + std.mem.sort( + font.shape.Cell, + self.cell_buf.items, + {}, + struct { + fn lt(_: void, a: font.shape.Cell, b: font.shape.Cell) bool { + return a.x < b.x; + } + }.lt, + ); + } + return self.cell_buf.items; } From 985e1a3ceaedc684dcc303bc205594c25279c5ab Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Fri, 14 Nov 2025 17:39:03 -0700 Subject: [PATCH 3/6] test(shaper/coretext): test non-monotonic CoreText output --- src/font/shaper/coretext.zig | 92 ++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/src/font/shaper/coretext.zig b/src/font/shaper/coretext.zig index 45844d3e2..c2cfb389c 100644 --- a/src/font/shaper/coretext.zig +++ b/src/font/shaper/coretext.zig @@ -1202,6 +1202,51 @@ test "shape Chinese characters" { try testing.expectEqual(@as(usize, 1), count); } +// This test exists because the string it uses causes CoreText to output a +// non-monotonic run, which we need to handle by sorting the resulting buffer. +test "shape Devanagari string" { + const testing = std.testing; + const alloc = testing.allocator; + + // We need a font that supports devanagari for this to work, if we can't + // find Arial Unicode MS, which is a system font on macOS, we just skip + // the test. + var testdata = testShaperWithDiscoveredFont( + alloc, + "Arial Unicode MS", + ) catch return error.SkipZigTest; + defer testdata.deinit(); + + // Make a screen with some data + var screen = try terminal.Screen.init(alloc, .{ .cols = 30, .rows = 3, .max_scrollback = 0 }); + defer screen.deinit(); + try screen.testWriteString("अपार्टमेंट"); + + // Get our run iterator + var shaper = &testdata.shaper; + var it = shaper.runIterator(.{ + .grid = testdata.grid, + .screen = &screen, + .row = screen.pages.pin(.{ .screen = .{ .y = 0 } }).?, + }); + + const run = try it.next(alloc); + try testing.expect(run != null); + const cells = try shaper.shape(run.?); + + try testing.expectEqual(@as(usize, 8), cells.len); + try testing.expectEqual(@as(u16, 0), cells[0].x); + try testing.expectEqual(@as(u16, 1), cells[1].x); + try testing.expectEqual(@as(u16, 2), cells[2].x); + try testing.expectEqual(@as(u16, 3), cells[3].x); + try testing.expectEqual(@as(u16, 4), cells[4].x); + try testing.expectEqual(@as(u16, 5), cells[5].x); + try testing.expectEqual(@as(u16, 5), cells[6].x); + try testing.expectEqual(@as(u16, 6), cells[7].x); + + try testing.expect(try it.next(alloc) == null); +} + test "shape box glyphs" { const testing = std.testing; const alloc = testing.allocator; @@ -1890,3 +1935,50 @@ fn testShaperWithFont(alloc: Allocator, font_req: TestFont) !TestShaper { .lib = lib, }; } + +/// Return a fully initialized shaper by discovering a named font on the system. +fn testShaperWithDiscoveredFont(alloc: Allocator, font_req: [:0]const u8) !TestShaper { + var lib = try Library.init(alloc); + errdefer lib.deinit(); + + var c = Collection.init(); + c.load_options = .{ .library = lib }; + + // Discover and add our font to the collection. + { + var disco = font.Discover.init(); + defer disco.deinit(); + var disco_it = try disco.discover(alloc, .{ + .family = font_req, + .size = 12, + .monospace = false, + }); + defer disco_it.deinit(); + var face: font.DeferredFace = (try disco_it.next()).?; + errdefer face.deinit(); + _ = try c.add( + alloc, + try face.load(lib, .{ .size = .{ .points = 12 } }), + .{ + .style = .regular, + .fallback = false, + .size_adjustment = .none, + }, + ); + } + + const grid_ptr = try alloc.create(SharedGrid); + errdefer alloc.destroy(grid_ptr); + grid_ptr.* = try .init(alloc, .{ .collection = c }); + errdefer grid_ptr.*.deinit(alloc); + + var shaper = try Shaper.init(alloc, .{}); + errdefer shaper.deinit(); + + return TestShaper{ + .alloc = alloc, + .shaper = shaper, + .grid = grid_ptr, + .lib = lib, + }; +} From 00c2216fe1f976abcab11670691ecd6f78d8804e Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Sun, 16 Nov 2025 10:32:57 -0700 Subject: [PATCH 4/6] style: add Offset.Slice.slice helper fn Makes code that interacts with these so much cleaner --- src/Surface.zig | 2 +- src/renderer/link.zig | 6 +++--- src/terminal/Screen.zig | 4 ++-- src/terminal/hyperlink.zig | 20 ++++++++++---------- src/terminal/page.zig | 10 +++++----- src/terminal/size.zig | 5 +++++ 6 files changed, 26 insertions(+), 21 deletions(-) diff --git a/src/Surface.zig b/src/Surface.zig index 308b6d1f7..aa7902741 100644 --- a/src/Surface.zig +++ b/src/Surface.zig @@ -4103,7 +4103,7 @@ fn osc8URI(self: *Surface, pin: terminal.Pin) ?[]const u8 { const cell = pin.rowAndCell().cell; const link_id = page.lookupHyperlink(cell) orelse return null; const entry = page.hyperlink_set.get(page.memory, link_id); - return entry.uri.offset.ptr(page.memory)[0..entry.uri.len]; + return entry.uri.slice(page.memory); } pub fn mousePressureCallback( diff --git a/src/renderer/link.zig b/src/renderer/link.zig index 39283cf5f..e16a85a68 100644 --- a/src/renderer/link.zig +++ b/src/renderer/link.zig @@ -131,7 +131,7 @@ pub const Set = struct { // then we use an alternate matching technique that iterates forward // and backward until it finds boundaries. if (link.id == .implicit) { - const uri = link.uri.offset.ptr(page.memory)[0..link.uri.len]; + const uri = link.uri.slice(page.memory); return try self.matchSetFromOSC8Implicit( alloc, matches, @@ -232,7 +232,7 @@ pub const Set = struct { if (link.id != .implicit) break; // If this link has a different URI then we found a boundary - const cell_uri = link.uri.offset.ptr(page.memory)[0..link.uri.len]; + const cell_uri = link.uri.slice(page.memory); if (!std.mem.eql(u8, uri, cell_uri)) break; sel.startPtr().* = cell_pin; @@ -258,7 +258,7 @@ pub const Set = struct { if (link.id != .implicit) break; // If this link has a different URI then we found a boundary - const cell_uri = link.uri.offset.ptr(page.memory)[0..link.uri.len]; + const cell_uri = link.uri.slice(page.memory); if (!std.mem.eql(u8, uri, cell_uri)) break; sel.endPtr().* = cell_pin; diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 8ed256869..73992bf88 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -1050,9 +1050,9 @@ pub fn cursorCopy(self: *Screen, other: Cursor, opts: struct { const other_page = &other.page_pin.node.data; const other_link = other_page.hyperlink_set.get(other_page.memory, other.hyperlink_id); - const uri = other_link.uri.offset.ptr(other_page.memory)[0..other_link.uri.len]; + const uri = other_link.uri.slice(other_page.memory); const id_ = switch (other_link.id) { - .explicit => |id| id.offset.ptr(other_page.memory)[0..id.len], + .explicit => |id| id.slice(other_page.memory), .implicit => null, }; diff --git a/src/terminal/hyperlink.zig b/src/terminal/hyperlink.zig index c608321b1..f0c2738b1 100644 --- a/src/terminal/hyperlink.zig +++ b/src/terminal/hyperlink.zig @@ -103,7 +103,7 @@ pub const PageEntry = struct { // Copy the URI { - const uri = self.uri.offset.ptr(self_page.memory)[0..self.uri.len]; + const uri = self.uri.slice(self_page.memory); const buf = try dst_page.string_alloc.alloc(u8, dst_page.memory, uri.len); @memcpy(buf, uri); copy.uri = .{ @@ -113,14 +113,14 @@ pub const PageEntry = struct { } errdefer dst_page.string_alloc.free( dst_page.memory, - copy.uri.offset.ptr(dst_page.memory)[0..copy.uri.len], + copy.uri.slice(dst_page.memory), ); // Copy the ID switch (copy.id) { .implicit => {}, // Shallow is fine .explicit => |slice| { - const id = slice.offset.ptr(self_page.memory)[0..slice.len]; + const id = slice.slice(self_page.memory); const buf = try dst_page.string_alloc.alloc(u8, dst_page.memory, id.len); @memcpy(buf, id); copy.id = .{ .explicit = .{ @@ -133,7 +133,7 @@ pub const PageEntry = struct { .implicit => {}, .explicit => |v| dst_page.string_alloc.free( dst_page.memory, - v.offset.ptr(dst_page.memory)[0..v.len], + v.slice(dst_page.memory), ), }; @@ -147,13 +147,13 @@ pub const PageEntry = struct { .implicit => |v| autoHash(&hasher, v), .explicit => |slice| autoHashStrat( &hasher, - slice.offset.ptr(base)[0..slice.len], + slice.slice(base), .Deep, ), } autoHashStrat( &hasher, - self.uri.offset.ptr(base)[0..self.uri.len], + self.uri.slice(base), .Deep, ); return hasher.final(); @@ -181,8 +181,8 @@ pub const PageEntry = struct { return std.mem.eql( u8, - self.uri.offset.ptr(self_base)[0..self.uri.len], - other.uri.offset.ptr(other_base)[0..other.uri.len], + self.uri.slice(self_base), + other.uri.slice(other_base), ); } @@ -196,12 +196,12 @@ pub const PageEntry = struct { .implicit => {}, .explicit => |v| alloc.free( page.memory, - v.offset.ptr(page.memory)[0..v.len], + v.slice(page.memory), ), } alloc.free( page.memory, - self.uri.offset.ptr(page.memory)[0..self.uri.len], + self.uri.slice(page.memory), ); } }; diff --git a/src/terminal/page.zig b/src/terminal/page.zig index 5c83fc7c8..b13c625ed 100644 --- a/src/terminal/page.zig +++ b/src/terminal/page.zig @@ -1198,7 +1198,7 @@ pub const Page = struct { }; errdefer self.string_alloc.free( self.memory, - page_uri.offset.ptr(self.memory)[0..page_uri.len], + page_uri.slice(self.memory), ); // Allocate an ID for our page memory if we have to. @@ -1228,7 +1228,7 @@ pub const Page = struct { .implicit => {}, .explicit => |slice| self.string_alloc.free( self.memory, - slice.offset.ptr(self.memory)[0..slice.len], + slice.slice(self.memory), ), }; @@ -1421,7 +1421,7 @@ pub const Page = struct { // most graphemes to fit within our chunk size. const cps = try self.grapheme_alloc.alloc(u21, self.memory, slice.len + 1); errdefer self.grapheme_alloc.free(self.memory, cps); - const old_cps = slice.offset.ptr(self.memory)[0..slice.len]; + const old_cps = slice.slice(self.memory); fastmem.copy(u21, cps[0..old_cps.len], old_cps); cps[slice.len] = cp; slice.* = .{ @@ -1440,7 +1440,7 @@ pub const Page = struct { const cell_offset = getOffset(Cell, self.memory, cell); const map = self.grapheme_map.map(self.memory); const slice = map.get(cell_offset) orelse return null; - return slice.offset.ptr(self.memory)[0..slice.len]; + return slice.slice(self.memory); } /// Move the graphemes from one cell to another. This can't fail @@ -1475,7 +1475,7 @@ pub const Page = struct { const entry = map.getEntry(cell_offset).?; // Free our grapheme data - const cps = entry.value_ptr.offset.ptr(self.memory)[0..entry.value_ptr.len]; + const cps = entry.value_ptr.slice(self.memory); self.grapheme_alloc.free(self.memory, cps); // Remove the entry diff --git a/src/terminal/size.zig b/src/terminal/size.zig index 8322ddb41..9c99f7732 100644 --- a/src/terminal/size.zig +++ b/src/terminal/size.zig @@ -28,6 +28,11 @@ pub fn Offset(comptime T: type) type { pub const Slice = struct { offset: Self = .{}, len: usize = 0, + + /// Returns a slice for the data, properly typed. + pub inline fn slice(self: Slice, base: anytype) []T { + return self.offset.ptr(base)[0..self.len]; + } }; /// Returns a pointer to the start of the data, properly typed. From bb2455b3fce9410114ebd391d144c60a9fa2677d Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Sun, 16 Nov 2025 11:27:59 -0700 Subject: [PATCH 5/6] fix(terminal/stream): handle executing C1 controls These can be unambiguously invoked in certain parser states, and as such we need to handle them. In real world use they are extremely rare, hence the branch hint. Without this, we get illegal behavior by trying to cast the value to the 7-bit C0 enum. --- src/terminal/stream.zig | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/terminal/stream.zig b/src/terminal/stream.zig index 23211fa80..de83dbe9c 100644 --- a/src/terminal/stream.zig +++ b/src/terminal/stream.zig @@ -660,6 +660,11 @@ pub fn Stream(comptime Handler: type) type { /// This function is abstracted this way to handle the case where /// the decoder emits a 0x1B after rejecting an ill-formed sequence. inline fn handleCodepoint(self: *Self, c: u21) !void { + // We need to increase the eval branch limit because a lot of + // tests end up running almost completely at comptime due to + // a chain of inline functions. + @setEvalBranchQuota(100_000); + if (c <= 0xF) { try self.execute(@intCast(c)); return; @@ -777,6 +782,18 @@ pub fn Stream(comptime Handler: type) type { } pub inline fn execute(self: *Self, c: u8) !void { + // If the character is > 0x7F, it's a C1 (8-bit) control, + // which is strictly equivalent to `ESC` plus `c - 0x40`. + if (c > 0x7F) { + @branchHint(.unlikely); + log.info("executing C1 0x{x} as ESC {c}", .{ c, c - 0x40 }); + try self.escDispatch(.{ + .intermediates = &.{}, + .final = c - 0x40, + }); + return; + } + const c0: ansi.C0 = @enumFromInt(c); if (comptime debug) log.info("execute: {f}", .{c0}); switch (c0) { From 9e44c9c956564a0e09fb402ea26edcbd12ebc834 Mon Sep 17 00:00:00 2001 From: Qwerasd Date: Sun, 16 Nov 2025 12:13:36 -0700 Subject: [PATCH 6/6] fix(terminal): avoid memory corruption in `cursorScrollDown` It was previously possible for `eraseRow` to move the cursor pin to a different page, and then the call to `cursorChangePin` would try to free the cursor style from that page even though that's not the page it belongs to, which creates memory corruption in release modes and integrity violations or assertions in debug mode. As a bonus, this should actually be faster this way than the old code, since it avoids needless work that `cursorChangePin` otherwise does. --- src/terminal/Screen.zig | 30 ++++++++++++------------------ 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/src/terminal/Screen.zig b/src/terminal/Screen.zig index 73992bf88..126165a40 100644 --- a/src/terminal/Screen.zig +++ b/src/terminal/Screen.zig @@ -756,6 +756,11 @@ pub fn cursorDownScroll(self: *Screen) !void { var dirty = page.dirtyBitSet(); dirty.set(0); } else { + // The call to `eraseRow` will move the tracked cursor pin up by one + // row, but we don't actually want that, so we keep the old pin and + // put it back after calling `eraseRow`. + const old_pin = self.cursor.page_pin.*; + // eraseRow will shift everything below it up. try self.pages.eraseRow(.{ .active = .{} }); @@ -763,26 +768,15 @@ pub fn cursorDownScroll(self: *Screen) !void { // because eraseRow will mark all the rotated rows as dirty // in the entire page. - // We need to move our cursor down one because eraseRows will - // preserve our pin directly and we're erasing one row. - const page_pin = self.cursor.page_pin.down(1).?; - self.cursorChangePin(page_pin); - const page_rac = page_pin.rowAndCell(); + // We don't use `cursorChangePin` here because we aren't + // actually changing the pin, we're keeping it the same. + self.cursor.page_pin.* = old_pin; + + // We do, however, need to refresh the cached page row + // and cell, because `eraseRow` will have moved the row. + const page_rac = self.cursor.page_pin.rowAndCell(); self.cursor.page_row = page_rac.row; self.cursor.page_cell = page_rac.cell; - - // The above may clear our cursor so we need to update that - // again. If this fails (highly unlikely) we just reset - // the cursor. - self.manualStyleUpdate() catch |err| { - // This failure should not happen because manualStyleUpdate - // handles page splitting, overflow, and more. This should only - // happen if we're out of RAM. In this case, we'll just degrade - // gracefully back to the default style. - log.err("failed to update style on cursor scroll err={}", .{err}); - self.cursor.style = .{}; - self.cursor.style_id = 0; - }; } } else { const old_pin = self.cursor.page_pin.*;