Merge pull request #813 from googlefonts/cyclical-components

prevent recursion error with cyclical components and provide descriptive error message

Lib/ufo2ft/util.py

@@ -16,6 +16,7 @@
 from fontTools.pens.transformPen import TransformPen
 
 from ufo2ft.constants import UNICODE_SCRIPT_ALIASES
+from ufo2ft.errors import InvalidDesignSpaceData, InvalidFontData
 from ufo2ft.fontInfoData import getAttrWithFallback
 
 logger = logging.getLogger(__name__)
@@ -234,8 +235,6 @@ def makeUnicodeToGlyphNameMapping(font, glyphOrder=None):
             if uni not in mapping:
                 mapping[uni] = glyphName
             else:
-                from ufo2ft.errors import InvalidFontData
-
                 raise InvalidFontData(
                     "cannot map '%s' to U+%04X; already mapped to '%s'"
                     % (glyphName, uni, mapping[uni])
@@ -462,14 +461,10 @@ def __str__(self):
 def getDefaultMasterFont(designSpaceDoc):
     defaultSource = designSpaceDoc.findDefault()
     if not defaultSource:
-        from ufo2ft.errors import InvalidDesignSpaceData
-
         raise InvalidDesignSpaceData(
             "Can't find base (neutral) master in DesignSpace document"
         )
     if not defaultSource.font:
-        from ufo2ft.errors import InvalidDesignSpaceData
-
         raise InvalidDesignSpaceData(
             "DesignSpace source '%s' is missing required 'font' attribute"
             % getattr(defaultSource, "name", "<Unknown>")
@@ -488,8 +483,6 @@ def _notdefGlyphFallback(designSpaceDoc):
     If the default master does not contain a .notdef either, return None since
     the auto-generated .notdef can be used.
     """
-    from ufo2ft.errors import InvalidDesignSpaceData
-
     try:
         baseUfo = getDefaultMasterFont(designSpaceDoc)
     except InvalidDesignSpaceData:
@@ -608,7 +601,9 @@ def ensure_all_sources_have_names(doc: DesignSpaceDocument) -> None:
         used_names.add(source.name)
 
 
-def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0):
+def getMaxComponentDepth(
+    glyph, glyphSet, maxComponentDepth=0, visited=None, rec_stack=None
+):
     """Return the height of a composite glyph's tree of components.
 
     This is equal to the depth of its deepest node, where the depth
@@ -617,10 +612,21 @@ def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0):
 
     For glyphs that contain no components, only contours, this is 0.
     Composite glyphs have max component depth of 1 or greater.
+
+    Raises InvalidFontData if a cyclical component reference is detected.
     """
     if not glyph.components:
         return maxComponentDepth
 
+    if visited is None:
+        visited = set()
+    if rec_stack is None:
+        rec_stack = []
+
+    assert glyph.name not in visited
+    visited.add(glyph.name)
+    rec_stack.append(glyph.name)
+
     maxComponentDepth += 1
 
     initialMaxComponentDepth = maxComponentDepth
@@ -629,10 +635,18 @@ def getMaxComponentDepth(glyph, glyphSet, maxComponentDepth=0):
             baseGlyph = glyphSet[component.baseGlyph]
         except KeyError:
             continue
-        componentDepth = getMaxComponentDepth(
-            baseGlyph, glyphSet, initialMaxComponentDepth
-        )
-        maxComponentDepth = max(maxComponentDepth, componentDepth)
+        if component.baseGlyph not in visited:
+            componentDepth = getMaxComponentDepth(
+                baseGlyph, glyphSet, initialMaxComponentDepth, visited, rec_stack
+            )
+            maxComponentDepth = max(maxComponentDepth, componentDepth)
+        elif component.baseGlyph in rec_stack:
+            raise InvalidFontData(
+                f"cyclical component reference:"
+                f" {' -> '.join(rec_stack)} => {component.baseGlyph}"
+            )
+
+    rec_stack.pop()
 
     return maxComponentDepth
 

tests/util_test.py

@@ -34,3 +34,52 @@ def test_overloaded_mapping_raises_error(FontClass):
         match=re.escape("cannot map 'B' to U+0041; already mapped to 'A'"),
     ):
         util.makeUnicodeToGlyphNameMapping(test_ufo)
+
+
+def test_getMaxComponentDepth_cyclical_reference():
+    # ufoLib2 lets you create cyclical component references (defcon would fail with
+    # RecursionError while creating them so we don't test it below).
+    # Here we test that we properly detect them and provide a descriptive error message.
+    # https://github.com/googlefonts/fontmake/issues/1066
+    test_ufo = pytest.importorskip("ufoLib2").Font()
+    glyph_a = test_ufo.newGlyph("A")
+    glyph_b = test_ufo.newGlyph("B")
+    glyph_c = test_ufo.newGlyph("C")
+
+    glyph_a.getPen().addComponent("C", (1, 0, 0, 1, 0, 0))
+    glyph_b.getPen().addComponent("A", (1, 0, 0, 1, 0, 0))
+    glyph_c.getPen().addComponent("B", (1, 0, 0, 1, 0, 0))
+
+    with pytest.raises(
+        InvalidFontData, match="cyclical component reference: A -> C -> B => A"
+    ):
+        util.getMaxComponentDepth(glyph_a, test_ufo)
+    with pytest.raises(
+        InvalidFontData, match="cyclical component reference: B -> A -> C => B"
+    ):
+        util.getMaxComponentDepth(glyph_b, test_ufo)
+    with pytest.raises(
+        InvalidFontData, match="cyclical component reference: C -> B -> A => C"
+    ):
+        util.getMaxComponentDepth(glyph_c, test_ufo)
+
+    glyph_d = test_ufo.newGlyph("D")
+    glyph_e = test_ufo.newGlyph("E")
+    glyph_f = test_ufo.newGlyph("F")
+    glyph_g = test_ufo.newGlyph("G")
+    glyph_h = test_ufo.newGlyph("H")
+
+    # adding same component multiple times should not cause infinite recursion
+    glyph_d.getPen().addComponent("E", (1, 0, 0, 1, 0, 0))
+    glyph_d.getPen().addComponent("E", (1, 0, 0, 1, 0, 0))
+    # G is reachable from both E and F, but there is no cycle.
+    glyph_e.getPen().addComponent("F", (1, 0, 0, 1, 0, 0))
+    glyph_f.getPen().addComponent("G", (1, 0, 0, 1, 0, 0))
+    glyph_e.getPen().addComponent("G", (1, 0, 0, 1, 0, 0))
+    glyph_g.getPen().addComponent("H", (1, 0, 0, 1, 0, 0))
+
+    assert util.getMaxComponentDepth(glyph_d, test_ufo) == 4
+    assert util.getMaxComponentDepth(glyph_e, test_ufo) == 3
+    assert util.getMaxComponentDepth(glyph_f, test_ufo) == 2
+    assert util.getMaxComponentDepth(glyph_g, test_ufo) == 1
+    assert util.getMaxComponentDepth(glyph_h, test_ufo) == 0