mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 19:59:26 +00:00
fix(scan): itemise change context only in the files a change touched
The PHP transfer replay (pre-push 13.3) spent its whole 7,200 s case in the head side's evidence pass. A probe on one PMPro graph named the cause: not compute -- context adds ~20% engine time -- but OUTPUT. Each evidence request listed every method its scope reached, up to 1,845 rows; 200 requests into the bundled Braintree SDK returned 52,831 rows, 9.4 MB, against 211 rows without context, over 22,125 requests in a 3 GB container. `affected_context` only ever attaches a location in a changed file, and every other row was checked (real file under the root, usable extent) and discarded. The scan now sends the changed files; the engine itemises those and folds the rest into one `context_elsewhere` row carrying exactly what that check reads. With no safe file list the engine itemises everything, as before. Equivalence: unit controls over valid, missing-file, no-extent and outside-root cases, with and without a changed-file row; a real-Joern control; and on PMPro itself, 2,440 relationships and zero gaps decided identically from both forms, 52,831 rows down to 3,241 with the most referenced file as the change. Probe: benchmarks/push/context_cost_probe.py. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01EmM1j46qgKMh3zqD1QMwEo
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
cda679cb3b
commit
3c195c9369
@@ -181,7 +181,7 @@ FRONTEND_BUILD_ATTEMPTS = 4
|
||||
# across 2,500 requests made a 42.5MB request file of which 41.7MB was the same string over and over. Joern
|
||||
# parsed that with ujson inside a 2GB heap and the whole batch died in ForkJoinPool, which the driver
|
||||
# correctly reported as `query_failed` for all 2,500 regions -- a whole-repository scan that decided nothing.
|
||||
_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply")
|
||||
_SHARED_REQUEST_FIELDS = ("hookCallbacks", "dispatchApply", "contextFilter", "contextPaths")
|
||||
|
||||
Runner = Callable[..., subprocess.CompletedProcess[str]]
|
||||
|
||||
|
||||
@@ -52,6 +52,8 @@
|
||||
evidenceOnly: String = "false",
|
||||
hookCallbacks: String = "",
|
||||
dispatchApply: String = "",
|
||||
contextFilter: String = "",
|
||||
contextPaths: String = "",
|
||||
requests: String = "",
|
||||
requestsFile: String = ""
|
||||
) = {
|
||||
@@ -96,7 +98,9 @@
|
||||
fileS: String,
|
||||
depthS: String,
|
||||
boundedS: String,
|
||||
contextS: String = ""
|
||||
contextS: String = "",
|
||||
contextFilterS: String = "",
|
||||
contextPathsS: String = ""
|
||||
): List[ujson.Obj] = {
|
||||
|
||||
// Word-boundary matching, never substring. `resolveUrl` contains `resolve`, so a bare-substring sanitizer
|
||||
@@ -717,21 +721,45 @@
|
||||
"startLine" -> m.lineNumber.getOrElse(-1), "endLine" -> m.lineNumberEnd.getOrElse(-1),
|
||||
"relationship" -> kind)
|
||||
val scoped = cpg.method.filterNot(_.isExternal).filter(m => inScope(m)).l
|
||||
val methods = scoped.map(m => location(m, if (labeledMethods.exists(_.id == m.id)) "entry" else "callee"))
|
||||
// Field carrying is deliberately same-file in the existing abstraction. Retain
|
||||
// its file context conservatively, never claim every method carries the value.
|
||||
val fieldFiles = if (fieldCarriedKind.isEmpty) Set.empty[String] else
|
||||
cpg.call.nameExact(FIELD_ACCESS).filter(c => inScope(c.method)).l.map(_.method.filename).toSet
|
||||
val fields = fieldFiles.toList.flatMap(f => methodsIn(f).filterNot(_.isExternal).map(m => location(m, "field")))
|
||||
val callbacks = if (hookApply.isEmpty) Nil else cpg.call.filter(c => hookApply.contains(c.name))
|
||||
.filter(c => inScope(c.method)).l.flatMap { call =>
|
||||
val hook = call.argument.l.headOption.map(a => unquote(a.code)).getOrElse("")
|
||||
hookTable.getOrElse(hook, Nil).flatMap(name => cpg.method.nameExact(name).filterNot(_.isExternal).l)
|
||||
}.distinct
|
||||
val hooks = callbacks.map(m => location(m, "hook"))
|
||||
// Export sanitizer locations as guard context, not proof of discharge. Removed
|
||||
// guards are retained through base spans and unchanged-line correspondence.
|
||||
val guards = scoped.filter(_.ast.l.exists(sanitizesHere)).map(m => location(m, "guard"))
|
||||
val located: List[(io.shiftleft.codepropertygraph.generated.nodes.Method, String)] =
|
||||
scoped.map(m => m -> (if (labeledMethods.exists(_.id == m.id)) "entry" else "callee")) ++
|
||||
fieldFiles.toList.flatMap(f => methodsIn(f).filterNot(_.isExternal).map(_ -> "field")) ++
|
||||
callbacks.map(_ -> "hook") ++
|
||||
scoped.filter(_.ast.l.exists(sanitizesHere)).map(_ -> "guard")
|
||||
// ONLY THE CHANGED FILES are itemised when the driver says which they are. The consumer attaches a
|
||||
// location to a change only when the location's file is one the change touched, and discards every
|
||||
// other row after checking it names a real file with a sane extent. Itemised anyway, a request whose
|
||||
// scope reaches a vendored SDK returned 1,845 rows, 200 of them 9.4 MB, and the whole-repository pass
|
||||
// on a WordPress plugin never finished a push. Everything else collapses into ONE row carrying exactly
|
||||
// what that check reads: the distinct files and how many locations had no usable extent.
|
||||
val itemise = contextFilterS != "true"
|
||||
// Newline-separated: a path may hold a comma, and the driver never filters when one holds a newline.
|
||||
val changedFiles = contextPathsS.split("\n").map(_.trim).filter(_.nonEmpty).toList
|
||||
def touched(m: io.shiftleft.codepropertygraph.generated.nodes.Method): Boolean =
|
||||
itemise || changedFiles.exists(p => m.filename == p || m.filename.endsWith("/" + p))
|
||||
val (kept, elsewhere) = located.partition { case (m, _) => touched(m) }
|
||||
val locations = kept.map { case (m, relationship) => location(m, relationship) }
|
||||
val elsewhereRows =
|
||||
if (elsewhere.isEmpty) Nil
|
||||
else {
|
||||
val unusable = elsewhere.count { case (m, _) =>
|
||||
val start = m.lineNumber.getOrElse(-1)
|
||||
start < 1 || m.lineNumberEnd.getOrElse(-1) < start
|
||||
}
|
||||
List(ujson.Obj("kind" -> "context_elsewhere", "locations" -> elsewhere.size, "unusableExtents" -> unusable,
|
||||
"paths" -> ujson.Arr.from(elsewhere.map(_._1.filename).distinct.sorted)))
|
||||
}
|
||||
// Reachability remains bounded. External/dynamic calls and calls outside the
|
||||
// current reachability set cannot establish a complete negative for a change.
|
||||
// A modeled argument does not summarize its consumer: unknown(req.body),
|
||||
@@ -761,7 +789,7 @@
|
||||
else if (missing)
|
||||
List(ujson.Obj("kind" -> "context_boundary", "reason" -> "dynamic_external_or_depth_context_unresolved:contributor-scan"))
|
||||
else Nil
|
||||
ujson.Obj("kind" -> "context_summary") :: (methods ++ fields ++ hooks ++ guards ++ boundaries)
|
||||
ujson.Obj("kind" -> "context_summary") :: (locations ++ elsewhereRows ++ boundaries)
|
||||
}
|
||||
return summary :: (perSink ++ contextRows)
|
||||
}
|
||||
@@ -913,7 +941,9 @@
|
||||
field("file"),
|
||||
field("callDepth"),
|
||||
field("boundedSinks"),
|
||||
field("contextEvidence")
|
||||
field("contextEvidence"),
|
||||
if (field("contextFilter").nonEmpty) field("contextFilter") else contextFilter,
|
||||
if (field("contextPaths").nonEmpty) field("contextPaths") else contextPaths
|
||||
): _*
|
||||
)
|
||||
println(ujson.write(ujson.Obj("id" -> id, "rows" -> rows, "ms" -> ((System.nanoTime() - started) / 1000000L).toDouble)))
|
||||
|
||||
@@ -194,6 +194,37 @@ AMBIGUOUS_CORRESPONDENCE = "ambiguous_line_correspondence"
|
||||
REACHABILITY_BOUNDED = "dynamic_external_or_depth_context_unresolved"
|
||||
|
||||
|
||||
def _elsewhere_unusable(row: Mapping[str, object], root: Path, checked: dict[str, bool]) -> bool:
|
||||
"""Whether a summarised set of locations holds one the itemised check would have rejected.
|
||||
|
||||
The same rejections as an itemised `context_method` row: a file that is not a real file under the root,
|
||||
or an extent that is missing or inverted. Each file is checked once per call, because the summary for
|
||||
every family of a region names the same files.
|
||||
"""
|
||||
try:
|
||||
if int(str(row.get("unusableExtents", 0))) > 0:
|
||||
return True
|
||||
except ValueError:
|
||||
return True
|
||||
paths = row.get("paths")
|
||||
if not isinstance(paths, list):
|
||||
return True
|
||||
resolved_root = root.resolve()
|
||||
for raw in paths:
|
||||
text = str(raw)
|
||||
if text not in checked:
|
||||
try:
|
||||
path = Path(text)
|
||||
absolute = path if path.is_absolute() else root / path
|
||||
absolute.resolve().relative_to(resolved_root)
|
||||
checked[text] = absolute.is_file()
|
||||
except (ValueError, OSError):
|
||||
checked[text] = False
|
||||
if not checked[text]:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def affected_context(
|
||||
context: ChangeContext,
|
||||
questions: Sequence[QuestionIdentity],
|
||||
@@ -224,6 +255,7 @@ def affected_context(
|
||||
# ambiguous anchor remains uncomparable.
|
||||
inherited_gap = any(not gap.endswith(":" + AMBIGUOUS_CORRESPONDENCE) for gap in gaps)
|
||||
renamed = {context.decode_path(r.base_path): context.decode_path(r.head_path) for r in context.renames}
|
||||
checked: dict[str, bool] = {}
|
||||
for identity in questions:
|
||||
if execution_budget is not None and time.monotonic() >= execution_budget.deadline_monotonic:
|
||||
gaps.append("change_context_deadline_exhausted")
|
||||
@@ -233,7 +265,9 @@ def affected_context(
|
||||
local_gaps = []
|
||||
if not any(r.get("kind") == "context_summary" for r in rows):
|
||||
local_gaps.append("context_projection_unavailable:contributor-scan")
|
||||
elif not any(r.get("kind") == "context_method" for r in rows) and not any(r.get("kind") == "context_boundary" for r in rows):
|
||||
elif not any(r.get("kind") in ("context_method", "context_elsewhere") for r in rows) and not any(
|
||||
r.get("kind") == "context_boundary" for r in rows
|
||||
):
|
||||
# Only when the query offered no account of its own. A query that already said why its
|
||||
# scope is empty -- a named function this file does not define, most often -- has given
|
||||
# the specific reason, and adding a generic one on top buries it.
|
||||
@@ -243,6 +277,13 @@ def affected_context(
|
||||
if row.get("kind") == "context_boundary":
|
||||
reason = str(row.get("reason", "context_unresolved"))
|
||||
(recorded_only if reason.startswith(REACHABILITY_BOUNDED) else local_gaps).append(reason)
|
||||
if row.get("kind") == "context_elsewhere":
|
||||
# Locations in files the change did not touch, summarised by the engine. None of them can be
|
||||
# attached to a change, so the only thing to do with them is what was always done before they
|
||||
# were discarded: check each names a real file under the root and had a usable extent.
|
||||
if _elsewhere_unusable(row, root, checked):
|
||||
local_gaps.append("context_location_unavailable")
|
||||
continue
|
||||
if row.get("kind") != "context_method":
|
||||
continue
|
||||
path = Path(str(row.get("path", "")))
|
||||
|
||||
@@ -340,6 +340,7 @@ def _scan_repository_impl(
|
||||
hooks,
|
||||
degradations=degradations,
|
||||
context_rows=context_rows if change_context is not None else None,
|
||||
context_files=_changed_files(change_context) if change_context is not None else None,
|
||||
unit=unit,
|
||||
)
|
||||
per_kind["evidence"] = round(time.monotonic() - tier_started, 2)
|
||||
@@ -878,6 +879,17 @@ def _sink_weights(
|
||||
return weights
|
||||
|
||||
|
||||
def _changed_files(context: ChangeContext) -> tuple[str, ...] | None:
|
||||
"""The head-side files a context location can be attached to, exactly the set `affected_context` reads.
|
||||
|
||||
``None`` when one cannot be sent intact -- a newline in a path -- so the engine itemises everything.
|
||||
"""
|
||||
changed = {context.decode_path(p) for p in context.changed_paths} | {context.decode_path(r.head_path) for r in context.renames}
|
||||
if any("\n" in path or not path for path in changed):
|
||||
return None
|
||||
return tuple(sorted(changed))
|
||||
|
||||
|
||||
def _request_families(work: Sequence[tuple[str, ScanRegion, ArbiterSpec]]) -> dict[str, str]:
|
||||
"""The family of each request id: the unit the sizer learns a cost rate for."""
|
||||
return {rid: str(getattr(spec, "family", "")) for rid, _region, spec in work}
|
||||
@@ -1224,6 +1236,7 @@ def _evidence_pass(
|
||||
*,
|
||||
degradations: list[Mapping[str, object]] | None = None,
|
||||
context_rows: dict[QuestionIdentity, list[Mapping[str, object]]] | None = None,
|
||||
context_files: tuple[str, ...] | None = None,
|
||||
unit: str = "repository",
|
||||
) -> tuple[dict[tuple[str, str, str], Evidence], int]:
|
||||
"""The evidence vector of every taint pair in ``work``, keyed by (path, function, family).
|
||||
@@ -1234,10 +1247,13 @@ def _evidence_pass(
|
||||
grouped_taint = _grouped(list(work), hook_callbacks=hooks).get("taint", {})
|
||||
if not grouped_taint:
|
||||
return {}, 0
|
||||
requests = {
|
||||
rid: {**dict(params), "evidenceOnly": "true", **({"contextEvidence": "true"} if context_rows is not None else {})}
|
||||
for rid, params in grouped_taint.items()
|
||||
}
|
||||
# With change context, the engine itemises locations only in the files the change touched and summarises
|
||||
# the rest, because those are the only locations `affected_context` can attach. `None` means the change's
|
||||
# files cannot be named safely, and then every location is itemised as before.
|
||||
context = {"contextEvidence": "true"} if context_rows is not None else {}
|
||||
if context and context_files is not None:
|
||||
context.update({"contextFilter": "true", "contextPaths": "\n".join(context_files)})
|
||||
requests = {rid: {**dict(params), "evidenceOnly": "true", **context} for rid, params in grouped_taint.items()}
|
||||
# Portioned by request count, because evidence has no per-request weight yet -- it is the pass that
|
||||
# PRODUCES the weights. It is cheap per request, 17,700 in 44 s, and it was still one all-or-nothing
|
||||
# batch: a 22,125-request evidence pass on a plugin's push transaction exceeded its ceiling under load and
|
||||
|
||||
Reference in New Issue
Block a user