From f15ef63edbbc5e4e685b018311e2f6424db0b0c9 Mon Sep 17 00:00:00 2001 From: Christian Duerr Date: Thu, 6 Jun 2019 13:04:12 +0000 Subject: [PATCH] Fix index out of bounds during selection This reworks the selection logic to prevent any possible index out of bounds exceptions by clamping the start and end points before doing anything else with them when converting selections to spans. This also fixes a bug where semantic selections would not automatically expand across double-width characters. Fixes #2486. --- alacritty_terminal/src/selection.rs | 174 +++++++++++----------------- alacritty_terminal/src/term/mod.rs | 13 ++- 2 files changed, 77 insertions(+), 110 deletions(-) diff --git a/alacritty_terminal/src/selection.rs b/alacritty_terminal/src/selection.rs index ad2f2ba9..7dd0be74 100644 --- a/alacritty_terminal/src/selection.rs +++ b/alacritty_terminal/src/selection.rs @@ -18,7 +18,6 @@ //! finalized when the button is released. The selection should be cleared //! when text is added/removed/scrolled on the screen. The selection should //! also be cleared if the user clicks off of the selection. -use std::cmp::{max, min}; use std::ops::Range; use crate::index::{Column, Point, Side}; @@ -130,28 +129,48 @@ impl Selection { } } - pub fn to_span(&self, term: &Term, alt_screen: bool) -> Option { + pub fn to_span(&self, term: &Term) -> Option { + // Get both sides of the selection + let (mut start, mut end) = match *self { + Selection::Simple { ref region } => (region.start.point, region.end.point), + Selection::Semantic { ref region } | Selection::Lines { ref region, .. } => { + (region.start, region.end) + }, + }; + + // Order the start/end + let needs_swap = Selection::points_need_swap(start, end); + if needs_swap { + std::mem::swap(&mut start, &mut end); + } + + // Clamp to visible region in grid/normal + let cols = term.dimensions().col; + let lines = term.dimensions().line.0 as isize; + let (start, end) = Selection::grid_clamp(start, end, lines, cols)?; + let span = match *self { - Selection::Simple { ref region } => Selection::span_simple(term, region, alt_screen), - Selection::Semantic { ref region } => { - Selection::span_semantic(term, region, alt_screen) + Selection::Simple { ref region } if needs_swap => { + Selection::span_simple(term, start, end, region.end.side, region.start.side) }, - Selection::Lines { ref region, initial_line } => { - Selection::span_lines(term, region, initial_line, alt_screen) + Selection::Simple { ref region } => { + Selection::span_simple(term, start, end, region.start.side, region.end.side) }, + Selection::Semantic { .. } => Selection::span_semantic(term, start, end), + Selection::Lines { .. } => Selection::span_lines(term, start, end), }; // Expand selection across double-width cells span.map(|mut span| { let grid = term.grid(); - if span.end.col < grid.num_cols() + if span.end.col < cols && grid[span.end.line][span.end.col].flags.contains(Flags::WIDE_CHAR_SPACER) { span.end.col = Column(span.end.col.saturating_sub(1)); } - if span.start.col.0 < grid.num_cols().saturating_sub(1) + if span.start.col.0 < cols.saturating_sub(1) && grid[span.start.line][span.start.col].flags.contains(Flags::WIDE_CHAR) { span.start.col += 1; @@ -170,99 +189,43 @@ impl Selection { } } - fn span_semantic(grid: &G, region: &Range>, alt_screen: bool) -> Option + fn span_semantic(term: &T, start: Point, end: Point) -> Option where - G: Search + Dimensions, + T: Search + Dimensions, { - let cols = grid.dimensions().col; - let lines = grid.dimensions().line.0 as isize; - - // Normalize ordering of selected cells - let (mut start, mut end) = if region.start < region.end { - (region.start, region.end) - } else { - (region.end, region.start) - }; - - if alt_screen { - Selection::alt_screen_clamp(&mut start, &mut end, lines, cols)?; - } - - let (mut start, mut end) = if start == end { - if let Some(end) = grid.bracket_search(start.into()) { + let (start, end) = if start == end { + if let Some(end) = term.bracket_search(start.into()) { (start.into(), end) } else { - (grid.semantic_search_right(start.into()), grid.semantic_search_left(end.into())) + (term.semantic_search_right(start.into()), term.semantic_search_left(end.into())) } - } else if start < end && start.line == end.line { - (grid.semantic_search_left(start.into()), grid.semantic_search_right(end.into())) } else { - (grid.semantic_search_right(start.into()), grid.semantic_search_left(end.into())) + (term.semantic_search_right(start.into()), term.semantic_search_left(end.into())) }; - if start > end { - std::mem::swap(&mut start, &mut end); - } - Some(Span { start, end }) } - fn span_lines( - grid: &G, - region: &Range>, - initial_line: isize, - alt_screen: bool, - ) -> Option + fn span_lines(term: &T, mut start: Point, mut end: Point) -> Option where - G: Dimensions, + T: Dimensions, { - let cols = grid.dimensions().col; - let lines = grid.dimensions().line.0 as isize; - - // First, create start and end points based on initial line and the grid - // dimensions. - let mut start = Point { col: cols - 1, line: initial_line }; - let mut end = Point { col: Column(0), line: initial_line }; - - // Now, expand lines based on where cursor started and ended. - if region.start.line < region.end.line { - // Start is below end - start.line = min(start.line, region.start.line); - end.line = max(end.line, region.end.line); - } else { - // Start is above end - start.line = min(start.line, region.end.line); - end.line = max(end.line, region.start.line); - } - - if alt_screen { - Selection::alt_screen_clamp(&mut start, &mut end, lines, cols)?; - } + start.col = term.dimensions().col - 1; + end.col = Column(0); Some(Span { start: start.into(), end: end.into() }) } - fn span_simple(grid: &G, region: &Range, alt_screen: bool) -> Option + fn span_simple( + term: &T, + mut start: Point, + mut end: Point, + start_side: Side, + end_side: Side, + ) -> Option where - G: Dimensions, + T: Dimensions, { - let start = region.start.point; - let start_side = region.start.side; - let end = region.end.point; - let end_side = region.end.side; - let cols = grid.dimensions().col; - let lines = grid.dimensions().line.0 as isize; - - // Make sure start is always the "bottom" and end is always the "top" - let (mut start, mut end, start_side, end_side) = - if start.line > end.line || start.line == end.line && start.col <= end.col { - // Selected upward; start/end are swapped - (end, start, end_side, start_side) - } else { - // Selected downward; no swapping - (start, end, start_side, end_side) - }; - // No selection for single cell with identical sides or two cell with right+left sides if (start == end && start_side == end_side) || (end_side == Side::Right @@ -277,7 +240,7 @@ impl Selection { if start_side == Side::Left && start != end { // Special case when selection starts to left of first cell if start.col == Column(0) { - start.col = cols - 1; + start.col = term.dimensions().col - 1; start.line += 1; } else { start.col -= 1; @@ -289,21 +252,22 @@ impl Selection { end.col += 1; } - if alt_screen { - Selection::alt_screen_clamp(&mut start, &mut end, lines, cols)?; - } - // Return the selection with all cells inclusive Some(Span { start: start.into(), end: end.into() }) } - // Clamp selection in the alternate screen to the visible region - fn alt_screen_clamp( - start: &mut Point, - end: &mut Point, + // Bring start and end points in the correct order + fn points_need_swap(start: Point, end: Point) -> bool { + start.line > end.line || start.line == end.line && start.col <= end.col + } + + // Clamp selection inside the grid to prevent out of bounds errors + fn grid_clamp( + mut start: Point, + mut end: Point, lines: isize, cols: Column, - ) -> Option<()> { + ) -> Option<(Point, Point)> { if end.line >= lines { // Don't show selection above visible region if start.line >= lines { @@ -326,7 +290,7 @@ impl Selection { start.col = cols - 1; } - Some(()) + Some((start, end)) } } @@ -384,7 +348,7 @@ mod test { let mut selection = Selection::simple(location, Side::Left); selection.update(location, Side::Right); - assert_eq!(selection.to_span(&term(1, 1), false).unwrap(), Span { + assert_eq!(selection.to_span(&term(1, 1)).unwrap(), Span { start: location, end: location }); @@ -401,7 +365,7 @@ mod test { let mut selection = Selection::simple(location, Side::Right); selection.update(location, Side::Left); - assert_eq!(selection.to_span(&term(1, 1), false).unwrap(), Span { + assert_eq!(selection.to_span(&term(1, 1)).unwrap(), Span { start: location, end: location }); @@ -417,7 +381,7 @@ mod test { let mut selection = Selection::simple(Point::new(0, Column(0)), Side::Right); selection.update(Point::new(0, Column(1)), Side::Left); - assert_eq!(selection.to_span(&term(2, 1), false), None); + assert_eq!(selection.to_span(&term(2, 1)), None); } /// Test adjacent cell selection from right to left @@ -430,7 +394,7 @@ mod test { let mut selection = Selection::simple(Point::new(0, Column(1)), Side::Left); selection.update(Point::new(0, Column(0)), Side::Right); - assert_eq!(selection.to_span(&term(2, 1), false), None); + assert_eq!(selection.to_span(&term(2, 1)), None); } /// Test selection across adjacent lines @@ -447,7 +411,7 @@ mod test { let mut selection = Selection::simple(Point::new(1, Column(1)), Side::Right); selection.update(Point::new(0, Column(1)), Side::Right); - assert_eq!(selection.to_span(&term(5, 2), false).unwrap(), Span { + assert_eq!(selection.to_span(&term(5, 2)).unwrap(), Span { start: Point::new(0, Column(1)), end: Point::new(1, Column(2)), }); @@ -470,19 +434,19 @@ mod test { selection.update(Point::new(1, Column(1)), Side::Right); selection.update(Point::new(1, Column(0)), Side::Right); - assert_eq!(selection.to_span(&term(5, 2), false).unwrap(), Span { + assert_eq!(selection.to_span(&term(5, 2)).unwrap(), Span { start: Point::new(0, Column(1)), end: Point::new(1, Column(1)), }); } #[test] - fn alt_scren_lines() { + fn alt_screen_lines() { let mut selection = Selection::lines(Point::new(0, Column(0))); selection.update(Point::new(5, Column(3)), Side::Right); selection.rotate(-3); - assert_eq!(selection.to_span(&term(5, 10), true).unwrap(), Span { + assert_eq!(selection.to_span(&term(5, 10)).unwrap(), Span { start: Point::new(0, Column(4)), end: Point::new(2, Column(0)), }); @@ -494,7 +458,7 @@ mod test { selection.update(Point::new(5, Column(3)), Side::Right); selection.rotate(-3); - assert_eq!(selection.to_span(&term(5, 10), true).unwrap(), Span { + assert_eq!(selection.to_span(&term(5, 10)).unwrap(), Span { start: Point::new(0, Column(4)), end: Point::new(2, Column(3)), }); @@ -506,7 +470,7 @@ mod test { selection.update(Point::new(5, Column(3)), Side::Right); selection.rotate(-3); - assert_eq!(selection.to_span(&term(5, 10), true).unwrap(), Span { + assert_eq!(selection.to_span(&term(5, 10)).unwrap(), Span { start: Point::new(0, Column(4)), end: Point::new(2, Column(4)), }); @@ -525,7 +489,7 @@ mod test { let mut selection = Selection::simple(Point::new(0, Column(1)), Side::Left); selection.update(Point::new(0, Column(8)), Side::Right); - assert_eq!(selection.to_span(&term, false).unwrap(), Span { + assert_eq!(selection.to_span(&term).unwrap(), Span { start: Point::new(0, Column(9)), end: Point::new(0, Column(0)), }); diff --git a/alacritty_terminal/src/term/mod.rs b/alacritty_terminal/src/term/mod.rs index 629f1cbf..2ec0621a 100644 --- a/alacritty_terminal/src/term/mod.rs +++ b/alacritty_terminal/src/term/mod.rs @@ -189,7 +189,12 @@ impl Search for Term { impl selection::Dimensions for Term { fn dimensions(&self) -> Point { - Point { col: self.grid.num_cols(), line: self.grid.num_lines() } + let line = if self.mode.contains(TermMode::ALT_SCREEN) { + self.grid.num_lines() + } else { + Line(self.grid.len()) + }; + Point { col: self.grid.num_cols(), line } } } @@ -1062,9 +1067,8 @@ impl Term { } } - let alt_screen = self.mode.contains(TermMode::ALT_SCREEN); let selection = self.grid.selection.clone()?; - let Span { mut start, mut end } = selection.to_span(self, alt_screen)?; + let Span { mut start, mut end } = selection.to_span(self)?; let mut res = String::new(); @@ -1151,8 +1155,7 @@ impl Term { config: &'b Config, window_focused: bool, ) -> RenderableCellsIter<'_> { - let alt_screen = self.mode.contains(TermMode::ALT_SCREEN); - let selection = self.grid.selection.as_ref().and_then(|s| s.to_span(self, alt_screen)); + let selection = self.grid.selection.as_ref().and_then(|s| s.to_span(self)); let cursor = if window_focused || !config.cursor.unfocused_hollow() { self.cursor_style.unwrap_or(self.default_cursor_style)