fix(extractor): model TcpListener.Stop() as a release (Stop is the cleanup, not a leak)#62
Conversation
…eanup) 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| // exemption (a Timer/Process Stop() would NOT release). | ||
| public void TcpListenerNeverStopped() | ||
| { | ||
| var tcpLeak = new TcpListener(IPAddress.Loopback, 0); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds ChangesTcpListener.Stop() release detection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d6c475392
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && 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.
When callers reuse a listener (listener.Stop(); listener.Start(); ...), this release leaves the local in the released state forever. TcpListener.Stop closes the current socket and the next Start creates a new one on the same object (see Microsoft docs: https://learn.microsoft.com/en-us/dotnet/api/system.net.sockets.tcplistener.stop), so a valid restart followed by a final Stop/Dispose will be reported as OWN002/OWN003, while a restart without final cleanup can be reported as use-after-release instead of the real OWN001 leak. Please either model Start as a reacquire after Stop or avoid treating Stop as a terminal release when the object is reused.
Useful? React with 👍 / 👎.
| 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")) |
There was a problem hiding this comment.
Because this release predicate only lives in EmitFlowExpr, StatementMayThrow still treats TcpListener.Stop() as an arbitrary throwing call and injects an exceptional exit before the cleanup when it appears inside a try. That creates a synthetic path that skips the release, so shapes like try { listener.Stop(); } finally { } can still produce OWN001 even though this change is explicitly modeling Stop as cleanup; the Stop predicate needs to be shared with the dispose-shaped/throw-edge logic too.
Useful? React with 👍 / 👎.
3 participants
The false positive (Codex catch on #61)
Triaging the ShareX re-mine, I nearly locked an undisposed-
TcpListenerfinding as a corpus regression — Codex caught that it's a false positive, and it was right:TcpListenerisIDisposable, andStop()isn't in our release set — so the flow detector flaggedlistenerOWN001. ButTcpListener.Dispose()just delegates toStop()(which disposes the listen socket). AfterStop()there's no held resource — so it's not a leak, and flagging it dents the 0-FP precision claim.The fix
EmitFlowExprnow modelstcpListener.Stop()as a release, alongside theDispose()/Close()/pool-Return/Show()releases:TcpListener-specific (resolved on the method's containing type —Stop()on aTimer/Process/Stopwatchis untouched) and path-sensitive like the WinFormsShow()model: a listener neverStop()'d (nor disposed) still leaks.Pinned in CI
FlowLocalsSamplemethodTcpListenerStoppednew TcpListener(...); Start(); Stop();TcpListenerNeverStoppednew TcpListener(...); Start();(noStop)Stop()-specific)The sibling not fixed (StreamReader)
The same triage had a
StreamReader-over-using'd-stream finding — also a non-leak in that shape. But unlikeTcpListener, the extractor's general behaviour there is correct: aStreamReaderover a stream not otherwise disposed genuinely leaks it. Suppressing it would need stream-ownership reasoning and cost real recall, so it's left as a rare accepted residual (not a systematic FP class). Detailed indocs/notes/tcplistener-stop-release.md.This supersedes the closed #61 (which would have locked these FPs).
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation