Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(text): fix out of bounds access triggered by zero-width graphemes #1001

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(text): fix out of bounds access triggered by zero-width graphemes #1001

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/text/span.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ impl WidgetRef for Span<'_> {
for g in self.styled_graphemes(Style::default()) {
let symbol_width = g.symbol.width();
let next_x = current_x.saturating_add(symbol_width as u16);
if next_x > max_x {
if next_x > max_x || current_x >= max_x {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

next_x is the same or bigger than current_x so it’s somewhat pointless to check that the smaller one is ≥ while the bigger is only >.

I am not sure where the panic is happening but I assume it’s in the for loop over the range? Can that be written more clearly then which avoids the pre-check? Like when not possible to create the range, then skip it completely, otherwise loop over it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The panic occurs on the statement immediately after the break. buf.get_mut(current_x, y). In particular, current_x == max_x and is out of bounds.

I don't think either check is pointless, suppose we have the current check next_x > max_x, then next_x = current_x = max_x = 1 does not break the loop (and panics).
If we keep only current_x >= max_x, then next_x = 100 && current_x = max_x = 1 does not break and that panics further down in the range loop.

Let me know if you have a better idea to fix this!

break;
}
buf.get_mut(current_x, y)
Expand Down
10 changes: 9 additions & 1 deletion src/text/text.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,11 +871,19 @@
assert_buffer_eq!(buf, expected_buf);
}

#[test]
fn render_span_with_zero_width_graphemes() {
let area = Rect::new(0, 0, 1, 1);
let mut buffer = Buffer::empty(area);
Line::from("a\0b").render(area, &mut buffer);
assert_buffer_eq!(buffer, Buffer::with_lines(["a"]));

Check warning on line 879 in src/text/text.rs

View check run for this annotation

Codecov / codecov/patch

src/text/text.rs#L879 

Added line #L879 was not covered by tests
}

#[rstest]
fn render_out_of_bounds(mut small_buf: Buffer) {
let out_of_bounds_area = Rect::new(20, 20, 10, 1);
Text::from("Hello, world!").render(out_of_bounds_area, &mut small_buf);
assert_eq!(small_buf, Buffer::empty(small_buf.area));
assert_buffer_eq!(small_buf, Buffer::empty(small_buf.area));

Check warning on line 886 in src/text/text.rs

View check run for this annotation

Codecov / codecov/patch

src/text/text.rs#L886 

Added line #L886 was not covered by tests
}

#[test]
Expand Down