-
Notifications
You must be signed in to change notification settings - Fork 0
fix(extractor): model TcpListener.Stop() as a release (Stop is the cleanup, not a leak) #62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -712,6 +712,22 @@ | |
| 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) }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When callers reuse a listener ( Useful? React with 👍 / 👎. |
||
| 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, | ||
|
|
@@ -1101,7 +1117,7 @@ | |
| .Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries) | ||
| .Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase)) | ||
| .ToList(); | ||
| var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase); | ||
|
Check warning on line 1120 in frontend/roslyn/OwnSharp.Extractor/Program.cs
|
||
| var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList(); | ||
| // P-004 WPF profile: widen the reference set with assemblies named by the | ||
| // OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this release predicate only lives in
EmitFlowExpr,StatementMayThrowstill treatsTcpListener.Stop()as an arbitrary throwing call and injects an exceptional exit before the cleanup when it appears inside atry. That creates a synthetic path that skips the release, so shapes liketry { listener.Stop(); } finally { }can still produce OWN001 even though this change is explicitly modelingStopas cleanup; the Stop predicate needs to be shared with the dispose-shaped/throw-edge logic too.Useful? React with 👍 / 👎.