diff --git a/corpus/oracle-fp-baseline.txt b/corpus/oracle-fp-baseline.txt index ce2708c5..7c964838 100644 --- a/corpus/oracle-fp-baseline.txt +++ b/corpus/oracle-fp-baseline.txt @@ -37,17 +37,28 @@ protobuf-net/protobuf-net | ProtoTranscoder.cs | OWN001 | 'sync' | non-product ( # a get-only property over a constructed XsltArgumentList field, handler captures `this`). # Now fixed at the source by PropertyReturnsOwnedMember — confirmed cleared on a live # protobuf oracle run (own-only 0, the finding absent from own-only and baselined) — deleted. -# FIX LANDED (using-statement local release): `using (timer) { ... }` over an already-acquired -# local is now threaded as a scope-exit release in --flow-locals. Kept here pending a live-oracle -# re-run on protobuf to confirm-and-delete. Fixture: local-dispose-via-using-statement. -protobuf-net/protobuf-net | Page.xaml.cs | OWN001 | local 'timer' | non-product (assorted/ Silverlight sample); the timer is disposed by an enclosing `using (timer) { ... }` the extractor missed +# RE-TRIAGED 2026-06-28 (live protobuf oracle run + raw source). This is a TRUE POSITIVE, not an +# FP: `Stopwatch` here is `Nuxleus.Performance.Stopwatch` (the file has `using Nuxleus.Performance;` +# and uses `Stopwatch.UnitPrecision`/`UnitPrecision.NANOSECONDS`/`timer.Scope = () => …` — NOT +# System.Diagnostics.Stopwatch), a custom IDisposable scope-timer. `Stopwatch timer = new Stopwatch()` +# is genuinely never disposed: the disposing `using (timer) { … }` block is COMMENTED OUT. The +# extractor flags it via the flow-locals path, which requires ImplementsIDisposable — so it +# correctly resolved the custom type as IDisposable. Kept baselined as NON-PRODUCT sample (assorted/ +# Silverlight perf demo), exactly like the assorted/ NetTranscoder `sync` entry. The earlier +# "missed using" and "non-IDisposable Stopwatch FP" readings were both wrong; issue #161 is closed. +protobuf-net/protobuf-net | Page.xaml.cs | OWN001 | local 'timer' | non-product (assorted/ Silverlight perf demo); TRUE POSITIVE — `timer` is a custom IDisposable Nuxleus.Performance.Stopwatch genuinely never disposed (the using(timer) block is commented out). Baselined as non-product, not as an FP # --- JamesNK/Newtonsoft.Json ---------------------------------------------------- -# STAYS BASELINED (soundness wall): JsonTextWriter is a THIRD-PARTY wrapper — the BCL pass-through -# exemption (IsNoOpDisposeWrapper) covers only StreamReader/StreamWriter/BinaryReader/BinaryWriter, -# whose disposal contract is known. Proving JsonTextWriter.Dispose is a no-op needs body analysis we -# don't do. See docs/notes/no-op-dispose-wrapper.md. -JamesNK/Newtonsoft.Json | TraceJsonReader.cs | OWN001 | _textWriter | no-op dispose: a JsonTextWriter over an in-memory StringWriter/StringBuilder holds no unmanaged resource, so leaving it undisposed releases nothing +# STAYS BASELINED — and NOT soundly auto-fixable (investigated 2026-06-28, JsonTextWriter source). +# JsonTextWriter is NOT a no-op type to dispose: Close() runs base.Close() (auto-completes open JSON +# tokens — writes closing brackets to the underlying writer) and CloseBufferAndWriter(), which RETURNS +# its rented `_writeBuffer` to `_arrayPool` when an ArrayPool is configured — a real pooled-buffer +# release (the very POOL-leak class Own.NET tracks elsewhere) — and closes the underlying writer. So a +# recursive "Dispose-is-a-no-op" recognizer would be UNSOUND (it can leak a pooled buffer) or would +# correctly DECLINE — either way it does not clear this. The `_textWriter` instance is benign only by +# INSTANCE facts (no ArrayPool set + the sink is a StringWriter), not a type-level no-op, so it stays +# baselined — consistent with excluding writers from IsNoOpDisposeWrapper. See no-op-dispose-wrapper.md. +JamesNK/Newtonsoft.Json | TraceJsonReader.cs | OWN001 | _textWriter | benign-by-instance, NOT a no-op type: JsonTextWriter.Close() returns a (possibly pooled) write buffer and auto-completes JSON tokens; harmless here only because no ArrayPool is set and the sink is a StringWriter — kept baselined (not soundly auto-fixable) JamesNK/Newtonsoft.Json | JsonSerializer.cs | OWN001 | serializer.Error | intra-call self-subscription: the serializer is freshly created from the same JsonSerializerSettings whose .Error handler it subscribes; source and handler are co-lifetimed # --- JoshClose/CsvHelper -------------------------------------------------------- diff --git a/corpus/real-world/pool-transfer-via-wrapper-local/after.cs b/corpus/real-world/pool-transfer-via-wrapper-local/after.cs new file mode 100644 index 00000000..a4f71bc8 --- /dev/null +++ b/corpus/real-world/pool-transfer-via-wrapper-local/after.cs @@ -0,0 +1,23 @@ +using System; +using System.Buffers; + +sealed class Holder : IDisposable +{ + readonly int[] _buf; + public Holder(int[] buf) => _buf = buf; + public void Dispose() => ArrayPool.Shared.Return(_buf); +} + +static class Demo +{ + // FIX: the rented buffer is wrapped into a local Holder that is RETURNED — ownership transfers to + // the Holder (which Returns the buffer on Dispose), so the buffer does not leak here. The extractor + // follows the buffer through the wrapper local because that local provably leaves the method + // (`return holder`), so it stays silent. Mirrors StackExchange.Redis Lease.Create. + public static Holder Run(int n) + { + var buf = ArrayPool.Shared.Rent(n); + var holder = new Holder(buf); + return holder; + } +} diff --git a/corpus/real-world/pool-transfer-via-wrapper-local/before.cs b/corpus/real-world/pool-transfer-via-wrapper-local/before.cs new file mode 100644 index 00000000..004dfda6 --- /dev/null +++ b/corpus/real-world/pool-transfer-via-wrapper-local/before.cs @@ -0,0 +1,24 @@ +using System; +using System.Buffers; + +// A wrapper that takes ownership of a rented buffer and returns it to the pool on Dispose. +sealed class Holder : IDisposable +{ + readonly int[] _buf; + public Holder(int[] buf) => _buf = buf; + public void Dispose() => ArrayPool.Shared.Return(_buf); +} + +static class Demo +{ + // BUG: the rented buffer is wrapped into a LOCAL Holder that never leaves the method — it is + // dropped (never disposed, never returned, never handed out) — so the buffer is genuinely never + // returned to the pool. A real pooled-buffer leak (OWN001). The wrapper being method-scoped is + // exactly the case the transfer exemption must NOT cover. + public static void Run(int n) + { + var buf = ArrayPool.Shared.Rent(n); + var holder = new Holder(buf); // method-scoped; dropped -> buffer leaks + _ = holder.GetHashCode(); + } +} diff --git a/corpus/real-world/pool-transfer-via-wrapper-local/case.own b/corpus/real-world/pool-transfer-via-wrapper-local/case.own new file mode 100644 index 00000000..5df323f3 --- /dev/null +++ b/corpus/real-world/pool-transfer-via-wrapper-local/case.own @@ -0,0 +1,19 @@ +// OwnLang model of a rented ArrayPool buffer that (in the fix) is transferred to a wrapper which +// leaves the method through a local — StackExchange.Redis Lease.Create (`var lease = new +// Lease(arr); return lease;`) and RedisServer scan (`var r = new ScanResult(keys); SetResult(m, r)`). +// The before.cs wraps the buffer in a METHOD-SCOPED Holder that is dropped, so the buffer is never +// returned -> a real leak, modelled here as an acquire with no `release` (OWN001). The after.cs +// returns the wrapper, so the extractor follows the transfer through the escaping local and stays +// silent (extractor-level; the before.cs/after.cs benchmark exercises it end to end). See notes.md. +module Corpus +resource Buffer { + acquire rent + release give + emit_type "int[]" + emit_acquire "ArrayPool.Shared.Rent({args})" + emit_release "ArrayPool.Shared.Return({0})" +} +fn Run(n: int) { + let buf = acquire Buffer(n); // ArrayPool.Rent; wrapped in a method-scoped Holder, never returned + // no `release buf;` -> OWN001 +} diff --git a/corpus/real-world/pool-transfer-via-wrapper-local/expected-diagnostics.txt b/corpus/real-world/pool-transfer-via-wrapper-local/expected-diagnostics.txt new file mode 100644 index 00000000..ed2a1929 --- /dev/null +++ b/corpus/real-world/pool-transfer-via-wrapper-local/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN001 diff --git a/corpus/real-world/pool-transfer-via-wrapper-local/notes.md b/corpus/real-world/pool-transfer-via-wrapper-local/notes.md new file mode 100644 index 00000000..aa8baf5f --- /dev/null +++ b/corpus/real-world/pool-transfer-via-wrapper-local/notes.md @@ -0,0 +1,45 @@ +# pool-transfer-via-wrapper-local + +A rented `ArrayPool` buffer handed to a **wrapper object bound to a local**, where that local +then **leaves the method** — `var w = new Wrapper(buf); return w;` / `Sink(m, w);` — is an ownership +**transfer** (the wrapper carries the buffer out and is responsible for `Return`), not a leak here. +Generalises the existing direct `return new Wrapper(buf)` / `_field = new Wrapper(buf)` transfer to +the one-extra-hop case an intermediate local introduces. + +Mined on **StackExchange.Redis** (2026-07-01 oracle sweep): + +- `Lease.Create` — `var arr = ArrayPool.Shared.Rent(length); var lease = new Lease(arr, + length); … return lease;` +- `RedisServer` scan — `keys = ArrayPool.Shared.Rent(count); … var r = new ScanResult( + cursor, keys, count, true); SetResult(message, r);` + +Both were false `OWN001` "rented but never returned" — the buffer is transferred to `Lease`/ +`ScanResult`, which owns the `Return`. + +- **before.cs** — the buffer is wrapped into a **method-scoped** `Holder` that is dropped (never + disposed / returned / handed out) → the buffer genuinely leaks → `OWN001` (the bug is caught; the + transfer exemption must **not** cover a wrapper that never leaves the method). +- **after.cs** — the `Holder` local is **returned** → the buffer transfers out with it → **clean**. + +## Recognition rule + +`PassedToEscapingCtor` already exempts a pooled buffer passed to a constructor whose result is the +direct `return` value or is assigned to a real field. `WrapperLocalEscapes` adds the one-extra-hop +case: the `new Wrapper(buf)` is bound to a **local** `w`, and `w` provably leaves the method — +`return w`, ` = w`, or `w` handed as a **call argument**. The Codex one-level rule is +preserved for the non-escaping case: a wrapper local that stays method-scoped is a borrow and the +buffer still leaks. + +## Honesty caveat + +Precision-first, mirroring the direct-transfer rule: we cannot prove the wrapper actually `Return`s +the buffer, so a wrapper that escapes but silently drops the buffer would be a (rare) missed leak. +The `w`-as-call-argument signal is the loosest — a `Sink(w)` that merely reads `w` and drops it would +be exempted — but handing a buffer-owning wrapper to a call is a transfer in idiomatic pooling code, +and erring toward no-false-positive matches the existing transfer stance. + +To stay sound against a **reused local** (Codex P2), the exemption is bound to the **declaration +form only** (`var w = new Wrapper(buf)`) and **bails if `w` is reassigned anywhere** — otherwise +`var w = new Wrapper(buf); w = other; return w;` (or `Sink(w); … w = new Wrapper(buf);`) would let a +`return`/call of a *different* value untrack `buf`, missing a real leak. A reused wrapper local keeps +the buffer flagged. diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index d7781767..d81c6830 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -186,9 +186,21 @@ architectural strictness, and the borrow-checker showcase): the subscriber stays silent (no false positive) where the token tier only warns, and a released `-=` mitigates the capture. Proven by the `capture` fixture, the `StaticEventEscapeViewModel` sample (CI `wpf-extractor` → OWN014), and the - `corpus/wpf/systemevents-region-escape` reduction (P-004 WPF005 ✅). Remaining: - migrating the *injected*-source subscription tier (today an honest OWN001 - warning) once lifetime modelling can prove or refute those sources. + `corpus/wpf/systemevents-region-escape` reduction (P-004 WPF005 ✅). The + **injected-source tier migration is now shipped** via the DI graph (P-006 + P-004, + `di_source_life`): an injected `+=` whose `source_type` resolves in the registration + graph reroutes through the region engine — a source proven to strictly **outlive** the + subscriber escalates the OWN001 *warning* to **OWN014** (a proven captive/region escape, + error-tier), a source **co-lifetimed or shorter** is *refuted* and dropped silently, and + an **unresolved** source (not in the DI graph) correctly stays the honest OWN001 warning + ("may outlive"). Covered by `test_ownir.py` (208/208 bridge checks, incl. the DI-sourced + escape + the "no DI info → stays a warning" regression). Residual is by design, not a gap: + a non-DI injected source has no provable lifetime, so warning is the honest verdict. + Escalating *those* by a curated "known app-lived source type" allowlist (e.g. MVVM + `IMessenger`/`IEventAggregator`) is **deliberately deferred as FP-prone** — modern + CommunityToolkit `WeakReferenceMessenger` holds recipients *weakly* (no leak), so a + type-name allowlist would mis-flag the safe case; it needs weak-vs-strong reference + evidence we don't model. 3. **DI lifetimes** — registration + constructor graph; captive dependency (P-006). ◑ *In progress* — DI001 lands end to end: the C# extractor builds the registration + constructor graph from `Add{Singleton,Scoped,Transient}` (generic diff --git a/docs/notes/no-op-dispose-wrapper.md b/docs/notes/no-op-dispose-wrapper.md index d3457381..02497796 100644 --- a/docs/notes/no-op-dispose-wrapper.md +++ b/docs/notes/no-op-dispose-wrapper.md @@ -55,22 +55,31 @@ The motivating oracle finding was Newtonsoft.Json's `TraceJsonReader._textWriter ```csharp _sw = new StringWriter(CultureInfo.InvariantCulture); -_textWriter = new JsonTextWriter(_sw); // a no-op in truth — but JsonTextWriter is third-party +_textWriter = new JsonTextWriter(_sw); // looks like a no-op — but JsonTextWriter is NOT a no-op type ``` -Structurally identical, but `JsonTextWriter` is a **third-party** wrapper (`JsonWriter`, -not a BCL `TextWriter`). We cannot prove *its* `Dispose` is pass-through without modelling -its body — and asserting a no-op we cannot prove is exactly the unsound over-reach that -sank the static-class subscriber exemption (see -[`oracle-known-fps.md` → Rejected approaches](oracle-known-fps.md)). So Newtonsoft's -`_textWriter` stays a **baselined** finding in -[`oracle-fp-baseline.txt`](../../corpus/oracle-fp-baseline.txt), not a silent drop. - -Retiring that baseline entry would need a general, recursive "Dispose is a no-op" -analysis over a first-party type's body (it disposes only dispose-optional members and -holds no unmanaged handle). That is a larger, higher-risk capability; until it lands, only -the BCL pass-through adapters — whose disposal contract is documented and fixed — are -exempted. +It looks structurally identical to the BCL reader case, but a 2026-06-28 read of the real +`JsonTextWriter` source shows it is **not a no-op type to dispose**: + +- `Close()` runs `base.Close()` — which **auto-completes open JSON tokens** (writes closing + brackets to the underlying writer) — then `CloseBufferAndWriter()`; +- `CloseBufferAndWriter()` **returns its rented `_writeBuffer` to `_arrayPool`** when an + `ArrayPool` is configured — a real **pooled-buffer release** (the very POOL-leak class + Own.NET tracks elsewhere) — and closes the underlying writer. + +So disposal has genuine side effects (a conditional pooled-buffer return, token-completion +writes). This is the same reason we **exclude writers** (`StreamWriter`/`BinaryWriter`) from +`IsNoOpDisposeWrapper` — a writer's `Dispose` does real work. A recursive "Dispose is a +no-op" recognizer over the body would therefore either be **unsound** (it can leak a pooled +buffer) or correctly **decline** — so it would not clear this finding regardless. Asserting a +no-op we cannot prove is exactly the unsound over-reach that sank the static-class subscriber +exemption (see [`oracle-known-fps.md` → Rejected approaches](oracle-known-fps.md)). + +The `_textWriter` *instance* is benign only by **instance facts** — no `ArrayPool` is set on +it, and its sink is a `StringWriter` — not by any type-level no-op property. So it stays a +**baselined** finding in [`oracle-fp-baseline.txt`](../../corpus/oracle-fp-baseline.txt), not +a silent drop, and the "recursive Dispose-no-op analysis" idea is **shelved as not worth +building** (it wouldn't soundly clear the one case that motivated it). ## Corpus diff --git a/docs/notes/oracle-known-fps.md b/docs/notes/oracle-known-fps.md index ab17ca84..dd873661 100644 --- a/docs/notes/oracle-known-fps.md +++ b/docs/notes/oracle-known-fps.md @@ -24,12 +24,16 @@ reason: we and the oracles occupy orthogonal niches. | disposition | count | what happens on re-run | |---|---:|---| | **Fixed in the extractor** | 6 | no longer fire (5 NLog `WaitForDispose` timers + protobuf `XsltOptions` self-cycle — see below) | -| **Baselined FP** | 6 | moved to "Known FP (baselined)", out of the triage queue | +| **Baselined FP** | 5 | moved to "Known FP (baselined)", out of the triage queue | | **Non-product (path filter)** | 2 | dropped by `--exclude-tests` (`unittest` rule) | | **True positive — kept visible** | 4 | stays in "Own.NET only" (real catch, oracle can't express) | -| **True-but-benign — kept, baselined-as-sample** | 2 | (protobuf `assorted/` samples) baselined as non-product | +| **True-but-benign — kept, baselined-as-sample** | 3 | (protobuf `assorted/` samples) baselined as non-product | -The 6 baselined FPs + the 2 non-product-sample reals = 8 findings, covered by +(Re-triage 2026-06-28: `Page.xaml.cs` `timer` moved from "Baselined FP" 6→5 to +"True-but-benign sample" 2→3 — it is a real leak of a custom `IDisposable` +`Nuxleus.Performance.Stopwatch`, not the BCL non-disposable type first assumed.) + +The 5 baselined FPs + the 3 non-product-sample reals = 8 findings, covered by **7 rules** in `corpus/oracle-fp-baseline.txt` (the two `NetTranscoder` copies share one basename-keyed rule); the 2 test-base findings are the `--exclude-tests` drops; the 4 true positives are deliberately **not** suppressed. @@ -87,7 +91,7 @@ this. | `src/protobuf-net.Core/ProtoWriter.BufferWriter.cs` `_nullWriter` | **FP → baseline** | intentional null-object kept attached for pooled reuse; `Dispose()` comments *"don't cascade dispose to the null one"* | | `assorted/.../ProtoTranscoder.cs` `sync` (×2 copies) | **true-but-benign → baseline (non-product sample)** | `NetTranscoder` isn't `IDisposable`; one `ReaderWriterLockSlim` for app lifetime in a sample/extension tree | | `assorted/ProtoGen/CommandLineOptions.cs` `XsltMessageEncountered` | **FP → baseline** | self-subscription: publisher (`xsltOptions`) and the lambda are both owned by the same `CommandLineOptions`, co-lifetimed | -| `assorted/SilverlightExtended/Page.xaml.cs` `timer` | **FP → baseline** | disposed by an enclosing `using (timer) { … }` the extractor missed (sample code) | +| `assorted/SilverlightExtended/Page.xaml.cs` `timer` | **true-but-non-product → baseline (re-triaged 2026-06-28)** | TRUE POSITIVE: `timer` is a custom `IDisposable` `Nuxleus.Performance.Stopwatch` (NOT `System.Diagnostics.Stopwatch`) never disposed — the disposing `using (timer)` is commented out. Correctly flagged; baselined as non-product sample. | | `src/BuildToolsUnitTests/AnalyzerTestBase.cs` `logging.Log` | **test noise → path filter** | xUnit fixture; per-test lifetime | | `src/BuildToolsUnitTests/GeneratorTestBase.cs` `logging.Log` | **test noise → path filter** | xUnit fixture; per-test lifetime | @@ -142,14 +146,24 @@ entry and let the oracle re-confirm clean. its owned object), so the `current?.WaitForDispose(...)` is seen as a release — own-only 4 → 3, the NLog baseline now empty. Restricted to a `null`/`default` replacement: an exchange installing a new non-null value re-arms the field and is - declined (precision-first). **Also fixed — the protobuf `Page.xaml.cs` local.** The - `using (preExistingLocal)` form — `var timer = new ...; using (timer) { ... }`, where - the resource is an already-acquired tracked local — is now threaded as a scope-exit - release in the `--flow-locals` lowering (no `acquire`; only the missing release is - added, mirroring the `MemoryPool` owner branch). Its baseline entry is marked - fix-landed, pending a live-oracle re-run on protobuf to confirm-and-delete. Corpus - fixtures: `field-dispose-via-helper`, `field-dispose-via-exchange`, - `local-dispose-via-using-statement`. + declined (precision-first). **The `using (preExistingLocal)` form is now handled** — + `var r = new ...; using (r) { ... }`, an already-acquired tracked local, is threaded as + a scope-exit release in the `--flow-locals` lowering (no `acquire`; only the missing + release, mirroring the `MemoryPool` owner branch), with sound throw-routing + (`onThrowDefinite`). Corpus fixtures: `local-dispose-via-using-statement`, + `using-statement-throw-releases`. **It does NOT clear the protobuf `Page.xaml.cs` + baseline entry — because that entry is a TRUE POSITIVE, not an FP.** A live protobuf + oracle run (2026-06-28) + the raw source settled it: `Stopwatch` there is + `Nuxleus.Performance.Stopwatch` (the file has `using Nuxleus.Performance;` and uses + `Stopwatch.UnitPrecision` / `timer.Scope = () => …`, NOT `System.Diagnostics.Stopwatch`), + a custom **`IDisposable`** scope-timer that is genuinely never disposed (the disposing + `using (timer)` block is commented out). The extractor flags it via the flow-locals path, + which gates on `ImplementsIDisposable` — so it correctly resolved the custom type and + reported a real leak. It stays baselined as **non-product sample** (assorted/ Silverlight + demo), like the `NetTranscoder` `sync` entry — not as an FP. (Two earlier readings — + "missed `using`" and "non-`IDisposable` `Stopwatch` FP" — were both wrong; issue #161 was + opened on the second and then closed as invalid.) Other custom-sink fixtures: + `field-dispose-via-helper`, `field-dispose-via-exchange`. 2. **No-op `Dispose` not modelled — PARTLY FIXED** *(Newtonsoft `TraceJsonReader. _textWriter`).* We flag any undisposed `IDisposable` structurally, without modelling @@ -164,14 +178,18 @@ entry and let the oracle re-confirm clean. native/extra resource), and a path that builds `new StreamReader(path)` (a real file handle) keeps the field flagged. Corpus fixture `field-noop-dispose-wrapper`; full rationale - [`no-op-dispose-wrapper.md`](no-op-dispose-wrapper.md). **Still open — the soundness - wall:** Newtonsoft's `_textWriter` is a `JsonTextWriter` over a `StringWriter` — - structurally the same no-op, but `JsonTextWriter` is a **third-party** wrapper whose - `Dispose` we cannot prove is pass-through without modelling its body. Suppressing it - would be the same unsound over-reach as the rejected static-class exemption, so it - **stays baselined**. Retiring it needs a general recursive "Dispose-is-a-no-op" body - analysis (a first-party type that disposes only dispose-optional members and holds no - unmanaged handle) — larger and higher-risk; deferred. Note the NLog `_xmlSource` / + [`no-op-dispose-wrapper.md`](no-op-dispose-wrapper.md). **Stays baselined — and NOT + soundly auto-fixable (investigated 2026-06-28):** Newtonsoft's `_textWriter` is a + `JsonTextWriter` over a `StringWriter`, but reading the real `JsonTextWriter` source shows + it is **not a no-op type** — `Close()` runs `base.Close()` (auto-completes open JSON + tokens, writing closing brackets) and `CloseBufferAndWriter()`, which **returns its rented + `_writeBuffer` to `_arrayPool`** when an `ArrayPool` is set (a real pooled-buffer release, + the POOL-leak class we track) and closes the writer. So a recursive "Dispose-is-a-no-op" + recognizer would be **unsound** (it can leak a pooled buffer) or correctly **decline** — + either way it would not clear this. The instance is benign only by **instance facts** (no + `ArrayPool` set + the sink is a `StringWriter`), not a type-level no-op — the same reason + we exclude writers from `IsNoOpDisposeWrapper`. The recursive-analysis idea is therefore + **shelved as not worth building**, not merely deferred. Note the NLog `_xmlSource` / `_reusable*Stream` reals are also third-party wrappers (`CharEnumerator`, `ReusableStreamCreator`) of the same benign shape and stay **kept visible** for the same reason — we can't prove their disposal is a no-op, so we don't silently drop them. @@ -200,6 +218,45 @@ entry and let the oracle re-confirm clean. `docs-src/`) are handled per-entry in the baseline rather than by polluting the generic `_is_test_path` with repo-specific directory names. +## 2026-07-01 fresh-repo sweep (Dapper, StackExchange.Redis) + +A second cross-tool sweep over two general-purpose libraries not in the original +five, to hunt new FP classes / recall gaps. + +**DapperLib/Dapper — clean.** 0 own-only findings: the ADO.NET owned-API +recognition added no noise on a heavy 3000-line ADO codebase. Oracle-only was +Infer#'s `WrappedBasicReader` "not closed" (a wrapped reader **returned to the +caller** — the ownership-transfer FP class `oracle.md` already dings Infer# for) and +CodeQL `cs/dispose-not-called-on-throw` inside large `Read`/`NextResult` methods that +`--flow-locals` honestly skips. No product bug, no FP class. + +**StackExchange/StackExchange.Redis — two pooled-buffer-transfer FP classes, both +fixed.** A rented `ArrayPool` buffer handed to a wrapper that then escapes was flagged +`OWN001` "rented but never returned", though the wrapper owns the `Return`: + +1. `Lease.Create` — `var arr = Rent(length); var lease = new Lease(arr, length); + return lease;`. The `new Wrapper(buf)` bound to a **local that then leaves the + method** was not recognised as a transfer (the direct `return new Wrapper(buf)` rule + is one-level). **Fix:** `WrapperLocalEscapes` — a wrapper local that provably leaves + (`return w` / ` = w` / `w` as a call arg) transfers the buffer; a method-scoped + wrapper still leaks (Codex's one-level rule preserved). Corpus fixture + `pool-transfer-via-wrapper-local`. +2. `RedisServer` scan — `RedisKey[] keys; … keys = Rent(count); … new ScanResult(cursor, + keys, count, true)` (the `ScanResult` owns the `Return` via `Recycle()`, called by + `CursorEnumerable` — confirmed in source). The **bare-LOCAL assignment rent** was + mis-classified as a field rent by the syntactic POOL001 pass (`FieldName` is purely + syntactic) and flagged with no escape analysis. **Fix:** gate that pass's assignment + arm on `model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol` — a bare-local + assignment rent belongs to the flow pass (which does escape analysis but does not + track the assignment form), so it is honestly untracked rather than flagged unsoundly. + +Both verified end to end: live Redis oracle re-runs took own-only 10 → 9 → 6, all +pooled-buffer findings cleared; CI golden + corpus-benchmark green. The remaining 6 +Redis own-only are real or honest warnings — the `sentinelPrimaryReconnectTimer` / +`_inputCancel` / `_outputCancel` undisposed `Timer`/`CTS` fields (benign true positives, +like serilog's `_shutdownSignal`), the two `connection.Connection*` injected-source +subscriptions (warning tier — unknown lifetime), and the `toys/` sample host — none FPs. + ## Rejected approaches ### Static-class subscriber exemption (the CsvHelper `ConsoleHost` over-reach) diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index bc4baca3..dce47c19 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2103,7 +2103,64 @@ static bool PassedToEscapingCtor(IdentifierNameSyntax idn, SemanticModel model) { Parent: BaseObjectCreationExpressionSyntax oce } } && (oce.Parent is ReturnStatementSyntax || (oce.Parent is AssignmentExpressionSyntax a && a.Right == oce - && model.GetSymbolInfo(a.Left).Symbol is IFieldSymbol)); + && model.GetSymbolInfo(a.Left).Symbol is IFieldSymbol) + // One extra hop: `var w = new Wrapper(…, buf, …); … return w / _field = w / Sink(…, w, …)`. + // The buffer transfers to the wrapper, which then ESCAPES through the local. Mined FP on + // StackExchange.Redis (Lease.Create `return new Lease(arr)` via a local; RedisServer scan + // `new ScanResult(keys)` handed to SetResult). The Codex one-level rule is PRESERVED for the + // non-escaping case: a wrapper local that never leaves the method (and never Returns the + // buffer) still leaks — WrapperLocalEscapes returns false there, so it stays flagged. + || WrapperLocalEscapes(oce, model)); + +// The object-creation `oce` (`new Wrapper(…, buf, …)`) is bound to a LOCAL that itself provably +// leaves the enclosing method — `var w = new Wrapper(buf); … return w` / `this._f = w` / `Sink(…, +// w, …)`. Only then is the buffer an ownership TRANSFER (the wrapper carries it out); a wrapper local +// that stays method-scoped is a borrow and the buffer still leaks (Codex). Precision-first, mirroring +// the direct `return new Wrapper(buf)` rule: we cannot prove the wrapper Returns the buffer, but a +// pooled buffer handed to an escaping owner is a transfer, not a leak here. +static bool WrapperLocalEscapes(BaseObjectCreationExpressionSyntax oce, SemanticModel model) +{ + // Only the DECLARATION form `var w = new Wrapper(buf)` — a fresh local bound exactly once. The + // assignment form (`w = new Wrapper(buf)`, w pre-declared and reused) is NOT handled: a reused + // local makes "does *this* wrapper escape" flow-dependent (`Sink(w); … w = new Wrapper(buf)` uses + // an earlier value; `w = new Wrapper(buf); w = other; return w` returns a different one), which + // this syntactic scan cannot answer soundly — untracking `buf` there would MISS a real leak + // (Codex P2). So we bind only a declarator and additionally bail if `w` is reassigned anywhere. + if (oce.Parent is not EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax vd } + || model.GetDeclaredSymbol(vd) is not ILocalSymbol w) + return false; + var method = oce.FirstAncestorOrSelf(); + SyntaxNode? body = method?.Body ?? (SyntaxNode?)method?.ExpressionBody; + if (body is null) + return false; + bool escapes = false; + // Do NOT descend into nested lambdas / local functions: a use of `w` captured inside a + // callable that itself never leaves the method (`Action a = () => Sink(w);`) is not an escape + // of `w`, and counting it would untrack `buf` and hide a real leak (CodeRabbit). The `w` + // declaration and its top-level uses stay in scope; only nested-callable bodies are pruned. + foreach (var id in body.DescendantNodes(n => + n is not (AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax)) + .OfType()) + { + if (!SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(id).Symbol, w)) + continue; + // `w` REASSIGNED (`w = …` / `w op= …`, w on the LHS): it no longer provably refers to the + // wrapper that owns `buf`, so we cannot claim an escape. Bail — the buffer stays flagged + // (sound: a false negative here would be a missed pool leak, worse than keeping the report). + if (id.Parent is AssignmentExpressionSyntax reassign && reassign.Left == id) + return false; + // return w; / = w; / Sink(…, w, …) — the wrapper (and thus `buf`) leaves the + // method. Recorded, not returned early, so a later reassignment can still veto it. + if (id.Parent is ReturnStatementSyntax) + escapes = true; + else if (id.Parent is AssignmentExpressionSyntax fa && fa.Right == id + && model.GetSymbolInfo(fa.Left).Symbol is IFieldSymbol) + escapes = true; + else if (id.Parent is ArgumentSyntax { Parent: ArgumentListSyntax { Parent: InvocationExpressionSyntax } }) + escapes = true; + } + return escapes; +} // Is `t` the System.Buffers.MemoryPool type — the Dispose-based pool. Mirrors IsArrayPoolType // (checked on the resolved symbol, so an aliased/injected `MemoryPool` receiver binds and a @@ -3976,9 +4033,22 @@ or ImplicitObjectCreationExpressionSyntax // --flow-locals; skip it here so it is not double-reported. EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax vd } => (flowLocals ? null : vd.Identifier.Text, false), - // a field/assignment rent (`_buf = pool.Rent(...)`) is NOT a - // flow candidate, so this pass keeps it in both modes. - AssignmentExpressionSyntax asg => (FieldName(asg.Left), true), + // a FIELD-backed rent (`_buf = pool.Rent(...)`) is NOT a flow + // candidate — rented in one member, Returned class-wide in another + // (Dispose) — so this pass keeps it in both modes. GATED on the target + // being a real field: `FieldName` is purely syntactic, so a bare LOCAL + // assignment (`keys = pool.Rent(count)`, keys pre-declared) would otherwise + // be mis-classified as a field rent and flagged with NO escape/transfer + // analysis — a false positive when the local is handed to an escaping owner + // (mined on StackExchange.Redis RedisServer scan: `keys = ArrayPool. + // Shared.Rent(count); … new ScanResult(cursor, keys, count, true)` — the + // ScanResult owns the Return via Recycle()). A bare-local assignment rent + // belongs to the flow pass (which does escape analysis); it does not track + // the assignment form, so such a rent is honestly not tracked here rather + // than flagged unsoundly. + AssignmentExpressionSyntax asg + when model.GetSymbolInfo(asg.Left).Symbol is IFieldSymbol + => (FieldName(asg.Left), true), _ => ((string?)null, false), }; if (name != null)