Repository navigation
fix(analyzer): resolve alias-introduced operands at the one place resolver - #121
Merged
Merged
Conversation
…seam
A function whose return type is a bare type variable can hand back a value
it was given rather than a fresh one, and the caller's obligation was not
moved when it did: `let h2 = id(h)` armed a second independent obligation
on the same runtime value, so `close(h)` and `close(h2)` both type-checked
and double-freed.
Resolve what a call's result aliases in one place. `_return_origin.py`
summarises, per callable, which parameters a bare-type-variable return can
originate from, failing closed to every parameter when the body cannot be
resolved; `_e3.py` adds the args-based call-site backstop for the callables
the summary cannot reach. `_call_result_alias_args` is the single seam both
feed, and `_move_linear_operand` consults it, so every operand position that
already moves a bare identifier now moves through a laundering call as well
and a chained `id(id(c))` recurses.
Three structural changes ride with it because the fix needs them:
- `_discharge_return_operand` is extracted from `_check_return`, giving the
return operand one seam instead of a branch list inline in the statement
checker;
- the `consume self` receiver routes every operand shape through the move
seam rather than an open-coded `Ident`/`FieldAccess` pair, which let an
unlisted receiver shape fall through silently: `probes/m37` binds the
receiver through a call and printed `closing 7` twice on all three
backends;
- `_callables.py` holds the `("fun", name)` / `("method", ty, name)` /
lambda summary-key constructors that `_ifc*` and the new return-origin
table both key on, so the two do not spell the same tuple independently.
Ten corpus programs move, all OK to ERR. Eight are genuine double-frees on
at least two backends. The remaining two, `atk7/ho_combinator_owned` and
`atk7/ho_trait_transform`, are correct programs refused by the fail-closed
default on a callee whose origin cannot be resolved; that cost was accepted
when the fail-closed direction was chosen.
…esolver
A rule that enforces "used at most once" must first resolve its operand to a
place, and every such rule recognised exactly two spellings: a bare `Ident`
and an `Ident`-rooted `FieldAccess`. The language has two more ways to name
an existing value, and they compose: a pattern BINDING (a `match` arm binder,
a destructuring `let`) and a SELECTION (an `if` / `match` expression yielding
an existing place without naming it). Both were invisible to every
position-driven rule while the value still flowed at runtime, so the direct
spelling of a double-free was rejected and the alias spelling of the same
program was accepted.
Record two binding-time facts, each with exactly one producer, and feed them
to `_path_of`, the resolver those rules already share:
- `_linear_alias` maps a pattern binder to the place it VIEWS, produced only
by `_linear_bind_pattern_view`, saved and restored around each arm like the
arm's own name scope. It is not a second obligation: the obligation stays
under the scrutinee's name.
- `_selection_place` maps an if/match node to `("fresh",)`,
`("place", place, ty)` or `("reject",)`, produced only by the two selection
checkers at the point where the arms have been checked and their binders
are still in scope. It carries the resolved place STRING, never a node: a
node only means the right thing while the arm scope is live, and the move
position that consumes a selection runs after that scope has popped.
With the operand resolved, the position-independent bar
`_check_linear_conditional_alias` and its two helpers are deleted. It could
not tell a read from a move or one place from two, so it rejected correct
programs; the rules it stood in for now fire individually and say which value
is at fault.
The rules folded onto the resolved place rather than growing a case each:
- `_move_linear_place` is the one body behind all eleven move positions,
splitting on the place rather than the operand's syntax;
- `_move_transfer_operand` owns the borrowed-transfer reject for the return,
become, consume-argument, consume-self-receiver and laundering-call
positions, which each open-coded it;
- `_linear_check_borrowed_escape` resolves the place at the aggregate-pack
sites, and its call moves to after the operand is typed at the struct- and
typestate-literal sites, because a selection has no recorded place until it
has been checked;
- `_transfer_borrowed_marker` decides, once, whether a let/var/assign target
inherits the BORROWED marker. Decided per RHS shape, an alias spelling
reached the bind with no marker and the target was armed as a fresh owner,
laundering the caller's still-owned value; and because the marker was lost
one statement early, the pack-site escape rule then saw a name that was no
longer borrowed;
- the anonymous drop and the assign-target poison lift key on the resolved
place, so an alias spelling pops the same binding instead of leaving it
live and reporting the leak a second time;
- the lambda RESULT becomes a transfer position, so the expression-bodied and
block-bodied spellings of one closure agree.
Measured against main on a 209-program corpus: 63 verdict movers, 47 OK to
ERR and 16 ERR to OK. Of the 47, 45 double-free or leak on at least two
backends; the two correct ones are `atk7/ho_combinator_owned` and
`atk7/ho_trait_transform`, already accounted for by sibling A's fail-closed
default. The 16 newly accepted are false-alarm removals, each byte-identical
across the legacy, `--ir` and `--wasm` backends with every resource closed
once. Outside the corpus: zero diagnostic differences on 234 in-repo and 1338
downstream `.capa`, and `--check` plus `--manifest` byte-identical on six
downstream projects through the real CLI.
`TestLinearConditionalAlias` moves and is rewritten honestly: eight of its
nine still reject and now assert the specific diagnostic, and
`test_nested_wrapper_form` becomes a must-accept test because every arm hands
over the same value and it is consumed exactly once, which runs and closes
its resource once on all three backends.
…ntax Three review rounds each found the same defect at a different rule: a rule that enforces "used at most once" resolving its operand by SYNTAX rather than through `_path_of`. The `consume self` receiver open-coded an `Ident`/`FieldAccess` branch list; `_linear_check_borrowed_escape` tested `isinstance(expr, Ident)` at five pack sites and accepted six laundered double-frees; the borrowed propagation tested `isinstance(value, Ident)` and `value.name` at four bind sites and accepted sixteen. Each was found by someone thinking to construct the right program, which is why each round found one and none found the next. All three share one signature, so the class is machine checkable rather than only sample checkable. This guard walks the analyzer's own source and reports any function that keys `_live_linear`, `_borrowed_linear`, `_consumed`, `_linear_names` or `_linear_field_moved` on an operand's `.name`, or on a value narrowed with `isinstance(x, A.Ident)`, without calling the resolver. The detector is scoped per statement, with an `isinstance` narrowing propagating into the branch it guards, because that is how the receiver defect was written: it narrowed and branched without ever reading `.name`. Scoping it that way is also what keeps it quiet on the code that is fine, and the negatives here pin each of those shapes: bookkeeping over an already-resolved place, a `.name` read with no single-use state, and resolve-then-decide. `_check_fun`'s parameter seeding is the only allowed entry, and it is a producer rather than a rule: a parameter's name IS its place, there is no operand expression to resolve. The allowance is pinned to the function name, so moving the seeding or adding a second producer fails rather than inheriting it, and a separate test fails if an ALLOWED entry ever names a function that no longer exists. A source-reading guard is worth nothing if it has only been shown to run, so three tests prove it goes red: on the reintroduced syntactic shape, on a narrowing branch list with no `.name` read, and on the SHIPPED source of `_transfer_borrowed_marker` with its resolution reverted to the syntactic form it used to have. Confirmed out of band as well by mutating that rule on disk: the guard named `_linear.py:635 _transfer_borrowed_marker`, ten behaviour tests went red, the sixteen borrowed-alias double-frees re-opened, and restoring the file returned all of it to green with the digest matching the baseline. Running the guard turned up two functions the hand enumeration had missed, and both were genuine: `_linear_transfer_if_alias` still spelled an already-resolved place as `value.name`, and the type read beside it duplicated `_operand_leaf_ty`. Both now use the resolved place, which is behaviour neutral on the 209 program corpus and says what the code means.
…ot the function The guard shipped with a false negative aimed at exactly the case it exists to catch, and it was found by testing it the only way that counts: revert a real fix in a throwaway build, confirm the build is behaviourally distinct, then run the guard. It exempted a whole FUNCTION when that function called `_path_of` anywhere. `_move_linear_operand` resolves further down its own body, so reverting the laundering-call branch at its top to the syntactic `isinstance(arg, Ident)` plus `arg.name` form produced a build that ACCEPTS three double-frees on all three backends while this guard reported ten passing tests. The exemption was function-scoped and the defect is branch-scoped, and that is backwards for this class specifically: the functions most likely to hide a stray syntactic branch are the big resolve-then-decide seams, which by construction resolve somewhere. `_scan` now carries the exemption down the branch structure alongside the narrowing it already carried, so a statement is excused only when it, or a branch enclosing it, actually resolved. Dropping the whole-function exemption costs nothing: zero flagged functions across the analyzer package, so the allow-list stays at its single parameter seeding entry. The three existing bite tests all passed on the defeated build, because each used a shape the broken exemption happened not to cover. They stay, but they are no longer the evidence. `test_the_guard_bites_on_the_case_that_defeated_it` reverts the SHIPPED source of the laundering-call fold and asserts the flag, so widening the exemption again turns it red while the other three stay green. `_calls_resolver` is deleted with its last caller.
Running each mutation against the TEST SUITE rather than against a probe
corpus answers a different question, and the answer was worse than expected.
A corpus says whether a rule is load-bearing for those programs. It does not
say whether a test would go red if the rule were neutralised, and eight of
seventeen mutations turned out to be caught only by files in a gitignored
scratch directory. For regression purposes that is unguarded.
Six of the eight were load-bearing rules with real corpus witnesses, and the
gap between them was one shape. Every binder test in this module spelled the
alias `match 0 { _ -> a }`, a binder over an UNRELATED scrutinee, whose
outcome is recorded by `_selection_place`. None spelled `match a { v -> ... }`,
a binder that VIEWS its scrutinee, recorded by `_linear_alias` and resolved
through `_path_of`. Neutralising that resolution moves 32 corpus programs and
re-opens eight double-frees, and every test here still passed.
The twelve members added take their shapes from the witness programs rather
than being written to trip a mutation:
- the view binder at four positions, a bare value, a carrier field, a borrowed
scrutinee and a `consume self` receiver, plus the consumed-once negative;
- a binder over a FIELD scrutinee, which pins that the resolved PLACE is
carried rather than the arm's own name, with the two-matches-sharing-a-name
negative that fails if the name were carried instead;
- the two unresolvable bindings, an or-pattern and a generic-passthrough
scrutinee, which must fail closed;
- a non-fresh call arm, a mixed place-and-call selection, and the
provably-fresh factory negative.
Measured after: M3, M4, M6, M7, M9, M11 and M13 all go red on the suite where
all seven previously passed. M5 remains the only survivor and its reason is
now measured rather than argued: its branch IS reached, by an indexed arm, but
neutralising it moves no verdict because the container-of-linear rule rejects
that program first. No claim rests on it.
…raversal The test that exists to stop the guard scanning nothing did not look at the guard. It re-implemented its own `os.listdir` plus `.py` filter, then compared that against itself, so it asserted a property of its own copy of the walk and never touched the detector. That is a second hand-synced copy of the enumeration, which is the defect this release exists to remove, sitting inside the guard built against that defect. Measured rather than argued, in both directions: - narrowing the detector's own extension filter so it traverses ZERO modules left all eleven tests in the module GREEN, including this one. The guard was fully vacuous under that mutation and said nothing; - truncating the detector's walk to two of the seventeen modules also left the old test green. The replacement observes instead of re-deriving. It spies the per-file entry point, invokes the top-level scan for real, and asserts that the set of module names the detector was actually called with EQUALS the set of Python files in the analyzer directory. The expected set is still read from the directory rather than listed in the test, so a module added to the package cannot silently go unscanned, and the required-modules check now runs against what was visited rather than against a second listing. Both mutations above now fail, and only this test fails in each, so the bite is attributable. On shipped source it observes all seventeen modules.
…osed class The module docstring ended on "the fourth instance cannot be added without a test going red". That is not true of this detector and the sentence invited exactly the wrong reading of a green result. Replaced with the bound in three parts, each of which is load-bearing: - it sees ONE textual signature, so a rule that decides a single-use question any other way is outside it entirely; - it only sees rules touching one of the names in SINGLE_USE_SETS, so a discipline that grows new state is invisible here until that list is widened, and nothing enforces widening it; - eleven known spellings evade it today. 25 candidate evasions were constructed and run against the detector: 14 flagged, 11 not. The eleven are named individually rather than counted, because a guard whose holes are unwritten invites the belief that it has none. All eleven were checked against the shipped analyzer and none occurs, so they are latent rather than live. The `getattr` hits that do exist are on types, not on operands near single-use state, and there is no `match` statement, no module-level function and no staticmethod touching those sets. The reproducible list is cited so a future change to the detector can be re-scored, with a drop in the flagged count called out as a regression.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| import unittest | ||
|
|
||
| from tests.analyzer._helpers import check, errors_of |
|
|
||
| import unittest | ||
|
|
||
| from tests.analyzer._helpers import check, errors_of |
| expected = {n for n in os.listdir(adir) if n.endswith(".py")} | ||
| self.assertGreater(len(expected), 5, sorted(expected)) | ||
|
|
||
| import tests.analyzer.test_single_use_resolution_guard as _self |
…ng call `_path_of` resolved an `Ident`, a `FieldAccess` and an `IfExpr`/`MatchExpr` receiver. A `Call`/`MethodCall` receiver resolved to None, so `idc(h).c` denoted no place, the move seam moved nothing, and the projected field stayed consumable through its carrier as well. Three lines double-freed and a four-line variant triple-freed, identically on all three backends. The projection receiver now goes through `_receiver_path_of`, which resolves a call through the E3 origin seam's new pure half (`_call_result_alias_operand`) and recurses through itself, so `idc(idc(h)).c` unwraps every layer. Because the resolution lands in `_path_of`, all nine move positions and all five value kinds inherit it at once rather than each growing a case; and because it RESOLVES rather than failing closed, `close(idc(a).c)` alone stays accepted, which is the same program as the already-accepted `close(a.c)`. Resolving alone still fails OPEN on an AMBIGUOUS receiver: a callee whose result may alias either argument resolves to no place, which the move seam reads as nothing to move, so `pick2(h, h2).c` ran a double free while the whole-value `sink(pick2(h, h2))` was already rejected fail-closed. That was found by mutation-testing the resolution above, not by reading it. The fail-closed reject site now keys on the projection ROOT for both shapes it can take, selection and call, with one reporting helper and one root walk (`_projection_root_of`) shared with the borrowed-propagation bind. Tests: `TestCallReceiverProjection` pins the nine positions and five value kinds as a matrix over three receiver spellings, the repro programs, the laundering callee shapes, and the negatives that keep the measured-correct boundary where it is.
A pentest keyed a purely syntactic single-use rule on `_drop_exempt_linear`,
a set not in `SINGLE_USE_SETS`. The resulting analyzer was unsound -- a
`let tmpa = open()` never consumed was accepted -- while this guard reported
eleven tests OK and the whole suite passed. That is the bound part 2 of the
module docstring already stated, so the hole is not new; the exploit is what
shows the maintenance the bound calls for was overdue.
Six sets are now watched: `_drop_exempt_linear`, `_linear_alias` (which the
release that added this guard introduced and left outside it),
`_moved_subpath_sets`, `_linear_conditional_reported`,
`_linear_container_reported`, `_struct_aliases`. That widening alone does not
catch the exploit, because it decides from a name's TEXT and so reads no
`.name` and narrows nothing: testing a string against a literal other than
the path separator is added as a fourth signature. Measured against the
shipped analyzer that signature has zero hits and the exploit has one, and
the separator exclusion is what keeps `place.split(".", 1)[0]` quiet.
Three walk gaps close with it. `ast.Raise` and `ast.Delete` join the decision
statements, which matters because `del self._live_linear[expr.name]` is a
shipped discharge spelling. `for`/`with` HEADERS are scanned, not only their
bodies. And the analyzer receiver no longer has to be spelled `self`, so
`me = self` and a helper handed the analyzer no longer evade; that also
removed two entries from the known-evasion list, 14/25 flagged to 16/25.
The two decision predicates the scan carried, one for an `if` test and one
for a statement, are folded into `_judge`, and the two propagating signatures
into `_syntactic_test`. Without that fold the text signature would have been
written into one copy and not the other, and the exploit's two-statement
spelling would still have walked through -- which is exactly what happened in
the first draft, caught by its own new test going red.
The docstring's stated bound is corrected rather than quietly improved: four
signatures not one, nine known evasions not eleven, and the note that
"keying on a set not in the list" can never be closed by widening the list.
The Ident-target re-arm in _check_assign picked its preserve-husk fork with `isinstance(s.value, A.Ident) and s.value.name == s.target.name`, so only the literal spelling `a = a` preserved the target's moved-out sub-paths. Every wrapped spelling of the same self-assignment (a selection, a match binder, a single-origin laundering call, and their compositions) took the clear-then-transfer path, whose _clear_moved_subpaths wiped the husk state and re-armed a partially consumed value: a double-free with a clean --check on all three backends (fork-contest probes f4e / f4i / f4n). The fork now asks _receiver_path_of, the one operand-place resolver, whether the RHS resolves to the target's own place; the resolver's docstring names the second asker. Consequences, all measured: - ten wrapped husk self-assign spellings and the three laundered-close compositions flip to the direct spelling's rejection, and f4c (the direct-partial composition, inherited open on main) closes with them; - a LIVE wrapped self-assign now takes the drop rule like the direct spelling, matching main's verdict and wording; the round-2 record had blessed that acceptance, and it was the accepting half of the exact laundered cell this fix removes; - the legitimate re-arm (`close(a.c); a = mkh()`), the other-variable moves, and the fail-closed unresolvable spellings are unchanged; the 205-example sweep, the 60-cell matrix, the qa8 bind/kind corpora, the capture probes and the ninth fams all show zero movers. New tests: TestSelfAssignResolvedPlace, the self-assign spelling family at the assign position (RED-first: 15 subtest failures on the unfixed tree, from the same family the branch fixed at the other positions), plus boundary pins for the fresh re-arm, the other-target move, the fresh-name let, the field-target sibling, and the fail-closed members.
The qa gate's two comparison-shape mutants of the resolved-place predicate survived the full 5845-test suite: replacing `rhs_place == _tpath` with `_tpath.startswith(rhs_place)` or with a first-character comparison changed no test, because every name pair in TestSelfAssignResolvedPlace was a single letter, so no test had a target and an RHS place that were different but prefix-related. Both mutants are real, not equivalent: on `var a; var ab; sink(ab); ab = (if true then a else a); sink(ab)` the healthy tree accepts (the RHS resolves to `a`, a different place, an honest re-arm that runs `closing 2` / `closing 1` byte-identically on all three backends) and both mutants take the preserve fork, skip the move off `a`, and reject with a spurious leak of `a`. test_prefix_named_other_target_still_moves pins that acceptance. RED-first against each mutant in memory: exactly this one test fails under D and under E (failures=1 both times, the leak wording), and the restore re-hashed to the tip package hash. No analyzer code changes.
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
2 participants
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.
Closes a class of silent double-free and use-after-consume defects in the linear discipline.
The defect
Rules that enforce "a value may be used at most once" resolved their operand by syntax. The language has alias-introduction forms that syntax does not reach: a match arm binding, a struct-pattern destructuring
let, and a value selected by anifor amatch. A resource reached through one of those was treated as a different value from the one it aliases, so it could be released twice with no diagnostic, on all three backends.The change
Two facts are recorded at binding time, each with one producer, and fed to the one resolver every position-driven rule already calls, so all of them inherit the fix together rather than each learning it separately. A position-independent gate and three helpers are deleted, making this a net reduction in special-casing.
A new test fails when any single-use rule decides its operand syntactically. Pointed at the pre-fix source it flags eight sites across five modules, including the two rules that caused the largest findings during review.
Measured
python -m unittest discover tests: 5899 tests, OK, 23 skipped.pytest -q: 5888 passed, 23 skipped, 3575 subtests.--irand--wasmbackends.Scope, stated honestly
This closes the sixth known rule of this shape, not the last. The guard detects one textual signature and only sees rules touching the state sets it names; a rule outside those is invisible to it. Capability-side laundering is deliberately out of scope here and remains open.
Not yet run outside win32, which is what this PR is for.