diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 381abfcc..e04c0f3d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -510,7 +510,7 @@ jobs: # case disposes (no default) -> last case is the tail, no phantom no-match leak. # `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release # (member-binding form), so it is disposed on the return path -> silent (Codex review). - for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped; do + for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped defer; do if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi done echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)" @@ -780,9 +780,12 @@ jobs: # (Rent/Return: OWN003/OWN002, pool resolved via the Roslyn SemanticModel so an # ALIASED receiver is caught), factory acquires (System.IO.File.Open*/Create*) are # recognised alongside `new`, and the inter-procedural CONSUME contract is modelled: - # a first-party method owning a by-value IDisposable param is a handoff (a `call` - # op), so a use after the handoff trips OWN002 (the cut is the signature, like - # Rust's move). Remaining backlog: a cross-method use-after-dispose and an - # injected-source region-escape. A drop below the floor is a regression. - run: python scripts/benchmark.py --min-recall 10 + # a first-party method that owns a by-value IDisposable param — by disposing it + # directly OR by forwarding it to another first-party consumer (the TRANSITIVE chain, + # `ConsumesParam`) — is a handoff that releases the argument at the call site, so a use + # after the handoff trips OWN002 (the cut is the signature, like Rust's move). Remaining + # backlog: a FIELD-mediated cross-method use-after-dispose (dispose in one method, use in + # another via shared state) and an injected-source region-escape. A drop below the floor + # is a regression. + run: python scripts/benchmark.py --min-recall 11 diff --git a/corpus/real-world/README.md b/corpus/real-world/README.md index 1fc63176..7510991e 100644 --- a/corpus/real-world/README.md +++ b/corpus/real-world/README.md @@ -43,3 +43,5 @@ exception-path (анализ не моделирует исключения), co |------|-----|------------------| | `arraypool-use-after-return` | OWN002 | rented-буфер вернули в пул, потом ещё читали slice | | `arraypool-double-return` | OWN003 | один и тот же массив вернули в ArrayPool дважды ([#33767](https://github.com/dotnet/runtime/issues/33767)) | +| `ownership-handoff-use` | OWN002 | поток отдали потребителю (он его закрыл), потом ещё читали — use-after-handoff | +| `ownership-handoff-use-transitive` | OWN002 | то же, но потребитель не закрывает сам, а **пробрасывает** владение дальше (transitive consume) | diff --git a/corpus/real-world/ownership-handoff-use-transitive/after.cs b/corpus/real-world/ownership-handoff-use-transitive/after.cs new file mode 100644 index 00000000..91134e9b --- /dev/null +++ b/corpus/real-world/ownership-handoff-use-transitive/after.cs @@ -0,0 +1,28 @@ +using System; +using System.IO; + +// FIX: read the stream BEFORE handing ownership to the consumer chain, so nothing touches it +// after the handoff. Same transitive handoff (Consume -> Inner -> Dispose), correct order. The +// handoff still discharges ownership (Inner closes it), so there is no leak and no +// use-after-handoff -- the case must stay SILENT (this is the no-false-positive arm). +static class HandoffUseTransitive +{ + static void Inner(Stream s) + { + s.CopyTo(Stream.Null); + s.Dispose(); + } + + static void Consume(Stream sink) + { + Inner(sink); + } + + static long Run(string path) + { + var s = File.OpenRead(path); + var len = s.Length; // read BEFORE the handoff + Consume(s); // ownership moves to Consume -> Inner (disposed) + return len; // no use after the handoff -> silent + } +} diff --git a/corpus/real-world/ownership-handoff-use-transitive/before.cs b/corpus/real-world/ownership-handoff-use-transitive/before.cs new file mode 100644 index 00000000..2ed2ca15 --- /dev/null +++ b/corpus/real-world/ownership-handoff-use-transitive/before.cs @@ -0,0 +1,35 @@ +using System; +using System.IO; + +// A TRANSITIVE inter-procedural use-after-handoff: the stream is handed to `Consume`, which +// does NOT dispose it directly -- it FORWARDS it to `Inner`, which owns and closes it. So +// `Consume` consumes its parameter *transitively* (one hop further down the chain), and the +// caller's later read is a use-after-handoff. The consume signal travels through the +// forwarding chain: the extractor follows `Consume -> Inner -> Dispose` and models `Consume(s)` +// as a release of the argument at the call site, so a use after it trips OWN002. Unlike +// `ownership-handoff-use` (the callee disposes the param itself), here the callee only forwards. +static class HandoffUseTransitive +{ + // Direct consumer: owns and closes the stream. + static void Inner(Stream s) + { + s.CopyTo(Stream.Null); + s.Dispose(); // Inner owns and closes it + } + + // Transitive consumer: it does NOT close `sink` itself -- it forwards ownership to Inner. + // `sink` is still consumed (its obligation is discharged via Inner), so a caller must not + // touch the argument after the handoff. + static void Consume(Stream sink) + { + Inner(sink); // ownership forwarded to the real consumer + } + + // BUG: ownership moved into Consume (-> Inner, which disposed it), then the stream is read. + static long Run(string path) + { + var s = File.OpenRead(path); + Consume(s); // ownership moves to Consume -> Inner (disposed) + return s.Length; // use-after-handoff (s is disposed) -> OWN002 + } +} diff --git a/corpus/real-world/ownership-handoff-use-transitive/case.own b/corpus/real-world/ownership-handoff-use-transitive/case.own new file mode 100644 index 00000000..8db17844 --- /dev/null +++ b/corpus/real-world/ownership-handoff-use-transitive/case.own @@ -0,0 +1,28 @@ +// OwnLang model of a TRANSITIVE inter-procedural use-after-handoff. `take` does not close the +// stream itself -- it FORWARDS it to `inner`, which owns and closes it. So `take` consumes its +// parameter TRANSITIVELY (the obligation travels one hop further), and `run` touching the +// stream after `take(s)` is a use-after-handoff -> OWN002. The signature is still the cut: the +// caller is checked against `take`'s (transitively inferred) contract, not its body -- no +// whole-program analysis. (`consume` is an OwnLang keyword, so the reduction names the +// consumers `take`/`inner`; the C# consumers are `Consume`/`Inner`.) +module Corpus +resource Stream { + acquire open + release close +} +// direct consumer: it OWNS the stream and closes it. +fn inner(s: Stream) { + use s; + release s; +} +// transitive consumer: it forwards ownership to `inner` (does NOT close it itself), so it +// consumes `s` one hop down the chain. +fn take(s: Stream) { + inner(s); +} +// BUG: the stream is used after ownership moved into `take` (-> inner, which closed it). +fn run() { + let s = acquire Stream(); + take(s); // ownership consumed transitively (take -> inner) + use s; // <-- touched after handoff -> OWN002 +} diff --git a/corpus/real-world/ownership-handoff-use-transitive/expected-diagnostics.txt b/corpus/real-world/ownership-handoff-use-transitive/expected-diagnostics.txt new file mode 100644 index 00000000..3a36fa92 --- /dev/null +++ b/corpus/real-world/ownership-handoff-use-transitive/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN002 diff --git a/corpus/real-world/ownership-handoff-use-transitive/notes.md b/corpus/real-world/ownership-handoff-use-transitive/notes.md new file mode 100644 index 00000000..7055497e --- /dev/null +++ b/corpus/real-world/ownership-handoff-use-transitive/notes.md @@ -0,0 +1,33 @@ +# Inter-procedural use-after-handoff — the TRANSITIVE (forwarded) consumer + +**Pattern:** the same use-after-handoff as `ownership-handoff-use`, but the consumer does **not** +dispose the stream itself — it **forwards** it to another method that owns and closes it +(`Consume(sink) -> Inner(sink) -> sink.Dispose()`). The caller hands ownership to `Consume`, then +touches the stream again. The bug is the use **after** ownership moved; the handoff itself is +correct (the stream is closed, just one hop further down). + +**What the checker says:** using a resource after it was consumed (here, *transitively*) by a +callee is the generic **OWN002** (use after release) — the same code as use-after-dispose. + +**Why this case exists (the transitive consume-contract).** The consume contract was already +modelled for a callee that disposes a by-value `IDisposable` parameter *directly* +(`ownership-handoff-use`). But the inference deliberately **stopped at one hop**: a parameter +merely *handed to another call* was "genuinely ambiguous without that callee's contract, so we do +not infer it" (`ownlang/ownir.py`, the `passed` branch) — and the extractor's +`ConsumeReleaseArgs` only recognised a *direct* disposer. So `Consume` (which forwards rather than +disposes) was **not** seen as a consumer, the handoff was not a release, and the later `s.Length` +was invisible — a **miss**. + +Now the extractor's consumer detection (`ConsumesParam`) is **transitive**: a parameter is +consumed if the body either disposes it directly **or** forwards it to another first-party +consumer that consumes it — following `Consume -> Inner -> Dispose` through the chain, guarded +against cycles. Inspecting each callee's own body keeps it inter-procedural without a cross-call +signature table (and without a dangling-callee crash). Conservative: a parameter handed to an +unknown or merely-borrowing callee is **not** treated as consumed (no false release, no false +OWN002). This fixture is a **miss** before and a **catch** after — the ratchet row that lifts the +use-after-handoff capability across a forwarding hop. The `.own` reduction already checks the +transitive shape (the front-end's signature inference recurses); this brings it to real C#. + +**Honesty / scope.** `case.own` is a faithful hand reduction of the C# pattern, not C# the `.own` +checker ingested. `before.cs` / `after.cs` are representative of the bug and its fix, not a +verbatim copy of one PR. diff --git a/docs/proposals/P-016-deep-fact-extraction.md b/docs/proposals/P-016-deep-fact-extraction.md index 2a4a9085..3fadbf09 100644 --- a/docs/proposals/P-016-deep-fact-extraction.md +++ b/docs/proposals/P-016-deep-fact-extraction.md @@ -121,7 +121,12 @@ a CFG-carrying bridge, and the existing core checks real code. slice; do it before the general case. - **B3 — Move / ownership transfer.** `return disposable`, or passing it to a callee that consumes it → move / escape (P-005 D5). Intraprocedural, driven by declared - signatures, not whole-program tracing. + signatures, not whole-program tracing. *Landed (consume handoff):* a call to a + first-party consumer — a method that disposes a by-value `IDisposable` parameter + **directly, or by forwarding it to another first-party consumer** (the transitive + chain, `ConsumesParam`) — releases the argument at the call site, so a use after the + handoff trips OWN002 (`corpus/real-world/ownership-handoff-use{,-transitive}`). The + signal is each callee's own body, so it is inter-procedural without a signature table. - **B4 — Borrow / Span.** `Rent` → view → `Return`, `Span`/`ref` aliasing (P-007) — the borrow checker's crown jewel, hardest on C# (ref structs, Span lifetimes). - **B5 — Lifetime ordering.** WPF005 escape (`OWN014`) and DI captive (P-006, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index a9801dca..4d04c897 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -860,30 +860,21 @@ static bool IsInNamespace(INamedTypeSymbol? t, params string[] parts) } // The local names of arguments handed to a first-party CONSUMER at this call — a method -// whose own body disposes the by-value IDisposable parameter the argument binds to. Such -// an argument's ownership moves into the callee and is discharged there, so the handoff is -// modelled as a RELEASE of the argument at the call site (the same shape as pool -// `Return(buf)`); a later use is then a use-after-handoff (OWN002). Inspecting the callee's -// OWN body means no cross-call signature table and no dangling-callee crash — a callee with -// no body (interface / abstract / extern) or that does not dispose the param contributes -// nothing, and the argument stays an ordinary escape. Arguments resolve to parameters by -// NAME when `name:` is used, else by position; partial declarations are scanned for a body. +// that takes ownership of the by-value IDisposable parameter the argument binds to and +// discharges it: either by disposing it directly, or by forwarding it to another first-party +// consumer (the transitive case, `ConsumesParam`). Such an argument's ownership moves into +// the callee and is discharged there, so the handoff is modelled as a RELEASE of the argument +// at the call site (the same shape as pool `Return(buf)`); a later use is then a +// use-after-handoff (OWN002). Inspecting each callee's OWN body means no cross-call signature +// table and no dangling-callee crash — a callee with no body (interface / abstract / extern) +// or that does not consume the param contributes nothing, and the argument stays an ordinary +// escape. Arguments resolve to parameters by NAME when `name:` is used, else by position. static List ConsumeReleaseArgs(ExpressionSyntax e, SemanticModel model) { var consumed = new List(); if (e is not InvocationExpressionSyntax inv || model.GetSymbolInfo(inv).Symbol is not IMethodSymbol sym) return consumed; - SyntaxNode? body = null; - foreach (var r in sym.DeclaringSyntaxReferences) - if (r.GetSyntax() is BaseMethodDeclarationSyntax d - && ((SyntaxNode?)d.Body ?? d.ExpressionBody) is { } b) - { - body = b; - break; - } - if (body is null) - return consumed; var args = inv.ArgumentList.Arguments; for (int i = 0; i < args.Count; i++) { @@ -891,19 +882,93 @@ static List ConsumeReleaseArgs(ExpressionSyntax e, SemanticModel model) var p = args[i].NameColon is { } nc ? sym.Parameters.FirstOrDefault(q => q.Name == nc.Name.Identifier.Text) : (i < sym.Parameters.Length ? sym.Parameters[i] : null); - if (p is { RefKind: RefKind.None } && ImplementsIDisposable(p.Type) - && DisposesLocal(body, p.Name) - && args[i].Expression is IdentifierNameSyntax aid) + if (p is not null + && args[i].Expression is IdentifierNameSyntax aid + && ConsumesParam(sym, p, model, + new HashSet(SymbolEqualityComparer.Default))) consumed.Add(aid.Identifier.Text); } return consumed; } +// The body of a first-party method or LOCAL FUNCTION (block or expression-bodied), scanning +// partial declarations; null for an interface/abstract/extern method (no body to inspect). A +// directly-called local function runs synchronously, so a forwarding chain through one must be +// followed too (CodeRabbit) — `LocalFunctionStatementSyntax` carries the same Body/ExpressionBody. +static SyntaxNode? ConsumerBody(IMethodSymbol sym) +{ + foreach (var r in sym.DeclaringSyntaxReferences) + switch (r.GetSyntax()) + { + case BaseMethodDeclarationSyntax d when ((SyntaxNode?)d.Body ?? d.ExpressionBody) is { } b: + return b; + case LocalFunctionStatementSyntax l when ((SyntaxNode?)l.Body ?? l.ExpressionBody) is { } lb: + return lb; + } + return null; +} + +// The invocations that run IMMEDIATELY when `body` executes — excluding those inside a nested +// lambda / local-function body, which run deferred (when the delegate is later invoked), not at +// this method's call boundary. The same deferred-body rule the flow lowering already uses, so a +// dispose/forward stored in a callback is not mistaken for an immediate discharge (Codex/CodeRabbit). +static IEnumerable ImmediateInvocations(SyntaxNode body) => + body.DescendantNodes(n => n is not (AnonymousFunctionExpressionSyntax + or LocalFunctionStatementSyntax)) + .OfType(); + +// Does `method` CONSUME (take ownership of and discharge) its by-value IDisposable +// parameter `param`? Either (a) the body disposes it directly (`param.Dispose()`), or +// (b) — the transitive step — the body hands it to ANOTHER first-party consumer that +// consumes it (`Inner(param)` where `Inner` consumes its matching parameter). So a +// forwarding chain `Consume(sink) => Inner(sink) => sink.Dispose()` is recognised, and a +// caller's `Consume(s)` is still a handoff (a call-site release). Inspecting each callee's +// own body keeps it inter-procedural without a signature table; `visited` (keyed on the +// parameter) guards recursion on cyclic call graphs. Conservative: a param handed to an +// unknown/borrowing callee yields false (no release, no false OWN002). +static bool ConsumesParam(IMethodSymbol method, IParameterSymbol param, + SemanticModel model, HashSet visited) +{ + if (param.RefKind != RefKind.None || !ImplementsIDisposable(param.Type)) + return false; + if (!visited.Add(param.OriginalDefinition)) + return false; // cycle guard (per parameter) + if (ConsumerBody(method) is not { } body) + return false; // no body -> contributes nothing + var name = param.Name; + if (DisposesLocal(body, name)) // (a) disposes the parameter directly + return true; + // (b) transitive: the parameter is handed to another first-party consumer at an IMMEDIATE + // call (not one deferred in a nested lambda/local function). The body may live in another + // file, so bind its calls with that tree's OWN model (a SemanticModel only resolves nodes in + // its own tree); the Compilation is shared across all parsed inputs. + var bodyModel = model.Compilation.GetSemanticModel(body.SyntaxTree); + foreach (var inv in ImmediateInvocations(body)) + { + if (bodyModel.GetSymbolInfo(inv).Symbol is not IMethodSymbol callee) + continue; + var cargs = inv.ArgumentList.Arguments; + for (int i = 0; i < cargs.Count; i++) + { + if (cargs[i].Expression is not IdentifierNameSyntax aid || aid.Identifier.Text != name) + continue; + var cp = cargs[i].NameColon is { } nc + ? callee.Parameters.FirstOrDefault(q => q.Name == nc.Name.Identifier.Text) + : (i < callee.Parameters.Length ? callee.Parameters[i] : null); + if (cp is not null && ConsumesParam(callee, cp, model, visited)) + return true; + } + } + return false; +} + // Does `body` dispose the local/parameter named `name` — a `name.Dispose()` / `.Close()` / -// `.DisposeAsync()` call (the consume signal)? +// `.DisposeAsync()` call (the consume signal)? Only IMMEDIATE calls count (`ImmediateInvocations` +// excludes nested lambda / local-function bodies): a `name.Dispose()` inside a stored callback +// runs deferred, not at this call site, so it is not a discharge here. static bool DisposesLocal(SyntaxNode body, string name) { - foreach (var i in body.DescendantNodesAndSelf().OfType()) + foreach (var i in ImmediateInvocations(body)) if (i.Expression is MemberAccessExpressionSyntax m && m.Name.Identifier.Text is "Dispose" or "Close" or "DisposeAsync" && m.Expression is IdentifierNameSyntax id && id.Identifier.Text == name) diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index 0323156a..9dbe0132 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.IO; using System.Net; using System.Net.Sockets; @@ -484,6 +485,25 @@ public void TcpListenerNeverStopped() var tcpLeak = new TcpListener(IPAddress.Loopback, 0); tcpLeak.Start(); } + + // a registry of deferred disposers: the parameter's dispose is captured in a STORED callback + // that runs LATER (when the registry is drained), not at this method's call boundary — so + // storing it does NOT consume the parameter here (the transitive-consume inference must not + // descend into nested lambda bodies). Intentionally not drained in this sample, so nothing is + // actually disposed via the callback — `defer` below is disposed only by its own Dispose(). + private static readonly List _deferredDisposers = new(); + private static void DeferredConsumer(Stream s) => _deferredDisposers.Add(() => s.Dispose()); + + // control (Codex/CodeRabbit on PR #68): passing a local to DeferredConsumer only STORES a + // deferred disposer (it does not dispose at the call site), so the later use must NOT be a + // phantom use-after-handoff — no false OWN002. `defer` is disposed normally here -> SILENT. + public void DeferredHandoffNoFalsePositive() + { + var defer = new MemoryStream(); + DeferredConsumer(defer); // stores a deferred disposer -> NOT a release here + defer.WriteByte(1); // must NOT trip OWN002 (it would, without the fix) + defer.Dispose(); // disposed here -> balanced -> silent + } } // A domain exception type literally named `Exception`, in a non-System namespace — the