crystal-0001: collect_ancestors O(2^D) diamond traversal; clojure/elixir/nim/lua CLEAN; count 621→622

defects/crystal/patch/crystal-0001-collect-ancestors-diamond-hashset.md

@@ -0,0 +1,175 @@
+# UNDF: UNDF-2026-000000369
+# crystal-0001: collect_ancestors / lookup_defs O(2^D) diamond module traversal
+
+## Metadata
+
+| Field | Value |
+|-------|-------|
+| ID | crystal-0001 |
+| Ecosystem | Crystal |
+| Repo | https://github.com/crystal-lang/crystal |
+| File | `src/compiler/crystal/types.cr` |
+| Function | `collect_ancestors`, `lookup_defs`, `include` (cycle-check path) |
+| CWE | CWE-407 (Inefficient Algorithmic Complexity) |
+| Severity | MEDIUM |
+| Complexity | O(2^D) where D = diamond nesting depth |
+| Speedup | ~32x at D=5 nested diamonds (32 vs 1 unique module traversals) |
+
+## Summary
+
+The Crystal compiler's `Type#collect_ancestors` method (and `lookup_defs` which
+recursively walks `parents`) contains no visited-type guard. When modules form a
+diamond-shaped inclusion hierarchy, shared ancestors are traversed once per path
+that leads to them, producing O(2^D) work for D levels of nested diamonds.
+
+## Vulnerable Code
+
+**`src/compiler/crystal/types.cr`, lines 384–394:**
+
+```crystal
+def ancestors
+  ancestors = [] of Type
+  collect_ancestors(ancestors)
+  ancestors
+end
+
+protected def collect_ancestors(ancestors)
+  parents.try &.each do |parent|
+    ancestors << parent
+    parent.collect_ancestors(ancestors)  # no visited set — recurses into shared modules multiple times
+  end
+end
+```
+
+**`src/compiler/crystal/types.cr`, lines 408–433 (`lookup_defs`):**
+
+```crystal
+def lookup_defs(name : String, all_defs : Array(Def), lookup_ancestors_for_new : Bool? = false)
+  self.defs.try &.[name]?.try &.each do |item|
+    all_defs << item.def unless all_defs.find(&.same?(item.def))  # O(N) linear dedup
+  end
+  # ...
+  my_parents.try &.each do |parent|
+    parent.lookup_defs(name, all_defs, lookup_ancestors_for_new)  # no visited set
+  end
+end
+```
+
+## Diamond Scenario
+
+```crystal
+module M; def foo; end; end
+module A; include M; end   # A.parents = [M]
+module B; include M; end   # B.parents = [M]
+class C; include A; include B; end  # C.parents = [A, B] — M not directly in C.parents
+```
+
+- `C.parents = [A, B]`
+- `C.collect_ancestors` → visits A → visits M (1st), visits B → visits M (2nd)
+- `C.lookup_defs("foo")` → recurses A → recurses M (1st), recurses B → recurses M (2nd)
+
+With D nested diamond levels:
+
+```
+module M0; def foo; end; end
+module M1a; include M0; end; module M1b; include M0; end
+module M2a; include M1a; include M1b; end
+module M2b; include M1a; include M1b; end
+class C; include M2a; include M2b; end
+```
+
+This causes 2^D traversals of M0's `lookup_defs` and `collect_ancestors`.
+
+## Hot Paths Affected
+
+`ancestors` (via `collect_ancestors`) is called in:
+- `Type#include` (cycle detection) — called for every `include` at compile time
+- `AbstractDefChecker#implements_with_ancestors?` — called for every abstract method
+- `TypeDeclarationProcessor` — called 4× per type per compilation pass
+- `call.cr`, `new.cr`, doc generator — all rebuild the list from scratch each call
+
+`lookup_defs` is called for every method lookup that needs to collect all overloads.
+
+## Fix
+
+Add a `Set(UInt64)` (keyed on `object_id`) to `collect_ancestors` and a visited
+set passed through `lookup_defs` to skip already-visited types:
+
+```crystal
+def ancestors
+  ancestors = [] of Type
+  visited = Set(UInt64).new
+  collect_ancestors(ancestors, visited)
+  ancestors
+end
+
+protected def collect_ancestors(ancestors, visited : Set(UInt64))
+  parents.try &.each do |parent|
+    next unless visited.add?(parent.object_id)
+    ancestors << parent
+    parent.collect_ancestors(ancestors, visited)
+  end
+end
+```
+
+For `lookup_defs`, pass a `visited : Set(UInt64)` parameter and skip types already
+in the set before recursing into parents.
+
+## Patch
+
+```diff
+--- a/src/compiler/crystal/types.cr
++++ b/src/compiler/crystal/types.cr
+@@ -384,11 +384,14 @@ module Crystal
+     def ancestors
+       ancestors = [] of Type
+-      collect_ancestors(ancestors)
++      visited = Set(UInt64).new
++      collect_ancestors(ancestors, visited)
+       ancestors
+     end
+
+-    protected def collect_ancestors(ancestors)
++    protected def collect_ancestors(ancestors, visited : Set(UInt64))
+       parents.try &.each do |parent|
+-        ancestors << parent
+-        parent.collect_ancestors(ancestors)
++        next unless visited.add?(parent.object_id)
++        ancestors << parent
++        parent.collect_ancestors(ancestors, visited)
+       end
+     end
+@@ -408,15 +411,17 @@ module Crystal
+-    def lookup_defs(name : String, all_defs : Array(Def), lookup_ancestors_for_new : Bool? = false)
++    def lookup_defs(name : String, all_defs : Array(Def), lookup_ancestors_for_new : Bool? = false,
++                    visited : Set(UInt64)? = nil)
+       self.defs.try &.[name]?.try &.each do |item|
+         all_defs << item.def unless all_defs.find(&.same?(item.def))
+       end
+       # ...
++      visited ||= Set(UInt64).new
+       my_parents.try &.each do |parent|
+-        parent.lookup_defs(name, all_defs, lookup_ancestors_for_new)
++        next unless visited.not_nil!.add?(parent.object_id)
++        parent.lookup_defs(name, all_defs, lookup_ancestors_for_new, visited)
+       end
+     end
+```
+
+## Benchmark
+
+| Scenario | D=0 | D=1 | D=3 | D=5 |
+|----------|-----|-----|-----|-----|
+| collect_ancestors calls to M (unpatched) | 1 | 2 | 8 | 32 |
+| collect_ancestors calls to M (patched) | 1 | 1 | 1 | 1 |
+| Ratio | 1x | 2x | 8x | 32x |
+
+At D=5 nested diamonds (a deep framework with shared mixins), compilation throughput
+for type checking is degraded ~32× for ancestor-related operations.
+
+## References
+
+- CWE-407: Inefficient Algorithmic Complexity
+- `src/compiler/crystal/types.cr` lines 384–434
+- `src/compiler/crystal/semantic/type_declaration_processor.cr` lines 508, 582, 591, 614, 733
+- `src/compiler/crystal/semantic/abstract_def_checker.cr` lines 94, 390

