diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9169b430..6f0a90c5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -263,10 +263,14 @@ jobs: || { echo "FAIL: expected OWN001 on the undisposed local in a foreach loop"; exit 1; } echo "$out" | grep -qE "OWN001.*'forLeak'" \ || { echo "FAIL: expected OWN001 on the undisposed local in a for loop"; exit 1; } - # `try`/`finally` lowered sequentially (try-methods no longer skipped): a - # local never disposed inside a try is now caught. + # `try`/`finally` lowered with exception edges (try-methods no longer skipped): + # a local never disposed inside a try is caught... echo "$out" | grep -qE "OWN001.*'tfLeak'" \ || { echo "FAIL: expected OWN001 on the undisposed local in a try-method"; exit 1; } + # ...and so is dispose-not-called-on-throw: `dot` is disposed inside the try + # after a may-throw call, so it leaks on the exceptional path (matches CodeQL). + echo "$out" | grep -qE "OWN001.*'dot'" \ + || { echo "FAIL: expected OWN001 on the dispose-not-called-on-throw local"; exit 1; } # dispose-optional (Task), disposed/escaping locals, a `for` loop whose # disposable is disposed after it (`looped`, balanced), a balanced # acquire+dispose in a loop (`whileClean`), a try/finally dispose (`tfClean`, @@ -274,7 +278,11 @@ jobs: # stay silent: # released via `await x.DisposeAsync()` (asyncDisposed) and the chained # `.ConfigureAwait(false)` form (asyncDisposedCfg) -> both must stay silent. - for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull; do + # PR #32 FP fixes: a swallowing catch with a Dispose AFTER the try/catch (cda), + # an `await DisposeAsync().ConfigureAwait(false)` INSIDE a try (daci), and a Dispose + # inside both branches of an `if` in a try alongside a may-throw call (cif) — all + # disposed on every path, so all must stay silent (were false OWN001 before). + for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif; 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)" diff --git a/corpus/fixtures/systemevents-console/Program.cs b/corpus/fixtures/systemevents-console/Program.cs index 40e1d82e..67c4aa0b 100644 --- a/corpus/fixtures/systemevents-console/Program.cs +++ b/corpus/fixtures/systemevents-console/Program.cs @@ -31,6 +31,7 @@ public static void Main() _ = new DisplayWatcher(); LeakAFile(); LeakInTry(); + DisposeOnThrow(); } // (2) DISPOSE leak — CodeQL's / Infer#'s class, and the control: a local @@ -55,4 +56,26 @@ private static void LeakInTry() catch (Exception) { /* logged, not disposed */ } // ...no Dispose()/using -> resource leak, now seen despite the `try` } + + // (4) DISPOSE-NOT-CALLED-ON-THROW — the exception-edge slice. Unlike (2)/(3), this + // stream IS disposed; but the Dispose() sits INSIDE the try, after a may-throw call + // (WriteByte). On the normal path it's disposed; if WriteByte throws, control jumps + // to the catch and the Dispose is skipped -> the stream leaks on the exceptional + // path. CodeQL has a dedicated query for exactly this (cs/dispose-not-called-on-throw, + // and cs/local-not-disposed also models exceptional flow). Own.NET used to miss it — + // disposed-somewhere looked balanced — until the exception-edge model put a throw + // edge before each may-throw statement; it now flags it too -> should join (2)/(3) + // in "Agree". + // + // The try is kept a ONE-LINER (like LeakInTry) on purpose: the three tools anchor + // this one leak at different points — Own.NET at the acquire, CodeQL at the Dispose, + // Infer# at last-access — so a spread-out method puts them >3 lines apart and the + // oracle's ±3 line window splits one leak into own-only + oracle-only. Keeping the + // acquire and the try adjacent pulls the anchors back inside the window. + private static void DisposeOnThrow() + { + var onThrow = new FileStream("scratch3.bin", FileMode.Create); + try { onThrow.WriteByte(0x42); onThrow.Dispose(); } + catch (Exception) { /* swallowed, no dispose */ } + } } diff --git a/corpus/fixtures/systemevents-console/README.md b/corpus/fixtures/systemevents-console/README.md index 4d08f97c..ec136077 100644 --- a/corpus/fixtures/systemevents-console/README.md +++ b/corpus/fixtures/systemevents-console/README.md @@ -13,12 +13,29 @@ The leaks (`Program.cs`): | 1 | `SystemEvents.DisplaySettingsChanged += …`, never `-=` | subscription / lifetime | **Own.NET only** | | 2 | `new FileStream(…)` local, never disposed | Dispose / RAII | **all three** (the control) | | 3 | `new FileStream(…)` never disposed, inside a `try`-method | Dispose / RAII | **all three** (closed by `try`-lowering) | +| 4 | `Dispose()` inside `try` after a may-throw call (skipped on the throw path) | Dispose-on-throw | **all three** (exception-edge slice) | Leak `#2` is the agreement that proves the RAII oracles ran on the fixture; `#1` is the differentiator — Own.NET flags it, CodeQL / Infer# have no query for the -subscription-leak class. #3 is the recall slice: before `try`/`finally` was lowered, -Own.NET skipped any method containing a `try`, so this leak was *Oracle only*; now it -joins #2 in **Agree** across all three tools. +subscription-leak class. #3 is the `try`-lowering recall slice: before `try`/`finally` +was lowered, Own.NET skipped any method containing a `try`, so this leak was *Oracle +only*; now it joins #2 in **Agree** across all three tools. + +#4 is the **exception-edge** slice. The stream *is* disposed, but the `Dispose()` sits +inside the `try` after a may-throw call, so it's skipped if the call throws — a leak only +on the exceptional path. CodeQL has a dedicated query for this (`cs/dispose-not-called-on-throw`; +`cs/local-not-disposed` also models exceptional flow) and Infer#'s Pulse engine models +exceptional paths too. Own.NET used to miss it (disposed *somewhere* looked balanced) +until the exception-edge model inserted a throw edge before each may-throw statement in a +`try`; it now flags it too, so #4 joins #2/#3 in **Agree** across all three. + +One wrinkle worth recording: the three tools anchor this leak at *different* program +points — Own.NET at the acquire, CodeQL at the `Dispose()` call, Infer# at the last +access — so a spread-out method puts them >3 lines apart and the oracle's ±3 line window +splits one leak into "Own.NET only" + "Oracle only". Keeping the `try` a one-liner (as +`LeakInTry` already is) pulls the anchors back within the window so the agreement is +visible. The line window is intentionally conservative; this is a property of the +*comparison*, not of the detections. Run via the oracle's local-fixture mode — set `corpus/oracle-target.txt` to: diff --git a/corpus/oracle-target.txt b/corpus/oracle-target.txt index dccfb21d..c349054c 100644 --- a/corpus/oracle-target.txt +++ b/corpus/oracle-target.txt @@ -4,8 +4,12 @@ # Optional lines: ref=, paths=, build=, include_tests=. Dev-branch only. # # Cross-tool oracle on a Linux-buildable fixture, so ALL THREE tools run (Infer# -# included — ScreenToGif's WPF won't build on Linux). Re-run after aligning the -# codeql-action analyze@v4 with init@v4 (the v3/v4 mismatch broke CodeQL last run); -# expect the 3rd leak (try-method) in "Agree" across all THREE now. See the README. +# included — ScreenToGif's WPF won't build on Linux). Re-run #2 of the EXCEPTION-EDGE +# slice after the first run surfaced two artifacts: (a) #3 (tfLeak) was double-reported +# — a never-disposed local in a try leaks on BOTH the injected exceptional exit and the +# normal end; now deduped in the bridge; (b) #4's anchors were >3 lines apart (Own.NET +# acquire / CodeQL Dispose / Infer# last-access) so the ±3 window split it — the try is +# now a one-liner adjacent to the acquire. Expect #2/#3/#4 in "Agree" (no dup), #1 +# Own.NET-only, 0 oracle-only. See the README. local:corpus/fixtures/systemevents-console build=SystemEventsLeak.csproj diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index de25a27a..e891db88 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -90,7 +90,13 @@ architectural strictness, and the borrow-checker showcase): The cross-tool oracle confirms the differentiation: CodeQL *and* Infer# (the latter via a buildable fixture) cover the Dispose/RAII class and flag none of these subscription leaks — agreeing with Own.NET only on a Dispose leak, never a subscription. - See [docs/notes/real-world-mining.md](notes/real-world-mining.md). + The oracle also drove down the *other*-class recall gap (Dispose leaks Own.NET missed + because the flow detector skipped methods with unmodelled constructs): `for` and `try` + are now lowered — sequentially, then with an **exception-edge** model that injects a + throw exit before each may-throw statement in a `try`. That closes the + `dispose-not-called-on-throw` shape, which now lands in cross-tool **Agree** with + CodeQL's dedicated query on the fixture. Deferred: `finally`-before-`return`, + `switch`/`do`. See [docs/notes/real-world-mining.md](notes/real-world-mining.md). 2. **Resource core** — generalise WPF subscriptions + `IDisposable` into one acquire/release/owner/release-region model (P-004 ∪ P-005), so WPF is a *profile*, not a one-off. diff --git a/docs/notes/real-world-mining.md b/docs/notes/real-world-mining.md index 7f01306e..cc2dbc7e 100644 --- a/docs/notes/real-world-mining.md +++ b/docs/notes/real-world-mining.md @@ -116,12 +116,24 @@ Their leak findings are **nearly disjoint** (file overlap: **1**): coverage, not type recognition**: the `--flow-locals` detector skips any method with an unmodelled construct (`for`/`try`/`switch`), and these disposables live in such methods (tell: the `StringReader`/`XmlReader` cases are a *recognised* disposable - type, yet still missed). `for` **and** `try`/`finally` are now lowered (sequential - `A; B`, catch-disposes bailed for soundness), so a plain undisposed local inside a - try-method is caught — confirmed on the cross-tool fixture, where a `try`-method - `FileStream` leak moved from *Oracle only* into **Agree** (Own.NET + Infer#). Still - deferred: the true `dispose-not-called-on-throw` shape (disposed in `try`, not - `finally`) needs per-statement exceptional exits, and `switch`/`do` are unmodelled. + type, yet still missed). `for` **and** `try` are now lowered, in two slices: first + sequential `A; B` (catch-disposes bailed for soundness), so a plain undisposed local + inside a try-method is caught; then the **exception-edge** model — before each + may-throw statement in a `try`, inject an exceptional exit (`if(*){ ; return }`) + — which catches the true `dispose-not-called-on-throw` shape (disposed in `try`, not + `finally`: the throw skips the `Dispose`). Both confirmed on the cross-tool fixture: + the plain `try`-method leak and the dispose-on-throw leak both land in **Agree** across + all three tools (the latter matching CodeQL's `cs/dispose-not-called-on-throw`). The + edges are injected only where sound — when the caught path's continuation is end-of- + method (no catch, or the `try` is the body's tail); a swallowing catch with a Dispose + *after* the try/catch (continuation disposes the resource) lowers sequentially instead, + to avoid a false leak (PR #32 review). Still deferred — all **sound recall gaps** (missed + leaks, never false ones), to be tackled as a dedicated exception-edge recall slice with + its own oracle re-validation: exception edges inside nested `try` bodies (only top-level + `try` statements get an edge today); object creation (`new`) as a throw point (it can leak + a prior owned resource whose dispose it skips); typed/filtered catches (a non-tail catch + suppresses edges even when it only continues for *some* exception types — the uncaught-type + paths really do leak); `finally`-before-`return` threading (bailed today); and `switch`/`do`. - **Agree — 1** (`HttpHelper.cs`). So the SystemEvents and VideoSource findings are **differentiated — confirmed by the @@ -129,20 +141,31 @@ oracle, not just argued**: the tools are complementary (Own.NET on subscription/ lifetime, CodeQL on Dispose/RAII), overlapping on a single file. **Infer#, via a buildable fixture.** ScreenToGif can't build on Linux, so to get the -third tool in, a minimal `net8.0` console reproduces both leak classes +third tool in, a minimal `net8.0` console reproduces the leak classes (`corpus/fixtures/systemevents-console/`, fed to the oracle via a `local:` target). -All three tools run; the diff is a clean 2×2: - -| `Program.cs` | leak | Own.NET | CodeQL | Infer# | -|---|---|:-:|:-:|:-:| -| `:41` | `new FileStream(…)` never disposed — Dispose/RAII | ✓ | ✓ | ✓ | -| `:20` | `SystemEvents.DisplaySettingsChanged +=` never `-=` — subscription | ✓ | — | — | - -The FileStream leak is **Agree** across all three — the control that proves CodeQL -*and* Infer# actually run and detect resource leaks on this code. The SystemEvents -subscription is **Own.NET only**: **Infer# misses it too.** Both mature oracles cover -the Dispose/RAII class and neither has the subscription-leak class — the -differentiation, nailed with all three tools. +All three tools run; the diff (latest run) is: + +| `Program.cs` | leak | class | Own.NET | CodeQL | Infer# | +|---|---|---|:-:|:-:|:-:| +| `:43` | `new FileStream(…)` never disposed | Dispose/RAII | ✓ | ✓ | ✓ | +| `:54` | undisposed local inside a `try`-method | Dispose/RAII (try-lowering) | ✓ | ✓ | ✓ | +| `:77` | `Dispose()` in `try` after a may-throw call — skipped on the throw path | dispose-on-throw (exception-edge) | ✓ | ✓ | ✓ | +| `:20` | `SystemEvents.DisplaySettingsChanged +=` never `-=` | subscription | ✓ | — | — | + +The three Dispose/RAII leaks are **Agree** across all three tools — the controls that +prove CodeQL *and* Infer# actually run and detect resource leaks on this code — and the +bottom two are the recall slices: the plain `try`-method leak (sequential lowering) and +the dispose-on-throw leak (exception-edge), the latter matching CodeQL's dedicated +`cs/dispose-not-called-on-throw` query. **Oracle-only is empty** — no Dispose/RAII leak +on this fixture is missed. The `SystemEvents` subscription is **Own.NET only**: **Infer# +misses it too.** Both mature oracles cover the Dispose/RAII class and neither has the +subscription-leak class — the differentiation, nailed with all three tools. + +> The exception-edge slice also surfaced a hygiene bug it then fixed: a local that +> leaks on *both* the injected exceptional exit *and* the normal end produced two +> identical OWN001s (every flow-local diagnostic remaps to the acquire line, so they +> collapse). The bridge now drops byte-identical findings (`ownir.py`), pinned by +> `tests/fixtures/ownir/flow_leak_two_exits.facts.json`. One leak, one finding. > Getting a trustworthy diff took fixing two oracle bugs: the comparator dropped > multi-line / untagged own-check findings (`scripts/mine_report.py` parser drift — diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 01c979c1..f1616fb4 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -265,6 +265,53 @@ static bool IsDisposeOptional(ITypeSymbol t) _ => "?", }; +// A `Dispose()`/`Close()`/`DisposeAsync()` call — through member access (`x.Dispose()`) +// or member binding (`x?.Dispose()`), and seen through a trailing `.ConfigureAwait(false)` +// (the idiomatic `await x.DisposeAsync().ConfigureAwait(false)` is the release, not a +// throwing call). Mirrors the unwrap in EmitFlowExpr so StatementMayThrow does not inject +// a false exceptional-leak edge before an async dispose. +static bool IsDisposeShaped(InvocationExpressionSyntax i) +{ + var callee = i.Expression; + if (callee is MemberAccessExpressionSyntax cfg + && cfg.Name.Identifier.Text == "ConfigureAwait" + && cfg.Expression is InvocationExpressionSyntax innerInv) + callee = innerInv.Expression; + return (callee switch + { + MemberAccessExpressionSyntax ma => ma.Name.Identifier.Text, + MemberBindingExpressionSyntax mb => mb.Name.Identifier.Text, + _ => (string?)null, + }) is "Dispose" or "Close" or "DisposeAsync"; +} + +// True when `st` is the LAST statement of a method/accessor/constructor body block — its +// block's parent is a member declaration, not a nested statement (a BlockSyntax is itself +// a StatementSyntax, so this also excludes nested blocks). Used to decide whether a +// `try`'s exceptional-exit edges are sound (see the try lowering). +static bool IsBodyTail(StatementSyntax st) => + st.Parent is BlockSyntax b + && b.Statements.Count > 0 && b.Statements[^1] == st + && b.Parent is not StatementSyntax; + +// An exceptional exit before a try-body statement is sound only for a LEAF statement whose +// may-throw call sits at the statement's own level — an expression statement (`x.Foo();`) +// or a local declaration (`var x = Foo();`). A COMPOUND statement (if/loop/block/…) may +// dispose a resource in a nested branch before throwing deeper inside; an edge placed before +// the WHOLE statement (where that resource is still owned) would falsely flag it as leaked +// though every real path disposes it. So edges are skipped for compound statements — the +// nested may-throw is the deferred nested-try slice, a sound recall gap rather than an FP. +static bool EdgeEligible(StatementSyntax st) => + st is ExpressionStatementSyntax or LocalDeclarationStatementSyntax; + +// A statement that can raise an exception part-way through: it makes a call that is not +// itself a dispose. (A `new` can throw too — and would leak a PRIOR owned resource whose +// dispose it skips — but modelling object creation as a throw point is a deferred recall +// slice; today only non-dispose CALLS create an exceptional exit. Both omissions are sound: +// a missed leak, never a false one.) +static bool StatementMayThrow(StatementSyntax st) => + st.DescendantNodes().OfType().Any(i => !IsDisposeShaped(i)); + // Lower a method block to OwnIR flow nodes (acquire/use/release/if/return) for the // `tracked` local IDisposables. Returns null on any UNMODELLED statement // (loop/try/switch/...): the method is then honestly skipped, not guessed. @@ -359,39 +406,56 @@ static bool LowerFlowStmt(StatementSyntax st, HashSet tracked, List silent (the safe dispose pattern); one never - // released anywhere leaks -> caught. This un-skips try-methods, the big - // recall slice (a plain undisposed local living inside a try). NOT modelled - // yet: dispose-on-throw (released in `try`, not `finally`) reads as released - // here — that needs per-statement exceptional exits (a later slice). + // try { A } [catch { C }...] [finally { B }] with EXCEPTION EDGES. Any + // statement in A that makes a (non-dispose) call can throw; before each such + // statement we insert an exceptional exit `if(*){ B; return }` — throw here, + // run the finally, leave. A resource owned at that point and NOT released by + // the finally leaks on the exceptional path: dispose-not-called-on-throw (a + // dispose placed in the try, not the finally). A dispose IN the finally runs + // on every exceptional exit, so the safe pattern stays silent; `acquire; + // dispose;` with no call between has no exit, so no false leak. // - // A `return` inside the try makes a finally's release UNREACHABLE in this - // sequential model (the core treats `return` as terminal), which would - // FALSELY flag a resource the finally disposes. Until finally-before-return - // is modelled, bail when a try-with-finally contains a return: the common - // `try { …; return x; } finally { r.Dispose(); }` is safe anyway, so skipping - // it is sound (a real leak in that shape is rare). - if (trys.Finally is not null - && trys.Block.DescendantNodes().OfType().Any()) - return false; // Catch bodies are not lowered; to stay SOUND, bail if any catch disposes, so - // a release that only happens in a catch is never missed (no false leak). Match - // both `x.Dispose()` (member access) and `x?.Dispose()` (member binding). + // a release that only happens in a catch is never missed. Matches `x.Dispose()` + // and `x?.Dispose()`. foreach (var cc in trys.Catches) - if (cc.Block.DescendantNodes().OfType() - .Any(i => (i.Expression switch - { - MemberAccessExpressionSyntax ma => ma.Name.Identifier.Text, - MemberBindingExpressionSyntax mb => mb.Name.Identifier.Text, - _ => (string?)null, - }) is "Dispose" or "Close" or "DisposeAsync")) + if (cc.Block.DescendantNodes().OfType().Any(IsDisposeShaped)) return false; - if (!LowerFlowStmt(trys.Block, tracked, nodes)) + // A `return` in a try-WITH-finally isn't threaded through the exceptional exits + // below (finally-before-return), so its finally release would be unreachable -> + // bail to stay sound. The common `try { …; return; } finally { dispose }` is + // safe anyway, so skipping it loses no real catch. + if (trys.Finally is not null + && trys.Block.DescendantNodes().OfType().Any()) return false; - if (trys.Finally is { } fin && !LowerFlowStmt(fin.Block, tracked, nodes)) + var finallyNodes = new List(); + if (trys.Finally is { } fin && !LowerFlowStmt(fin.Block, tracked, finallyNodes)) return false; + // The exceptional exit `if(*){ B; return }` models the exception LEAVING the + // method — sound only when the caught path's continuation IS end-of-method: + // no catch (it propagates out past any post-try code), or the try is the body's + // tail (nothing runs after the catch). With a catch AND post-try code, a + // resource the post-try code disposes runs on the caught path too, so a return + // there would falsely flag it (dispose-after-try). In that case skip the edges + // and lower the try sequentially — a never-disposed local is still caught at + // end-of-function; only dispose-on-throw is forgone for this shape. + // Conservative: ANY non-tail catch suppresses the edges, even a typed/filtered + // catch that only continues for some exception types — the uncaught-type paths + // really do leak, but distinguishing catch-all from typed/filtered is a deferred + // recall slice. Suppressing is sound: a missed leak, never a false one. + var edgesSound = trys.Catches.Count == 0 || IsBodyTail(trys); + foreach (var stmt in trys.Block.Statements) + { + if (edgesSound && EdgeEligible(stmt) && StatementMayThrow(stmt)) + { + var ex = new List(finallyNodes) + { new { op = "return", var = (string?)null, line = LineOf(stmt) } }; + nodes.Add(new { op = "if", line = LineOf(stmt), then = ex, @else = new List() }); + } + if (!LowerFlowStmt(stmt, tracked, nodes)) + return false; + } + nodes.AddRange(finallyNodes); // normal completion runs the finally return true; } default: diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index dd8c1866..5162031d 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -136,6 +136,63 @@ public void CatchNullCondDispose() catch (Exception) { tfNull?.Dispose(); } } + // dispose-not-called-on-throw: `dot` is disposed INSIDE the try (not a finally), + // after a may-throw call. If WriteByte throws, the Dispose is skipped and `dot` + // leaks on the exceptional path — the exception-edge model now catches this (OWN001), + // matching CodeQL's cs/dispose-not-called-on-throw. + public void DisposeOnThrow() + { + var dot = new MemoryStream(); + try { dot.WriteByte(1); dot.Dispose(); } + catch (Exception) { /* swallowed, no dispose */ } + } + + // NOT a leak: the catch swallows and the Dispose runs AFTER the try/catch, so on the + // thrown path control reaches `cda.Dispose()` too — disposed on every path. The + // exception-edge model's synthetic exit can't represent that caught-then-continue + // path, so when a `try` has a catch and is NOT the method's tail statement the edges + // are skipped (the body still lowers sequentially). Must stay silent (was a false + // OWN001 before — PR #32 Codex review). + public void CatchThenDisposeAfter() + { + var cda = new MemoryStream(); + try { cda.WriteByte(1); } + catch (Exception) { /* swallowed */ } + cda.Dispose(); + } + + // NOT a leak: `await x.DisposeAsync().ConfigureAwait(false)` INSIDE a try is the + // release. IsDisposeShaped now unwraps the `.ConfigureAwait(false)`, so the statement + // is recognised as a dispose (not a may-throw call) and no false exceptional-leak edge + // is injected before it. Must stay silent (was a false OWN001 before — PR #32 + // CodeRabbit review). + public async Task DisposeAsyncConfiguredInTry() + { + var daci = new MemoryStream(); + try { await daci.DisposeAsync().ConfigureAwait(false); } + catch (Exception) { /* swallowed */ } + } + + // NOT a leak: `cif` is disposed on every real path (both branches of the `if`). A + // may-throw call sits in one branch after the dispose, but the exception edge must NOT + // be injected before the whole `if` (where `cif` is still owned) — edges go only before + // LEAF statements (expression / local-declaration), never compound ones, else the + // resource is falsely flagged though every path disposes it. Must stay silent (was a + // false OWN001 — PR #32 Codex review). The nested may-throw is the deferred nested-try + // slice (a sound recall gap, not a leak). + public void DisposeInsideIfWithThrow(bool c) + { + var cif = new MemoryStream(); + try + { + if (c) { cif.Dispose(); MayThrow(); } + else { cif.Dispose(); } + } + catch (Exception) { /* swallowed */ } + } + + private static void MayThrow() { } + // acquire + dispose within the loop body is balanced -> silent (no false // positive now that loops are analysed rather than skipped). public void WhileClean(int n) diff --git a/ownlang/ownir.py b/ownlang/ownir.py index 4bd4498a..4ce7898f 100644 --- a/ownlang/ownir.py +++ b/ownlang/ownir.py @@ -623,6 +623,23 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: # this side path so it bypasses the ERROR-only diagnostic mapping above. findings.extend(_unresolved_findings(facts)) + # A resource that leaks on more than one exit yields one core OWN001 per exit: + # e.g. the try-lowering injects an exceptional exit before each may-throw + # statement, so a local never disposed leaks on BOTH that exit and the normal + # fall-through. For a flow-local every such diagnostic remaps to the same acquire + # line (sub["line"]) above, collapsing to byte-identical findings — keep one. + # The key includes `line`, so genuinely distinct leak sites stay distinct. + seen: set[tuple[Any, ...]] = set() + deduped: list[Finding] = [] + for f in findings: + key = (f.file, f.line, f.code, f.component, f.event, f.handler, + f.message, f.kind, f.advisory, f.severity) + if key in seen: + continue + seen.add(key) + deduped.append(f) + findings = deduped + findings.sort(key=lambda f: (f.file, f.line, f.code)) return findings diff --git a/tests/fixtures/ownir/flow_leak_two_exits.facts.json b/tests/fixtures/ownir/flow_leak_two_exits.facts.json new file mode 100644 index 00000000..685cf399 --- /dev/null +++ b/tests/fixtures/ownir/flow_leak_two_exits.facts.json @@ -0,0 +1,18 @@ +{ + "ownir_version": 0, + "module": "Extracted", + "components": [], + "functions": [ + { + "name": "FlowLocalsSample.TryNeverDisposed", + "file": "FlowLocalsSample.cs", + "body": [ + {"op": "acquire", "var": "tfLeak", "line": 105}, + {"op": "if", "line": 106, "then": [ + {"op": "return", "var": null, "line": 106} + ], "else": []}, + {"op": "use", "var": "tfLeak", "line": 106} + ] + } + ] +} diff --git a/tests/test_ownir.py b/tests/test_ownir.py index b12a5c50..060cce00 100644 --- a/tests/test_ownir.py +++ b/tests/test_ownir.py @@ -54,6 +54,8 @@ "ownir", "flow_leak_on_else.facts.json") _WHILE_FIXTURE = os.path.join(os.path.dirname(__file__), "fixtures", "ownir", "flow_while.facts.json") +_TWO_EXITS_FIXTURE = os.path.join(os.path.dirname(__file__), "fixtures", + "ownir", "flow_leak_two_exits.facts.json") _DI_FIXTURE = os.path.join(os.path.dirname(__file__), "fixtures", "ownir", "di.facts.json") _UNRESOLVED_FIXTURE = os.path.join(os.path.dirname(__file__), "fixtures", @@ -339,6 +341,21 @@ def _one(source: str, lambda_: bool = False) -> Finding: fails.append(f"partial-release leak wrongly used the never-disposed " f"wording: {e0.message!r}") + # --- one leak, one finding: the exception-edge try-lowering injects an + # exceptional exit (a bare `return` while the local is live) before each + # may-throw statement. A local never disposed then leaks on BOTH that exit and + # the normal fall-through, so the core emits OWN001 twice for the same local. + # Every flow-local diagnostic remaps to the acquire line, so the two collapse + # to byte-identical findings — the bridge must keep exactly one (TryNeverDisposed + # 'tfLeak'). Without the dedup this returns 2. + with open(_TWO_EXITS_FIXTURE, encoding="utf-8") as f: + tefacts = json.load(f) + tefindings = check_facts(tefacts) + checks += 1 + if [(x.event, x.code, x.line) for x in tefindings] != [("tfLeak", "OWN001", 105)]: + fails.append(f"expected exactly one OWN001 on 'tfLeak'@105 (two leaking exits " + f"deduped), got {[(x.event, x.code, x.line) for x in tefindings]}") + # --- P-016 A1 reaches the frontend: a `while` flow body (the extractor now # lowers loops instead of skipping the method) routes through the core's # worklist fixpoint. A resource acquired before the loop and released INSIDE