Repository: harfbuzz/rustybuzz (language: rust)

Issue:
Assertion failure
I'm having a problem with rustbuzz crashing inside of [diffenator3](https://github.com/googlefonts/diffenator3) for a particular font ([NotoSansMalayalam[wdth,wght].ttf.gz](https://github.com/user-attachments/files/17679225/NotoSansMalayalam.wdth.wght.ttf.gz)). 

Here's a reproducer:

```rust
        let font = include_bytes!("NotoSansMalayalam[wdth,wght].ttf");
        let mut face = Face::from_slice(font, 0).unwrap();
        let mut buffer = UnicodeBuffer::new();
        let string = "സ്ഥലമായിരുന്നതിനാല്‍";
        buffer.push_str(string);
        println!(">{}", string);
        let plan = ShapePlan::new(
            &face,
            Direction::LeftToRight,
            Some(script::MALAYALAM),
            None,
            &[],
        );

        let output = shape_with_plan(&face, &plan, buffer);
```

And here's the backtrace:

```
                              ⋮ 12 frames hidden ⋮
13: core::panicking::panic::hc3763f3effcb9929
    at <unknown source file>
14: rustybuzz::hb::buffer::hb_buffer_t::move_to::he6a9580defbf1514
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/buffer.rs:1156
    1154 │         }
    1155 │
    1156 >         assert!(i <= self.out_len + (self.len - self.idx));
    1157 │
    1158 │         if self.out_len < i {
15: rustybuzz::hb::ot_layout_gsubgpos::apply_lookup::hb0bb938a671a37e1
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout_gsubgpos.rs:980
     978 │     }
     979 │
     980 >     ctx.buffer.move_to(end);
     981 │ }
     982 │
16: <ttf_parser::ggg::chained_context::ChainedContextLookup as rustybuzz::hb::ot_layout_gsubgpos::Apply>::apply::h6da40ea28f40f5f6
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout_gsubgpos.rs:663
     661 │                 ctx.buffer
     662 │                     .unsafe_to_break_from_outbuffer(Some(start_index), Some(end_index));
     663 >                 apply_lookup(
     664 │                     ctx,
     665 │                     usize::from(input_coverages.len()),
17: rustybuzz::hb::ot_layout_gsub_table::<impl rustybuzz::hb::ot_layout_gsubgpos::Apply for ttf_parser::tables::gsub::SubstitutionSubtable>::apply::h3f7e621c5b38371c
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout_gsub_table.rs:49
      47 │             Self::Ligature(t) => t.apply(ctx),
      48 │             Self::Context(t) => t.apply(ctx),
      49 >             Self::ChainContext(t) => t.apply(ctx),
      50 │             Self::ReverseChainSingle(t) => t.apply(ctx),
      51 │         }
18: rustybuzz::hb::ot::layout::GSUB::subst_lookup::<impl rustybuzz::hb::ot_layout_gsubgpos::Apply for rustybuzz::hb::ot_layout_common::SubstLookup>::apply::h1180360a0b8d580a
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot/layout/GSUB/subst_lookup.rs:35
      33 │         if self.digest().may_have_glyph(ctx.buffer.cur(0).as_glyph()) {
      34 │             for subtable in &self.subtables {
      35 >                 if subtable.apply(ctx).is_some() {
      36 │                     return Some(());
      37 │                 }
19: rustybuzz::hb::ot_layout::apply_forward::h40254d8f0cec948a
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout.rs:302
     300 │         if (cur.mask & ctx.lookup_mask()) != 0
     301 │             && ctx.check_glyph_property(cur, ctx.lookup_props)
     302 >             && lookup.apply(ctx).is_some()
     303 │         {
     304 │             ret = true;
20: rustybuzz::hb::ot_layout::apply_string::h41a8546a60574f08
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout.rs:282
     280 │         }
     281 │         ctx.buffer.idx = 0;
     282 >         apply_forward(ctx, lookup);
     283 │
     284 │         if !T::IN_PLACE {
21: rustybuzz::hb::ot_layout::apply_layout_table::h29833b22a4aa51bc
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout.rs:256
     254 │                     ctx.per_syllable = lookup_map.per_syllable;
     255 │
     256 >                     apply_string::<T>(&mut ctx, lookup);
     257 │                 }
     258 │             }
22: rustybuzz::hb::ot_layout_gsub_table::substitute::h1449b349b6791f0f
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_layout_gsub_table.rs:13
      11 │
      12 │ pub fn substitute(plan: &hb_ot_shape_plan_t, face: &hb_font_t, buffer: &mut hb_buffer_t) {
      13 >     apply_layout_table(plan, face, buffer, face.gsub.as_ref());
      14 │ }
      15 │
23: rustybuzz::hb::ot_shape::hb_ot_substitute_plan::hccd88507dd191d9a
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_shape.rs:393
     391 │         aat_layout::hb_aat_layout_substitute(ctx.plan, ctx.face, ctx.buffer);
     392 │     } else {
     393 >         super::ot_layout_gsub_table::substitute(ctx.plan, ctx.face, ctx.buffer);
     394 │     }
     395 │ }
24: rustybuzz::hb::ot_shape::substitute_pre::hcbfcfe8bf658fb43
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_shape.rs:346
     344 │ fn substitute_pre(ctx: &mut hb_ot_shape_context_t) {
     345 │     hb_ot_substitute_default(ctx);
     346 >     hb_ot_substitute_plan(ctx);
     347 │
     348 │     if ctx.plan.apply_morx && ctx.plan.apply_gpos {
25: rustybuzz::hb::ot_shape::shape_internal::hc835d90c43eef53c
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/ot_shape.rs:334
     332 │     }
     333 │
     334 >     substitute_pre(ctx);
     335 │     position(ctx);
     336 │     substitute_post(ctx);
26: rustybuzz::hb::shape::shape_with_plan::h2df57445f599ab4b
    at /Users/simon/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustybuzz-0.20.0/src/hb/shape.rs:71
      69 │         #[cfg(not(feature = "wasm-shaper"))]
      70 │         {
      71 >             shape_internal(&mut hb_ot_shape_context_t {
      72 │                 plan,
      73 │                 face,
27: diffenator3_lib::render::renderer::tests::test_rustybuzz_crash::he1916ce60e7b4dcd
    at /Users/simon/hacks/typography/diffenator3/diffenator3-lib/src/render/renderer.rs:223
     221 │         );
     222 │
     223 >         let output = shape_with_plan(&face, &plan, buffer);
     224 │         panic!("Fail")
     225 │     }
```


