mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 19:59:26 +00:00
fix(cpg): a graph the frontend emptied must not read as a clean repository
Scanning a two-file WordPress slice returned 64 regions examined, zero
findings and zero degradations. The graph it examined had no methods in it.
php2cpg 4.0.623 feeds several files to one `php-parse` process and reads its
stdout and stderr merged. php-parse writes a `====> File <next>:` banner to
stderr the moment it starts the next file, while the previous file's JSON --
megabytes of it -- is still draining from a block-buffered stdout. The banner
lands inside the JSON, ujson reports
expected json value or } got "=" at index 5011645
and the frontend drops that file. Reproducible on real plugin code, and the
size that matters is one file's AST dump, not the repository's:
class.memberorder.php alone (1834 lines) -> 142KB CPG, no failures
the same file + a two-line file -> 5.7KB CPG, BOTH dropped
One oversized file empties the whole graph. `joern-parse` then does not
propagate any of the frontend's warnings: 13 lines of output, exit status 0,
"Successfully wrote graph". This is what made a 637-file plugin unscannable
and reported it as clean -- it was never a scale limit.
Two fixes, because the failure arrives on two different paths:
* `_unparsed_files` reads the per-file warnings the frontend does emit, and
the scan reports them as `files_unparsed`. This covers the direct-frontend
path where the warnings are visible.
* The batched query now carries a `__census__` of the graph -- method and
file counts -- under a key no request id can collide with, and a scan over
a graph with no methods degrades as `cpg_empty`. This covers joern-parse,
which shows nothing. It rides the batch rather than costing its own
invocation, because avoiding a JVM start is the entire reason batching
exists.
Neither invents a verdict. There is no rung for "the graph was empty" because
the ladder labels verdicts and none was produced, which is exactly why it has
to be a degradation.
pmpro two-file slice before: 0 findings, degradations=[]
after: 0 findings, degradations=[cpg_empty]
Gates byte-identical; 840 tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
37a6594703
commit
e0442d146c
@@ -22,6 +22,7 @@ from __future__ import annotations
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
import re
|
||||
import shutil
|
||||
import subprocess
|
||||
import tempfile
|
||||
@@ -94,6 +95,11 @@ class CpgResult:
|
||||
cpg_path: Path
|
||||
run: Callable[[str, Mapping[str, object]], object | None]
|
||||
run_batch: Callable[[str, Mapping[str, Mapping[str, object]]], dict[str, list[object]] | None] | None = None
|
||||
# Files the frontend read and could not turn into a graph. A CPG is not all-or-nothing: `php2cpg` logs a
|
||||
# warning per file it drops and still exits 0 with a graph that is missing them, so a scan over the
|
||||
# remainder is a scan of LESS CODE THAN IT WAS ASKED ABOUT, and silence about those files is the same
|
||||
# quiet failure as an empty query result. The names travel with the CPG so the driver can say so.
|
||||
unparsed: tuple[str, ...] = ()
|
||||
|
||||
|
||||
class CpgBackend(Protocol):
|
||||
@@ -144,33 +150,45 @@ class JoernBackend:
|
||||
cpg_path = scratch / "cpg.bin"
|
||||
command = [parse or "joern-parse", self._heap_flag(), str(root), "--output", str(cpg_path)]
|
||||
completed = self._run(command, timeout=self.build_timeout, cwd=scratch)
|
||||
unparsed = _unparsed_files(completed)
|
||||
if completed is None or completed.returncode != 0 or not cpg_path.is_file():
|
||||
detail = (completed.stderr or completed.stdout or "")[-400:] if completed is not None else "timeout"
|
||||
logger.warning("cpg build failed for %s: %s", root, detail)
|
||||
if not self._build_with_frontend(root, cpg_path, scratch, language):
|
||||
retried = self._build_with_frontend(root, cpg_path, scratch, language)
|
||||
if retried is None:
|
||||
return None
|
||||
unparsed = retried
|
||||
if unparsed:
|
||||
logger.warning("the frontend could not parse %d file(s) under %s: %s", len(unparsed), root, ", ".join(unparsed[:5]))
|
||||
return CpgResult(
|
||||
cpg_path=cpg_path,
|
||||
run=lambda query, params: self.query(cpg_path, query, params),
|
||||
run_batch=lambda query, requests: self.query_batch(cpg_path, query, requests),
|
||||
unparsed=unparsed,
|
||||
)
|
||||
|
||||
def _build_with_frontend(self, root: Path, cpg_path: Path, scratch: Path, language: str) -> bool:
|
||||
"""Retry the build through the language frontend alone. False when there is nothing to retry with."""
|
||||
def _build_with_frontend(self, root: Path, cpg_path: Path, scratch: Path, language: str) -> tuple[str, ...] | None:
|
||||
"""Retry the build through the language frontend alone.
|
||||
|
||||
Returns the files the frontend dropped -- possibly none -- or ``None`` when there is nothing to retry
|
||||
with or the retry failed too. An empty tuple and ``None`` are different answers here for the same
|
||||
reason they are everywhere else in this module: one means "built, and complete", the other means
|
||||
"could not build".
|
||||
"""
|
||||
frontend = _FRONTENDS.get(language.lower())
|
||||
if not frontend:
|
||||
return False
|
||||
return None
|
||||
binary = shutil.which(frontend)
|
||||
if binary is None:
|
||||
logger.warning("no frontend %s on PATH to retry the build for %s", frontend, root)
|
||||
return False
|
||||
return None
|
||||
completed = self._run([binary, self._heap_flag(), str(root), "-o", str(cpg_path)], timeout=self.build_timeout, cwd=scratch)
|
||||
if completed is None or completed.returncode != 0 or not cpg_path.is_file():
|
||||
detail = (completed.stderr or completed.stdout or "")[-400:] if completed is not None else "timeout"
|
||||
logger.warning("%s also failed for %s: %s", frontend, root, detail)
|
||||
return False
|
||||
return None
|
||||
logger.info("built the cpg for %s with %s after joern-parse failed", root, frontend)
|
||||
return True
|
||||
return _unparsed_files(completed)
|
||||
|
||||
def query_batch(self, cpg_path: Path, query: str, requests: Mapping[str, Mapping[str, object]]) -> dict[str, list[object]] | None:
|
||||
"""Run ONE script invocation carrying many requests, keyed back to their ids.
|
||||
@@ -261,6 +279,41 @@ class JoernBackend:
|
||||
return None
|
||||
|
||||
|
||||
# Every Joern frontend logs a dropped file the same way, at WARN, and then carries on:
|
||||
#
|
||||
# WARN AstCreationPass Failed to process '/abs/path/to/file.php'
|
||||
#
|
||||
# The quotes are part of the line, which is what makes this safely greppable rather than a guess at column
|
||||
# positions. Several passes report the same file, so the caller de-duplicates.
|
||||
_UNPARSED = re.compile(r"Failed to process '([^']+)'")
|
||||
|
||||
|
||||
def _unparsed_files(completed: subprocess.CompletedProcess[str] | None) -> tuple[str, ...]:
|
||||
"""The files the frontend read and dropped, in first-seen order.
|
||||
|
||||
This exists because a Joern frontend fails a file WITHOUT failing the build. `php2cpg` 4.0.623 feeds
|
||||
several files to one `php-parse` process and reads its stdout and stderr merged; php-parse writes a
|
||||
`====> File <next>:` banner to stderr the moment it starts the next file, while the previous file's JSON
|
||||
-- megabytes of it -- is still draining from a block-buffered stdout. The banner lands inside the JSON,
|
||||
ujson reports `expected json value got "="`, and the frontend drops that file. Reproducibly, on real
|
||||
plugin code:
|
||||
|
||||
one 1834-line file alone -> a 142KB CPG, no failures
|
||||
the same file plus a two-line file -> a 5.7KB CPG, BOTH files dropped, exit status 0
|
||||
|
||||
So the size that matters is one file's AST dump, not the repository's, and one oversized file can empty
|
||||
the whole graph. Before this, that arrived as a scan reporting no findings and no degradations -- a clean
|
||||
bill of health over a graph containing nothing at all.
|
||||
"""
|
||||
if completed is None:
|
||||
return ()
|
||||
seen: dict[str, None] = {}
|
||||
for stream in (completed.stdout or "", completed.stderr or ""):
|
||||
for match in _UNPARSED.finditer(stream):
|
||||
seen.setdefault(match.group(1), None)
|
||||
return tuple(seen)
|
||||
|
||||
|
||||
def _render(value: object) -> str:
|
||||
"""CPGQL parameters are strings; a sequence becomes a comma-separated list the script splits."""
|
||||
if isinstance(value, str):
|
||||
|
||||
@@ -73,7 +73,13 @@
|
||||
def field(name: String): String = req.obj.get(name).map(_.str).getOrElse("")
|
||||
id -> ujson.Arr(rowsFor(field("settings"), field("function"), field("file")): _*)
|
||||
}
|
||||
println(ujson.write(ujson.Obj.from(answers)))
|
||||
// A census of the graph, under a key no request id can collide with (ids are numbers). A frontend can
|
||||
// fail every file and STILL exit 0 with a valid, empty CPG -- `joern-parse` does not even propagate the
|
||||
// per-file warnings -- so "no rows" and "no graph" are indistinguishable to the driver without this.
|
||||
// It rides the batch rather than costing its own invocation, because JVM startup is what this whole
|
||||
// batching design exists to avoid.
|
||||
val census = ujson.Arr(ujson.Obj("methods" -> cpg.method.size.toString, "files" -> cpg.file.size.toString))
|
||||
println(ujson.write(ujson.Obj.from(answers.toSeq :+ ("__census__" -> census))))
|
||||
} else {
|
||||
println(ujson.write(ujson.Arr(rowsFor(settings, function, file): _*)))
|
||||
}
|
||||
|
||||
@@ -185,7 +185,13 @@
|
||||
): _*
|
||||
)
|
||||
}
|
||||
println(ujson.write(ujson.Obj.from(answers)))
|
||||
// A census of the graph, under a key no request id can collide with (ids are numbers). A frontend can
|
||||
// fail every file and STILL exit 0 with a valid, empty CPG -- `joern-parse` does not even propagate the
|
||||
// per-file warnings -- so "no rows" and "no graph" are indistinguishable to the driver without this.
|
||||
// It rides the batch rather than costing its own invocation, because JVM startup is what this whole
|
||||
// batching design exists to avoid.
|
||||
val census = ujson.Arr(ujson.Obj("methods" -> cpg.method.size.toString, "files" -> cpg.file.size.toString))
|
||||
println(ujson.write(ujson.Obj.from(answers.toSeq :+ ("__census__" -> census))))
|
||||
} else {
|
||||
println(ujson.write(ujson.Arr(rowsFor(operations, dischargers, function, file, operationRequires, dischargersByKind): _*)))
|
||||
}
|
||||
|
||||
@@ -280,7 +280,13 @@
|
||||
): _*
|
||||
)
|
||||
}
|
||||
println(ujson.write(ujson.Obj.from(answers)))
|
||||
// A census of the graph, under a key no request id can collide with (ids are numbers). A frontend can
|
||||
// fail every file and STILL exit 0 with a valid, empty CPG -- `joern-parse` does not even propagate the
|
||||
// per-file warnings -- so "no rows" and "no graph" are indistinguishable to the driver without this.
|
||||
// It rides the batch rather than costing its own invocation, because JVM startup is what this whole
|
||||
// batching design exists to avoid.
|
||||
val census = ujson.Arr(ujson.Obj("methods" -> cpg.method.size.toString, "files" -> cpg.file.size.toString))
|
||||
println(ujson.write(ujson.Obj.from(answers.toSeq :+ ("__census__" -> census))))
|
||||
} else {
|
||||
println(
|
||||
ujson.write(ujson.Arr(rowsFor(sources, sinks, sanitizers, function, parameterSources, file, callDepth, boundedSinks): _*))
|
||||
|
||||
@@ -126,6 +126,16 @@ def scan_repository(
|
||||
degradations=({"stage": "model", "reason": "cpg_build_failed"},),
|
||||
)
|
||||
|
||||
# A CPG the frontend built with files missing is not the repository the caller asked about, and no
|
||||
# verdict over it can say anything about the code that was dropped. There is no rung for that, because
|
||||
# the ladder labels verdicts and none was ever produced here -- so it is a degradation, named, with the
|
||||
# files listed. See `cpg.backend._unparsed_files` for how a frontend drops a file while exiting 0.
|
||||
if getattr(cpg, "unparsed", ()): # a backend need not carry the field
|
||||
unparsed = tuple(cpg.unparsed)
|
||||
degradations.append(
|
||||
{"stage": "model", "reason": "files_unparsed", "count": len(unparsed), "files": [Path(name).name for name in unparsed[:20]]}
|
||||
)
|
||||
|
||||
# Count calls here rather than reading a client's own meter: the budget is this driver's contract and
|
||||
# must hold for any client, including one that keeps no usage log.
|
||||
counted = _CountingClient(client) if client is not None else None
|
||||
@@ -150,6 +160,7 @@ def scan_repository(
|
||||
# the queries themselves are milliseconds once the CPG is loaded.
|
||||
rows_by_id: dict[str, list[object]] = {}
|
||||
per_kind: dict[str, float] = {}
|
||||
census_reported = False
|
||||
query_started = time.monotonic()
|
||||
for kind, requests in _grouped(work).items():
|
||||
kind_started = time.monotonic()
|
||||
@@ -162,6 +173,18 @@ def scan_repository(
|
||||
# failed at CPG load and the scan reported 500 regions examined with nothing found.
|
||||
degradations.append({"stage": "model", "reason": "query_failed", "kind": kind, "requests": len(requests)})
|
||||
else:
|
||||
# The graph census rides the batch under a reserved key. A frontend that fails every file
|
||||
# still exits 0 with a valid CPG containing nothing, and `joern-parse` does not propagate
|
||||
# its warnings, so without this a scan of an EMPTY graph is indistinguishable from a scan
|
||||
# that found nothing -- which is how a two-file WordPress slice reported a clean bill of
|
||||
# health over a 12KB graph with no methods in it at all.
|
||||
census = answered_batch.pop("__census__", None)
|
||||
if census and not census_reported:
|
||||
entry = census[0] if isinstance(census[0], Mapping) else {}
|
||||
methods = int(str(entry.get("methods", "0")) or 0)
|
||||
if methods == 0:
|
||||
degradations.append({"stage": "model", "reason": "cpg_empty", "files": int(str(entry.get("files", "0")) or 0)})
|
||||
census_reported = True
|
||||
rows_by_id.update(answered_batch)
|
||||
else: # a backend without batching still works, one call at a time
|
||||
for rid, params in requests.items():
|
||||
|
||||
Reference in New Issue
Block a user