Merge pull request #370 from mitchellh/random-crash

Multiple fixes to bugs found while reading random bytes

src/font/Group.zig

@@ -303,7 +303,8 @@ fn indexForCodepointExact(self: Group, cp: u32, style: Style, p: ?Presentation)
 /// necessarily force the font to load.
 pub fn hasCodepoint(self: *Group, index: FontIndex, cp: u32, p: ?Presentation) bool {
     const list = self.faces.getPtr(index.style);
-    const item = list.items[@intCast(index.idx)];
+    if (index.idx >= list.items.len) return false;
+    const item = list.items[index.idx];
     return switch (item) {
         .deferred => |v| v.hasCodepoint(cp, p),
         .loaded => |face| loaded: {
@@ -330,7 +331,7 @@ pub fn presentationFromIndex(self: *Group, index: FontIndex) !font.Presentation
 pub fn faceFromIndex(self: *Group, index: FontIndex) !*Face {
     if (index.special() != null) return error.SpecialHasNoFace;
     const list = self.faces.getPtr(index.style);
-    const item = &list.items[@intCast(index.idx)];
+    const item = &list.items[index.idx];
     return switch (item.*) {
         .deferred => |*d| deferred: {
             const face = try d.load(self.lib, self.size);
@@ -553,6 +554,30 @@ test {
     }
 }
 
+test "face count limit" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+    const testFont = @import("test.zig").fontRegular;
+
+    var atlas_greyscale = try font.Atlas.init(alloc, 512, .greyscale);
+    defer atlas_greyscale.deinit(alloc);
+
+    var lib = try Library.init();
+    defer lib.deinit();
+
+    var group = try init(alloc, lib, .{ .points = 12 });
+    defer group.deinit();
+
+    for (0..FontIndex.Special.start - 1) |_| {
+        try group.addFace(.regular, .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) });
+    }
+
+    try testing.expectError(error.GroupFull, group.addFace(
+        .regular,
+        .{ .loaded = try Face.init(lib, testFont, .{ .points = 12 }) },
+    ));
+}
+
 test "box glyph" {
     const testing = std.testing;
     const alloc = testing.allocator;

src/terminal/Screen.zig

@@ -412,6 +412,8 @@ pub const Row = struct {
 
     /// Attach a grapheme codepoint to the given cell.
     pub fn attachGrapheme(self: Row, x: usize, cp: u21) !void {
+        assert(x < self.storage.len - 1);
+
         const cell = &self.storage[x + 1].cell;
         const key = self.getId() + x + 1;
         const gop = try self.screen.graphemes.getOrPut(self.screen.alloc, key);
@@ -448,6 +450,8 @@ pub const Row = struct {
 
     /// Removes all graphemes associated with a cell.
     pub fn clearGraphemes(self: Row, x: usize) void {
+        assert(x < self.storage.len - 1);
+
         // Our row is now dirty
         self.storage[0].header.flags.dirty = true;
 
@@ -1071,12 +1075,12 @@ pub fn copyRow(self: *Screen, dst: RowIndex, src: RowIndex) !void {
 /// the top/bottom are within it).
 ///
 /// This can be used to implement terminal scroll regions efficiently.
-pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count: usize) void {
+pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count_req: usize) void {
     const tracy = trace(@src());
     defer tracy.end();
 
     // Avoid a lot of work if we're doing nothing.
-    if (count == 0) return;
+    if (count_req == 0) return;
 
     // Convert our top/bottom to screen y values. This is the y offset
     // in the entire screen buffer.
@@ -1088,7 +1092,7 @@ pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count: usi
 
     // We can only scroll up to the number of rows in the region. The "+ 1"
     // is because our y values are 0-based and count is 1-based.
-    assert(count <= (bot_y - top_y + 1));
+    const count = @min(count_req, bot_y - top_y + 1);
 
     // Get the storage pointer for the full scroll region. We're going to
     // be modifying the whole thing so we get it right away.
@@ -4014,6 +4018,22 @@ test "Screen: scrollRegionUp multiple count" {
     }
 }
 
+test "Screen: scrollRegionUp count greater than available lines" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 4, 5, 0);
+    defer s.deinit();
+    try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n4ABCD");
+
+    s.scrollRegionUp(.{ .active = 1 }, .{ .active = 2 }, 10);
+    {
+        // Test our contents rotated
+        var contents = try s.testString(alloc, .screen);
+        defer alloc.free(contents);
+        try testing.expectEqualStrings("1ABCD\n\n\n4ABCD", contents);
+    }
+}
 test "Screen: scrollRegionUp fills with pen" {
     const testing = std.testing;
     const alloc = testing.allocator;

src/terminal/Terminal.zig

@@ -990,7 +990,11 @@ pub fn setCursorColAbsolute(self: *Terminal, col_req: usize) void {
 
     // TODO: test
 
-    assert(!self.modes.get(.origin)); // TODO
+    // TODO
+    if (!self.modes.get(.origin)) {
+        log.err("setCursorColAbsolute: cursor origin mode handling not implemented yet", .{});
+        return;
+    }
 
     if (self.status_display != .main) return; // TODO
 
@@ -1116,7 +1120,6 @@ pub fn eraseLine(
 
         else => {
             log.err("unimplemented erase line mode: {}", .{mode});
-            @panic("unimplemented");
         },
     }
 }
@@ -1129,25 +1132,27 @@ pub fn eraseLine(
 /// scrolling region, it is adjusted down.
 ///
 /// Does not change the cursor position.
-///
+pub fn deleteChars(self: *Terminal, count_req: usize) !void {
-/// TODO: test
-pub fn deleteChars(self: *Terminal, count: usize) !void {
     const tracy = trace(@src());
     defer tracy.end();
 
+    // Count defaults to 1 and we can't delete more than we have remaining
+    // in the row.
+    const count = @min(self.cols - self.screen.cursor.x, count_req);
+    if (count == 0) return;
+
     const line = self.screen.getRow(.{ .active = self.screen.cursor.y });
+    for (0..count) |i| {
+        const x = self.screen.cursor.x + i;
+        const copy_x = x + count;
+        if (copy_x >= self.cols) {
+            line.getCellPtr(x).* = self.screen.cursor.pen;
+            continue;
+        }
 
-    // Our last index is at most the end of the number of chars we have
+        const copy_cell = line.getCellPtr(copy_x);
-    // in the current line.
+        line.getCellPtr(x).* = copy_cell.*;
-    const end = self.cols - count;
+        copy_cell.char = 0;
-
-    // Shift
-    var i: usize = self.screen.cursor.x;
-    while (i < end) : (i += 1) {
-        const j = i + count;
-        const j_cell = line.getCellPtr(j);
-        line.getCellPtr(i).* = j_cell.*;
-        j_cell.char = 0;
     }
 }
 
@@ -1420,6 +1425,13 @@ pub fn deleteLines(self: *Terminal, count: usize) !void {
     const tracy = trace(@src());
     defer tracy.end();
 
+    // If our cursor is outside of the scroll region, do nothing.
+    if (self.screen.cursor.y < self.scrolling_region.top or
+        self.screen.cursor.y > self.scrolling_region.bottom)
+    {
+        return;
+    }
+
     // Move the cursor to the left margin
     self.screen.cursor.x = 0;
 
@@ -2127,6 +2139,34 @@ test "Terminal: deleteLines with scroll region, large count" {
     }
 }
 
+test "Terminal: deleteLines with scroll region, cursor outside of region" {
+    const alloc = testing.allocator;
+    var t = try init(alloc, 80, 80);
+    defer t.deinit(alloc);
+
+    // Initial value
+    try t.print('A');
+    t.carriageReturn();
+    try t.linefeed();
+    try t.print('B');
+    t.carriageReturn();
+    try t.linefeed();
+    try t.print('C');
+    t.carriageReturn();
+    try t.linefeed();
+    try t.print('D');
+
+    t.setScrollingRegion(1, 3);
+    t.setCursorPos(4, 1);
+    try t.deleteLines(1);
+
+    {
+        var str = try t.plainString(testing.allocator);
+        defer testing.allocator.free(str);
+        try testing.expectEqualStrings("A\nB\nC\nD", str);
+    }
+}
+
 test "Terminal: insertLines" {
     const alloc = testing.allocator;
     var t = try init(alloc, 2, 5);
@@ -2611,6 +2651,70 @@ test "Terminal: print wide char with 1-column width" {
     try t.print('😀'); // 0x1F600
 }
 
+test "Terminal: deleteChars" {
+    const alloc = testing.allocator;
+    var t = try init(alloc, 5, 5);
+    defer t.deinit(alloc);
+
+    for ("ABCDE") |c| try t.print(c);
+    t.setCursorPos(1, 2);
+
+    try t.deleteChars(2);
+    {
+        var str = try t.plainString(testing.allocator);
+        defer testing.allocator.free(str);
+        try testing.expectEqualStrings("ADE", str);
+    }
+}
+
+test "Terminal: deleteChars zero count" {
+    const alloc = testing.allocator;
+    var t = try init(alloc, 5, 5);
+    defer t.deinit(alloc);
+
+    for ("ABCDE") |c| try t.print(c);
+    t.setCursorPos(1, 2);
+
+    try t.deleteChars(0);
+    {
+        var str = try t.plainString(testing.allocator);
+        defer testing.allocator.free(str);
+        try testing.expectEqualStrings("ABCDE", str);
+    }
+}
+
+test "Terminal: deleteChars more than half" {
+    const alloc = testing.allocator;
+    var t = try init(alloc, 5, 5);
+    defer t.deinit(alloc);
+
+    for ("ABCDE") |c| try t.print(c);
+    t.setCursorPos(1, 2);
+
+    try t.deleteChars(3);
+    {
+        var str = try t.plainString(testing.allocator);
+        defer testing.allocator.free(str);
+        try testing.expectEqualStrings("AE", str);
+    }
+}
+
+test "Terminal: deleteChars more than line width" {
+    const alloc = testing.allocator;
+    var t = try init(alloc, 5, 5);
+    defer t.deinit(alloc);
+
+    for ("ABCDE") |c| try t.print(c);
+    t.setCursorPos(1, 2);
+
+    try t.deleteChars(10);
+    {
+        var str = try t.plainString(testing.allocator);
+        defer testing.allocator.free(str);
+        try testing.expectEqualStrings("A", str);
+    }
+}
+
 // https://github.com/mitchellh/ghostty/issues/272
 // This is also tested in depth in screen resize tests but I want to keep
 // this test around to ensure we don't regress at multiple layers.

src/terminal/charsets.zig

@@ -85,14 +85,15 @@ const dec_special = tech: {
     break :tech table;
 };
 
-const max_u8 = std.math.maxInt(u8);
+/// Our table length is 256 so we can contain all ASCII chars.
+const table_len = std.math.maxInt(u8) + 1;
 
 /// Creates a table that maps ASCII to ASCII as a getting started point.
-fn initTable() [max_u8]u16 {
+fn initTable() [table_len]u16 {
-    var result: [max_u8]u16 = undefined;
+    var result: [table_len]u16 = undefined;
     var i: usize = 0;
-    while (i < max_u8) : (i += 1) result[i] = @intCast(i);
+    while (i < table_len) : (i += 1) result[i] = @intCast(i);
-    assert(i == max_u8);
+    assert(i == table_len);
     return result;
 }
 
@@ -105,9 +106,9 @@ test {
 
         const table = @field(Charset, field.name).table();
 
-        // Yes, I could use `max_u8` here, but I want to explicitly use a
+        // Yes, I could use `table_len` here, but I want to explicitly use a
         // hardcoded constant so that if there are miscompilations or a comptime
         // issue, we catch it.
-        try testing.expectEqual(@as(usize, 255), table.len);
+        try testing.expectEqual(@as(usize, 256), table.len);
     }
 }

src/termio/Exec.zig

@@ -1216,8 +1216,7 @@ const StreamHandler = struct {
     pub fn setCursorRow(self: *StreamHandler, row: u16) !void {
         if (self.terminal.modes.get(.origin)) {
             // TODO
-            log.err("setCursorRow: implement origin mode", .{});
+            log.err("setCursorRow: unimplemented origin mode handling, misrendering may occur", .{});
-            unreachable;
         }
 
         self.terminal.setCursorPos(row, self.terminal.screen.cursor.x + 1);