Skip to content

Commit

Permalink
[skrifa] color: add traversal depth limit (#841)
Browse files Browse the repository at this point in the history
Adds a maximum recursion depth for COLRv1 paint graphs. The limit chosen is 64 which matches the one used in HarfBuzz.

This makes skrifa more robust to potential stack overflow attacks and will enable no_std support without taking a hashbrown dependency for the visited set.
  • Loading branch information
dfrg authored Mar 15, 2024
1 parent b9d3245 commit b95258b
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 0 deletions.
3 changes: 3 additions & 0 deletions skrifa/src/color/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ pub enum PaintError {
ParseError(ReadError),
GlyphNotFound(GlyphId),
PaintCycleDetected,
DepthLimitExceeded,
}

impl std::fmt::Display for PaintError {
Expand All @@ -86,6 +87,7 @@ impl std::fmt::Display for PaintError {
write!(f, "No COLRv1 glyph found for glyph id: {glyph_id}")
}
PaintError::PaintCycleDetected => write!(f, "Paint cycle detected in COLRv1 glyph."),
PaintError::DepthLimitExceeded => write!(f, "Depth limit exceeded in COLRv1 glyph."),
}
}
}
Expand Down Expand Up @@ -353,6 +355,7 @@ impl<'a> ColorGlyph<'a> {
&instance,
painter,
&mut visited_set,
0,
)?;

if clipbox.is_some() {
Expand Down
21 changes: 21 additions & 0 deletions skrifa/src/color/traversal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ use super::{
Brush, ColorPainter, ColorStop, PaintCachedColorGlyph, PaintError, Transform,
};

/// Depth at which we will stop traversing and return an error.
///
/// Used to prevent stack overflows. Also allows us to avoid using a HashSet
/// in no_std builds.
///
/// This limit matches the one used in HarfBuzz:
/// HB_MAX_NESTING_LEVEL: <https://github.com/harfbuzz/harfbuzz/blob/c2f8f35a6cfce43b88552b3eb5c05062ac7007b2/src/hb-limits.hh#L53>
/// hb_paint_context_t: <https://github.com/harfbuzz/harfbuzz/blob/c2f8f35a6cfce43b88552b3eb5c05062ac7007b2/src/OT/Color/COLR/COLR.hh#L74>
const MAX_TRAVERSAL_DEPTH: u32 = 64;

pub(crate) fn get_clipbox_font_units(
colr_instance: &ColrInstance,
glyph_id: GlyphId,
Expand Down Expand Up @@ -111,7 +121,11 @@ pub(crate) fn traverse_with_callbacks(
instance: &ColrInstance,
painter: &mut impl ColorPainter,
visited_set: &mut HashSet<usize>,
recurse_depth: u32,
) -> Result<(), PaintError> {
if recurse_depth >= MAX_TRAVERSAL_DEPTH {
return Err(PaintError::DepthLimitExceeded);
}
match paint {
ResolvedPaint::ColrLayers { range } => {
for layer_index in range.clone() {
Expand All @@ -125,6 +139,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
)?;
visited_set.remove(&paint_id);
}
Expand Down Expand Up @@ -425,6 +440,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
&mut optimizer,
visited_set,
recurse_depth + 1,
);

// In case the optimization was not successful, just push a clip, and continue unoptimized traversal.
Expand All @@ -435,6 +451,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
);
painter.pop_clip();
}
Expand Down Expand Up @@ -462,6 +479,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
);
if clipbox.is_some() {
painter.pop_clip();
Expand Down Expand Up @@ -495,6 +513,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
);
painter.pop_transform();
result
Expand All @@ -510,6 +529,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
);
result?;
painter.push_layer(*mode);
Expand All @@ -518,6 +538,7 @@ pub(crate) fn traverse_with_callbacks(
instance,
painter,
visited_set,
recurse_depth + 1,
);
painter.pop_layer();
painter.pop_layer();
Expand Down

0 comments on commit b95258b

Please sign in to comment.