defects/crystal/unit/test_crystal_0001.py

@@ -0,0 +1,266 @@
+"""
+Unit test for crystal-0001:
+  collect_ancestors / lookup_defs O(2^D) diamond module traversal
+
+This test simulates the Crystal compiler's parent-traversal logic in Python
+to verify the exponential blowup and the linear-time fix.
+"""
+
+try:
+    import pytest
+except ImportError:
+    # Allow running without pytest installed
+    import unittest as pytest
+    pytest.main = lambda *a, **kw: None
+
+
+def make_module(name, defs=None):
+    """Simulate a Crystal module type node."""
+    return {
+        "name": name,
+        "parents": [],
+        "defs": dict(defs or {}),
+    }
+
+
+# --- Unpatched: collect_ancestors without visited set ---
+
+def collect_ancestors_unpatched(node, result, call_counter):
+    """Replicates Crystal's vulnerable collect_ancestors (no visited guard)."""
+    for parent in node["parents"]:
+        result.append(parent)
+        call_counter[0] += 1
+        collect_ancestors_unpatched(parent, result, call_counter)
+
+
+def ancestors_unpatched(node):
+    result = []
+    counter = [0]
+    collect_ancestors_unpatched(node, result, counter)
+    return result, counter[0]
+
+
+# --- Patched: collect_ancestors with visited set ---
+
+def collect_ancestors_patched(node, result, visited):
+    """Replicates the fixed collect_ancestors (HashSet visited guard)."""
+    for parent in node["parents"]:
+        node_id = id(parent)
+        if node_id in visited:
+            continue
+        visited.add(node_id)
+        result.append(parent)
+        collect_ancestors_patched(parent, result, visited)
+
+
+def ancestors_patched(node):
+    result = []
+    visited = set()
+    collect_ancestors_patched(node, result, visited)
+    return result
+
+
+# --- lookup_defs unpatched ---
+
+def lookup_defs_unpatched(node, name, all_defs, call_counter):
+    """Replicates Crystal's vulnerable lookup_defs (no visited guard)."""
+    for defn in node["defs"].get(name, []):
+        if defn not in all_defs:
+            all_defs.append(defn)
+    for parent in node["parents"]:
+        call_counter[0] += 1
+        lookup_defs_unpatched(parent, name, all_defs, call_counter)
+
+
+def lookup_defs_patched(node, name, all_defs, visited):
+    """Replicates fixed lookup_defs (visited set prevents re-traversal)."""
+    node_id = id(node)
+    if node_id in visited:
+        return
+    visited.add(node_id)
+    for defn in node["defs"].get(name, []):
+        if defn not in all_defs:
+            all_defs.append(defn)
+    for parent in node["parents"]:
+        lookup_defs_patched(parent, name, all_defs, visited)
+
+
+# ---- Test helpers ----
+
+def build_diamond(depth):
+    """
+    Build a D-level diamond module graph:
+      M0 is the base (has 'foo')
+      At depth=1: C -> [L0, R0]; L0 -> [M0]; R0 -> [M0]   (M0 visited 2x)
+      At depth=2: C -> [L1, R1]; L1,R1 both -> [L0, R0]; L0,R0 -> [M0] (M0 4x)
+      At depth=D: M0 is visited 2^D times by unpatched traversal.
+    """
+    m0 = make_module("M0", defs={"foo": ["M0.foo"]})
+
+    if depth == 0:
+        c = make_module("C")
+        c["parents"].append(m0)
+        return c, m0
+
+    # Bottom pair: both include M0
+    left = make_module("L0")
+    right = make_module("R0")
+    left["parents"].append(m0)
+    right["parents"].append(m0)
+
+    for d in range(1, depth):
+        new_left = make_module(f"L{d}")
+        new_right = make_module(f"R{d}")
+        # Each new node includes BOTH left and right — true diamond at every level
+        new_left["parents"].append(left)
+        new_left["parents"].append(right)
+        new_right["parents"].append(left)
+        new_right["parents"].append(right)
+        left, right = new_left, new_right
+
+    c = make_module("C")
+    c["parents"].append(left)
+    c["parents"].append(right)
+    return c, m0
+
+
+# ---- Tests ----
+
+class TestCollectAncestorsUnpatched:
+    def test_no_diamond_d0(self):
+        """Single module, no parents."""
+        m = make_module("M")
+        result, calls = ancestors_unpatched(m)
+        assert result == []
+        assert calls == 0
+
+    def test_linear_chain_d2(self):
+        """A <- B <- C, no diamond — linear traversal."""
+        a = make_module("A")
+        b = make_module("B")
+        b["parents"].append(a)
+        c = make_module("C")
+        c["parents"].append(b)
+        result, calls = ancestors_unpatched(c)
+        assert len(result) == 2  # B, A
+        assert calls == 2
+
+    def test_diamond_d1_visits_m0_twice(self):
+        """Diamond at depth=1: M0 should be visited twice (unpatched)."""
+        c, m0 = build_diamond(1)
+        result, calls = ancestors_unpatched(c)
+        # M0 appears twice in the result list
+        assert result.count(m0) == 2
+        assert calls == 4  # A, M0, B, M0
+
+    def test_diamond_d2_visits_m0_four_times(self):
+        """Diamond at depth=2: M0 visited 4 times (unpatched)."""
+        c, m0 = build_diamond(2)
+        result, calls = ancestors_unpatched(c)
+        assert result.count(m0) == 4
+
+    def test_diamond_exponential_growth(self):
+        """Confirm 2^D traversal count for M0 (unpatched)."""
+        for d in range(1, 7):
+            c, m0 = build_diamond(d)
+            result, _ = ancestors_unpatched(c)
+            expected = 2 ** d
+            actual = result.count(m0)
+            assert actual == expected, (
+                f"depth={d}: expected {expected} visits to M0, got {actual}"
+            )
+
+
+class TestCollectAncestorsPatched:
+    def test_diamond_d1_visits_m0_once(self):
+        """Patched: M0 visited exactly once despite diamond."""
+        c, m0 = build_diamond(1)
+        result = ancestors_patched(c)
+        assert result.count(m0) == 1
+
+    def test_diamond_d5_visits_m0_once(self):
+        """Patched: M0 visited exactly once even at depth=5."""
+        c, m0 = build_diamond(5)
+        result = ancestors_patched(c)
+        assert result.count(m0) == 1
+
+    def test_correct_unique_ancestors(self):
+        """Patched: ancestor list has no duplicates."""
+        c, m0 = build_diamond(4)
+        result = ancestors_patched(c)
+        assert len(result) == len(set(id(x) for x in result)), (
+            "Patched collect_ancestors must not produce duplicate entries"
+        )
+
+
+class TestLookupDefsUnpatched:
+    def test_finds_def_in_diamond(self):
+        """lookup_defs finds M0.foo even in a diamond (but with redundant work)."""
+        c, _ = build_diamond(2)
+        all_defs = []
+        counter = [0]
+        lookup_defs_unpatched(c, "foo", all_defs, counter)
+        assert all_defs == ["M0.foo"]
+
+    def test_exponential_calls(self):
+        """Unpatched lookup_defs recurses O(2^D) into M0 for deep diamonds."""
+        for d in range(1, 6):
+            c, _ = build_diamond(d)
+            all_defs = []
+            counter = [0]
+            lookup_defs_unpatched(c, "foo", all_defs, counter)
+            # calls into M0 alone = 2^D; total parent.lookup_defs calls >= 2^D
+            assert counter[0] >= 2 ** d, (
+                f"depth={d}: expected >= {2**d} recursive calls, got {counter[0]}"
+            )
+
+
+class TestLookupDefsPatched:
+    def test_finds_def_in_diamond(self):
+        """Patched lookup_defs still finds M0.foo correctly."""
+        c, _ = build_diamond(3)
+        all_defs = []
+        lookup_defs_patched(c, "foo", all_defs, visited=set())
+        assert all_defs == ["M0.foo"]
+
+    def test_no_def_not_found(self):
+        """Patched lookup_defs returns empty when no such def."""
+        c, _ = build_diamond(2)
+        all_defs = []
+        lookup_defs_patched(c, "bar", all_defs, visited=set())
+        assert all_defs == []
+
+    def test_linear_calls_patched(self):
+        """Patched lookup_defs visits each node at most once."""
+        for d in range(1, 8):
+            c, m0 = build_diamond(d)
+            all_defs = []
+            visited = set()
+            lookup_defs_patched(c, "foo", all_defs, visited)
+            # M0 must appear in visited exactly once
+            assert id(m0) in visited
+            assert all_defs == ["M0.foo"]
+
+
+class TestSpeedupRatio:
+    def test_speedup_at_depth5(self):
+        """At D=5, unpatched visits M0 32x; patched visits M0 1x."""
+        d = 5
+        c, m0 = build_diamond(d)
+
+        # Unpatched
+        result_u, _ = ancestors_unpatched(c)
+        unpatched_visits = result_u.count(m0)
+
+        # Patched
+        result_p = ancestors_patched(c)
+        patched_visits = result_p.count(m0)
+
+        assert unpatched_visits == 2 ** d  # 32
+        assert patched_visits == 1
+        ratio = unpatched_visits / patched_visits
+        assert ratio >= 32, f"Expected >= 32x ratio, got {ratio}x"
+
+
+if __name__ == "__main__":
+    pytest.main([__file__, "-v"])