Merge pull request #1620 from mitchellh/eraserows

Fix off-by-one errors in eraseRowBounded which could cause scroll region crashes

src/terminal/PageList.zig

@@ -2042,7 +2042,11 @@ pub fn eraseRow(
         //    5         5         5  |      6
         //    6         6         6  |      7
         //    7         7         7 <'      4
-        try page.data.cloneRowFrom(&next.data, &rows[page.data.size.rows - 1], &next_rows[0]);
+        try page.data.cloneRowFrom(
+            &next.data,
+            &rows[page.data.size.rows - 1],
+            &next_rows[0],
+        );
 
         page = next;
         rows = next_rows;
@@ -2101,7 +2105,9 @@ pub fn eraseRowBounded(
         var pin_it = self.tracked_pins.keyIterator();
         while (pin_it.next()) |p_ptr| {
             const p = p_ptr.*;
-            if (p.page == page and p.y > pn.y and p.y < pn.y + limit) p.y -= 1;
+            if (p.page == page and
+                p.y >= pn.y and
+                p.y <= pn.y + limit) p.y -= 1;
         }
 
         return;
@@ -2119,14 +2125,18 @@ pub fn eraseRowBounded(
         var pin_it = self.tracked_pins.keyIterator();
         while (pin_it.next()) |p_ptr| {
             const p = p_ptr.*;
-            if (p.page == page and p.y > pn.y) p.y -= 1;
+            if (p.page == page and p.y >= pn.y) p.y -= 1;
         }
     }
 
     while (page.next) |next| {
         const next_rows = next.data.rows.ptr(next.data.memory.ptr);
 
-        try page.data.cloneRowFrom(&next.data, &rows[page.data.size.rows - 1], &next_rows[0]);
+        try page.data.cloneRowFrom(
+            &next.data,
+            &rows[page.data.size.rows - 1],
+            &next_rows[0],
+        );
 
         page = next;
         rows = next_rows;
@@ -4439,6 +4449,133 @@ test "PageList erase a one-row active" {
     }
 }
 
+test "PageList eraseRowBounded less than full row" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 80, 10, null);
+    defer s.deinit();
+
+    // Pins
+    const p_top = try s.trackPin(s.pin(.{ .active = .{ .y = 5, .x = 0 } }).?);
+    defer s.untrackPin(p_top);
+    const p_bot = try s.trackPin(s.pin(.{ .active = .{ .y = 8, .x = 0 } }).?);
+    defer s.untrackPin(p_bot);
+    const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?);
+    defer s.untrackPin(p_out);
+
+    // Erase only a few rows in our active
+    try s.eraseRowBounded(.{ .active = .{ .y = 5 } }, 3);
+    try testing.expectEqual(s.rows, s.totalRows());
+
+    try testing.expectEqual(s.pages.first.?, p_top.page);
+    try testing.expectEqual(@as(usize, 4), p_top.y);
+    try testing.expectEqual(@as(usize, 0), p_top.x);
+
+    try testing.expectEqual(s.pages.first.?, p_bot.page);
+    try testing.expectEqual(@as(usize, 7), p_bot.y);
+    try testing.expectEqual(@as(usize, 0), p_bot.x);
+
+    try testing.expectEqual(s.pages.first.?, p_out.page);
+    try testing.expectEqual(@as(usize, 9), p_out.y);
+    try testing.expectEqual(@as(usize, 0), p_out.x);
+}
+
+test "PageList eraseRowBounded full rows single page" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 80, 10, null);
+    defer s.deinit();
+
+    // Pins
+    const p_in = try s.trackPin(s.pin(.{ .active = .{ .y = 7, .x = 0 } }).?);
+    defer s.untrackPin(p_in);
+    const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?);
+    defer s.untrackPin(p_out);
+
+    // Erase only a few rows in our active
+    try s.eraseRowBounded(.{ .active = .{ .y = 5 } }, 10);
+    try testing.expectEqual(s.rows, s.totalRows());
+
+    // Our pin should move to the first page
+    try testing.expectEqual(s.pages.first.?, p_in.page);
+    try testing.expectEqual(@as(usize, 6), p_in.y);
+    try testing.expectEqual(@as(usize, 0), p_in.x);
+
+    try testing.expectEqual(s.pages.first.?, p_out.page);
+    try testing.expectEqual(@as(usize, 8), p_out.y);
+    try testing.expectEqual(@as(usize, 0), p_out.x);
+}
+
+test "PageList eraseRowBounded full rows two pages" {
+    const testing = std.testing;
+    const alloc = testing.allocator;
+
+    var s = try init(alloc, 80, 10, null);
+    defer s.deinit();
+
+    // Grow to two pages so our active area straddles
+    {
+        const page = &s.pages.last.?.data;
+        for (0..page.capacity.rows - page.size.rows) |_| _ = try s.grow();
+        try s.growRows(5);
+        try testing.expectEqual(@as(usize, 2), s.totalPages());
+        try testing.expectEqual(@as(usize, 5), s.pages.last.?.data.size.rows);
+    }
+
+    // Pins
+    const p_first = try s.trackPin(s.pin(.{ .active = .{ .y = 4, .x = 0 } }).?);
+    defer s.untrackPin(p_first);
+    const p_first_out = try s.trackPin(s.pin(.{ .active = .{ .y = 3, .x = 0 } }).?);
+    defer s.untrackPin(p_first_out);
+    const p_in = try s.trackPin(s.pin(.{ .active = .{ .y = 8, .x = 0 } }).?);
+    defer s.untrackPin(p_in);
+    const p_out = try s.trackPin(s.pin(.{ .active = .{ .y = 9, .x = 0 } }).?);
+    defer s.untrackPin(p_out);
+
+    {
+        try testing.expectEqual(s.pages.last.?.prev.?, p_first.page);
+        try testing.expectEqual(@as(usize, p_first.page.data.size.rows - 1), p_first.y);
+        try testing.expectEqual(@as(usize, 0), p_first.x);
+
+        try testing.expectEqual(s.pages.last.?.prev.?, p_first_out.page);
+        try testing.expectEqual(@as(usize, p_first_out.page.data.size.rows - 2), p_first_out.y);
+        try testing.expectEqual(@as(usize, 0), p_first_out.x);
+
+        try testing.expectEqual(s.pages.last.?, p_in.page);
+        try testing.expectEqual(@as(usize, 3), p_in.y);
+        try testing.expectEqual(@as(usize, 0), p_in.x);
+
+        try testing.expectEqual(s.pages.last.?, p_out.page);
+        try testing.expectEqual(@as(usize, 4), p_out.y);
+        try testing.expectEqual(@as(usize, 0), p_out.x);
+    }
+
+    // Erase only a few rows in our active
+    try s.eraseRowBounded(.{ .active = .{ .y = 4 } }, 4);
+
+    // In page in first page is shifted
+    try testing.expectEqual(s.pages.last.?.prev.?, p_first.page);
+    try testing.expectEqual(@as(usize, p_first.page.data.size.rows - 2), p_first.y);
+    try testing.expectEqual(@as(usize, 0), p_first.x);
+
+    // Out page in first page should not be shifted
+    try testing.expectEqual(s.pages.last.?.prev.?, p_first_out.page);
+    try testing.expectEqual(@as(usize, p_first_out.page.data.size.rows - 2), p_first_out.y);
+    try testing.expectEqual(@as(usize, 0), p_first_out.x);
+
+    // In page is shifted
+    try testing.expectEqual(s.pages.last.?, p_in.page);
+    try testing.expectEqual(@as(usize, 2), p_in.y);
+    try testing.expectEqual(@as(usize, 0), p_in.x);
+
+    // Out page is not shifted
+    try testing.expectEqual(s.pages.last.?, p_out.page);
+    try testing.expectEqual(@as(usize, 4), p_out.y);
+    try testing.expectEqual(@as(usize, 0), p_out.x);
+}
+
 test "PageList clone" {
     const testing = std.testing;
     const alloc = testing.allocator;

src/terminal/Screen.zig

@@ -182,6 +182,17 @@ pub fn assertIntegrity(self: *const Screen) void {
         assert(self.cursor.x < self.pages.cols);
         assert(self.cursor.y < self.pages.rows);
 
+        // Our cursor x/y should always match the pin. If this doesn't
+        // match then it indicates that the tracked pin moved and we didn't
+        // account for it by either calling cursorReload or manually
+        // adjusting.
+        const pt: point.Point = self.pages.pointFromPin(
+            .active,
+            self.cursor.page_pin.*,
+        ) orelse unreachable;
+        assert(self.cursor.x == pt.active.x);
+        assert(self.cursor.y == pt.active.y);
+
         if (self.cursor.style_id == style.default_id) {
             // If our style is default, we should have no refs.
             assert(self.cursor.style.default());
@@ -407,12 +418,12 @@ pub fn cursorUp(self: *Screen, n: size.CellCountInt) void {
     assert(self.cursor.y >= n);
     defer self.assertIntegrity();
 
+    self.cursor.y -= n; // Must be set before cursorChangePin
     const page_pin = self.cursor.page_pin.up(n).?;
     self.cursorChangePin(page_pin);
     const page_rac = page_pin.rowAndCell();
     self.cursor.page_row = page_rac.row;
     self.cursor.page_cell = page_rac.cell;
-    self.cursor.y -= n;
 }
 
 pub fn cursorRowUp(self: *Screen, n: size.CellCountInt) *pagepkg.Row {
@@ -431,6 +442,8 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void {
     assert(self.cursor.y + n < self.pages.rows);
     defer self.assertIntegrity();
 
+    self.cursor.y += n; // Must be set before cursorChangePin
+
     // We move the offset into our page list to the next row and then
     // get the pointers to the row/cell and set all the cursor state up.
     const page_pin = self.cursor.page_pin.down(n).?;
@@ -438,9 +451,6 @@ pub fn cursorDown(self: *Screen, n: size.CellCountInt) void {
     const page_rac = page_pin.rowAndCell();
     self.cursor.page_row = page_rac.row;
     self.cursor.page_cell = page_rac.cell;
-
-    // Y of course increases
-    self.cursor.y += n;
 }
 
 /// Move the cursor to some absolute horizontal position.
@@ -467,12 +477,12 @@ pub fn cursorAbsolute(self: *Screen, x: size.CellCountInt, y: size.CellCountInt)
     else
         self.cursor.page_pin.*;
     page_pin.x = x;
+    self.cursor.x = x; // Must be set before cursorChangePin
+    self.cursor.y = y;
     self.cursorChangePin(page_pin);
     const page_rac = page_pin.rowAndCell();
     self.cursor.page_row = page_rac.row;
     self.cursor.page_cell = page_rac.cell;
-    self.cursor.x = x;
-    self.cursor.y = y;
 }
 
 /// Reloads the cursor pointer information into the screen. This is expensive
@@ -545,6 +555,14 @@ pub fn cursorDownScroll(self: *Screen) !void {
             // eraseRow will shift everything below it up.
             try self.pages.eraseRow(.{ .active = .{} });
 
+            // 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();
+            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.
@@ -558,14 +576,6 @@ pub fn cursorDownScroll(self: *Screen) !void {
                 self.cursor.style_id = 0;
                 self.cursor.style_ref = null;
             };
-
-            // 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();
-            self.cursor.page_row = page_rac.row;
-            self.cursor.page_cell = page_rac.cell;
         }
     } else {
         const old_pin = self.cursor.page_pin.*;

src/terminal/Terminal.zig

@@ -1095,19 +1095,25 @@ pub fn index(self: *Terminal) !void {
             // Otherwise use a fast path function from PageList to efficiently
             // scroll the contents of the scrolling region.
 
-            // eraseRow and eraseRowBounded will end up moving the cursor pin
-            // up by 1, so we save its current position and restore it after.
-            const cursor_x = self.screen.cursor.x;
-            const cursor_y = self.screen.cursor.y;
-            defer {
-                self.screen.cursorAbsolute(cursor_x, cursor_y);
-            }
+            // Preserve old cursor just for assertions
+            const old_cursor = self.screen.cursor;
 
             try self.screen.pages.eraseRowBounded(
                 .{ .active = .{ .y = self.scrolling_region.top } },
-                self.scrolling_region.bottom - self.scrolling_region.top
+                self.scrolling_region.bottom - self.scrolling_region.top,
             );
 
+            // eraseRow and eraseRowBounded will end up moving the cursor pin
+            // up by 1, so we need to move it back down. A `cursorReload`
+            // would be better option but this is more efficient and this is
+            // a super hot path so we do this instead.
+            if (comptime std.debug.runtime_safety) {
+                assert(self.screen.cursor.x == old_cursor.x);
+                assert(self.screen.cursor.y == old_cursor.y);
+            }
+            self.screen.cursor.y -= 1;
+            self.screen.cursorDown(1);
+
             // The operations above can prune our cursor style so we need to
             // update. This should never fail because the above can only FREE
             // memory.