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

[skrifa] autohint: cycle detection for GSUB lookups #1296

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[skrifa] autohint: cycle detection for GSUB lookups #1296

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17690f2
[skrifa] autohint: cycle detection for GSUB lookups
dfrg Dec 20, 2024
7039d07
clippy
dfrg Dec 20, 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
110 changes: 83 additions & 27 deletions skrifa/src/outline/autohint/shape.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use raw::{

// To prevent infinite recursion in contextual lookups. Matches HB
// <https://github.com/harfbuzz/harfbuzz/blob/c7ef6a2ed58ae8ec108ee0962bef46f42c73a60c/src/hb-limits.hh#L53>
const MAX_NESTING_DEPTH: u32 = 64;
const MAX_NESTING_DEPTH: usize = 64;

/// Determines the fidelity with which we apply shaping in the
/// autohinter.
Expand Down Expand Up @@ -223,7 +223,7 @@ impl<'a> Shaper<'a> {
};
// And now process associated lookups
for index in feature.lookup_list_indices().iter() {
gsub_handler.process_lookup(index.get(), 0);
gsub_handler.process_lookup(index.get());
}
}
}
Expand Down Expand Up @@ -294,7 +294,7 @@ impl ClusterShaper<'_> {
lookup_index: u16,
cluster: &mut ShapedCluster,
glyph_ix: usize,
nesting_depth: u32,
nesting_depth: usize,
) -> bool {
if nesting_depth > MAX_NESTING_DEPTH {
return false;
Expand Down Expand Up @@ -389,6 +389,9 @@ struct GsubHandler<'a> {
// Keep track of our range of touched gids in the style list
min_gid: usize,
max_gid: usize,
// Stack for detecting cycles in lookups
lookup_stack: [u16; MAX_NESTING_DEPTH],
lookup_depth: usize,
}

impl<'a> GsubHandler<'a> {
Expand All @@ -411,13 +414,19 @@ impl<'a> GsubHandler<'a> {
need_blue_substs,
min_gid,
max_gid: 0,
lookup_stack: [0; MAX_NESTING_DEPTH],
lookup_depth: 0,
}
}

fn process_lookup(&mut self, lookup_index: u16, nesting_depth: u32) {
if nesting_depth > MAX_NESTING_DEPTH {
return;
fn process_lookup(&mut self, lookup_index: u16) {
if self.enter_lookup(lookup_index) {
self.process_lookup_inner(lookup_index);
self.exit_lookup();
}
}

fn process_lookup_inner(&mut self, lookup_index: u16) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

might as well mark inline always?

Copy link
Member Author

Choose a reason for hiding this comment

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

sgtm

let Ok(subtables) = self
.lookup_list
.lookups()
Expand All @@ -441,7 +450,7 @@ impl<'a> GsubHandler<'a> {
// Check input coverage for blue strings if
// required and if we're not under a contextual
// lookup
if self.need_blue_substs && nesting_depth == 0 {
if self.need_blue_substs && self.lookup_depth == 1 {
self.check_blue_coverage(Ok(coverage));
}
}
Expand All @@ -450,7 +459,7 @@ impl<'a> GsubHandler<'a> {
self.capture_glyph(gid.get().to_u32());
}
// See above
if self.need_blue_substs && nesting_depth == 0 {
if self.need_blue_substs && self.lookup_depth == 1 {
self.check_blue_coverage(table.coverage());
}
}
Expand All @@ -465,7 +474,7 @@ impl<'a> GsubHandler<'a> {
}
}
// See above
if self.need_blue_substs && nesting_depth == 0 {
if self.need_blue_substs && self.lookup_depth == 1 {
self.check_blue_coverage(table.coverage());
}
}
Expand Down Expand Up @@ -499,10 +508,7 @@ impl<'a> GsubHandler<'a> {
{
for rule in set.seq_rules().iter().filter_map(|rule| rule.ok()) {
for rec in rule.seq_lookup_records() {
self.process_lookup(
rec.lookup_list_index(),
nesting_depth + 1,
);
self.process_lookup(rec.lookup_list_index());
}
}
}
Expand All @@ -517,17 +523,14 @@ impl<'a> GsubHandler<'a> {
set.class_seq_rules().iter().filter_map(|rule| rule.ok())
{
for rec in rule.seq_lookup_records() {
self.process_lookup(
rec.lookup_list_index(),
nesting_depth + 1,
);
self.process_lookup(rec.lookup_list_index());
}
}
}
}
SequenceContext::Format3(table) => {
for rec in table.seq_lookup_records() {
self.process_lookup(rec.lookup_list_index(), nesting_depth + 1);
self.process_lookup(rec.lookup_list_index());
}
}
}
Expand All @@ -546,10 +549,7 @@ impl<'a> GsubHandler<'a> {
set.chained_seq_rules().iter().filter_map(|rule| rule.ok())
{
for rec in rule.seq_lookup_records() {
self.process_lookup(
rec.lookup_list_index(),
nesting_depth + 1,
);
self.process_lookup(rec.lookup_list_index());
}
}
}
Expand All @@ -566,17 +566,14 @@ impl<'a> GsubHandler<'a> {
.filter_map(|rule| rule.ok())
{
for rec in rule.seq_lookup_records() {
self.process_lookup(
rec.lookup_list_index(),
nesting_depth + 1,
);
self.process_lookup(rec.lookup_list_index());
}
}
}
}
ChainedSequenceContext::Format3(table) => {
for rec in table.seq_lookup_records() {
self.process_lookup(rec.lookup_list_index(), nesting_depth + 1);
self.process_lookup(rec.lookup_list_index());
}
}
}
Expand All @@ -592,6 +589,29 @@ impl<'a> GsubHandler<'a> {
}
}

