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

[andyyu2004]

Whilst indirectly fuzzing this library I found a panic in the span rendering logic. This PR provides a minimized reproduction and a small patch to avoid the panic.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 89.3%. Comparing base (742a5ea) to head (d82e211).

Files	Patch %	Lines
src/text/text.rs	71.4%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1001     +/-   ##
=======================================
- Coverage   89.3%   89.3%   -0.1%     
=======================================
  Files         61      61             
  Lines      15298   15305      +7     
=======================================
+ Hits       13670   13675      +5     
- Misses      1628    1630      +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[EdJoPaTo]

I have only looked over this in the browser without experimenting with it in code so maybe I have false assumptions about the specifics here. I might have overlooked something obvious.

[EdJoPaTo]

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.

[andyyu2004]

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!

[EdJoPaTo]

Sorry for not looking into this sooner. The source code here is hard to read and a bit confusing. The variable names are also not that helpful to understand what is happening in the code.

I refactored this to easier to understand the logic in my opinion. This basically says "get graphemes while there is width remaining" now. Any thoughts on this?

 impl WidgetRef for Span<'_> {
     fn render_ref(&self, area: Rect, buf: &mut Buffer) {
         let area = area.intersection(buf.area);
-        let Rect {
-            x: mut current_x,
-            y,
-            ..
-        } = area;
-        let max_x = area.right();
-        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 {
-                break;
-            }
-            buf.get_mut(current_x, y)
-                .set_symbol(g.symbol)
-                .set_style(g.style);
+        let mut remaining_width = area.width;
+        let graphemes = self.styled_graphemes(Style::new()).map_while(|grapheme| {
+            let symbol_width = grapheme.symbol.width() as u16;
+            remaining_width = remaining_width.checked_sub(symbol_width)?;
+            Some((grapheme, symbol_width))
+        });
+        let Rect { mut x, y, .. } = area;
+        for (grapheme, symbol_width) in graphemes {
+            buf.get_mut(x, y)
+                .set_symbol(grapheme.symbol)
+                .set_style(grapheme.style);

             // multi-width graphemes must clear the cells of characters that are hidden by the
             // grapheme, otherwise the hidden characters will be re-rendered if the grapheme is
             // overwritten.
-            for i in (current_x + 1)..next_x {
+            let x_after_symbol = x.saturating_add(symbol_width);
+            for i in (x + 1)..x_after_symbol {
                 buf.get_mut(i, y).reset();
                 // it may seem odd that the style of the hidden cells are not set to the style of
                 // the grapheme, but this is how the existing buffer.set_span() method works.
                 // buf.get_mut(i, y).set_style(g.style);
             }
-            current_x = next_x;
+            x = x_after_symbol;
         }
     }
 }

(Will result in a merge conflict with #1047. I somewhat use #1047 already in this before-diff. The new code works with and without #1047 and is way easier to understand in my opinion.)

[EdJoPaTo]

There is also Buffer::set_span. There should be a single place to implement this. Either here or there. Then there is a single place to fix a bug instead of multiple ones.

Line has a similar problem. There is Buffer::set_line and impl WidgetRef for Line<'_>.

Also Buffer::set_stringn has a similar logic… Too many places with similar code.