feat(corpus): lock the ShareX TcpListener + StreamReader leaks as regressions (recall 10→12)#61
feat(corpus): lock the ShareX TcpListener + StreamReader leaks as regressions (recall 10→12)#61PhysShell wants to merge 1 commit into
Conversation
…ressions (recall 10→12)
The two remaining real (if minor) local-disposable leaks from the ShareX triage — both
already caught by the extractor (plain `new` acquires), now pinned as corpus regressions so
the catch can't silently regress:
* sharex-tcplistener-not-disposed — ShareX.HelpersLib/Helpers/WebHelpers.cs:191
(GetRandomUnusedPort): a TcpListener is Start()'d and Stop()'d but never disposed.
TcpListener is IDisposable and Stop() is not Dispose() -> the listen socket leaks
(CA2000 / CodeQL cs/local-not-disposed). The "grab a free port" idiom.
* sharex-streamreader-not-disposed — ShareX.HelpersLib/Helpers/Helpers.cs:854: a
StreamReader wraps a MemoryStream to read it to a string and is never disposed — a
separate IDisposable from the (using-scoped) stream.
Each in the established shape (before.cs / after.cs / case.own / expected-diagnostics.txt /
notes.md), self-contained on BCL types (no ref pack). after.cs `using`-scopes the reader /
listener -> silent. Ratchets the benchmark floor 10->12 so both catches are pinned.
Validated locally: both case.own -> OWN001; tests/test_corpus.py 10/10; the modeled
before.cs facts -> OWN001 'is never disposed' for both. The before.cs->facts extractor step
is validated in CI (corpus-benchmark).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
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 (11)
📝 WalkthroughWalkthroughTwo new real-world ShareX corpus cases are added for undisposed ChangesShareX Corpus Cases and CI Threshold
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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://avatars.githubusercontent.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: 637c4900b0
ℹ️ 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".
| TcpListener listener = new TcpListener(IPAddress.Loopback, 0); // <-- OWN001: never disposed | ||
| listener.Start(); | ||
| int port = ((IPEndPoint)listener.LocalEndpoint).Port; | ||
| listener.Stop(); // Stop() releases the listen socket but is NOT Dispose() -> the listener leaks |
There was a problem hiding this comment.
For the ShareX pattern this case is trying to model, Stop() is the cleanup: in the .NET implementation used by the benchmark, TcpListener.Dispose() just delegates to Stop(), and Stop() disposes the underlying socket and clears it. Once this line runs (the real method does it in finally), there is no leaked listen socket, so adding this case with expected OWN001 and ratcheting CI to require it makes the corpus protect a false positive rather than a regression.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
You're right, and this is the important kind of catch — thank you. TcpListener.Dispose() delegates to Stop() (which disposes the listen socket), so a Stop()'d listener holds no resource: flagging it OWN001 is a false positive, and locking it would have pinned an FP against our 0-FP principle. The same applies to the StreamReader case (its MemoryStream is separately using'd).
So I'm closing this PR rather than locking these. And since the extractor genuinely over-fires on Stop()-cleaned TcpListener, that's a real precision gap worth fixing — I'll harden it to model tcpListener.Stop() as a release (TcpListener-specific, resolved via the symbol; Stop() on a Timer/Process/etc. is untouched), so a never-stopped listener still leaks but a stopped one stays silent. Following up in a separate PR.
Generated by Claude Code
…eanup) (#62) 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. 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 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). The sibling StreamReader FP from the same triage is deliberately NOT changed: the extractor's general behaviour there is correct (a StreamReader over a stream not otherwise disposed genuinely leaks it); only the narrow using-scoped-stream variant is a non-leak, a rare accepted residual. See docs/notes/tcplistener-stop-release.md. Supersedes the closed #61 (which would have locked these FPs). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
Closed — these are false positives, not real leaks (Codex review, #discussion_r3448065424).
TcpListener:Dispose()delegates toStop()(which disposes the listen socket), so aStop()'d listener holds no resource — flagging it OWN001 is an FP.StreamReader: itsMemoryStreamis separatelyusing'd, so not disposing the reader leaks nothing.Locking these as must-catch OWN001 regressions would pin false positives against the 0-FP precision principle. Instead of locking the TcpListener FP, the follow-up hardens the extractor to model
tcpListener.Stop()as a release (TcpListener-specific), so a stopped listener stays silent while a never-stopped one still leaks.Original description belowThe two remaining real (if minor) local-disposable leaks from the ShareX triage — both already caught by the extractor, now pinned as corpus regressions. (Superseded: see above.)