Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid various overflows in CFF #1196

Merged
merged 2 commits into from
Oct 21, 2024
Merged

Avoid various overflows in CFF #1196

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52d8a0a
Avoid various overflows in CFF
dfrg Oct 21, 2024
77d88ae
add link to FT delta code; more wrapping_sub
dfrg Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion read-fonts/src/tables/postscript/stack.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,9 @@ impl Stack {
/// "The second and subsequent numbers in a delta are encoded as the
/// difference between successive values."
///
/// Roughly equivalent to the FreeType logic at
/// <https://gitlab.freedesktop.org/freetype/freetype/-/blob/57617782464411201ce7bbc93b086c1b4d7d84a5/src/cff/cffparse.c#L1431>
///
/// See <https://learn.microsoft.com/en-us/typography/opentype/spec/cff2#table-6-operand-types>
pub fn apply_delta_prefix_sum(&mut self) {
if self.top > 1 {
Expand All @@ -212,7 +215,7 @@ impl Stack {
.iter_mut()
.zip(&mut self.value_is_fixed)
{
sum += if *is_fixed {
let fixed_value = if *is_fixed {
// FreeType reads delta values using cff_parse_num which
// which truncates the fractional parts of 16.16 values
// See delta parsing:
Expand All @@ -224,6 +227,12 @@ impl Stack {
} else {
Fixed::from_i32(*value)
};
// See <https://github.com/googlefonts/fontations/issues/1193>
// The "DIN Alternate" font contains incorrect blue values
// that cause an overflow in this computation. FreeType does
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional: Would you have a link to the equivalent logic in FreeType similar to this delta accumulation here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The equivalent logic is here: https://gitlab.freedesktop.org/freetype/freetype/-/blob/57617782464411201ce7bbc93b086c1b4d7d84a5/src/cff/cffparse.c#L1431 along with the "fixed" case below. I'll add this link to the code.

// not use checked arithmetic so we need to explicitly use
// wrapping behavior to produce matching outlines.
sum = sum.wrapping_add(fixed_value);
*value = sum.to_bits();
*is_fixed = true;
}
Expand Down
15 changes: 9 additions & 6 deletions skrifa/src/outline/cff/hint.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,19 +267,22 @@ impl HintState {
///
/// See <https://gitlab.freedesktop.org/freetype/freetype/-/blob/80a507a6b8e3d2906ad2c8ba69329bd2fb2a85ef/src/psaux/psblues.c#L465>
fn capture(&self, bottom_edge: &mut Hint, top_edge: &mut Hint) -> bool {
// We use some wrapping arithmetic on this value below to avoid panics
// on overflow and match FreeType's behavior
// See <https://github.com/googlefonts/fontations/issues/1193>
let fuzz = self.blue_fuzz;
let mut captured = false;
let mut adjustment = Fixed::ZERO;
for zone in self.zones() {
if zone.is_bottom
&& bottom_edge.is_bottom()
&& (zone.cs_bottom_edge - fuzz) <= bottom_edge.cs_coord
&& bottom_edge.cs_coord <= (zone.cs_top_edge + fuzz)
&& zone.cs_bottom_edge.wrapping_sub(fuzz) <= bottom_edge.cs_coord
&& bottom_edge.cs_coord <= zone.cs_top_edge.wrapping_add(fuzz)
drott marked this conversation as resolved.
Show resolved Hide resolved
{
// Bottom edge captured by bottom zone.
adjustment = if self.suppress_overshoot {
zone.ds_flat_edge
} else if zone.cs_top_edge - bottom_edge.cs_coord >= self.blue_shift {
} else if zone.cs_top_edge.wrapping_sub(bottom_edge.cs_coord) >= self.blue_shift {
// Guarantee minimum of 1 pixel overshoot
bottom_edge
.ds_coord
Expand All @@ -294,13 +297,13 @@ impl HintState {
}
if !zone.is_bottom
&& top_edge.is_top()
&& (zone.cs_bottom_edge - fuzz) <= top_edge.cs_coord
&& top_edge.cs_coord <= (zone.cs_top_edge + fuzz)
&& zone.cs_bottom_edge.wrapping_sub(fuzz) <= top_edge.cs_coord
&& top_edge.cs_coord <= zone.cs_top_edge.wrapping_add(fuzz)
{
// Top edge captured by top zone.
adjustment = if self.suppress_overshoot {
drott marked this conversation as resolved.
Show resolved Hide resolved
zone.ds_flat_edge
} else if top_edge.cs_coord - zone.cs_bottom_edge >= self.blue_shift {
} else if top_edge.cs_coord.wrapping_sub(zone.cs_bottom_edge) >= self.blue_shift {
// Guarantee minimum of 1 pixel overshoot
top_edge
.ds_coord
Expand Down