fix(extractor): exempt WinForms modeless Form.Show() locals from the disposable-leak detector#57
Merged
Merged
Conversation
…disposable-leak detector
Mining ShareX (a real WinForms app) with the --flow-locals local-IDisposable
detector surfaced a systematic false-positive class: our WPF-tuned detector
flagged the single most idiomatic WinForms line —
var form = new SettingsForm(); // : System.Windows.Forms.Form
form.Show(); // modeless
as an OWN001 leak. It is not. Control.Show() opens the form *modeless*: WinForms
owns the lifetime and disposes the form when the user closes it (disposing it
yourself would be a bug). A *modal* dialog shown via ShowDialog() is the caller's
to dispose — and an undisposed one IS a real leak that must stay flagged. The
discriminator is exactly the show method.
The fix adds two extractor helpers — DerivesFromWinFormsForm (walks the base
chain for System.Windows.Forms.Form) and IsModelessShownForm (a Form-derived
local with a local.Show() in the method body) — and one guard on the
--flow-locals candidate condition: drop a modeless-shown Form-derived local from
the tracked set. It is per-local and Show()-only by design: a ShowDialog()'d
form, a Form constructed but never shown, and any non-Form disposable are all
untouched. No new finding path, no core change — a narrowing of the candidate
set, exactly like the IsDisposeOptional exemptions.
Pinned by a self-contained WinFormsModelessSample.cs (a stub
System.Windows.Forms.Form stands in for the framework type, which is not loaded
in the flow step) wired into the wpf-extractor --flow-locals steps:
modeless Show() silent; modal ShowDialog() never disposed -> OWN001; modal
ShowDialog() + Dispose silent. The core half (facts -> verdict) was validated
locally on the hand-built flow facts before pushing. Writeup in
docs/notes/winforms-modeless-precision.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://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // never is -> real leak. The Show()-only exemption deliberately does not cover it. | ||
| public void OpenModalLeak() | ||
| { | ||
| var modalLeak = new ModalDialog(); |
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends the ChangesWinForms modeless Form exemption in --flow-locals
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 627f0c2f74
ℹ️ 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".
…th-sensitive) Codex review on #57: the first cut dropped a Form local from tracking whenever a local.Show() appeared anywhere in the method body — a method-wide exemption that is not path-sensitive. `var f = new Form(); if (open) f.Show();` went silent even though the open == false path constructs a form that is never handed to WinForms and never disposed (a real leak). Switch to the project's own call-site-release shape — the same one pool Return and the inter-procedural consume contract use: a modeless local.Show() transfers ownership to the framework ON THAT PATH, so EmitFlowExpr emits a `release` of the local at the show site (guarded by the Form-derived receiver type). The flow engine then does the rest path-sensitively: - `f.Show();` -> acquire+release balanced -> silent - `if (open) f.Show();` -> released on then, not else -> OWN001 (every-path) - `d.ShowDialog();` -> ShowDialog not matched (use) -> OWN001 if undisposed Removes the IsModelessShownForm helper and the candidate-loop guard; keeps DerivesFromWinFormsForm (now used by the release branch). No core change — reuses the existing release op like the other call-site-release branches. WinFormsModelessSample gains OpenModelessConditional pinning the path-sensitivity (`'condForm' may not be disposed on every path`); the CI step asserts it alongside the unconditional-modeless silence, the modal ShowDialog leak, and the disposed-modal silence. Core half validated locally on the hand-built flow facts before pushing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // would have wrongly silenced this. | ||
| public void OpenModelessConditional(bool open) | ||
| { | ||
| var condForm = new ModelessForm(); |
PhysShell
pushed a commit
that referenced
this pull request
Jun 21, 2026
#57 models a modeless WinForms Form.Show() as a path-sensitive call-site release, so the first hunt's dominant noise (`new SomeForm().Show()` flagged as OWN001) should be gone. Re-mine ShareX on the post-#57 extractor to confirm the drop and surface the next layer — remaining precision targets (Controls.Add / components-container ownership) or genuine leaks underneath. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
added a commit
that referenced
this pull request
Jun 21, 2026
… (recall 9→10) (#58) Re-mining ShareX on the post-#57 extractor (modeless-Form FPs gone) left a short, mostly-real list of local-disposable findings. The cleanest real bug: ShareX.UploadersLib/FileUploaders/Vault_ooo.cs:216 (DeriveCryptoData) creates an Rfc2898DeriveBytes (PBKDF2 deriver, holds an HMAC) to derive the AES key + IV, then returns without disposing it — a real leak on every upload. It does not escape (the returned CryptoData holds only the derived byte[]s), so the flow detector catches it as OWN001. An oversight, not a pattern: the sibling EncryptBytes() already `using`-scopes its aes/MemoryStream/CryptoStream. Captured as corpus/real-world/sharex-rfc2898-derivebytes-leak/ (before.cs / after.cs / case.own / expected-diagnostics.txt / notes.md) and ratchets the benchmark floor 9→10 so the catch is pinned. notes.md records the honest recall gap the same method exposes: a second leak, rng = RandomNumberGenerator.Create(), missed because it is a static factory (the detector knows `new` and File.Open*/Create*, not arbitrary X.Create()) — a separate slice; after.cs disposes both so the fix is genuinely clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
pushed a commit
that referenced
this pull request
Jun 21, 2026
…es for consume inference
Codex P2: ConsumesParam's DescendantNodesAndSelf scan (and the pre-existing DisposesLocal)
descended into nested lambdas / local functions, so a parameter disposed or forwarded ONLY
inside a stored callback — `Register(Stream s) { callbacks.Add(() => Inner(s)); }` — was
wrongly treated as consumed at the call site. That would release the caller's argument at
`Register(s)` and report a phantom use-after-handoff (false OWN002), even though the dispose
runs DEFERRED, not at the call boundary.
Both scans now use DescendantNodesAndSelf(descendIntoChildren: not lambda/local-function) —
the same deferred-body rule the flow lowering already follows (PR #57/#59). A dispose/forward
inside a nested function contributes nothing to the consume inference. Conservative: it can
only DROP a consume (a missed handoff = a leak, never a false use-after-release).
Pinned by a flow-locals silent control: DeferredHandoffNoFalsePositive's `defer` is passed to
a deferred-lambda "consumer", used, then disposed normally — it must stay SILENT (it would
trip a false OWN002 without the fix). The transitive corpus case (Inner forwarded directly,
not in a lambda) is unchanged. corpus 9/9, ownir 116/116.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The false positive
Mining ShareX (a large, real WinForms app) with the
--flow-localslocal-
IDisposabledetector surfaced a systematic false-positive class — ourWPF-tuned detector flagged the single most idiomatic line in WinForms:
as an OWN001 leak (
formis anIDisposable, constructed, never disposed). Itis not a leak.
Control.Show()opens the form modeless: the window stays openafter the method returns and WinForms disposes it when the user closes it — the
framework owns the lifetime. Disposing it yourself in
OpenSettingswould be thebug. This shape is in every "open a tool window" handler, so the FP is systematic.
The contrast — and the line that must stay flagged — is a modal dialog:
The discriminator is exactly the show method:
Show()→ framework-owned (exempt);ShowDialog()→ caller-owned (tracked).The fix
Two extractor helpers + one guard on the
--flow-localscandidate condition:DerivesFromWinFormsForm(ITypeSymbol?)— walks the base chain forSystem.Windows.Forms.Form;IsModelessShownForm(name, body, type)— aForm-derived local with alocal.Show()somewhere in the method body (aShowDialog()is deliberatelynot matched);
It is per-local and
Show()-only by design: aShowDialog()'d form, aFormconstructed but never shown, and any non-
Formdisposable are all untouched. No newfinding path, no core change — a narrowing of the candidate set, exactly like the
existing
IsDisposeOptionalexemptions.Pinned in CI
frontend/roslyn/samples/WinFormsModelessSample.cs(self-contained — a stubSystem.Windows.Forms.Formstands in for the framework type, which is not loaded inthe flow step) wired into the
wpf-extractor--flow-localssteps:OpenModelessnew ModelessForm().Show()OpenModalLeaknew ModalDialog().ShowDialog(), never disposed'modalLeak' is never disposedOpenModalOkThe core half (facts → verdict) was validated locally on the hand-built flow facts
before pushing; the C# half (source → facts) is validated by the sample in CI, where
the rest of the frontend is. Writeup:
docs/notes/winforms-modeless-precision.md.This closes the one systematic WinForms FP (modeless forms); the broader
Controls.Add/componentsownership story is a separate slice on theprecision-hardening backlog the ShareX hunt produced.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Form.Show()is handled as an ownership transfer (avoids false positives), while modalShowDialog()still correctly flags undisposed dialogs and stays silent when properly disposed.Show()vsShowDialog()and the path-sensitive handling for conditional modeless forms.--flow-localson an updated WinForms sample and asserts expected leak findings.