Discussion / hints:
Does the same font/text crash HB by any chance?

That piece of code is the nastiest part of OT / HB.
No crash in HB, no. I'll try adding some debugging prints around the values of `end`, `match_positions` etc and run it again in both.
The problem is this line:

```
        end = (end as isize + delta) as _;
```

In this case, `end` is initially 0, `delta` is -2, so the value of end after this statement is 18446744073709551615. An attempt to resize the buffer to 18 exabytes does not go well.
I'm getting confused between harfruzz and rustybuzz. Fix in https://github.com/harfbuzz/harfruzz/pull/15
Will open a PR for here with a test case.
There are a few other instances of this exact construct as well as a large number of `as` invocations on integer types that are not very safe or idiomatic. That doesn't necessarily mean it is even possible to trigger other issues like this one, but it does mean there could be a little more defensive coding here.

Fix patch:
diff --git a/src/hb/ot_layout_gsubgpos.rs b/src/hb/ot_layout_gsubgpos.rs
index c632361f..ab38569f 100644
--- a/src/hb/ot_layout_gsubgpos.rs
+++ b/src/hb/ot_layout_gsubgpos.rs
@@ -170,6 +170,9 @@ pub fn match_lookahead(
     start_index: usize,
     end_index: &mut usize,
 ) -> bool {
+    // Function should always be called with a non-zero starting index
+    // c.f. https://github.com/harfbuzz/rustybuzz/issues/142
+    assert!(start_index >= 1);
     let mut iter = skipping_iterator_t::new(ctx, start_index - 1, true);
     iter.set_glyph_data(0);
     iter.enable_matching(match_func);
@@ -312,9 +315,9 @@ impl<'a, 'b> skipping_iterator_t<'a, 'b> {
     }
 
     pub fn prev(&mut self, unsafe_from: Option<&mut usize>) -> bool {
-        let stop = 0;
+        let stop: usize = 0;
 
-        while self.buf_idx > stop as usize {
+        while self.buf_idx > stop {
             self.buf_idx -= 1;
             let info = &self.ctx.buffer.out_info()[self.buf_idx];
 
@@ -858,7 +861,7 @@ fn apply_lookup(
 
     // All positions are distance from beginning of *output* buffer.
     // Adjust.
-    let mut end = {
+    let mut end: isize = {
         let backtrack_len = ctx.buffer.backtrack_len();
         let delta = backtrack_len as isize - ctx.buffer.idx as isize;
 
@@ -867,7 +870,7 @@ fn apply_lookup(
             match_positions[j] = (match_positions[j] as isize + delta) as _;
         }
 
-        backtrack_len + match_end - ctx.buffer.idx
+        backtrack_len as isize + match_end as isize - ctx.buffer.idx as isize
     };
 
     for record in lookups {
@@ -928,8 +931,8 @@ fn apply_lookup(
         //
         // It should be possible to construct tests for both of these cases.
 
-        end = end.saturating_add_signed(delta);
-        if end < match_positions[idx] {
+        end += delta;
+        if end < match_positions[idx] as isize {
             // End might end up being smaller than match_positions[idx] if the recursed
             // lookup ended up removing many items.
             // Just never rewind end beyond start of current position, since that is
@@ -938,8 +941,8 @@ fn apply_lookup(
             // https://bugs.chromium.org/p/chromium/issues/detail?id=659496
             // https://github.com/harfbuzz/harfbuzz/issues/1611
             //
-            delta += match_positions[idx] as isize - end as isize;
-            end = match_positions[idx];
+            delta += match_positions[idx] as isize - end;
+            end = match_positions[idx] as isize;
         }
 
         // next now is the position after the recursed lookup.
@@ -977,7 +980,7 @@ fn apply_lookup(
         }
     }
 
-    ctx.buffer.move_to(end);
+    ctx.buffer.move_to(end.try_into().unwrap());
 }
 
 /// Value represents glyph class.
@@ -1316,6 +1319,9 @@ pub fn ligate_input(
                 if this_comp == 0 {
                     this_comp = last_num_comps;
                 }
+                // Avoid the potential for a wrap-around bug when subtracting from an unsigned integer
+                // c.f. https://github.com/harfbuzz/rustybuzz/issues/142
+                assert!(comps_so_far >= last_num_comps);
                 let new_lig_comp = comps_so_far - last_num_comps + this_comp.min(last_num_comps);
                 _hb_glyph_info_set_lig_props_for_mark(cur, lig_id, new_lig_comp);
             }
@@ -1344,6 +1350,9 @@ pub fn ligate_input(
                 break;
             }
 
+            // Avoid the potential for a wrap-around bug when subtracting from an unsigned integer
+            // c.f. https://github.com/harfbuzz/rustybuzz/issues/142
+            assert!(comps_so_far >= last_num_comps);
             let new_lig_comp = comps_so_far - last_num_comps + this_comp.min(last_num_comps);
             _hb_glyph_info_set_lig_props_for_mark(info, lig_id, new_lig_comp)
         }


Test patch: