Skip to content

Commit 10617e7

Browse files
committed
fix(go): expand each node once in _reach, not once per predecessor
Found by evaluating against the analyzer's own real-app corpus. _reach built each level as a comprehension and updated `seen` afterwards, so `seen` did not yet contain the level being built: a node with k predecessors in the current frontier was appended k times and the next level expanded every copy. The frontier tracked the number of paths rather than the number of nodes. The answer was never wrong -- a set cannot hold duplicates, and over 250 memos ports the reachable set is identical at every depth -- so only cost revealed it. On memos (208,293 source nodes, 743,154 edges): 151,417,783 frontier expansions at depth 8 against 670,450, 83.65s against 0.11s, and unbounded it did not terminate. depth=None is the default on paths_between, flows_to_call, flows_to_argument and taint, so the unusable case was the default one. After the fix, memos answers in 17s end to end: 11,477 ports swept at a max of 0.1s each, RSS flat at 3.4 GB, and taint finds 40 paths into database/sql.DB.QueryContext -- the HTTP-to-SQL-through-an-interface flow that app is in the corpus for. Pinned by counting expansions rather than by timing or frontier size: the committed fixture is too small to blow up, so the test wraps the adjacency and asserts each node is looked up at most once. It fails on the old walk. Refs #436
1 parent badc527 commit 10617e7

2 files changed

Lines changed: 80 additions & 2 deletions

File tree

‎cldk/analysis/go/codeanalyzer/codeanalyzer.py‎

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -730,12 +730,33 @@ def _reach(self, ref: str, direction: str, depth: int | None) -> set:
730730
Level-by-level rather than a plain stack, because ``depth`` is a hop budget and a
731731
depth-first walk cannot count hops without revisiting. Shared by the slices and by the two
732732
flow predicates, so "reachable" means one thing on this backend.
733+
734+
**A node enters the next frontier once, which is the whole point of adding to ``seen``
735+
inside the loop rather than after it.** The obvious spelling builds the level as a
736+
comprehension and updates ``seen`` afterwards::
737+
738+
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
739+
seen.update(nxt)
740+
741+
That is correct but not usable: ``seen`` does not yet contain the level being built, so a
742+
node with *k* predecessors in the current frontier is appended *k* times and the next level
743+
expands every copy. The frontier then tracks the number of **paths** rather than the number
744+
of nodes. Measured on memos (208,293 source nodes, 743,154 edges), over 250 ports: identical
745+
answers at every depth — duplicates cannot change a set — but 151,417,783 frontier
746+
expansions at depth 8 against 670,450, which is 83.65s against 0.11s, and unbounded it does
747+
not terminate at all. Since ``depth=None`` is the default on
748+
:meth:`paths_between`, :meth:`flows_to_call`, :meth:`flows_to_argument` and :meth:`taint`,
749+
the unusable case was the default one.
733750
"""
734751
edges = self._sdg[0][direction]
735752
seen, frontier, hops = {ref}, [ref], 0
736753
while frontier and (depth is None or hops < depth):
737-
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
738-
seen.update(nxt)
754+
nxt = []
755+
for src in frontier:
756+
for d in edges.get(src, ()):
757+
if d not in seen:
758+
seen.add(d)
759+
nxt.append(d)
739760
frontier = nxt
740761
hops += 1
741762
return seen

‎tests/analysis/go/test_go_dataflow.py‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
"""
1717

1818
import json
19+
from collections import Counter
1920
from pathlib import Path
2021
from unittest.mock import MagicMock
2122

@@ -382,6 +383,62 @@ def test_max_paths_caps_witnesses_and_says_so(httpsql):
382383
assert len(capped.paths) <= 1
383384

384385

386+
def test_reach_expands_each_node_exactly_once(backend):
387+
"""``_reach`` must add to ``seen`` *inside* the level, not after it.
388+
389+
The obvious spelling builds the level as a comprehension and updates ``seen`` afterwards, so
390+
``seen`` does not yet contain the level being built: a node with k predecessors in the current
391+
frontier is appended k times, and the next level expands every copy. The frontier then tracks
392+
the number of **paths** rather than the number of nodes.
393+
394+
Only cost reveals it, because a set cannot hold duplicates and the answer stays correct —
395+
measured on the memos corpus (208,293 source nodes, 743,154 edges), 151,417,783 frontier
396+
expansions at depth 8 against 670,450, 83.65s against 0.11s, and unbounded it never terminated.
397+
``depth=None`` is the default on ``paths_between`` / ``flows_to_*`` / ``taint``, so the unusable
398+
case was the default one.
399+
400+
Pinned by counting expansions rather than by timing, which would be flaky, and rather than by
401+
frontier size, which this fixture is too small to blow up: the adjacency is wrapped so every
402+
lookup is recorded, and a correct walk looks each node up at most once.
403+
"""
404+
forward = backend._sdg[0]["forward"]
405+
seeds = [r for r in backend._port_refs if forward.get(r)]
406+
assert seeds, "fixture has no port with an outgoing edge"
407+
408+
class Counting(dict):
409+
def __init__(self, inner):
410+
super().__init__(inner)
411+
self.fetches = Counter()
412+
413+
def get(self, key, default=None):
414+
self.fetches[key] += 1
415+
return super().get(key, default)
416+
417+
for ref in seeds:
418+
counting = Counting(forward)
419+
backend._sdg[0]["forward"] = counting
420+
try:
421+
backend._reach(ref, "forward", None)
422+
finally:
423+
backend._sdg[0]["forward"] = forward
424+
repeats = {k: n for k, n in counting.fetches.items() if n > 1}
425+
assert not repeats, f"{ref}: {len(repeats)} node(s) expanded more than once, e.g. {next(iter(repeats.items()))}"
426+
427+
428+
def test_reach_is_monotone_and_terminates_unbounded(backend):
429+
"""An unbounded reach must terminate and contain every bounded one.
430+
431+
The frontier bug made the unbounded case non-terminating on a real application while every
432+
bounded case still answered, so a suite that only ever passed a `depth` would not have seen it.
433+
"""
434+
for ref in [r for r in backend._port_refs if backend._sdg[0]["forward"].get(r)][:12]:
435+
unbounded = backend._reach(ref, "forward", None)
436+
for depth in (1, 2, 3):
437+
assert backend._reach(ref, "forward", depth) <= unbounded, f"{ref}: depth={depth} escapes the unbounded reach"
438+
# And the walk is a fixed point: one more hop than the graph's diameter adds nothing.
439+
assert backend._reach(ref, "forward", len(unbounded) + 1) == unbounded
440+
441+
385442
def test_the_two_slices_are_complementary(httpsql):
386443
"""Forward is what a value affects, backward what affects it. The sink's port must be in the
387444
source's forward slice, and symmetrically."""

0 commit comments

Comments
 (0)