From ac9f99c21fb391cbb4040edaef80e5dce21baaa7 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 21 Jun 2026 06:19:27 +0000 Subject: [PATCH 1/2] fix(extractor): treat closure-captured locals as escaped (ShareX throttler FP) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Triaging the ShareX re-mine left one false positive among the surviving local-disposable findings: Helpers.ForEachAsync's `SemaphoreSlim throttler`. It is captured by the async lambdas of `items.Select(async i => { await throttler.WaitAsync(); ... })` and those lambdas escape via the returned `Task.WhenAll(tasks)`, so the semaphore must outlive the method frame — it cannot be disposed at method scope and is not a method-local leak. The flow detector flagged it OWN001. The escape filter already untracks a local that escapes by return / out / argument (an ambiguous ownership transfer). A capture into a closure is the same kind of escape, so the filter now also untracks a candidate whose reference is lexically inside a lambda / anonymous method / local function (a syntactic ancestor walk to the method body — no data-flow analysis, crash-proof). It does not require proving the closure escapes: a captured local MAY outlive the method, and the precision-first stance is to not flag what cannot be proven to leak (a sound, bounded recall gap, like the argument-passing case). Not a blanket SemaphoreSlim exemption: SemaphoreSlim is not dispose-optional (accessing AvailableWaitHandle allocates a handle Dispose must release), so a method-bounded one must still be flagged. The bug is the capture/escape, not the type. Pinned by two FlowLocalsSample cases in the wpf-extractor --flow-locals step: ThrottlerCaptured (a SemaphoreSlim captured by a returned async lambda -> silent) and SemaphoreLeaks (a non-captured SemaphoreSlim never disposed -> OWN001, the control proving the exemption is closure-capture). The existing UnitOfWorkFlowSample OWN001 is unaffected: its `uow` is the receiver of `uow.Member` (outside the `.Where(p => ...)` lambda bodies) and the `join ... in uow.TempProducts` is query syntax, not an AnonymousFunctionExpression -- verified the ancestor walk never marks `uow` captured. Writeup in docs/notes/closure-capture-escape-precision.md. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 9 +- .../notes/closure-capture-escape-precision.md | 83 +++++++++++++++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 19 +++++ frontend/roslyn/samples/FlowLocalsSample.cs | 27 ++++++ 4 files changed, 137 insertions(+), 1 deletion(-) create mode 100644 docs/notes/closure-capture-escape-precision.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7b1f73cb..bd417aff 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -380,6 +380,13 @@ jobs: || { echo "FAIL: expected OWN001 on the undisposed local in a do-while loop"; exit 1; } echo "$out" | grep -qE "OWN001.*'swLeak'" \ || { echo "FAIL: expected OWN001 on the switch default-branch leak"; exit 1; } + # closure-capture escape (precision): a SemaphoreSlim captured by a returned async + # lambda outlives the method, so it cannot be disposed at method scope -> escaped -> + # silent ('captured'). A SemaphoreSlim NOT captured and never disposed STILL leaks -> + # OWN001 ('semLeak'), proving the exemption is closure-capture, not a blanket + # SemaphoreSlim dispose-optional (reduced from a ShareX FP — Helpers.ForEachAsync). + echo "$out" | grep -qE "OWN001.*'semLeak' is never disposed" \ + || { echo "FAIL: expected OWN001 on the non-captured SemaphoreSlim leak"; exit 1; } # dispose-optional (Task), disposed/escaping locals, a `for` loop whose # disposable is disposed after it (`looped`, balanced), a balanced # acquire+dispose in a loop (`whileClean`), a try/finally dispose (`tfClean`, @@ -401,7 +408,7 @@ jobs: # case disposes (no default) -> last case is the tail, no phantom no-match leak. # `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release # (member-binding form), so it is disposed on the return path -> silent (Codex review). - for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf; do + for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured; do if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi done echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)" diff --git a/docs/notes/closure-capture-escape-precision.md b/docs/notes/closure-capture-escape-precision.md new file mode 100644 index 00000000..1814435b --- /dev/null +++ b/docs/notes/closure-capture-escape-precision.md @@ -0,0 +1,83 @@ +# Closure-capture escape — a local captured by a lambda is not method-bounded + +A precision fix driven by triaging the **re-mine of ShareX** (after the WinForms +modeless-`Form` fix, #57): of the seven local-disposable findings that survived, six +were real or defensible and **one was a false positive** — `Helpers.ForEachAsync`'s +`SemaphoreSlim throttler`: + +```csharp +public static Task ForEachAsync(IEnumerable items, Func body, int max) +{ + SemaphoreSlim throttler = new SemaphoreSlim(max, max); + + IEnumerable tasks = items.Select(async input => + { + await throttler.WaitAsync(); // throttler used INSIDE the lambda + try { await body(input); } finally { throttler.Release(); } + }); + + return Task.WhenAll(tasks); // the lambdas (and throttler) escape +} +``` + +The flow detector saw `throttler = new SemaphoreSlim(...)` (an undisposed `IDisposable` +local) and flagged OWN001. But `throttler` is **captured by the async lambdas**, and +those lambdas escape the method — they run while the returned `Task.WhenAll(tasks)` is +awaited by the caller. The semaphore must stay alive until every task finishes, so it +*cannot* be disposed at method scope. It is not a method-local leak. + +## The fix — capture into a closure is an escape + +The flow detector already untracks a local that escapes by **return**, **out/ref**, or +being **passed as an argument** (an ambiguous ownership transfer). A capture into a +closure is the same kind of escape — the closure can be stored, returned, or run async, +so the local outlives the method frame. The escape filter now also untracks a candidate +local when any reference to it is **lexically inside a lambda / anonymous method / local +function** body: + +```csharp +// in the --flow-locals escape filter, before the return/out/arg checks: +var capturedInClosure = false; +for (var a = idn.Parent; a is not null && a != mbody; a = a.Parent) + if (a is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax) + { capturedInClosure = true; break; } +if (capturedInClosure) { escapedLocals.Add(nm); continue; } +``` + +It is purely syntactic (an ancestor walk to the method body), so it is crash-proof and +needs no escape/data-flow analysis of where the closure goes. It deliberately does **not** +require proving the closure escapes: a captured local *may* outlive the method, and the +precision-first stance is to not flag what we cannot prove leaks. + +### Why not just exempt `SemaphoreSlim`? + +`SemaphoreSlim` is **not** unconditionally dispose-optional: accessing +`AvailableWaitHandle` lazily allocates a wait handle that `Dispose()` must release (CA2000 +flags an undisposed one). Blanket-exempting it (the way `Task`/`DataTable` are exempt in +`IsDisposeOptional`) would be unsound — it would hide a real method-local semaphore leak. +The bug here is the **capture/escape**, not the type, so the fix targets the capture. + +## The recall trade-off (sound, bounded) + +The rule is conservative: a local captured by a closure that does **not** escape, and is +never disposed, is now silenced too (e.g. `var s = new MemoryStream(); Action a = () => +s.Use(); a(); /* never disposed */`). Proving such a closure stays method-local is exactly +the data-flow analysis the syntactic rule avoids, so this is an accepted recall gap, never +a false positive — the same precision-over-recall trade the escape filter already makes for +argument-passing. + +## Pinned in CI + +`frontend/roslyn/samples/FlowLocalsSample.cs` gains two cases, asserted in the +`wpf-extractor` `--flow-locals` step: + +- `ThrottlerCaptured` — a `SemaphoreSlim` captured by a returned async lambda → **silent** + (the FP this removes); +- `SemaphoreLeaks` — a `SemaphoreSlim` **not** captured by any closure and never disposed → + **OWN001**, proving the exemption is closure-capture, not a blanket `SemaphoreSlim` + dispose-optional. + +The existing `UnitOfWorkFlowSample` OWN001 is unaffected: its `uow` is always the +*receiver* of `uow.Member` (outside the `.Where(p => …)` lambda bodies, which reference +`p`), and the `join … in uow.TempProducts` is query syntax, not an +`AnonymousFunctionExpressionSyntax` — so the ancestor walk never marks `uow` captured. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 29262a65..c2e91de3 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1450,6 +1450,25 @@ or ImplicitObjectCreationExpressionSyntax } init var nm = idn.Identifier.Text; if (!candidates.Contains(nm)) continue; + // Captured into a CLOSURE (lambda / anonymous method / local function): + // the closure can outlive the method — stored, returned, or run async — + // so the local is no longer method-bounded and cannot be disposed at + // method scope. Conservatively treat the capture as an escape (don't + // flag it), the same way a returned/out-passed local is untracked. + // Reduced from a ShareX false positive: a SemaphoreSlim throttler captured + // by the async lambdas of a returned `Task.WhenAll(...)` (Helpers.ForEachAsync). + var capturedInClosure = false; + for (var a = idn.Parent; a is not null && a != mbody; a = a.Parent) + if (a is AnonymousFunctionExpressionSyntax or LocalFunctionStatementSyntax) + { + capturedInClosure = true; + break; + } + if (capturedInClosure) + { + escapedLocals.Add(nm); + continue; + } // ... unless it is handed to a CONSUMER (a first-party method that // disposes a by-value IDisposable param) as a bare `Consume(s);` // statement: that is a handoff RELEASED at the call site, not an escape diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index 2b65da94..5930ec22 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -403,6 +403,33 @@ public async Task DisposedAsyncConfigured() asyncDisposedCfg.WriteByte(1); await asyncDisposedCfg.DisposeAsync().ConfigureAwait(false); } + + // NOT a leak (precision): a SemaphoreSlim captured into an async lambda whose Task is + // returned outlives the method — the caller awaits it, so it cannot be disposed at + // method scope. A local referenced inside a closure (lambda / anonymous method / local + // function) is treated as escaped, exactly like a returned/out-passed local. Reduced + // from a ShareX false positive (Helpers.ForEachAsync's `throttler` captured by the async + // lambdas of a returned Task.WhenAll). Silent. + public Task ThrottlerCaptured(int max) + { + var captured = new SemaphoreSlim(max, max); + Func run = async () => + { + await captured.WaitAsync(); + captured.Release(); + }; + return run(); + } + + // control (must still leak): a SemaphoreSlim NOT captured by any closure and never + // disposed -> OWN001. Proves the exemption is about CLOSURE CAPTURE (escape), not + // SemaphoreSlim being blanket dispose-optional — it is not (accessing AvailableWaitHandle + // allocates a handle Dispose must release), so it must stay tracked when method-bounded. + public void SemaphoreLeaks() + { + var semLeak = new SemaphoreSlim(1, 1); + semLeak.Wait(); + } } // A domain exception type literally named `Exception`, in a non-System namespace — the From 6e1b44a115f065809c84fe477caa902a87b24519 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 21 Jun 2026 06:25:30 +0000 Subject: [PATCH 2/2] fix(extractor): don't let a nameof operand masquerade as capture/escape (Codex #59) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review: a `nameof(x)` operand inside a lambda exposes an IdentifierNameSyntax under the closure, but `nameof` is a compile-time string that captures nothing — so the new ancestor-only closure check would mark the local escaped and suppress a real OWN001. The same hole already existed on the argument path (`nameof(s)` makes `s` look like an argument). Skip nameof operands at the top of the escape filter, closing both paths: the local stays tracked and a method-bounded leak is still reported. Pinned by a NameofInLambda sample case (a MemoryStream mentioned only via nameof inside a lambda, never disposed -> OWN001) asserted in the wpf-extractor --flow-locals step. Validated locally: nofLeak -> OWN001 'is never disposed' through the core. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 5 +++++ docs/notes/closure-capture-escape-precision.md | 6 ++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 9 +++++++++ frontend/roslyn/samples/FlowLocalsSample.cs | 11 +++++++++++ 4 files changed, 31 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bd417aff..4f65577c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -387,6 +387,11 @@ jobs: # SemaphoreSlim dispose-optional (reduced from a ShareX FP — Helpers.ForEachAsync). echo "$out" | grep -qE "OWN001.*'semLeak' is never disposed" \ || { echo "FAIL: expected OWN001 on the non-captured SemaphoreSlim leak"; exit 1; } + # a `nameof(x)` operand inside a lambda is NOT a closure capture (it is a compile-time + # string) -> the local stays method-bounded and still leaks -> OWN001 (Codex review on + # #59: nameof must not masquerade as a capture/escape). + echo "$out" | grep -qE "OWN001.*'nofLeak' is never disposed" \ + || { echo "FAIL: expected OWN001 on the nameof-in-lambda local (not a capture)"; exit 1; } # dispose-optional (Task), disposed/escaping locals, a `for` loop whose # disposable is disposed after it (`looped`, balanced), a balanced # acquire+dispose in a loop (`whileClean`), a try/finally dispose (`tfClean`, diff --git a/docs/notes/closure-capture-escape-precision.md b/docs/notes/closure-capture-escape-precision.md index 1814435b..c9b84464 100644 --- a/docs/notes/closure-capture-escape-precision.md +++ b/docs/notes/closure-capture-escape-precision.md @@ -49,6 +49,12 @@ needs no escape/data-flow analysis of where the closure goes. It deliberately do require proving the closure escapes: a captured local *may* outlive the method, and the precision-first stance is to not flag what we cannot prove leaks. +One syntactic exception: a **`nameof(x)`** operand is skipped before the escape checks. It +exposes an `IdentifierNameSyntax` under the closure (or as an argument), but `nameof` is a +compile-time string that captures and transfers nothing — so it must not be mistaken for a +capture (nor, on the argument path, an ownership transfer), or a still-leaking method-bounded +local would be wrongly untracked (Codex review). Pinned by the `NameofInLambda` sample case. + ### Why not just exempt `SemaphoreSlim`? `SemaphoreSlim` is **not** unconditionally dispose-optional: accessing diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index c2e91de3..da6116ba 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1450,6 +1450,15 @@ or ImplicitObjectCreationExpressionSyntax } init var nm = idn.Identifier.Text; if (!candidates.Contains(nm)) continue; + // A `nameof(x)` operand is a compile-time string, not a real reference: it + // neither uses, captures, nor transfers the local. Skip it so it triggers no + // escape rule — otherwise `nameof(s)` would look like an argument (the arg + // rule below) or, inside a lambda, a closure capture, and wrongly untrack a + // still-leaking method-bounded local (Codex review on PR #59). + if (idn.Parent is ArgumentSyntax { Parent: ArgumentListSyntax + { Parent: InvocationExpressionSyntax ninv } } + && ninv.Expression is IdentifierNameSyntax { Identifier.Text: "nameof" }) + continue; // Captured into a CLOSURE (lambda / anonymous method / local function): // the closure can outlive the method — stored, returned, or run async — // so the local is no longer method-bounded and cannot be disposed at diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index 5930ec22..33a5ec1d 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -430,6 +430,17 @@ public void SemaphoreLeaks() var semLeak = new SemaphoreSlim(1, 1); semLeak.Wait(); } + + // OWN001 (Codex review on #59): a `nameof(x)` operand inside a lambda exposes an + // identifier under the closure, but `nameof` is a compile-time string — it captures + // nothing. `nofLeak` is only mentioned via nameof, so it is still method-bounded and + // never disposed -> a real leak. The nameof operand must not be mistaken for a capture. + public void NameofInLambda() + { + var nofLeak = new MemoryStream(); + Action log = () => System.Console.WriteLine(nameof(nofLeak)); + log(); + } } // A domain exception type literally named `Exception`, in a non-System namespace — the