fix(core): Base only analysis clean - #361
Draft
Saloed wants to merge 23 commits into
Draft
Conversation
Adds a third access-path representation alongside Tree and Automata. A BaseOnly access is a packed Long carrying three positional slots (static / field / terminal) rather than a path tree, so a method summary family collapses to a bounded number of edges no matter how many concrete field chains reach it. The shared access layer gains the extension points BaseOnly needs: - MethodEdgesInitialToFinalApSet.add now returns every stored final whose metadata the insertion changed, not just the inserted pair. Tree already behaved this way; Cactus and Automata silently dropped deltas when a merged exclusion widened. - addAll inserts many premises against one conclusion without materialising a path edge per premise. - filterEdgesByFinalTo and collectSummariesByFinalTo thread an optional full final-fact pattern so an indexed storage can narrow candidates. - ExclusionSet.Concrete accepts a pluggable persistent accessor set and computes its hash lazily. ConcurrentReadSafeLong2ObjectMap and ConcurrentReadSafeLongSet are the primitive-long counterparts of the existing read-safe collections, for the packed-Long summary storages: one writer, many readers, no removals. Design and conformance notes live in docs/baseonly-*.md.
Several regression samples need Spring and Stirling types on the classpath but must not pull in the real frameworks. samples-dependency provides minimal stand-ins for the annotations and response types they reference.
Import reordering, a stray blank line and a missing trailing newline. No behaviour change; separated so the following commits contain only real edits.
The exception application graph treated every call expression inside a try region as a thrower and wired it to the region's catchers. Because the method instruction graph collapses normal and exceptional successors into one unlabelled edge set, a statement's normal-completion facts were then propagated into the handler. Observed on ThingsBoard: a catch parameter receiving the result of a call made inside the try, and the catch of pushEntityActionToRuleEngine retaining 1,543 entries while rejecting 37,713 duplicate additions. A catcher is now reachable only from explicit throw instructions and from try exits. This is an approximation change, not a complete fix: a mid-try call that genuinely throws no longer reaches the handler, which the new test asserts.
…gine runAnalysis gains an ap-mode argument and two post-analysis callbacks, and the harness exposes hooks for the rules provider, the unit resolver and the unroll strategy. Needed by the regression tests added later in this series, which have to run the same sample under Tree and BaseOnly and then read engine state.
Two rules reporting at the same sink must not collapse onto one partial fingerprint. Splits the SARIF result extraction out of generateSarifReport so a test can request fingerprint generation.
Zero-to-zero edges discover the call graph; fact edges propagate over it. Interleaving them means facts are propagated across a call graph that is still being discovered, and the same conclusions are re-derived as more callees appear. The per-method worklist now drains zero-to-zero edges first, and an analyzer with a pending zero-to-zero backlog sorts ahead of the rest of the unit runner queue. Ordering only; no edge is added or dropped.
… path Behaviour-preserving reductions on paths taken for every fact at every call statement: - TaintRulesStorage.getConfigForMethod was a @synchronized method on one shared object, serialising every analyzer thread's rule lookup. The per-method and per-pattern caches become concurrent maps and the monitor goes away; a negative result is now memoised in the same map instead of a side set. - The condition rewriter is constructed lazily and true conditions short-circuit, so a statement with no conditional rule no longer walks a rewrite. - ConditionSimplifier.mkFalse returns a shared instance instead of allocating. - The call flow function looks up cleaner and pass-through rules before building the evaluators, and returns early when there are none. - The summary rewriter returns the original edge by identity when the rewrite is provably identity. This matters beyond allocation: the engine's unchanged-edge fast paths compare edges by reference. - EvaluatedCleanAction drops its prev back-pointer, an unbounded chain retaining every intermediate cleaner state, and clearPosition de-duplicates its two branches.
…sible bridges Three changes to virtual dispatch resolution: - methodOverridesCache was keyed on the method alone, but its value depends on the constrained base class, so the first base class seen for a method won the cache for every later query. Now keyed on both. - A bridge method whose target's parameter type cannot match any call-site argument would always throw in its checkcast, so it is dropped from the candidate set. Targets generic hierarchies such as DaoUtil.convertDataList, which resolved 78 toData() targets on ThingsBoard. - A virtual call whose *declared* target is an Object method short-circuits to resolution failure. Previously this only applied when the resolved method was outside a known unit, so o.toString() on a project class entered that class's toString. Evidence: BaseSqlEntity#equals produced 11,540 summaries for 3,404 steps. This is a deliberate recall trade and is measured in the branch notes. Re-keying the cache is correct but on its own it makes Spring Data custom fragment implementations unreachable, and the old key was the only reason they worked. A fragment impl implements only the fragment interface (ProductRepoCustom -> ProductRepoCustomImpl); it never implements the repository interface the receiver is typed as (ProductRepo), and no class in the project implements that interface at all - Spring synthesises the proxy at runtime. Constrained by the repository interface, findOverrides therefore returns nothing, and the call to the fragment method resolves to failure. Under the old method-only key a lookup of the same method against the fragment interface, performed anywhere earlier in the run, populated the cache and the repository-typed query silently reused it - an order-dependent accident, not a rule. The fallback restores those targets deterministically: when a constrained lookup of an abstract interface method yields nothing, retry once against the method's own declaring interface, and only when that interface is itself in the project. Widening is thus bounded to interfaces the project defines - a library interface such as CrudRepository never gets widened to every implementor on the classpath.
Virtual dispatch resolution for one statement is a pure function of the method context, but the later shallow-scan pruning consults it on the hot path. Caches it on the analysis context, keyed by statement index, and cleared with the rest of the per-phase caches.
Four defects that only become reachable once analysis runs in more than one phase over the same engine: - resetEdgeProcessingStorage did not clear analyzerEnqueued, so an analyzer still marked enqueued when the runner queue was wiped never re-enqueued itself and silently dropped all further work for that method. - The summary serializer and its deserialised-summary cache were bound to the previous ap manager, whose representation the next phase cannot read. - Memory-pressure state lived on the long-lived manager rather than per run, so a phase that ended under pressure made the next phase declare OOM on its first GC notification. - A cancelled phase failed the enclosing coroutine scope. Cancelled now extends CancellationException so it cancels only its own coroutine. Also returns TIMEOUT with per-item placeholders when a phase is entered with a non-positive budget, instead of activating an already-expired cancellation.
Conductor timed out in trace resolution. Instrumenting one run showed the resolver was not revisiting entries but multiplying alternatives: 104,330 raw call choices became 8,602,938 resolved call-summary alternatives that projected onto only 115,295 distinct (statement, edges) continuations - a 74.6x multiplicity, with the same backward transfer repeated for each. Four quotients, all preserving the resolved trace set: - TraceEdges replaces the flat premise set with alternatives-per-final-fact, so one trace request stands for what used to be a cartesian product of summary traces. If the merged request exceeds the action budget the resolver falls back to resolving each exact premise cube, so nothing is lost. - Exclusion sets no longer distinguish graph nodes: traces are compared and cached after normalising every exclusion to the universe. - All start traces of one summary that differ only in their method-entry premise collapse onto one summary node plus one boundary node per entry, so caller search runs once per boundary instead of once per (summary, start) pair. - Start traces are memoised across field-generalisation-equivalent requests, and the least field-specific request is resolved first. Also: a summary whose start is already determined by its non-Zero premises skips the backward walk, concrete-field call summaries covered by an applicable wildcard are dropped before the weakest-entry antichain, and root filtering uses one reverse BFS instead of a forward BFS per root.
…a shallow pass
Splits the pipeline into prescan, shallow scan and full scan, and restricts the
expensive final pass to the rules a cheap pass proved actionable:
Prescan TreeApManager, no facts, harvests relevant rule ids
ShallowScan BaseOnlyApManager(fieldSensitive), forward analysis, vulnerability
confirmation, actionable-rule search
FullScan configured ap mode, restricted to the discovered rules
SelectedTaintRulesProvider (and its Go twin) decorate the real provider with
per-instruction rule buckets, so a source or sink rule is evaluated only at the
exact statement where the shallow scan proved it actionable, with actionsAfter
narrowed to the selected actions. Cleaners and pass-throughs deliberately keep
delegating to the full configuration, so no sanitizer is lost and the
over-approximation stays sound.
Actionable rules come from TraceActionSearcher, which walks the resolved
source-to-sink corridor and projects surviving entries to statement -> rule ->
actions. Each vulnerability gets a single wall-clock budget covering both trace
resolution and rule search; on exhaustion it is reported Unprocessed rather than
Failed, and falls back to the rules that actually fired during the shallow
forward pass. That fallback is a global over-approximation (on Conductor, 5,577
forward-only atoms against 1,039 in common), so it is narrowed three ways:
caller reachability to the sink method, taint-mark reachability through the
observed summary transitions, and the semgrep rule graph.
These six concerns are one commit because they are one change in the code:
selectPhase, the TaintAnalyzer driver and the manager interface each interleave
all of them line by line, and no ordering of them compiles on its own.
The shallow scan reads its ap mode from a new TaintAnalyzerOptions field,
shallowScanApMode, rather than hardcoding the field-sensitive BaseOnly manager,
and AnalysisTest.runAnalysis forwards a shallowApMode argument into it. Without
that seam the phase could only ever be exercised under its production default,
and every test below that depends on shallow-scan behaviour would be blind. The
default itself is BaseOnlyField, so production behaviour is unchanged. The seam
lands here rather than with the rest of the harness work because neither the
shallow scan nor ApMode.BaseOnlyField exists before this commit.
ApMode gains BaseOnly and BaseOnlyField here, so regression tests that have to
run under them can only be written from this commit onwards; the Spring Data
fragment test gains its BaseOnlyField case for that reason.
Generated Spring entry points all carried the handler's name and the description ()V, so two overloads of the same handler produced one synthetic method and one of them was lost from every name-and-descriptor lookup. Overloads now get a stable index suffix.
EntityActionService#pushEntityActionToRuleEngine is analyzed under about 110 distinct argument-type contexts on ThingsBoard, and constructActionData under 62. Zero and ClassStatic facts depend on neither the receiver nor the argument types, so every one of those contexts re-tabulated identical flows. During the shallow scan only, such facts are routed to the callee's EmptyMethodContext analyzer. A method context is left alone when it constrains a lambda class or a functional interface, because the call resolver excludes lambda classes from override enumeration and that constraint is the only evidence of the callee implementation. Applied symmetrically at call handling and at trace lookup; the backward resolver must normalise the same way or it cannot find the shared summaries. The call-handling half lives in NormalMethodAnalyzer.handleResolvedMethodCall: every resolved callee is registered with the unit-runner manager once, and the callee's method context is passed through overApproximateMethodContext before its entry points are enumerated, with "context independent" meaning a zero fact or a ClassStatic-based fact.
Under a field-insensitive representation many premises reach the same conclusion, and the analyzer re-evaluated the flow function once per premise. The BaseOnly worklist is now keyed by (statement, final fact) with the premises batched behind it, and flow functions can declare that their result does not depend on which premise carried the fact, in which case the transfer is computed once and fanned out over the group. Non-deterministic summary results are de-duplicated before dispatch, summary sequents are accumulated and dispatched once, and side-effect requirements publish only their delta.
The actionable-rule search re-derives the same values many times for the same statement and fact. Adds four memos, each a pure function of its key and each cleared with the surrounding per-phase state: per-method trace-resolver state (entry-edge presence, call-pass summaries, callee entry points, zero entry facts), the non-deterministic required-initials search keyed by edge-set version, per-statement summary handlers and prepared summaries, and the JVM trace preconditions. Removing all of these was measured on both benchmark projects: thingsboard 124s -> 159s, conductor 35s -> 41s, with identical finding sets. They stay. JIRMethodCallPrecondition memoises through the same cachedCallTracePrecondition entry as the sequent precondition; its fact and alias preconditions are folded into one addFactPreconditions helper so both paths share a single cached result.
…ter flows The shallow scan is the phase that decides which rules the full scan is allowed to see, so anything it fails to reach is silently unreportable. These tests pin the shapes that were observed to be at risk and that no existing suite covered. Class-static: a static field written in one method and read in another, and a Spring repository whose result reaches a sink through a static holder. Both are the pattern where a class-static fact has to survive an arbitrary call chain. Cross-entry-point and thread dispatch: a flow whose source and sink are reached from different Spring controller entry points, and one that crosses a Runnable handed to a thread, so the fact leaves the entry point that seeded it. Getter chains and collections: a taint that reaches the sink only through a collection element read back by a getter - the case a field-insensitive representation is most likely to lose. Shallow rule selection and return sinks: that a rule actionable only inside a narrowed selection still survives into the full scan, that a controller return value is treated as a sink, and that BaseOnly trace resolution can still resolve the corresponding traces backwards.
Adapted from upstream cbe3b3f ("fix(analyzer): Use dedicated storage for abstract static edges (#364)"). An edge whose initial and final access paths are both the depth-0 ClassStatic access carries no access-path information at all - only its exclusion set is ever interesting. Kept in the general initial-to-final storage, one such edge is recorded per distinct exclusion set per statement, and on a project with many static pseudo-variables that storage dominates the per-method edge set. AbstractStaticEdges replaces the whole family at a statement with a single exclusion set: adding an edge unions into it and reports a delta only when the union grows, and the read paths rebuild the (initial, final) pair from the manager's most-abstract access paths. addTaintedFactEdge and the class-static branch of addFactToFactSupports route to it via isAbstractStaticEdge, and the three collectApAtStatement overloads consult it before the general storage. Two adaptations were needed against upstream: - on this branch MethodEdgesInitialToFinalApSet.add returns a List of changed (initial, final) pairs rather than a nullable Pair, because BaseOnly layered summaries need every changed final, so the override returns a singleton list or an empty list instead of a value or null. - addFactToFactSupports does not exist upstream, so upstream could not route it. It is the second producer of abstract static edges here, and routing only addTaintedFactEdge would leave the two storages disagreeing about which edges exist; its ClassStatic depth-0 case now dispatches per initial fact.
Rules are resolved per (method, rule) and the resolved objects are retained for the lifetime of the analyzer, so every method that matches a rule gets its own copy of structurally identical values. Measured over 2000 resolved methods: ContainsMark duplicated 505x, CommonCondition$Atom 486x, TaintSinkMeta 6849x. Retained payload for the duplicated values dropped from 17,295,256 B to 53,520 B once they are shared. ResolvedRuleInterner is a ConcurrentHashMap-backed canonicaliser: intern for single values, internList for the lists that hang off every resolved rule (assign actions, tracked facts, pass-through and cleaner copies), and internCondition to canonicalise a condition bottom-up so equal subtrees are shared as well. One interner lives on TaintConfiguration and is handed to every MethodTaintConfigurationResolver, which is what makes the sharing cross-method rather than per-method. The enabler for the duplication is the exit-sink anyFunction() rule, which matches every method in the program and therefore resolves its meta and tracked facts once per method. TaintMarkManager becomes a ConcurrentHashMap. It is reached from the resolvers, which run concurrently, and a plain HashMap resized under concurrent getOrPut is a real corruption hazard rather than a theoretical one.
Runs the differential corpus through the real analyzer rather than the access -path unit tests: reference install, mutation and transfer fuzz samples, trace projection, resolution and shape fuzz samples, the kkFileView setter-identity regression, and the summary field-explosion sample. Each asserts the same source-to-sink flow under Tree and BaseOnly.
…nt removed Two documents. baseonly-clean-branch-selection.md is the per-change ranking the branch was distilled from: what is architecture, what is a measured win, what is a correctness fix, and what was stripped as telemetry. It also carries the measured baseline-vs-branch numbers and the ablations, including the correction that the telemetry strip was load artifact and not a speed-up. Two of its Tier 1 entries - shallow-scan statement collapsing and class-static call skipping - are no longer on the branch. They are moved into a Tier 6 section that records why, rather than edited out: the reasoning that promoted them on two benchmarks is worth keeping next to the evidence that removed them on twenty-eight. The changes made in their place are recorded as Tier 7. e2e-regression-2026-08-19.md is that evidence: 28 projects against the fork point, 23 genuinely lost findings, each traced to a specific commit. It is kept as it was written, with a status note and a rewritten "what to fix" section, so the analysis that drove the rewrite is not paraphrased after the fact. The commit hashes it cites belong to the pre-rewrite history and no longer resolve on this branch.
The shallow scan keeps four per-analyzer memoisation maps that are cleared only at phase transitions, so they accumulate for the whole phase. Measured at the point thingsboard aborts under a 12g heap, baseOnlyF2FTransfers held 2,815,052 entries against a live worklist of 10,321, and baseOnlyPreparedF2FSummaries held 27,438,544 IdentityHashMap slots. Together they were 38% of the 6.23 GB the shallow phase adds on top of its 5.16 GB baseline. Hold all four behind SoftReference, the mechanism JIRMethodAnalysisContext already uses for its call flow-function and summary-handler caches, so the collector can reclaim them under pressure and they are rebuilt on demand. All four are pure deterministic memos, which is what makes an unannounced drop safe. createFactToFactTransfer reaches only sequentFlowAssign or unchanged, and passes error lambdas for the refinement and side-effect callbacks. prepareFactToFactSummary and its ND sibling are rewrites rather than registrations: they delegate to summaryRewriter.rewriteSummaryFact, and createFactReader always copies before handing out a reader. Nothing depends on the identity of a cached value; the IdentityHashMap is a key structure only, and consumers dedup structurally. The negative half, unsupportedBaseOnlyF2FTransfers, is safe for a stronger reason: the only overriding implementation returns null on its first line, from a syntactic test on the instruction, so re-deriving that null executes nothing. Its key set is disjoint from the positive half by determinism of the function rather than by cache bookkeeping, so the two may be dropped independently and in any order. AnalysisUnitRunnerManager gains the RefManager its single implementation already declared, rather than threading one through BaseOnlyApManager's 48 construction sites. Measured on thingsboard, two runs per configuration. Under a 12g heap the analysis stops aborting: exit 253 with 13 findings becomes exit 0 with 14, and peak post-GC live set falls from 11073-11079 MB to 8447-8458 MB. Under 14g it falls from 12040-12071 MB to 8072-8082 MB. Wall clock is unchanged to slightly faster, because forced full collections drop from four or five to one. The recovered finding is unvalidated-redirect-in-spring-app at AdminController.java line 483, lost to the abort rather than to the analysis. The last two caches are noise on this benchmark: Edge$NDFactToFact has no live instances there at all, and the negative set is near-empty on JVM. They are converted for uniformity and for Go, where createFactToFactTransfer is the constant null default and the negative set therefore records every conclusion while saving no work. This buys headroom, not immunity. The fact explosion that fills these structures is untouched, and the depth-based throttle remains unreachable in BaseOnly mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Saloed
force-pushed
the
saloed/base-only-clean
branch
from
August 20, 2026 20:22
e2bd0f8 to
c3b6efe
Compare
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.