Merge pull request #4384 from benandez/fix/3869-followup

#3869 follow-up: WOFF2 pre-check counts contours as wuff does (crash on open); tripped hinting isn't cached

crates/fonts/src/fontdb.rs

@@ -95,6 +95,9 @@ pub struct FontFace {
     #[cfg_attr(target_arch = "wasm32", allow(dead_code))]
     hint_slow: std::sync::atomic::AtomicU32,
     hinting_off: std::sync::atomic::AtomicBool,
+    /// A hinted draw slower than this is a strike ([`HINT_STRIKE`]; tests shorten it).
+    #[cfg(not(target_arch = "wasm32"))]
+    hint_strike: std::time::Duration,
 }
 
 /// Limits on what a face's TrueType instructions may run over (#3692). skrifa's interpreter budget
@@ -233,7 +236,7 @@ impl FontFace {
         (!pen.over).then_some(pen.path)
     }
     fn draw_outline(&self, gid: u32, max_elements: usize) -> FlipPen {
-        let mut pen = FlipPen { path: BezPath::new(), room: max_elements, over: false };
+        let mut pen = FlipPen { path: BezPath::new(), room: max_elements, over: false, hinted: false };
         if let Some(f) = self.skrifa()
             && let Some(g) = f.outline_glyphs().get(GlyphId::new(gid))
         {
@@ -242,11 +245,19 @@ impl FontFace {
             #[cfg(not(target_arch = "wasm32"))]
             let started = std::time::Instant::now();
             let settings = hinter.map_or_else(|| DrawSettings::unhinted(Size::unscaled(), self.location()), |h| DrawSettings::hinted(h, false));
-            let drawn = g.draw(settings, &mut pen);
+            pen.hinted = hinter.is_some();
+            let mut drawn = g.draw(settings, &mut pen);
             #[cfg(not(target_arch = "wasm32"))]
-            if hinter.is_some() && self.count_hinted(started.elapsed() > HINT_STRIKE) {
+            if hinter.is_some() && self.count_hinted(started.elapsed() > self.hint_strike) {
                 log::warn!("Hinting glyph {gid} in {:?} took {:?}: drawing the face unhinted", self.family, started.elapsed());
             }
+            // A draw that was hinted when hinting went off (by itself, or by another thread's draw
+            // meanwhile) is the kind that was slow or misassembled: draw it again unhinted, so
+            // the outline cache never keeps it (#3692).
+            if pen.hinted && self.hinting_off.load(Ordering::Relaxed) {
+                pen = FlipPen { path: BezPath::new(), room: max_elements, over: false, hinted: false };
+                drawn = g.draw(DrawSettings::unhinted(Size::unscaled(), self.location()), &mut pen);
+            }
             if let Err(e) = drawn {
                 log::warn!("Cannot draw glyph {gid} in {:?}: {e}", self.family);
                 pen.path = BezPath::new();
@@ -1015,6 +1026,8 @@ fn make_face(bytes: FontBytes, index: u32, family: String, style: String, legacy
         hint_draws: Default::default(),
         hint_slow: Default::default(),
         hinting_off: Default::default(),
+        #[cfg(not(target_arch = "wasm32"))]
+        hint_strike: HINT_STRIKE,
     })
 }
 
@@ -1996,6 +2009,8 @@ struct FlipPen {
     path: BezPath,
     room: usize,
     over: bool,
+    /// Drawn with the face's TrueType interpreter.
+    hinted: bool,
 }
 
 impl FlipPen {
@@ -2375,6 +2390,35 @@ mod tests {
         assert!(!face.glyph_outline(face.glyph_for('a')).elements().is_empty(), "drawn unhinted");
     }
 
+    #[test]
+    #[cfg(not(target_arch = "wasm32"))]
+    fn the_draw_that_turns_hinting_off_is_redrawn_unhinted_for_the_outline_cache() {
+        // #3692: the glyph whose slow hinted draw trips the breaker may be misassembled; it must not be
+        // what the outline cache keeps.
+        let data = hint_reliant_font(660);
+        let mut face = super::make_face(
+            super::FontBytes::Owned(std::sync::Arc::new(data)),
+            0,
+            "MingLiU JBMono".into(),
+            "Regular".into(),
+            "MingLiU JBMono".into(),
+            Vec::new(),
+        )
+        .unwrap();
+        assert!(face.hinter().is_some());
+        face.hint_strike = std::time::Duration::ZERO; // every hinted draw is slow
+        let gid = face.glyph_for('a');
+        assert!(face.draw_outline(gid, usize::MAX).hinted, "hinted while the breaker is closed");
+        assert!(face.draw_outline(gid, usize::MAX).hinted);
+        let tripping = face.draw_outline(gid, usize::MAX);
+        assert!(face.hinting_off.load(std::sync::atomic::Ordering::Relaxed), "the third slow draw trips the breaker");
+        assert!(!tripping.hinted, "the tripping draw is returned unhinted");
+        assert!(!tripping.path.elements().is_empty());
+        // And what the cache keeps is that outline.
+        let db = FontDb::with_font_dirs(vec![]);
+        assert_eq!(db.outline(&face, gid).elements(), tripping.path.elements());
+    }
+
     #[test]
     fn named_instances_and_collection_members_are_capped() {
         // #3716: a file's faces are capped before they are built, however the file multiplies them.

crates/fonts/src/fontdb/user.rs

@@ -273,9 +273,11 @@ fn glyf_points_fit(t: &[u8]) -> Option<bool> {
     let points = t.get(contours_end..contours_end.checked_add(u32_at(12)?)?)?;
     let mut at = 0;
     for g in 0..glyphs {
-        let n = i16::from_be_bytes(contours.get(2 * g..2 * g + 2)?.try_into().ok()?);
+        // The decoder reads the count as unsigned and only 0xFFFF is a composite glyph (no points
+        // here): 0x8000..=0xFFFE are that many contours, so they are counted, not read as negative.
+        let n = u16::from_be_bytes(contours.get(2 * g..2 * g + 2)?.try_into().ok()?);
         let mut total = 0usize;
-        for _ in 0..n.max(0) {
+        for _ in 0..if n == 0xFFFF { 0 } else { n } {
             // 255UInt16
             let code = *points.get(at)?;
             at += 1;
@@ -745,15 +747,20 @@ pub(crate) mod tests {
     /// A WOFF2 file holding a transformed `glyf` (and `loca`) whose one glyph has two contours of
     /// `points` points each (and no coordinates: the decoder finds it damaged in the end).
     fn woff2_glyph_with(points: u16) -> Vec<u8> {
-        let total = 2 * points as usize;
+        let [hi, lo] = points.to_be_bytes();
+        woff2_glyph_with_contours(2, &[253, hi, lo].repeat(2), 2 * points as usize)
+    }
+
+    /// As [`woff2_glyph_with`], for a glyph whose contour count is `n` and whose point-count stream
+    /// is `counts`, with `total` flags.
+    fn woff2_glyph_with_contours(n: u16, counts: &[u8], total: usize) -> Vec<u8> {
         let mut glyf = vec![0, 0, 0, 0, 0, 1, 0, 0]; // reserved, optionFlags, numGlyphs 1, indexFormat
         // nContour, nPoints, flag, glyph, composite, bbox and instruction stream sizes.
-        for size in [2, 6, total, 0, 0, 4, 0] {
+        for size in [2, counts.len(), total, 0, 0, 4, 0] {
             glyf.extend_from_slice(&(size as u32).to_be_bytes());
         }
-        glyf.extend_from_slice(&2i16.to_be_bytes());
-        let [hi, lo] = points.to_be_bytes();
-        glyf.extend_from_slice(&[253, hi, lo, 253, hi, lo]); // 255UInt16 in its word form
+        glyf.extend_from_slice(&n.to_be_bytes());
+        glyf.extend_from_slice(counts); // 255UInt16 point counts
         glyf.resize(glyf.len() + total, 1); // on-curve flags
         glyf.extend_from_slice(&[0; 4]); // no explicit bounding boxes
         let mut dir = vec![10]; // glyf, transform version 0
@@ -801,6 +808,25 @@ pub(crate) mod tests {
         assert!(matches!(decode_font_file(&woff2_glyph_with(3)), Err(UserFontError::BadWebFont(e)) if e != TOO_MANY_POINTS));
     }
 
+    #[test]
+    fn a_woff2_glyph_with_a_contour_count_past_i16_max_is_counted() {
+        // #3703: the decoder reads the contour count as unsigned (only 0xFFFF is a composite), so
+        // 0x8000..=0xFFFE are that many contours; read as negative they counted no points and the
+        // decoder then allocated about 1 GB for a 1.3 MB font.
+        for n in [0x7FFFu16, 0x8000, 0xFFFE] {
+            let file = woff2_glyph_with_contours(n, &vec![3; usize::from(n)], 3 * usize::from(n));
+            assert_eq!(decode_font_file(&file), Err(UserFontError::BadWebFont(TOO_MANY_POINTS.into())), "{n:#x}");
+        }
+    }
+
+    #[test]
+    fn a_woff2_composite_glyph_is_not_counted_as_having_points() {
+        // 0xFFFF marks a composite glyph: the point check skips it (the decoder reads its own
+        // streams), so it is not refused for its points.
+        let file = woff2_glyph_with_contours(0xFFFF, &[], 0);
+        assert!(matches!(decode_font_file(&file), Err(UserFontError::BadWebFont(e)) if e != TOO_MANY_POINTS));
+    }
+
     #[test]
     fn an_added_font_is_listed_resolved_and_measured() {
         let db = db();