diff --git a/.github/workflows/oracle.yml b/.github/workflows/oracle.yml index 0d64a80a..0a290055 100644 --- a/.github/workflows/oracle.yml +++ b/.github/workflows/oracle.yml @@ -260,6 +260,11 @@ jobs: # Compare on product code by default: tests/benchmarks pollute the diff # (and Infer# only built the product project). include_tests keeps them. [[ "${INCLUDE_TESTS,,}" == "true" ]] || args+=(--exclude-tests) + # Suppress verified false positives (triaged against real source) so the + # triage queue surfaces only NEW own-only findings. Matched by name, not + # line, so it survives the target drifting at HEAD. See the file header and + # docs/notes/oracle-known-fps.md. + [[ -f corpus/oracle-fp-baseline.txt ]] && args+=(--baseline corpus/oracle-fp-baseline.txt) [[ -f infer-out/report.sarif ]] && args+=(--infersharp infer-out/report.sarif) cq=$(find codeql-out -name '*.sarif' -type f 2>/dev/null | head -1 || true) [[ -n "$cq" ]] && args+=(--codeql "$cq") diff --git a/corpus/oracle-fp-baseline.txt b/corpus/oracle-fp-baseline.txt new file mode 100644 index 00000000..d112123f --- /dev/null +++ b/corpus/oracle-fp-baseline.txt @@ -0,0 +1,48 @@ +# Cross-tool oracle — verified false-positive baseline. +# +# Each line is one own-only finding that was triaged against the target's REAL +# source (at the run commit) and confirmed NOT to be a leak. The comparator +# (scripts/oracle_compare.py --baseline) moves these out of "Own.NET only" into a +# separate "Known FP (baselined)" section, so a re-run's triage queue shows only +# genuinely-new findings. Full per-entry rationale: docs/notes/oracle-known-fps.md. +# +# Format (|-delimited, '#' starts a full-line comment, blank lines ignored): +# | | | | +# +# The match key is (repo, basename, OWN code, message-substring) — deliberately +# NOT the line number: re-runs clone the target at its current HEAD and the lines +# drift, but the field/event/local name in the message is stable. Keep substrings +# specific enough to bind to exactly the intended finding. +# +# This file suppresses ONLY confirmed false positives. True findings stay visible +# even when benign — e.g. serilog BatchingSink._shutdownSignal (a CTS never +# Dispose()d) and NLog's XmlParser._xmlSource / FileTarget._reusable*Stream +# (fields genuinely never disposed, but wrapping managed-memory-only resources). +# Those are NOT baselined; they are real catches the oracle's leak query misses. + +# --- NLog/NLog : all WaitForDispose timer FPs fixed at the source ---------------- +# All five NLog timers disposed through the `WaitForDispose(this Timer)` sink are now +# cleared in the extractor — no baseline entries remain. Four (AsyncTaskTarget +# _taskTimeoutTimer/_lazyWriterTimer, AsyncTargetWrapper _lazyWriterTimer, +# BufferingTargetWrapper _flushTimer) by CallReleasesReceiver (direct / simple-alias +# receiver), and TimeoutContinuation _timeoutTimer by RefExchangeNulledField (its +# receiver is the `Interlocked.Exchange(ref _timeoutTimer, null)` result, now bound to +# the field). The three remaining NLog own-only findings (XmlParser _xmlSource, +# FileTarget _reusable*Stream) are genuine undisposed fields, kept visible. + +# --- protobuf-net/protobuf-net -------------------------------------------------- +protobuf-net/protobuf-net | ProtoWriter.BufferWriter.cs | OWN001 | _nullWriter | intentional null-object kept attached for pooled reuse; Dispose() documents "don't cascade dispose to the null one" +protobuf-net/protobuf-net | ProtoTranscoder.cs | OWN001 | 'sync' | non-product (protobuf 'assorted' sample/extension tree); NetTranscoder is a long-lived singleton holding one ReaderWriterLockSlim for app lifetime +# CommandLineOptions XsltMessageEncountered was here (self-cycle: source `XsltOptions` is +# 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. +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 + +# --- JamesNK/Newtonsoft.Json ---------------------------------------------------- +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 +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 -------------------------------------------------------- +JoshClose/CsvHelper | ConsoleHost.cs | OWN014 | AppDomain.CurrentDomain.ProcessExit | non-product (docs-src/ doc-generator) + process-lived subscriber: ConsoleHost itself lives for the whole process, so promoting it to a process-lived event source changes nothing +JoshClose/CsvHelper | ConsoleHost.cs | OWN014 | Console.CancelKeyPress | non-product (docs-src/ doc-generator) + process-lived subscriber: same — a process-lived host subscribing to a process-lived event source is not a lifetime leak diff --git a/corpus/real-world/field-dispose-via-exchange/after.cs b/corpus/real-world/field-dispose-via-exchange/after.cs new file mode 100644 index 00000000..02e54d36 --- /dev/null +++ b/corpus/real-world/field-dispose-via-exchange/after.cs @@ -0,0 +1,33 @@ +using System; +using System.Threading; + +// The "detach and dispose" teardown: stop + dispose the timer through the sink. +static class TimerSink +{ + public static void WaitForDispose(this Timer timer, TimeSpan timeout) + { + timer.Change(Timeout.Infinite, Timeout.Infinite); + timer.Dispose(); + } +} + +// FIX: the owned Timer field is released through the canonical atomic teardown — +// `Interlocked.Exchange(ref _timer, null)` hands back the live timer and nulls the +// field, then the sink disposes it. Recognising this needs two hops: the local +// `current` is bound to `_timer` because Exchange returns the field's owned object +// (RefExchangeNulledField), and `current?.WaitForDispose(...)` is a release because +// the sink disposes its receiver (CallReleasesReceiver). Together → no leak, silent. +sealed class Continuation : IDisposable +{ + Timer? _timer; + + public Continuation() => _timer = new Timer(_ => { }, null, 0, 1000); + + public void Dispose() => StopTimer(); + + void StopTimer() + { + var current = Interlocked.Exchange(ref _timer, null); + current?.WaitForDispose(TimeSpan.Zero); + } +} diff --git a/corpus/real-world/field-dispose-via-exchange/before.cs b/corpus/real-world/field-dispose-via-exchange/before.cs new file mode 100644 index 00000000..b532135d --- /dev/null +++ b/corpus/real-world/field-dispose-via-exchange/before.cs @@ -0,0 +1,28 @@ +using System; +using System.Threading; + +// The "detach and dispose" teardown mined on NLog's TimeoutContinuation: the owned +// Timer field is meant to be released via `Interlocked.Exchange(ref _timer, null)` +// (which hands back the live timer and nulls the field) followed by the +// `WaitForDispose(this Timer)` sink. Here the teardown is missing entirely. +static class TimerSink +{ + public static void WaitForDispose(this Timer timer, TimeSpan timeout) + { + timer.Change(Timeout.Infinite, Timeout.Infinite); + timer.Dispose(); + } +} + +// BUG: the owned Timer field is constructed and never released on any path → OWN001. +sealed class Continuation : IDisposable +{ + Timer? _timer; + + public Continuation() => _timer = new Timer(_ => { }, null, 0, 1000); + + public void Dispose() + { + // nothing — _timer is leaked + } +} diff --git a/corpus/real-world/field-dispose-via-exchange/case.own b/corpus/real-world/field-dispose-via-exchange/case.own new file mode 100644 index 00000000..74ef29a9 --- /dev/null +++ b/corpus/real-world/field-dispose-via-exchange/case.own @@ -0,0 +1,21 @@ +// OwnLang model of NLog's TimeoutContinuation teardown (src/NLog/Internal/ +// TimeoutContinuation.cs, StopTimer). The owned Timer field is released by the atomic +// detach-and-dispose idiom: `Interlocked.Exchange(ref _timeoutTimer, null)` hands back +// the live timer and nulls the field, then `WaitForDispose(this Timer)` stops and +// disposes it. before.cs omits the teardown — the generic OWN001 owned-field leak, +// modelled here as an acquire with no `release`. after.cs releases it via the idiom, +// which the extractor recognises by binding the exchange result to the field +// (RefExchangeNulledField) and following the sink's dispose effect (CallReleasesReceiver). +module Corpus +resource Timer { + acquire create + release dispose + kind "disposable" + emit_type "Timer" + emit_acquire "new Timer({args})" + emit_release "Interlocked.Exchange(ref {0}, null)?.WaitForDispose()" +} +fn Continuation(callback: int) { + let timer = acquire Timer(callback); // _timer = new Timer(_ => { }, null, 0, 1000) + // no `release timer;` — the timer is never detached or disposed (OWN001) +} diff --git a/corpus/real-world/field-dispose-via-exchange/expected-diagnostics.txt b/corpus/real-world/field-dispose-via-exchange/expected-diagnostics.txt new file mode 100644 index 00000000..ed2a1929 --- /dev/null +++ b/corpus/real-world/field-dispose-via-exchange/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN001 diff --git a/corpus/real-world/field-dispose-via-exchange/notes.md b/corpus/real-world/field-dispose-via-exchange/notes.md new file mode 100644 index 00000000..d06052a8 --- /dev/null +++ b/corpus/real-world/field-dispose-via-exchange/notes.md @@ -0,0 +1,34 @@ +# field-dispose-via-exchange + +The atomic **detach-and-dispose** teardown for an owned `IDisposable` field, mined on +NLog's `TimeoutContinuation.StopTimer` (`src/NLog/Internal/TimeoutContinuation.cs`): + +```csharp +var current = Interlocked.Exchange(ref _timeoutTimer, null); +current?.WaitForDispose(TimeSpan.Zero); +``` + +`Interlocked.Exchange(ref _field, null)` atomically nulls the field and **returns the +object it used to own**, so the local `current` aliases the field's just-detached +disposable; the `WaitForDispose(this Timer)` sink then stops and disposes it. + +- **before.cs** — the `Timer` field is constructed and never released → `OWN001`. +- **after.cs** — released via the exchange + sink → **clean**. + +## Recognition rule + +Two hops, each reusing existing machinery: + +1. **`RefExchangeNulledField`** binds the local `current` to `_timer`, because + `Interlocked.Exchange(ref _timer, null)` returns the field's owned object. Restricted + to a `null`/`default` replacement — the unambiguous teardown; an exchange that installs + a *new* non-null value re-arms the field with a fresh object the syntactic scan can't + follow, so crediting the field there could hide a real leak (declined, precision-first). + The bound alias joins the same `aliasToField` map as a plain `var x = _field;`. +2. **`CallReleasesReceiver`** recognises `current?.WaitForDispose(...)` as a release, + because the first-party extension sink disposes its receiver (proved via `ConsumesParam`). + +Together they clear `TimeoutContinuation`, the last NLog timer that the +[`field-dispose-via-helper`](../field-dispose-via-helper/notes.md) fix did not reach +(its receiver was the exchange result, not a tracked field alias). With this, all five +NLog `WaitForDispose` timer false positives are fixed at the source. diff --git a/corpus/real-world/field-dispose-via-helper/after.cs b/corpus/real-world/field-dispose-via-helper/after.cs new file mode 100644 index 00000000..c9b071d4 --- /dev/null +++ b/corpus/real-world/field-dispose-via-helper/after.cs @@ -0,0 +1,27 @@ +using System; +using System.Threading; + +// A "drain and dispose" sink: stops the timer, then disposes it. Because its +// receiver parameter is disposed in the body, a `_timer.WaitForDispose(...)` call +// releases the field — the extractor proves this by inspecting the sink's body +// (ConsumesParam on the reduced extension method's receiver), not by name. +static class TimerSink +{ + public static void WaitForDispose(this Timer timer, TimeSpan timeout) + { + timer.Change(Timeout.Infinite, Timeout.Infinite); + timer.Dispose(); + } +} + +// FIX: the owned Timer field is released through the sink on Dispose. No literal +// `_timer.Dispose()` appears, so recognising this requires following the sink's +// dispose effect — once we do, there is no leak and the case is silent. +sealed class Worker : IDisposable +{ + readonly Timer _timer; + + public Worker() => _timer = new Timer(_ => { }, null, 0, 1000); + + public void Dispose() => _timer.WaitForDispose(TimeSpan.Zero); +} diff --git a/corpus/real-world/field-dispose-via-helper/before.cs b/corpus/real-world/field-dispose-via-helper/before.cs new file mode 100644 index 00000000..7d232b84 --- /dev/null +++ b/corpus/real-world/field-dispose-via-helper/before.cs @@ -0,0 +1,29 @@ +using System; +using System.Threading; + +// A "drain and dispose" sink: the canonical shape mined on NLog, where a Timer +// field is released not by a literal `_timer.Dispose()` but by a custom extension +// method that stops the timer and then disposes it (NLog's +// `WaitForDispose(this Timer, TimeSpan)` in Common/AsyncHelpers.cs). +static class TimerSink +{ + public static void WaitForDispose(this Timer timer, TimeSpan timeout) + { + timer.Change(Timeout.Infinite, Timeout.Infinite); + timer.Dispose(); + } +} + +// BUG: the owned Timer field is constructed but never released on any path — +// neither a literal `.Dispose()` nor the sink. A genuine OWN001 owned-field leak. +sealed class Worker : IDisposable +{ + readonly Timer _timer; + + public Worker() => _timer = new Timer(_ => { }, null, 0, 1000); + + public void Dispose() + { + // nothing — _timer is leaked + } +} diff --git a/corpus/real-world/field-dispose-via-helper/case.own b/corpus/real-world/field-dispose-via-helper/case.own new file mode 100644 index 00000000..dbdb566a --- /dev/null +++ b/corpus/real-world/field-dispose-via-helper/case.own @@ -0,0 +1,23 @@ +// OwnLang model of the "drain and dispose" field-release pattern mined on NLog +// (src/NLog/Targets/AsyncTaskTarget.cs and siblings). A Timer field is owned by the +// component; the real fix releases it not with a literal `_timer.Dispose()` but +// through a custom extension method `WaitForDispose(this Timer, TimeSpan)` that +// stops the timer and then disposes it (Common/AsyncHelpers.cs). The before.cs +// leaks the timer (no release on any path) — the generic OWN001 owned-field leak +// modelled here as an acquire with no `release`. The after.cs releases it via the +// sink, which the extractor now recognises by inspecting the sink's body +// (ConsumesParam on its receiver parameter); see notes.md for that recognition rule +// and which NLog variants it does and does not reach. +module Corpus +resource Timer { + acquire create + release dispose + kind "disposable" + emit_type "Timer" + emit_acquire "new Timer({args})" + emit_release "{0}.WaitForDispose()" +} +fn Worker(callback: int) { + let timer = acquire Timer(callback); // _timer = new Timer(_ => { }, null, 0, 1000) + // no `release timer;` — the timer is never stopped or disposed (OWN001) +} diff --git a/corpus/real-world/field-dispose-via-helper/expected-diagnostics.txt b/corpus/real-world/field-dispose-via-helper/expected-diagnostics.txt new file mode 100644 index 00000000..ed2a1929 --- /dev/null +++ b/corpus/real-world/field-dispose-via-helper/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN001 diff --git a/corpus/real-world/field-dispose-via-helper/notes.md b/corpus/real-world/field-dispose-via-helper/notes.md new file mode 100644 index 00000000..3a8f0c90 --- /dev/null +++ b/corpus/real-world/field-dispose-via-helper/notes.md @@ -0,0 +1,41 @@ +# field-dispose-via-helper + +An owned `IDisposable` field is released not by a literal `_field.Dispose()` but by +a **first-party extension method that disposes its receiver** — the "drain and +dispose" sink. The canonical real-world shape is NLog's +`WaitForDispose(this Timer, TimeSpan)` (`Common/AsyncHelpers.cs`), which stops the +timer (`Change(Infinite, Infinite)`) and then disposes it; targets call +`_taskTimeoutTimer.WaitForDispose(...)` from `Dispose(bool disposing)` / +`CloseTarget()`. + +- **before.cs** — the `Timer` field is constructed and never released on any path + → `OWN001` (the bug is caught). +- **after.cs** — the field is released via `_timer.WaitForDispose(...)` → **clean**. + No literal `.Dispose()` on the field appears, so the only way to see the release + is to follow the sink's dispose effect. + +## Recognition rule + +The disposal scan already credits a field released anywhere in the class by +`field.Dispose()` / `.Close()` / `.DisposeAsync()` (directly, through a +`var t = _field;` alias, or null-conditional), so a field disposed in +`Dispose(bool disposing)` or `CloseTarget()` is already handled. This case adds the +missing hop: a call `field.M(...)` also releases the field when **`M` is a +first-party extension method whose receiver it disposes**. It is proved, not +guessed — `CallReleasesReceiver` reuses `ConsumesParam` on the reduced extension +method's receiver parameter (index 0), which inspects `M`'s real body, follows +first-party forwarding chains, is cycle-guarded, and requires an `IDisposable` +parameter. So an unknown or borrowing callee never credits a release. + +## Honesty caveat — what this does and does not reach + +This clears the NLog variants where the sink is called on the field **directly** or +through a simple `var t = _field;` alias (`AsyncTaskTarget`, `AsyncTargetWrapper`, +`BufferingTargetWrapper`). The `TimeoutContinuation` variant — where the receiver is +the result of `Interlocked.Exchange(ref _timer, null)`, the detach-and-dispose +teardown — is covered separately by the sibling +[`field-dispose-via-exchange`](../field-dispose-via-exchange/notes.md) fixture +(`RefExchangeNulledField` binds the exchange result to the field, then this same sink +recognition applies). Scope is intentionally limited to **extension methods**: an +instance method disposing its own `this` is not a real dispose-delegation shape and +would drag in virtual-dispatch reasoning. diff --git a/corpus/real-world/subscription-self-owned-property/after.cs b/corpus/real-world/subscription-self-owned-property/after.cs new file mode 100644 index 00000000..3be81561 --- /dev/null +++ b/corpus/real-world/subscription-self-owned-property/after.cs @@ -0,0 +1,20 @@ +using System; + +class Bus { public event EventHandler? Changed; } + +// FIX: the watcher now OWNS the bus — a get-only property over a field it constructs. +// The subscription is therefore a self-cycle: the Watcher, the owned Bus, and the +// handler form one object graph the GC collects together. Not a leak, even with no +// `-=` (mined on protobuf-net's CommandLineOptions, where `XsltOptions` is a get-only +// property over a constructed XsltArgumentList field and the handler captures `this`). +sealed class Watcher +{ + readonly Bus _bus = new Bus(); // constructed — owned by this + Bus Channel => _bus; // get-only property returning the owned field + int _count; + + public Watcher() + { + Channel.Changed += (s, e) => _count++; // self-owned source -> silent + } +} diff --git a/corpus/real-world/subscription-self-owned-property/before.cs b/corpus/real-world/subscription-self-owned-property/before.cs new file mode 100644 index 00000000..53ec5ba8 --- /dev/null +++ b/corpus/real-world/subscription-self-owned-property/before.cs @@ -0,0 +1,19 @@ +using System; + +class Bus { public event EventHandler? Changed; } + +// BUG: the watcher subscribes a `this`-capturing handler to an INJECTED bus — an +// external object of unknown, potentially longer lifetime — and never detaches it. +// The bus's handler list keeps the Watcher alive for the bus's lifetime: a real +// subscription leak (the source may outlive `this`). +sealed class Watcher +{ + readonly Bus _bus; // injected — lifetime owned by someone else + int _count; + + public Watcher(Bus bus) + { + _bus = bus; + _bus.Changed += (s, e) => _count++; // never -= : leak + } +} diff --git a/corpus/real-world/subscription-self-owned-property/case.own b/corpus/real-world/subscription-self-owned-property/case.own new file mode 100644 index 00000000..7d2664e8 --- /dev/null +++ b/corpus/real-world/subscription-self-owned-property/case.own @@ -0,0 +1,19 @@ +// OwnLang model of the self-owned-property subscription discrimination, mined on +// protobuf-net's CommandLineOptions: a `this`-capturing handler is subscribed to an +// event on a source the component OWNS (a get-only property over a constructed field), +// which is a collectable self-cycle, NOT a leak. before.cs is the leaky counterpart — +// the same subscription on an INJECTED bus (external lifetime), never detached — the +// generic OWN001 subscription leak, modelled here as a Subscription acquire with no +// release. The extractor tells the two apart by resolving the event source: an owned +// member (this/field/get-only-owned-property) is dropped (self-cycle), an injected one +// stays a warning. See notes.md for the recognition rule (PropertyReturnsOwnedMember). +module Corpus +resource Subscription { + acquire Subscribe + release Unsubscribe + kind "subscription token" +} +fn Watch(bus: int) { + let token = acquire Subscription(bus); // _bus.Changed += (s, e) => _count++ + // no `release token;` — the injected bus is never `-=`'d -> handler leak (OWN001) +} diff --git a/corpus/real-world/subscription-self-owned-property/expected-diagnostics.txt b/corpus/real-world/subscription-self-owned-property/expected-diagnostics.txt new file mode 100644 index 00000000..ed2a1929 --- /dev/null +++ b/corpus/real-world/subscription-self-owned-property/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN001 diff --git a/corpus/real-world/subscription-self-owned-property/notes.md b/corpus/real-world/subscription-self-owned-property/notes.md new file mode 100644 index 00000000..79550c77 --- /dev/null +++ b/corpus/real-world/subscription-self-owned-property/notes.md @@ -0,0 +1,38 @@ +# subscription-self-owned-property + +An event subscription whose SOURCE is a member the component **owns** — accessed +through a get-only **property** — is a collectable self-cycle, not a leak. Mined on +protobuf-net's `CommandLineOptions`: + +```csharp +private readonly XsltArgumentList xsltOptions = new XsltArgumentList(); +public XsltArgumentList XsltOptions => xsltOptions; // get-only, owned +... +XsltOptions.XsltMessageEncountered += delegate { messageCount++; }; // handler captures `this` +``` + +The owned `XsltArgumentList`, the `CommandLineOptions` instance, and the handler form +one object graph the GC collects together — no `-=` needed. + +- **before.cs** — the same subscription on an **injected** `Bus` (external, unknown + lifetime), never detached → a real subscription leak (`OWN001`, warning: the source + may outlive `this`). +- **after.cs** — the source is now a **get-only property over a constructed field** the + component owns → self-cycle → **silent**. + +## Recognition rule + +The self-owned-source exemption already drops a subscription whose source is `this`, or +a field/local the class constructs (`owned`). This case adds the missing receiver shape: +a `this`-instance **get-only property** whose value the class owns — +`PropertyReturnsOwnedMember`: + +- an auto-property `public T X { get; } = new T();` (value constructed in place), or +- a getter that returns a constructed member: `=> _owned`, `get => _owned`, or + `get { return _owned; }` where the returned field/property is in `owned`. + +**Get-only is required.** A settable property could be reassigned to an injected, +longer-lived object after construction, which we cannot prove bounded — so a property +with any setter falls through to the honest "injected" warning (precision-first: never +silently drop a real leak). Computed getters and getters returning a parameter/injected +field likewise fall through. diff --git a/docs/notes/oracle-known-fps.md b/docs/notes/oracle-known-fps.md new file mode 100644 index 00000000..1aa8da53 --- /dev/null +++ b/docs/notes/oracle-known-fps.md @@ -0,0 +1,197 @@ +# Oracle known false positives — triage of the 2026-06-27 five-repo run + +Companion to [`oracle.md`](oracle.md). On 2026-06-27 the cross-tool oracle ran +over five top-200-NuGet, general-purpose libraries — **Newtonsoft.Json, +CsvHelper, serilog, NLog, protobuf-net** — diffing Own.NET against Infer# and +CodeQL. This note records the triage of every **own-only** finding against the +target's real source, so the verdicts are durable and the [FP +baseline](../../corpus/oracle-fp-baseline.txt) that suppresses them on re-runs is +auditable. + +Headline: across all five repos, **0 own-only findings came from the new +owned-API recognition** (ADO `ExecuteReader`/`CreateCommand`, Xml/Json +`.Create`/`.Parse`, Socket `Accept`) — those libraries don't use those APIs in a +leaking shape, so the recognition extensions added **no noise on third-party +code**. Every own-only finding is a disposable-field or event-subscription +catch — Own.NET's differentiating niche, which the oracles' leak queries (local +not-disposed) structurally cannot express. `Agree = 0` on all five for the same +reason: we and the oracles occupy orthogonal niches. + +## Disposition summary + +20 own-only findings, triaged to ground truth: + +| 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 | +| **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 | + +The 6 baselined FPs + the 2 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. + +**Update (extractor fix landed — protobuf self-cycle).** `CommandLineOptions.XsltOptions. +XsltMessageEncountered` — a `this`-capturing handler subscribed to an event on +`XsltOptions`, a get-only property over a constructed field the class owns — is now +**fixed at the source** by `PropertyReturnsOwnedMember` (the self-owned-source exemption +now covers a property receiver, not just `this`/fields/locals). A live protobuf re-run +confirmed it: own-only **0**, the finding absent from own-only and baselined. See +root-cause #3. Corpus fixture: `subscription-self-owned-property`. Newtonsoft's +`serializer.Error` stays baselined — its source escapes (a returned `Create()` result) +and the handler is a parameter's delegate, so proving non-leak needs lifetime modelling; +the "may outlive" warning is honest, not a clear FP. + +**Update (extractor fix landed — all NLog timers).** All 5 of the original NLog +`WaitForDispose` timer FPs are now **fixed at the source**, baseline entries deleted. +Four (`AsyncTaskTarget._taskTimeoutTimer`/`_lazyWriterTimer`, +`AsyncTargetWrapper._lazyWriterTimer`, `BufferingTargetWrapper._flushTimer`) by +`CallReleasesReceiver` — a live NLog re-run confirmed own-only leak total **8 → 4**. +The fifth, `TimeoutContinuation._timeoutTimer` (disposed through the +`Interlocked.Exchange(ref _timer, null)` result), by `RefExchangeNulledField`, which +binds the exchange result to the field so the sink call is seen as a release +(own-only **4 → 3**, NLog baseline now empty). See root-cause #1. Corpus fixtures: +`field-dispose-via-helper`, `field-dispose-via-exchange`. + +## Per-finding verdicts + +### NLog/NLog — 8 findings (disposable fields) + +| field / class | verdict | why | +|---|---|---| +| `_timeoutTimer` / TimeoutContinuation | **FP → baseline** | `Dispose()` → `StopTimer()` → `Timer.WaitForDispose()` on the `Interlocked.Exchange(ref _timer, null)` result (ref-alias, out of fix scope) | +| `_taskTimeoutTimer` / AsyncTaskTarget | **FP → fixed** | `Dispose(bool disposing)` → `Timer.WaitForDispose()` (direct field receiver) | +| `_lazyWriterTimer` / AsyncTaskTarget | **FP → fixed** | `Dispose(bool disposing)` → `Timer.WaitForDispose()` (direct field receiver) | +| `_lazyWriterTimer` / AsyncTargetWrapper | **FP → fixed** | `CloseTarget()` → `StopLazyWriterThread()` → `Timer.WaitForDispose()` (simple alias) | +| `_flushTimer` / BufferingTargetWrapper | **FP → fixed** | `CloseTarget()` → `Timer.WaitForDispose()` (simple alias) | +| `_xmlSource` / XmlParser | **true positive → keep** | never disposed (benign: a `CharEnumerator` over a `StringReader` over a string — no unmanaged resource) | +| `_reusableFileWriteStream` / FileTarget | **true positive → keep** | never disposed (benign: `ReusableStreamCreator` over a `MemoryStream` — managed memory only) | +| `_reusableBatchFileWriteStream` / FileTarget | **true positive → keep** | never disposed (same) | + +`WaitForDispose(this Timer, TimeSpan)` (NLog `Common/AsyncHelpers.cs`) really does +dispose the timer (`Change(Infinite,Infinite)` then `Dispose()`). All five timer +FPs route disposal through it, on a **local alias** of the field +(`Interlocked.Exchange(ref _timer, null)` / `var t = _timer`), inside either a +`Dispose(bool disposing)` override or a `CloseTarget()` lifecycle hook. The +extractor's field-disposal scan only inspects the **top-level statements of the +parameterless `Dispose()`** for a direct `field.Dispose()` — so it sees none of +this. + +### protobuf-net/protobuf-net — 7 findings + +| location | verdict | why | +|---|---|---| +| `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) | +| `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 | + +The two `BuildToolsUnitTests` findings are now dropped by `--exclude-tests`: that +camelCase project name is one dot-less path segment, which the exact dot-component +guards missed, so `_is_test_path` gained a safe `unittest` substring rule. + +### serilog/serilog — 1 finding + +| location | verdict | why | +|---|---|---| +| `BatchingSink.cs` `_shutdownSignal` (CancellationTokenSource) | **true positive → keep** | `Dispose()` *and* `DisposeAsync()` only call `_shutdownSignal.Cancel()`, never `Dispose()` — a genuine (if benign) undisposed CTS | + +Not an async-dispose-tracing miss: the CTS is disposed in **neither** path. This +is a real catch the oracles' local-not-disposed query can't express, and it stays +visible. + +### JamesNK/Newtonsoft.Json — 2 findings + +| location | verdict | why | +|---|---|---| +| `TraceJsonReader.cs` `_textWriter` | **FP → baseline** | no-op dispose: a `JsonTextWriter` over an in-memory `StringWriter`/`StringBuilder` holds no unmanaged resource | +| `JsonSerializer.cs` `serializer.Error` | **FP → baseline** | intra-call self-subscription: the serializer is freshly built from the same `JsonSerializerSettings` whose `.Error` it subscribes; co-lifetimed | + +### JoshClose/CsvHelper — 2 findings + +| location | verdict | why | +|---|---|---| +| `docs-src/.../ConsoleHost.cs` `AppDomain.CurrentDomain.ProcessExit` | **FP → baseline** | process-lived subscriber (a docs-generator host) to a process-lived event source — promoting it to that lifetime is vacuous | +| `docs-src/.../ConsoleHost.cs` `Console.CancelKeyPress` | **FP → baseline** | same | + +## Root-cause categories and the fix that would retire each baseline + +The baselined FPs cluster into four analyzer limitations. Each baseline entry is a +standing request for the corresponding capability — when it lands, retire the +entry and let the oracle re-confirm clean. + +1. **Custom dispose-sink — MOSTLY FIXED** *(was 5 NLog timers; 4 now cleared, 1 + remains).* The disposal scan already covers the whole class (so + `Dispose(bool disposing)`, `CloseTarget()`, helper methods, simple `var t = _f;` + aliases, and null-conditional `_f?.Dispose()` were all already handled) — the one + gap was the disposing-method **name**: NLog releases its timers through a custom + extension `WaitForDispose(this Timer)`, not a literal `.Dispose()`. **Shipped fix:** + `CallReleasesReceiver` (extractor) credits `field.M(...)` as a release when `M` is a + first-party extension method whose receiver it disposes — proved by reusing + `ConsumesParam` on `M`'s reduced receiver parameter (inspects the real body, follows + first-party forwarding, cycle-guarded, IDisposable-only), never guessed from the + name. A live NLog re-run confirmed it: own-only 8 → 4, the 4 direct/simple-alias + timers cleared. The fifth, `TimeoutContinuation._timeoutTimer`, disposes the result + of `Interlocked.Exchange(ref _timer, null)`; **`RefExchangeNulledField`** now binds + that exchange result to the field (the idiom atomically nulls the field and returns + 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). **Still open:** the protobuf `Page.xaml.cs` + `using (preExistingLocal)` local form, which remains baselined until using-statement + alias tracking lands. Corpus fixtures: `field-dispose-via-helper`, + `field-dispose-via-exchange`. + +2. **No-op `Dispose` not modelled** *(Newtonsoft `TraceJsonReader._textWriter`).* + We flag any undisposed `IDisposable` structurally, without modelling that the + concrete `Dispose` releases nothing (`StringWriter`/`StringReader`/`MemoryStream` + over managed memory). *Fix:* a small "dispose-is-a-no-op" allowlist of BCL + in-memory types for the field case. (Related, already shipped for one case: + [`cts-field-dispose-optional.md`](cts-field-dispose-optional.md).) Note the NLog + `_xmlSource` / `_reusable*Stream` reals are the *same* benign shape but are + **kept visible** — they're genuinely undisposed; only Newtonsoft's is also + structurally a no-op AND not worth surfacing. Revisit whether benign-managed + field leaks should be downgraded as a class. + +3. **Lifetime-unaware subscription — PARTLY FIXED** *(was protobuf `XsltOptions`, + Newtonsoft `serializer.Error`; CsvHelper process-lived host).* OWN014's premise — + a long-lived source outlives a shorter-lived subscriber — fails when publisher and + subscriber are **co-lifetimed** or the subscriber is **itself process-lived**. + **Shipped fix:** `PropertyReturnsOwnedMember` extends the self-owned-source exemption + to a **property** receiver — `this.OwnedProp.Event += handler`, where `OwnedProp` is a + get-only property over a member the class constructs, is the same collectable + self-cycle as the owned field directly (get-only required: a settable property could + be reassigned to an injected object). Cleared protobuf `XsltOptions` on a live re-run + (own-only 0); corpus fixture `subscription-self-owned-property`. **Still open:** + Newtonsoft `serializer.Error` — the source is a returned `Create()` result (escapes) + and the handler is a parameter's delegate; the source's lifetime relative to the + handler is genuinely unprovable syntactically, so the "may outlive" warning is honest + (baselined, not a clear FP). CsvHelper's process-lived host needs a "subscriber is + itself process-lived" signal — still open. See + [`subscription-leaks-and-profiles.md`](subscription-leaks-and-profiles.md). + +4. **Non-product trees** *(protobuf `assorted/`, CsvHelper `docs-src/`, protobuf + `BuildToolsUnitTests/`).* Sample/extension/doc-generator code that was never + meant to be production-clean. The generic `unittest` rule now covers camelCase + test projects; the remaining repo-specific sample trees (`assorted/`, + `docs-src/`) are handled per-entry in the baseline rather than by polluting the + generic `_is_test_path` with repo-specific directory names. + +## How the baseline stays honest + +- **Matched by name, not line** — `(repo, file-basename, OWN code, + message-substring)`. Re-runs clone the target at HEAD and line numbers drift; + the field/event/local name in the message does not. +- **Suppresses only confirmed FPs.** True positives (serilog CTS; NLog benign + field leaks) are never baselined — they remain the visible proof the niche + works. +- **Self-retiring.** Each entry names the fix that obsoletes it. When that lands, + delete the line; if the FP was real after all, the oracle re-surfaces it. +- **Verified in CI.** `oracle_compare.py --selftest` covers the baseline + loader, the name-not-line match key, repo scoping, the `*` wildcard, the + render, and the `unittest` path rule. diff --git a/docs/notes/oracle.md b/docs/notes/oracle.md index 25c3ac26..6fa46fc3 100644 --- a/docs/notes/oracle.md +++ b/docs/notes/oracle.md @@ -28,6 +28,17 @@ Two classes sit **outside** the three-way diff and are reported separately: - **Oracle findings outside our scope** — Infer#'s `NULL_DEREFERENCE`, thread-safety, taint, etc. Listed as context (counts by rule), not a gap. +A fourth, optional bucket keeps the triage queue actionable across re-runs: + +- **Known FP (baselined)** — own-only findings already triaged against the + target's real source and confirmed false. The + [`corpus/oracle-fp-baseline.txt`](../../corpus/oracle-fp-baseline.txt) allowlist + (passed as `--baseline`) moves them out of "own-only" so a re-run surfaces only + *new* findings, matched by name (repo + basename + OWN code + message-substring) + rather than line so it survives the target drifting at HEAD. Confirmed **false + positives only** — true-but-benign catches stay visible. The five-repo triage + that seeded it is in [`oracle-known-fps.md`](oracle-known-fps.md). + ## Why this is a fair-but-honest comparison - **Own.NET needs no build.** The Roslyn extractor reads a best-effort diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 048ed900..6976abf4 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -479,14 +479,69 @@ left is MemberAccessExpressionSyntax m // text), and `owned` is AST-based — not a regex. NOTE: callers must exclude timers // — a *running* timer is rooted by the dispatcher regardless of who owns the field. static bool IsSelfOwnedSource(ExpressionSyntax left, IEventSymbol ev, - SemanticModel model, HashSet owned) + SemanticModel model, HashSet owned, + ISymbol? cls) { if (left is not MemberAccessExpressionSyntax m) return !ev.IsStatic; // bare event => an instance event on `this` if (m.Expression is ThisExpressionSyntax) return true; var recv = model.GetSymbolInfo(m.Expression).Symbol; - return (recv is IFieldSymbol or ILocalSymbol) && owned.Contains(recv.Name); + if ((recv is IFieldSymbol or ILocalSymbol) && owned.Contains(recv.Name)) + return true; + // A get-only PROPERTY over a member the class owns (`this.Child.Event += h`, Child a + // `=> _owned` / `{ get; } = new()` property) is the SAME collectable self-cycle as the + // owned field directly. GATED on a `this`/bare access (`Channel` / `this.Channel`): + // `other.Channel.Event += h` reaches ANOTHER instance's property, which may outlive + // this subscriber and retain the handler, so it stays flagged (Codex P2). + return recv is IPropertySymbol { IsStatic: false } p + && m.Expression is (IdentifierNameSyntax + or MemberAccessExpressionSyntax { Expression: ThisExpressionSyntax }) + && PropertyReturnsOwnedMember(p, owned, model, cls); +} + +// A GET-ONLY property whose value the class OWNS: an auto-property initialised to +// `new X()`, or one whose getter returns a field/property the class constructs +// (`=> _owned`, `get => _owned`, `get { return _owned; }`). Such a property is part of +// `this`'s own object graph, so a subscription on its event is the same collectable +// self-cycle as subscribing on the owned field. GET-ONLY is required: a settable +// property could be reassigned to an INJECTED, longer-lived object after construction, +// which we cannot prove bounded — so we decline it (precision-first: never silently +// drop a real leak). Only the dominant owned-property shapes are recognised; anything +// else (a computed getter, a getter returning a parameter/injected field) falls through. +static bool PropertyReturnsOwnedMember(IPropertySymbol prop, HashSet owned, + SemanticModel model, ISymbol? cls) +{ + if (prop.SetMethod is not null) + return false; + foreach (var sref in prop.DeclaringSyntaxReferences) + { + if (sref.GetSyntax() is not PropertyDeclarationSyntax pd) + continue; + // auto-property `public T X { get; } = new T();` — the value is constructed here. + if (pd.Initializer?.Value is ObjectCreationExpressionSyntax + or ImplicitObjectCreationExpressionSyntax) + return true; + // `=> expr`, `get => expr`, or `get { return expr; }` returning an owned member. + var get = pd.AccessorList?.Accessors + .FirstOrDefault(ac => ac.IsKind(SyntaxKind.GetAccessorDeclaration)); + var returned = pd.ExpressionBody?.Expression + ?? get?.ExpressionBody?.Expression + ?? get?.Body?.Statements.OfType().FirstOrDefault()?.Expression; + if (returned is null) + continue; + var pm = model.Compilation.GetSemanticModel(pd.SyntaxTree); + var rsym = pm.GetSymbolInfo(returned).Symbol; + // The returned member must be one the ANALYZED class declares — compare the + // symbol's CONTAINING TYPE, not just its name, so a same-named field on another + // type cannot satisfy ownership (CodeRabbit). `owned` holds only this class's + // constructed-field names, so the name check + the containing-type check agree. + if (rsym is IFieldSymbol or IPropertySymbol + && owned.Contains(rsym.Name) + && (cls is null || SymbolEqualityComparer.Default.Equals(rsym.ContainingType, cls))) + return true; + } + return false; } // P-004 (ext): a control fetching one of its OWN template parts — @@ -2494,6 +2549,57 @@ static bool DisposesLocal(SyntaxNode body, string name) return false; } +// Does the call `recv.M(...)` RELEASE its receiver — i.e. is `M` a first-party +// EXTENSION method whose body disposes the value it is invoked on? The dispose is +// then laundered through a custom "drain and dispose" sink (e.g. NLog's +// `timer.WaitForDispose(timeout)`, which calls `timer.Change(...)` then disposes +// it) rather than a literal `recv.Dispose()`, so the disposal scan that only keys +// on `Dispose`/`Close`/`DisposeAsync` misses it and reports a false leak. We reuse +// `ConsumesParam` on the reduced extension method's RECEIVER parameter (the +// `this T` at index 0): that inspects M's real body, follows first-party +// forwarding, is cycle-guarded, and demands an IDisposable param — so we only ever +// credit a release we can SEE happen, never an unknown/borrowing callee. Scope is +// deliberately narrow (extension methods only): an instance method disposing its +// own `this` is not a real dispose-delegation shape, and widening to it would also +// have to reason about virtual dispatch — left out to keep the surface minimal. +static bool CallReleasesReceiver(IMethodSymbol? sym, SemanticModel model) +{ + if (sym?.ReducedFrom is not { } def || def.Parameters.Length == 0) + return false; + return ConsumesParam(def, def.Parameters[0], model, + new HashSet(SymbolEqualityComparer.Default)); +} + +// `Interlocked.Exchange(ref _field, null)` — the atomic "detach and hand back" +// teardown idiom — atomically nulls the field and RETURNS the object it used to +// own, so a local bound to that result aliases the field's just-detached owned +// object: disposing the local releases the field's resource (mined on NLog's +// TimeoutContinuation.StopTimer, `Interlocked.Exchange(ref _timeoutTimer, null) +// ?.WaitForDispose(...)`). Returns that field's name so the alias map can bind it +// like a plain `var x = _field;` alias. Restricted to a `null`/`default` replacement +// — the unambiguous teardown: an Exchange that installs a NEW non-null value re-arms +// the field with a fresh object whose fate the syntactic scan cannot follow, so +// crediting the field there could hide a real leak (precision-first: decline). +static string? RefExchangeNulledField(ExpressionSyntax init, SemanticModel model) +{ + if (init is not InvocationExpressionSyntax inv + || model.GetSymbolInfo(inv).Symbol is not IMethodSymbol m + || m.Name != "Exchange" + || m.ContainingType is not { Name: "Interlocked" } ct + || ct.ContainingNamespace?.ToString() != "System.Threading") + return null; + var args = inv.ArgumentList.Arguments; + if (args.Count != 2 + || !args[0].RefKindKeyword.IsKind(SyntaxKind.RefKeyword) + || !(args[1].Expression.IsKind(SyntaxKind.NullLiteralExpression) + || args[1].Expression.IsKind(SyntaxKind.DefaultLiteralExpression) + || args[1].Expression is DefaultExpressionSyntax)) // default(T) for a ref type is null too + return null; + return ThisFieldName(args[0].Expression) is { } f + && model.GetSymbolInfo(args[0].Expression).Symbol is IFieldSymbol + ? f : null; +} + // A field/local type treated as owned-disposable (syntax-only heuristic — no // semantic model): a curated set plus a few suffixes. Gated on the class `new`ing // the value, so injected/borrowed disposables are not flagged. Timer types are @@ -3201,7 +3307,7 @@ or ImplicitObjectCreationExpressionSyntax // intent, not a leak (mined: Npgsql PoolManager's `AppDomain.CurrentDomain. // ProcessExit += (_,_) => ClearAll()` shutdown hook). A handler that captures // instance state still pins it to the process, so it stays OWN014 (Codex). - if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, selfOwned) + if (!isTimer && (IsSelfOwnedSource(a.Left, ev, model, selfOwned, clsSymbol) || IsStaticHandler(a.Right, model) || (IsProcessLifetimeAppDomainEvent(ev) && HandlerRetainsNoInstance(a.Right, model)))) @@ -3282,12 +3388,21 @@ or ImplicitObjectCreationExpressionSyntax reassignedAliases.Add(als); var aliasToField = new Dictionary(SymbolEqualityComparer.Default); foreach (var decl in cls.DescendantNodes().OfType()) - if (decl.Initializer?.Value is { } init - && ThisFieldName(init) is { } af - && model.GetSymbolInfo(init).Symbol is IFieldSymbol - && model.GetDeclaredSymbol(decl) is ILocalSymbol aliasSym - && !reassignedAliases.Contains(aliasSym)) + { + if (decl.Initializer?.Value is not { } init + || model.GetDeclaredSymbol(decl) is not ILocalSymbol aliasSym + || reassignedAliases.Contains(aliasSym)) + continue; + // a plain `var x = _field;` / `var x = this._field;` alias, OR the + // `Interlocked.Exchange(ref _field, null)` teardown whose result IS the + // field's just-detached owned object — both make `x` an alias of the field. + string? af = ThisFieldName(init) is { } direct + && model.GetSymbolInfo(init).Symbol is IFieldSymbol + ? direct + : RefExchangeNulledField(init, model); + if (af is not null) aliasToField[aliasSym] = af; + } // a `.Dispose()`/`.DisposeAsync()`/`.Close()` on a field — directly (`_f.Dispose()` / `this._f.…`) // or through an alias local (translated by SYMBOL via aliasToField) — releases that field. // `Close()` counts as a release here exactly as it already does for LOCAL disposables (DisposesLocal @@ -3300,8 +3415,13 @@ or ImplicitObjectCreationExpressionSyntax // `this`/bare receiver syntactically. foreach (var inv in cls.DescendantNodes().OfType()) if (inv.Expression is MemberAccessExpressionSyntax m - && m.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close" - && ThisFieldName(m.Expression) is { } df) + && ThisFieldName(m.Expression) is { } df + // a literal `.Dispose()/.Close()/.DisposeAsync()`, OR a first-party + // extension method that disposes its receiver (`timer.WaitForDispose()`): + // both release the field. The name check is first (cheap) — the symbol + // resolution behind CallReleasesReceiver only runs for the rarer custom call. + && (m.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close" + || CallReleasesReceiver(model.GetSymbolInfo(inv).Symbol as IMethodSymbol, model))) disposed.Add(model.GetSymbolInfo(m.Expression).Symbol is ILocalSymbol ls && aliasToField.TryGetValue(ls, out var fa) ? fa : df); // Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose @@ -3312,8 +3432,9 @@ or ImplicitObjectCreationExpressionSyntax // (The same alias-by-symbol translation applies — `cts?.Dispose()` on an aliasing local.) foreach (var cae in cls.DescendantNodes().OfType()) if (ThisFieldName(cae.Expression) is { } cdf // this-instance field / alias only (not `other._f?.Close()`) - && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } - && mb.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close") + && cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb } cinv + && (mb.Name.Identifier.Text is "Dispose" or "DisposeAsync" or "Close" + || CallReleasesReceiver(model.GetSymbolInfo(cinv).Symbol as IMethodSymbol, model))) disposed.Add(model.GetSymbolInfo(cae.Expression).Symbol is ILocalSymbol lc && aliasToField.TryGetValue(lc, out var fc) ? fc : cdf); diff --git a/scripts/oracle_compare.py b/scripts/oracle_compare.py index 32138354..9342befa 100644 --- a/scripts/oracle_compare.py +++ b/scripts/oracle_compare.py @@ -224,9 +224,72 @@ def near(a: Finding, b: Finding) -> bool: "files_both": own_files & ora_files, "files_own_only": own_files - ora_files, "files_oracle_only": ora_files - own_files, + "baselined": [], } +@dataclass +class BaselineRule: + """One verified-false-positive allowlist entry. Matched against an own-only + Finding by (repo, file-basename, OWN code, message-substring) — deliberately + NOT by line number, because re-runs clone the target at its current HEAD and + the line numbers drift. `repo` may be `*` to apply to every target. See + corpus/oracle-fp-baseline.txt and docs/notes/oracle-known-fps.md.""" + repo: str + basename: str + rule: str + substr: str + reason: str = "" + + def matches(self, target: str, f: Finding) -> bool: + if self.repo not in ("*", "") and self.repo.lower() != (target or "").lower(): + return False + return (self.basename.lower() == f.fkey + and self.rule == f.rule + and self.substr in f.message) + + +def _load_baseline(path: str) -> list[BaselineRule]: + """Parse the FP baseline file. One rule per line: + | | | | + `#` starts a full-line comment; blank lines are ignored. The reason is free + text (surfaced in the report); the first four fields are the match key. A line + with fewer than four `|`-fields is skipped (malformed, not a silent match-all).""" + rules: list[BaselineRule] = [] + for raw in Path(path).read_text(encoding="utf-8").splitlines(): + s = raw.strip() + if not s or s.startswith("#"): + continue + parts = [p.strip() for p in s.split("|")] + # need all four key fields, and a NON-EMPTY message-substring: an empty substr + # would make `substr in message` always true — a wildcard that a typo could turn + # into a blanket suppressor for that file/rule (CodeRabbit). Reason keeps any `|`. + if len(parts) < 4 or not parts[3]: + continue + repo, basename, rule, substr = parts[:4] + reason = " | ".join(parts[4:]).strip() if len(parts) > 4 else "" + rules.append(BaselineRule(repo, basename, rule, substr, reason)) + return rules + + +def apply_baseline(result: dict[str, Any], target: str, + rules: list[BaselineRule]) -> int: + """Move verified-FP findings out of `own_only` into `baselined` (each paired + with its reason), so the triage queue only ever shows genuinely-new own-only + findings. Returns the number moved.""" + kept: list[tuple[Finding, list[Finding]]] = [] + base: list[tuple[Finding, str]] = [] + for f, hits in result["own_only"]: + hit = next((r for r in rules if r.matches(target, f)), None) + if hit is not None: + base.append((f, hit.reason)) + else: + kept.append((f, hits)) + result["own_only"] = kept + result["baselined"] = base + return len(base) + + def _fmt_files(s: set[str], cap: int = 12) -> str: items = sorted(s) shown = ", ".join(f"`{x}`" for x in items[:cap]) @@ -238,7 +301,7 @@ def _fmt_files(s: set[str], cap: int = 12) -> str: def render_md(result: dict[str, Any], target: str, commit: str, oracles: list[str], tol: int, own_unparsed: int = 0, excluded_tests: int = 0, exclude_tests_mode: bool = False, - max_list: int = 50) -> str: + baseline_path: str = "", max_list: int = 50) -> str: """The human-facing comparison report.""" own_leak = result["own_leak"] ora_leak = result["oracle_leak"] @@ -284,12 +347,29 @@ def render_md(result: dict[str, Any], target: str, commit: str, for f, hits in agree[:max_list] ] + baselined = result.get("baselined", []) + own_only_note = (f" — {len(baselined)} verified FP(s) baselined out, see below" + if baselined else "") out += ["", f"## Own.NET only — {len(own_only)} " - "(candidate FP, or a catch the oracle misses)", ""] + f"(candidate FP, or a catch the oracle misses{own_only_note})", ""] out += ["_(none)_"] if not own_only else [ f"- `{f.path}:{f.line}` **[{f.rule}]** {f.message}" for f, _ in own_only[:max_list] ] + if baselined: + src = baseline_path or "the FP baseline" + out += ["", f"## Known FP (baselined) — {len(baselined)} " + f"(verified false positives, suppressed via `{src}`)", "", + "These were triaged against the real source and confirmed not to be " + "leaks (the resource is disposed through an indirection we don't trace, " + "has a no-op `Dispose`, is a self-/co-lifetimed subscription, or lives " + "in non-product code). They are kept out of the triage queue so a " + "re-run surfaces only genuinely-new findings. See " + "docs/notes/oracle-known-fps.md for the full rationale per entry.", ""] + out += [f"- `{f.path}:{f.line}` **[{f.rule}]** {f.message}" + + (f" — _{reason}_" if reason else "") + for f, reason in baselined[:max_list]] + out += ["", f"## Oracle only — {len(oracle_only)} (our recall gap, or an oracle FP)", ""] out += ["_(none)_"] if not oracle_only else [ f"- `{g.path}:{g.line}` **[{g.tool}:{g.rule}]** {g.message}" @@ -326,6 +406,9 @@ def render_md(result: dict[str, Any], target: str, commit: str, "- **Own.NET only** is the triage queue — each is a candidate false positive to " "harden, *or* a real catch the oracle's leak query can't express (double-dispose, " "use-after-dispose, a non-allowlisted owning type).", + "- **Known FP (baselined)**, when present, are own-only findings already triaged " + "to ground truth and confirmed false — suppressed so re-runs surface only new " + "findings. Add or retire entries in the FP baseline file as the analyser improves.", "- **Oracle only** is our recall gap: reduce one to a minimal `.cs`, decide if it " "is in scope (interprocedural? a field? a loop/`try` shape we skip?), then model " "it or record it as a known limitation.", @@ -354,10 +437,13 @@ def to_json(result: dict[str, Any], target: str, commit: str) -> dict[str, Any]: "oracle_only": len(result["oracle_only"]), "own_unique": len(result["own_unique"]), "oracle_other": len(result["oracle_other"]), + "baselined": len(result.get("baselined", [])), }, "agree": [{"finding": _fd(f), "oracles": [_fd(h) for h in hits]} for f, hits in result["agree"]], "own_only": [_fd(f) for f, _ in result["own_only"]], + "baselined": [{"finding": _fd(f), "reason": reason} + for f, reason in result.get("baselined", [])], "oracle_only": [_fd(g) for g in result["oracle_only"]], "own_unique": [_fd(f) for f in result["own_unique"]], "oracle_other": [_fd(g) for g in result["oracle_other"]], @@ -392,9 +478,15 @@ def _is_test_path(path: str) -> bool: # guards intact — exact match for short, collision-prone names (so a single # component `SnippetEngine`/`Documentation` is NOT dropped, only an exact # `snippet`/`doc`), prefix only for the long, unambiguous plural-able ones. + # `unittest` is matched as a *substring* (not exact / dot-component): camelCase + # test projects like `BuildToolsUnitTests` are one dot-less segment, so the + # exact guards miss them — but no product directory realistically embeds + # "unittest", so the substring is safe (unlike a bare "test", which "latest" + # would trip). for part in seg.split("."): if (part in ("test", "tests", "doc", "docs", "snippet", "snippets") - or part.startswith(("benchmark", "sample", "example"))): + or part.startswith(("benchmark", "sample", "example")) + or "unittest" in part): return True return False @@ -417,6 +509,10 @@ def main(argv: list[str] | None = None) -> int: ap.add_argument("--exclude-tests", action="store_true", help="drop findings under test/benchmark/sample/example paths, " "comparing the product code only across all tools") + ap.add_argument("--baseline", default="", + help="verified-FP allowlist file; matching own-only findings are " + "moved to a separate 'Known FP (baselined)' section so re-runs " + "surface only new findings (see corpus/oracle-fp-baseline.txt)") ap.add_argument("--json", dest="json_out", default="", help="also write the structured comparison as JSON") ap.add_argument("--selftest", action="store_true", @@ -463,12 +559,21 @@ def main(argv: list[str] | None = None) -> int: "test/benchmark/sample/example paths", file=sys.stderr) result = compare(own, oracles, args.line_tol) + + baselined = 0 + if args.baseline: + rules = _load_baseline(args.baseline) + baselined = apply_baseline(result, args.target, rules) + if baselined: + print(f"--baseline: moved {baselined} verified-FP finding(s) out of " + f"own-only (from {args.baseline})", file=sys.stderr) + if args.json_out: Path(args.json_out).write_text( json.dumps(to_json(result, args.target, args.commit), indent=2), encoding="utf-8") print(render_md(result, args.target, args.commit, present, args.line_tol, - own_unparsed, excluded, args.exclude_tests)) + own_unparsed, excluded, args.exclude_tests, args.baseline)) return 0 @@ -631,7 +736,54 @@ def _selftest() -> int: fails.append(f"non-SARIF JSON masked as clean: {len(not_sarif)} findings, " f"{ns_drift} unparsed") - total = 25 + # camelCase test projects (`BuildToolsUnitTests`) are one dot-less segment, so the + # exact dot-component guards miss them; the `unittest` substring catches them while + # leaving product code (and the `latest`-style "ends in test" trap) untouched. + if not _is_test_path("src/BuildToolsUnitTests/AnalyzerTestBase.cs"): + fails.append("_is_test_path should match camelCase *UnitTests* projects") + if any(_is_test_path(p) for p in ("src/Latest/Feed.cs", "src/UnitedThings/A.cs")): + fails.append("_is_test_path should not match 'latest'/'united' as unittest") + + # --baseline: a verified-FP allowlist moves matching own-only findings into a + # separate `baselined` bucket. The match key is (repo, basename, OWN code, + # message-substring) — NEVER the line number (re-runs drift lines). own_only here + # is [b.cs OWN001 "...local 'b'..."]. + bl_rule = BaselineRule("o/r", "B.cs", "OWN001", "local 'b'", "verified FP") + r_b = compare(own, oracles, tol=3) + moved = apply_baseline(r_b, "o/r", [bl_rule]) + if moved != 1 or [f.fkey for f, _ in r_b["baselined"]] != ["b.cs"] or r_b["own_only"]: + fails.append(f"baseline: expected b.cs moved, own_only emptied; got " + f"own_only={[f.fkey for f, _ in r_b['own_only']]} " + f"baselined={[f.fkey for f, _ in r_b['baselined']]}") + # repo-scoped: the same rule under a different target must NOT match. + r_b2 = compare(own, oracles, tol=3) + if apply_baseline(r_b2, "different/repo", [bl_rule]) != 0: + fails.append("baseline must not cross repos") + # a `*` repo applies to any target. + r_b3 = compare(own, oracles, tol=3) + if apply_baseline(r_b3, "anything", [BaselineRule("*", "B.cs", "OWN001", "local 'b'")]) != 1: + fails.append("baseline '*' repo should match any target") + # the report renders the baselined section (and only when there are entries). + if "## Known FP (baselined)" not in render_md(r_b, "o/r", "x", ["infersharp"], 3, + baseline_path="bl.txt"): + fails.append("baselined section not rendered when entries present") + if "## Known FP (baselined)" in render_md(r, "o/r", "x", ["infersharp"], 3): + fails.append("baselined section rendered with no entries") + # the loader: comments/blank lines ignored, <4 fields skipped, reason optional. + import tempfile + with tempfile.NamedTemporaryFile("w", suffix=".txt", delete=False) as tf: + tf.write("# comment\n\no/r | B.cs | OWN001 | local 'b' | a reason | with a pipe\n" + "too | few | fields\n" + "o/r | B.cs | OWN001 | | empty substr is a wildcard — must be skipped\n") + tf.flush() + loaded = _load_baseline(tf.name) + # the <4-field line and the empty-substring line are both skipped; reason keeps its `|`. + if len(loaded) != 1 or loaded[0].reason != "a reason | with a pipe": + fails.append(f"baseline loader wrong: {loaded}") + if any(r.substr == "" for r in loaded): + fails.append("baseline loader must reject an empty message-substring (wildcard)") + + total = 31 for f in fails: print(f"ORACLE SELFTEST FAIL: {f}") print(f"oracle_compare selftest: {total - len(fails)}/{total} checks passed")