googlefonts / fontations

Reading and writing font files
Apache License 2.0
384 stars 23 forks source link

Add overflow panic in path retrieval for CFF Font "DIN Alternate" #1193

Open drott opened 21 hours ago

drott commented 21 hours ago

Compare https://github.com/drott/fontations/tree/cffAbrt

Font copyright: "© Dutch Design: Albert-Jan Pool, 1995. Published by FontShop International FontFont release 15"

#15 0x7f70b815abe8 core::panicking::panic_const::panic_const_add_overflow::hce83bca72fd72e30
#16 0x7f70b6d8adf4 read_fonts::tables::postscript::stack::Stack::apply_delta_prefix_sum::h81fd966ef69e45bd
#17 0x7f70b6d8bfee read_fonts::tables::postscript::dict::parse_entry::hc425cef37d28a96c

This looks like an overflow in the

sum += ...delta...

accumulation in apply_delta_prefix_sum.

drott commented 21 hours ago

Reproduction in https://github.com/drott/fontations/tree/cffAbrt with subsetted font.

drott commented 21 hours ago

Subsetted font subsetted.zip

drott commented 21 hours ago

Stack trace of the error points to:

        BlueValues => {
            stack.apply_delta_prefix_sum();
            Entry::BlueValues(Blues::new(stack.fixed_values()))
        }

FontTools dumps the CFF PrivateDict as:

      <Private>
        <BlueValues value="-6 0 521 527 712 718 -32768 -32768"/>
        <BlueScale value="0.363636"/>
        <BlueShift value="7"/>
        <BlueFuzz value="1"/>
        <StdHW value="117"/>
        <StdVW value="131"/>
        <StemSnapH value="117"/>
        <StemSnapV value="131"/>
        <ForceBold value="1"/>
        <LanguageGroup value="0"/>
        <ExpansionFactor value="0.06"/>
        <initialRandomSeed value="0"/>
        <defaultWidthX value="543"/>
        <nominalWidthX value="551"/>
      </Private>
drott commented 21 hours ago
❯ cargo run --release -p fauntlet -- compare-glyphs ~/dev/cffAbrt/DIN_Alternate.otf 
[...]
   Compiling fauntlet v0.1.0 (/usr/local/google/home/drott/dev/fontations/fauntlet)
    Finished `release` profile [optimized] target(s) in 20.89s
     Running `target/release/fauntlet compare-glyphs /usr/local/google/home/drott/dev/cffAbrt/DIN_Alternate.otf`

Fauntlet completes without errors.

drott commented 20 hours ago

If I read https://adobe-type-tools.github.io/font-tech-notes/pdfs/T1_SPEC.pdf right, this probably means that the 4th pair in the BlueValues array is invalid, which shoud describe cap height and cap-height overshoot - if I counted correctly.

dfrg commented 20 hours ago

Confirmed that we match FreeType in both hinted and unhinted modes when debug assertions are disabled which means that FreeType simply allows these overflows.

Note that replacing the += with wrapping_add in Stack::apply_delta_prefix_sum prevents that crash, but causes further overflow panics in HintState::capture when values that are derived from the overflowing blues (here specifically, the fuzz value) are used in addition/subtraction operations.

FreeType doesn't do any checked arithmetic so to maintain a match, I see two options:

  1. Replace operators with wrapping_add and wrapping_sub at all call sites that are known to panic. We are likely to encounter further panics down the line.
  2. Change Fixed::add and Fixed::sub to always use wrapping arithmetic. Note that our multiply and division operators already effectively do this.
drott commented 19 hours ago

For context https://github.com/googlefonts/fontations/pull/966 https://github.com/googlefonts/fontations/pull/967

Are also relevant, in particular, adding clippy::arithmetic_side_effects might help in pointing out locations and going case by case? In the cursor case we used saturating math and do not completely ignore font bugs.

For this particular broken font to pass and to match FT behavior, we need FT-like overflow behavior here, but we could discuss what other locations clippy::arithmetic_side_effects points out and how we want to treat them? On the other hand I seem to remember that clippy did not find all locations. Might have to do with how we use the Fixed type and whether the compiler can spot the situations.