From 1d6c4753927b04d0a16a991f55120073f5db0378 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 21 Jun 2026 07:38:40 +0000 Subject: [PATCH] fix(extractor): model TcpListener.Stop() as a release (Stop is the cleanup) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review on #61: triaging the ShareX re-mine, the undisposed-TcpListener finding (WebHelpers.GetRandomUnusedPort: Start() then Stop() in a finally, no Dispose()) is a FALSE POSITIVE, not a leak. TcpListener.Dispose() just delegates to Stop(), which disposes the listen socket and clears it — so a Stop()'d listener holds no resource. Flagging it OWN001 dents the 0-FP precision claim (CA2000 / CodeQL flag it conventionally, but it is not a real resource leak). EmitFlowExpr now models tcpListener.Stop() as a release of the local, alongside the Dispose()/Close()/pool-Return/Show() releases. TcpListener-specific, resolved on the method's containing type via the SemanticModel (Stop() on a Timer / Process / Stopwatch / etc. does NOT dispose and stays a tracked use), and path-sensitive like the WinForms Show() model: a listener never Stop()'d (nor disposed) still leaks. Pinned by two FlowLocalsSample cases in the wpf-extractor --flow-locals step: TcpListenerStopped (Start() + Stop() -> silent, the FP this removes) and TcpListenerNeverStopped (Start(), no Stop -> OWN001, the control proving the release is Stop()-specific). Validated locally: tcpLeak -> OWN001, stopped silent. The sibling StreamReader FP from the same triage is deliberately NOT changed: unlike TcpListener, the extractor's general behaviour there is correct (a StreamReader over a stream not otherwise disposed genuinely leaks the stream); only the narrow using-scoped-stream variant is a non-leak, a rare accepted residual. See docs/notes/tcplistener-stop-release.md. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 8 +- docs/notes/tcplistener-stop-release.md | 75 +++++++++++++++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 16 ++++ frontend/roslyn/samples/FlowLocalsSample.cs | 22 ++++++ 4 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 docs/notes/tcplistener-stop-release.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b6f6d61a..11976281 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -399,6 +399,12 @@ jobs: # previously-missed leak in ShareX's DeriveCryptoData. echo "$out" | grep -qE "OWN001.*'rngLeak' is never disposed" \ || { echo "FAIL: expected OWN001 on the undisposed crypto-factory acquire"; exit 1; } + # TcpListener precision (Codex review on #61): Stop() IS the cleanup (Dispose() + # delegates to Stop()), so a Stop()'d listener is modelled as released -> silent + # ('stopped'); a listener NEVER Stop()'d still holds the socket -> OWN001 ('tcpLeak'), + # proving the release is Stop()-specific, not a blanket TcpListener exemption. + echo "$out" | grep -qE "OWN001.*'tcpLeak' is never disposed" \ + || { echo "FAIL: expected OWN001 on the never-stopped TcpListener"; 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`, @@ -420,7 +426,7 @@ jobs: # case disposes (no default) -> last case is the tail, no phantom no-match leak. # `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release # (member-binding form), so it is disposed on the return path -> silent (Codex review). - for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean; do + for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped; 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/tcplistener-stop-release.md b/docs/notes/tcplistener-stop-release.md new file mode 100644 index 00000000..66156beb --- /dev/null +++ b/docs/notes/tcplistener-stop-release.md @@ -0,0 +1,75 @@ +# `TcpListener.Stop()` is a release — Stop() is the cleanup, not a half-measure + +A precision fix from a **Codex review** (PR #61). While triaging the ShareX re-mine I +nearly locked an undisposed-`TcpListener` finding as a corpus regression — Codex caught +that it is a **false positive**, and it was right. + +## The false positive + +```csharp +public static int GetRandomUnusedPort() // ShareX WebHelpers.cs:191 +{ + TcpListener listener = new TcpListener(IPAddress.Loopback, 0); + try + { + listener.Start(); + return ((IPEndPoint)listener.LocalEndpoint).Port; + } + finally + { + listener.Stop(); + } +} +``` + +The flow detector flagged `listener` OWN001: it is an `IDisposable` (`TcpListener` +implements `IDisposable` since .NET Core 3.0), and `Stop()` is not in the release set +(`Dispose`/`Close`/`DisposeAsync`). But **`TcpListener.Dispose()` just delegates to +`Stop()`** — `Stop()` disposes the listen socket and clears it. So after `Stop()` there is +no held resource: an undisposed-but-stopped listener is **not a leak**, and flagging it +dents the 0-FP precision claim (CA2000 / CodeQL flag it conventionally, but it is not a +real resource leak). + +## The fix — Stop() is a release, for `TcpListener` + +`EmitFlowExpr` now models `tcpListener.Stop()` as a `release` of the local, alongside the +`Dispose()`/`Close()`/pool-`Return`/`Show()` releases: + +```csharp +if (expr is InvocationExpressionSyntax tlinv + && tlinv.Expression is MemberAccessExpressionSyntax + { Name.Identifier.Text: "Stop", Expression: IdentifierNameSyntax tlid } + && tracked.Contains(tlid.Identifier.Text) + && model.GetSymbolInfo(tlinv).Symbol is IMethodSymbol { ContainingType: { Name: "TcpListener" } tct } + && IsInNamespace(tct, "System", "Net", "Sockets")) +{ + nodes.Add(new { op = "release", var = tlid.Identifier.Text, line = LineOf(tlinv) }); + return; +} +``` + +It is **`TcpListener`-specific**, resolved on the method's containing type via the +SemanticModel — `Stop()` on a `Timer` / `Process` / `Stopwatch` / etc. does **not** dispose +and is untouched (stays a tracked use). And it is path-sensitive (a release at the call +site), like the WinForms `Show()` model: a listener never `Stop()`'d (nor disposed) still +leaks on the path that misses it. + +## Pinned in CI + +Two `FlowLocalsSample.cs` cases in the `wpf-extractor` `--flow-locals` step: + +- `TcpListenerStopped` — `new TcpListener(...); Start(); Stop();` → **silent** (the FP this + removes); +- `TcpListenerNeverStopped` — `new TcpListener(...); Start();` (no `Stop`) → **OWN001**, the + control proving the release is `Stop()`-specific, not a blanket `TcpListener` exemption. + +## The sibling that is *not* fixed (and why) + +The same triage had a `StreamReader` finding (a reader over a `using`-scoped `MemoryStream`, +the reader not disposed). That is also a non-leak *in that specific shape* — the stream is +disposed separately and the reader holds no unmanaged resource of its own. But unlike +`TcpListener`, the extractor's general behaviour there is **correct**: a `StreamReader` over +a stream that is *not* otherwise disposed genuinely leaks the stream (the reader owns it). +Suppressing it would need stream-ownership reasoning and would cost real recall, so it is +left as-is — the narrow `using`-scoped-stream variant is an accepted, rare residual, not a +systematic FP class like `TcpListener.Stop()`. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index e38a1677..2a42333c 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -712,6 +712,22 @@ static void EmitFlowExpr(ExpressionSyntax expr, HashSet tracked, Semanti nodes.Add(new { op = "release", var = sid.Identifier.Text, line = LineOf(sinv) }); return; } + // x.Stop() on a tracked System.Net.Sockets.TcpListener -> release: TcpListener.Dispose() + // just delegates to Stop() (which disposes the listen socket and clears it), so a + // Stop()'d listener holds no resource and is not a leak (Codex review on PR #61). It is + // TcpListener-specific, resolved via the method symbol — Stop() on a Timer / Process / etc. + // does NOT dispose, so it stays a tracked use; a listener never Stop()'d (nor Disposed) + // still leaks. + if (expr is InvocationExpressionSyntax tlinv + && tlinv.Expression is MemberAccessExpressionSyntax + { Name.Identifier.Text: "Stop", Expression: IdentifierNameSyntax tlid } + && tracked.Contains(tlid.Identifier.Text) + && model.GetSymbolInfo(tlinv).Symbol is IMethodSymbol { ContainingType: { Name: "TcpListener" } tct } + && IsInNamespace(tct, "System", "Net", "Sockets")) + { + nodes.Add(new { op = "release", var = tlid.Identifier.Text, line = LineOf(tlinv) }); + return; + } // x?.Dispose()/x?.Close()/x?.DisposeAsync() (null-conditional) is the release too — the // call is a member BINDING under a conditional access, not a member access. Mirrors // IsDisposeShaped so a `?.` dispose (e.g. in a finally) is not mistaken for a bare use, diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index ca4f0f15..0323156a 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -1,5 +1,7 @@ using System; using System.IO; +using System.Net; +using System.Net.Sockets; using System.Security.Cryptography; using System.Threading; using System.Threading.Tasks; @@ -462,6 +464,26 @@ public void CryptoFactoryDisposed() shaClean.ComputeHash(new byte[1]); shaClean.Dispose(); } + + // NOT a leak (precision): TcpListener.Stop() IS the cleanup — TcpListener.Dispose() just + // delegates to Stop(), which disposes the listen socket. So a Stop()'d listener holds no + // resource; the extractor models Stop() on a TcpListener as a release. Reduced from a + // ShareX false positive (WebHelpers.GetRandomUnusedPort, Codex review on #61). Silent. + public void TcpListenerStopped() + { + var stopped = new TcpListener(IPAddress.Loopback, 0); + stopped.Start(); + stopped.Stop(); + } + + // control (must still leak): a TcpListener never Stop()'d (nor Disposed) holds the listen + // socket -> OWN001. Proves the release is Stop()-specific, not a blanket TcpListener + // exemption (a Timer/Process Stop() would NOT release). + public void TcpListenerNeverStopped() + { + var tcpLeak = new TcpListener(IPAddress.Loopback, 0); + tcpLeak.Start(); + } } // A domain exception type literally named `Exception`, in a non-System namespace — the