mirror of
https://github.com/ghostty-org/ghostty.git
synced 2025-07-14 15:56:13 +03:00
terminal: scroll region scroll up copied the wrong length of data
Fixes #315 This function has various cases that can be hit depending on the state of the underlying circular buffer. It is a very well tested function but this particular branch wasn't tested and unsurprisingly turns out there is a bug in it. Consider the following circular buffer state representing a terminal screen with 1 column, 5 rows. Assume the circular buffer representing this screen is such that `head == tail` and `head = 4` (zero-indexed, full buffer size is 5). The head and tail are shown with an arrow below. ┌───────────────────────────────────────────────────────────────┐ │ B │ ├───────────────────────────────────────────────────────────────┤ │ C │ ├───────────────────────────────────────────────────────────────┤ │ D │ ├───────────────────────────────────────────────────────────────┤ │ E │ ├───────────────────────────────────────────────────────────────┤ ◀─── Head │ A │ Tail └───────────────────────────────────────────────────────────────┘ The screen contents are "A B C D E" with each character on a new line. Next, we set a scroll region from y=0 to y=3 (len=4). The scroll region contents are "A B C D". Next, we issue a "delete lines" command with n=2 (`CSI 2 M`) while the cursor is at the top-left corner. The delete lines command deletes the given number of lines (n=2 in this case) and shifts the remaining lines up. It does this only within the context of the scroll region. Therefore, for `CSI 2 M` we would expect our screen to become "C D _ _ E" (A, B are deleted, C, D shift up, and E is untouched because its outside of the scroll region). When executing this operation, we request the memory regions containing the scroll region. This results in two pointers and lengths. For our circular buffer state, we get the following: ┌───────────────────────────────────────────────────────────────┐ ◀──── ptr0 │ A │ └───────────────────────────────────────────────────────────────┘ ┌───────────────────────────────────────────────────────────────┐ ◀──── ptr1 │ B │ ├───────────────────────────────────────────────────────────────┤ │ C │ ├───────────────────────────────────────────────────────────────┤ │ D │ └───────────────────────────────────────────────────────────────┘ We get two pointers because our circular buffer wraps around the bottom. The diagram above shows them in top/bottom order not in memory order (The value of `ptr0 > ptr1` but it doesn't matter for the bug). The way the math works is as follows: 1. We calculate the number of lines we need to shift up. That value is `height - n`. Our height is 4 (scroll region height) and our n is 2 (`CSI 2 M`), so we know we're shifting up 2 lines. Let's call this `shift_lines`. 2. Our start copy offset is `n` because the lines we are retaining are exactly after the `n` we're deleting (i.e. we're deleting 2 lines so the start of the lines we're shifting up is the 3rd line). Let's call this `start_offset`. 3. We realize that our start offset is greater than the size of ptr0, so we must be copy from ptr1 into ptr0. Further, we know our start offset into ptr1 must be `start_offset - ptr0.len`. Let's call this `start_offset_ptr1 = 1`. 4. Copy `ptr1[start_offset_ptr1]` to `ptr0`. We copy up to `shift_lines` amount. `shift_lines` is 2 but `ptr0.len` is only `1`. So, we actually copy `@min(shift_lines, ptr0.len)` and have `1` line remaining. Let's call that `remaining = 1`. 5. Copy `remaining` from `ptr1[ptr0.len]` to `ptr1`. 6. Next we need to zero our remaining lines. Our slices only contain our scroll region so we know we can zero the memory from `ptr1[remaining]` to the end of `ptr1`. We know this because step 5 only copied `remaining` bytes. The end result looks like this: ┌───────────────────────────────────────────────────────────────┐ ◀──── ptr[0] │ C │ └───────────────────────────────────────────────────────────────┘ ┌───────────────────────────────────────────────────────────────┐ ◀──── ptr[1] │ D │ ├───────────────────────────────────────────────────────────────┤ │ │ ├───────────────────────────────────────────────────────────────┤ │ │ └───────────────────────────────────────────────────────────────┘ The bug was in step 6. We were incorrectly zeroing from `start_offset_ptr1` instead of `remaining`. This was just a simple typo. The results are devastating, but only under the exactly correct circumstances (those in this commit message). In that scenario, the bug produced the following: ┌───────────────────────────────────────────────────────────────┐ ◀────────── ptr[0] │ C │ └───────────────────────────────────────────────────────────────┘ ┌───────────────────────────────────────────────────────────────┐ ◀────────── ptr[1] │ D │ ├───────────────────────────────────────────────────────────────┤ │ C │ ├───────────────────────────────────────────────────────────────┤ │ │ └───────────────────────────────────────────────────────────────┘ Notice the incorrect "C" that remains and is not zeroed. This example showed a scenario with 1 column and leaving only 1 line out of the scroll region. The real bug in #315 would often mistakingly leave multiple lines in the scroll region. The effect was that scrolling produced garbage lines because you'd "scroll" and part of what should've scrolled would remain. This garbage state was only in the terminal screen state, so it didn't impact actual programs running or their data (i.e. vim). But, it made the program unusable.
This commit is contained in:
@ -1248,8 +1248,9 @@ pub fn scrollRegionUp(self: *Screen, top: RowIndex, bottom: RowIndex, count_req:
|
||||
const dst2 = slices[1];
|
||||
const src2_offset = bot_src_offset + src_len;
|
||||
const src2 = slices[1][src2_offset..];
|
||||
fastmem.move(StorageCell, dst2, src2);
|
||||
break :zero_offset .{ slices[0].len, src2_offset };
|
||||
const src2_len = remaining;
|
||||
fastmem.move(StorageCell, dst2, src2[0..src2_len]);
|
||||
break :zero_offset .{ src_len, src2_len };
|
||||
};
|
||||
|
||||
// Zero
|
||||
@ -4209,6 +4210,44 @@ test "Screen: scrollRegionUp buffer wrap alternate" {
|
||||
}
|
||||
}
|
||||
|
||||
test "Screen: scrollRegionUp buffer wrap alternative with extra lines" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
||||
var s = try init(alloc, 5, 5, 0);
|
||||
defer s.deinit();
|
||||
|
||||
// We artificially mess with the circular buffer here. This was discovered
|
||||
// when debugging https://github.com/mitchellh/ghostty/issues/315. I
|
||||
// don't know how to "naturally" get the circular buffer into this state
|
||||
// although it is obviously possible, verified through various
|
||||
// asciinema casts.
|
||||
//
|
||||
// I think the proper way to recreate this state would be to fill
|
||||
// the screen, scroll the correct number of times, clear the screen
|
||||
// with a fill. I can try that later to ensure we're hitting the same
|
||||
// code path.
|
||||
s.storage.head = 24;
|
||||
s.storage.tail = 24;
|
||||
s.storage.full = true;
|
||||
|
||||
// Scroll down, should still be bottom, but should wrap because
|
||||
// we're out of space.
|
||||
// try s.scroll(.{ .screen = 2 });
|
||||
// s.cursor.x = 0;
|
||||
try s.testWriteString("1ABCD\n2EFGH\n3IJKL\n4ABCD\n5EFGH");
|
||||
|
||||
// Scroll
|
||||
s.scrollRegionUp(.{ .screen = 0 }, .{ .screen = 3 }, 2);
|
||||
|
||||
{
|
||||
// Test our contents rotated
|
||||
var contents = try s.testString(alloc, .screen);
|
||||
defer alloc.free(contents);
|
||||
try testing.expectEqualStrings("3IJKL\n4ABCD\n\n\n5EFGH", contents);
|
||||
}
|
||||
}
|
||||
|
||||
test "Screen: clear history with no history" {
|
||||
const testing = std.testing;
|
||||
const alloc = testing.allocator;
|
||||
|
Reference in New Issue
Block a user