/// Called when we begin processing a new lookup.
///
/// Returns `false` if the lookup index is already being processed or if
/// the lookup would exceed our max depth.
fn enter_lookup(&mut self, lookup_index: u16) -> bool {
// Note: we just use a linear search here under the assumption that
// most fonts have shallow contextual lookup chains
if self.lookup_depth != 0
&& (self.lookup_depth == MAX_NESTING_DEPTH
|| self.lookup_stack[..self.lookup_depth].contains(&lookup_index))
{
return false;
}
self.lookup_stack[self.lookup_depth] = lookup_index;
self.lookup_depth += 1;
true
}

/// Called when we finish processing a single lookup.
fn exit_lookup(&mut self) {
self.lookup_depth = self.lookup_depth.saturating_sub(1);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love that this makes it relatively easy to add code that enters without calling exit. This would be somewhat harder if it was directly in the process lookup fn? - depicted below. Or by a guard that reduces depth on drop but I suppose that path takes you towards multiple mut refs.

    fn process_lookup(&mut self, lookup_index: u16) {
        // Guard: don't cycle and don't go exceed depth limit
        // Note: we use a linear search here under the assumption that
        // most fonts have shallow contextual lookup chains
        if self.lookup_depth != 0
            && (self.lookup_depth == MAX_NESTING_DEPTH
                || self.lookup_stack[..self.lookup_depth].contains(&lookup_index))
        {
            return;
        }
        self.lookup_stack[self.lookup_depth] = lookup_index;
        self.lookup_depth += 1;

        // Actually process the lookup
        self.process_lookup_inner(lookup_index);

        // Out we go again
        self.lookup_depth -= 1;
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with the sentiment but I kept the stack ops separate to support isolated unit testing of that functionality. If we embed them then we need to build a lookup list with the desired structure which seems less clear and more fragile to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have thought we need to build lookups with good/bad structure (one that cycles, one that has max depth, one that has max depth + 1, etc) to unit test regardless so... that's kind of fine?


/// Finishes processing for this set of GSUB lookups and
/// returns the range of touched glyphs.
fn finish(self) -> Option<Range<usize>> {
Expand Down Expand Up @@ -673,4 +693,40 @@ mod tests {
// from ttx, gid 1 is "H"
assert_eq!(cluster[0].id, GlyphId::new(1));
}

#[test]
fn exceed_max_depth() {
let font = FontRef::new(font_test_data::NOTOSERIF_AUTOHINT_SHAPING).unwrap();
let shaper = Shaper::new(&font, ShaperMode::BestEffort);
let lookup_list = shaper.gsub.as_ref().unwrap().lookup_list().unwrap();
let style = &style::STYLE_CLASSES[style::StyleClass::LATN];
let mut gsub_handler = GsubHandler::new(&shaper.charmap, &lookup_list, style, &mut []);
for ix in 0..MAX_NESTING_DEPTH {
// all okay up to max depth...
assert!(gsub_handler.enter_lookup(ix as u16));
}
// .. and this one fails
assert!(!gsub_handler.enter_lookup(MAX_NESTING_DEPTH as u16));
}

#[test]
fn detect_cycles() {
let font = FontRef::new(font_test_data::NOTOSERIF_AUTOHINT_SHAPING).unwrap();
let shaper = Shaper::new(&font, ShaperMode::BestEffort);
let lookup_list = shaper.gsub.as_ref().unwrap().lookup_list().unwrap();
let style = &style::STYLE_CLASSES[style::StyleClass::LATN];
let mut gsub_handler = GsubHandler::new(&shaper.charmap, &lookup_list, style, &mut []);
for ix in 0..10 {
// add 0 through 9 to the stack, all succeeding...
assert!(gsub_handler.enter_lookup(ix as u16));
}
for ix in 0..10 {
// ... and all fail when we try again
assert!(!gsub_handler.enter_lookup(ix as u16));
}
// exit lookup 9
gsub_handler.exit_lookup();
// ensure we can enter 9 again after exiting
assert!(gsub_handler.enter_lookup(9));
}
}
Loading