feat(wpf): field-mediated cross-method use-after-dispose → OWN002#75
Conversation
… field read in a subscribed handler → OWN002 An IDisposable field disposed in a class's Dispose()/DisposeAsync() and then DIRECTLY read (`_field.Member`) inside a live subscription-target handler (RHS of a `+=` or arg of a `.Subscribe(...)`, not torn down by a matching `-=`, with no `if (_disposed) return;` guard) is a use-after-dispose: an external event source can still invoke the handler after the object is torn down. The Roslyn extractor lowers it to a synthetic acquire/release/use flow (the trick the MemoryPool slices use), so the existing OwnIR bridge raises OWN002 — no new diagnostic, no second checker. Precise by construction to stay low-FP: the release must be in the dispose lifecycle; the handler must be a live (not unsubscribed) subscription target with no disposed-guard; and only a DIRECT field member access counts (an indirect use via a helper is deliberately not chased — that frontier stays an honest miss, the sibling handler-use-after-dispose case). New corpus case corpus/wpf/field-use-after-dispose (before → OWN002, guarded after → silent); benchmark recall floor 19 → 20. 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds extractor logic to detect WPF-style field-mediated use-after-dispose: a new ChangesWPF field-use-after-dispose detection
Sequence Diagram(s)sequenceDiagram
participant FlowLocalsPass
participant DisposedGuardBefore
participant flowFunctions
FlowLocalsPass->>FlowLocalsPass: enumerate IDisposable fields + disposal lines in Dispose()
FlowLocalsPass->>FlowLocalsPass: build live handler set (+=/.Subscribe minus -=)
loop each live handler method
FlowLocalsPass->>DisposedGuardBefore: check for disposed-flag early-return guard
DisposedGuardBefore-->>FlowLocalsPass: false → proceed
FlowLocalsPass->>FlowLocalsPass: locate first direct this-field member-access of disposed field
FlowLocalsPass->>flowFunctions: emit acquire(fieldDeclLine) / release(disposalLine) / use(accessLine)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1925-1931: The field identity matching in this code uses
text-based comparison via the FieldName method, which can incorrectly match
fields with identical names from different types (e.g., confusing this._conn
with other._conn). Replace the text-based field name matching with proper symbol
binding using IFieldSymbol instead. Resolve the symbol for both the disposal
discovery logic (around line 1928 where FieldName is called on dmm.Expression)
and the handler-use matching logic (around line 1970), and verify that the
resolved field symbol's ContainingType equals the current class being analyzed
to ensure you are tracking the correct field across both locations.
- Around line 1935-1952: The issue is that subscribed and unsubscribed HashSets
track only handler names without tracking which subscription source they are
associated with. When the same handler is subscribed to multiple sources and
unsubscribed from only one, the ExceptWith call incorrectly removes it from the
entire subscribed set. Instead of using simple string-based handler names as
keys, change the tracking to use a composite key combining the subscription
source (assignment target or method being subscribed to) with the handler name,
then derive the final set of live handlers by filtering based on these
subscription points rather than using ExceptWith on handler names alone.
- Around line 915-919: The HasDisposedGuard method is overly broad in its
heuristic for detecting disposal guards. It returns true when an if statement
contains "ispos" anywhere and has any return statement anywhere in the method
body, even if that return is not actually protecting the code. Tighten the guard
recognition by ensuring the return statement is directly in the if statement's
body (not nested in other control structures deeper within), and make the
identifier check more precise to match actual disposed/isDisposed variable
patterns rather than any occurrence of "ispos" as a substring. This will
eliminate false negatives where the heuristic incorrectly suppresses real
use-after-free findings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 31ab67ed-40c0-41b9-9b0a-605c0e02c2dd
📒 Files selected for processing (7)
.github/workflows/ci.ymlcorpus/wpf/field-use-after-dispose/after.cscorpus/wpf/field-use-after-dispose/before.cscorpus/wpf/field-use-after-dispose/case.owncorpus/wpf/field-use-after-dispose/expected-diagnostics.txtcorpus/wpf/field-use-after-dispose/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
| foreach (var inv in dm.DescendantNodes().OfType<InvocationExpressionSyntax>()) | ||
| if (inv.Expression is MemberAccessExpressionSyntax dmm | ||
| && dmm.Name.Identifier.Text is "Dispose" or "DisposeAsync" | ||
| && FieldName(dmm.Expression) is { } df | ||
| && dispoFieldLine.ContainsKey(df) | ||
| && !releasedAt.ContainsKey(df)) | ||
| releasedAt[df] = LineOf(inv); |
There was a problem hiding this comment.
Field identity matching is text-based and can misattribute member access.
At Line 1928 and Line 1970, FieldName(...) compares by identifier text only. That can conflate this._conn with other._conn and incorrectly model release/use on the wrong symbol.
Use symbol binding (IFieldSymbol) and verify ContainingType == current class for both disposal discovery and handler-use matching.
💡 Resolve field symbols instead of names
+static string? ThisFieldName(ExpressionSyntax expr, SemanticModel model, INamedTypeSymbol clsSymbol)
+{
+ var sym = model.GetSymbolInfo(expr).Symbol as IFieldSymbol;
+ return sym is not null
+ && SymbolEqualityComparer.Default.Equals(sym.ContainingType, clsSymbol)
+ ? sym.Name
+ : null;
+}
...
- && FieldName(dmm.Expression) is { } df
+ && clsSymbol is not null
+ && ThisFieldName(dmm.Expression, model, clsSymbol) is { } df
...
- if (FieldName(ma.Expression) is { } uf
+ if (clsSymbol is not null
+ && ThisFieldName(ma.Expression, model, clsSymbol) is { } uf
&& releasedAt.TryGetValue(uf, out var relLine))Also applies to: 1966-1972
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 1925 - 1931, The
field identity matching in this code uses text-based comparison via the
FieldName method, which can incorrectly match fields with identical names from
different types (e.g., confusing this._conn with other._conn). Replace the
text-based field name matching with proper symbol binding using IFieldSymbol
instead. Resolve the symbol for both the disposal discovery logic (around line
1928 where FieldName is called on dmm.Expression) and the handler-use matching
logic (around line 1970), and verify that the resolved field symbol's
ContainingType equals the current class being analyzed to ensure you are
tracking the correct field across both locations.
![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: 0473c89c01
ℹ️ 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".
| // low-FP). Heuristic: a top-level `if` whose condition mentions a "dispos"-named identifier and whose | ||
| // body returns. | ||
| static bool HasDisposedGuard(BlockSyntax body) => | ||
| body.Statements.OfType<IfStatementSyntax>().Any(ifs => |
There was a problem hiding this comment.
This treats any top-level if (_disposed) return; anywhere in the handler as a safe guard, but the new detector skips the whole handler before checking where the disposed field is read. If a handler touches _conn and only later checks _disposed, the access can still run after Dispose(), yet HasDisposedGuard returns true and suppresses the OWN002 finding. Please require the guard to be the first executable statement, or at least prove it occurs before the first disposed-field member access.
Useful? React with 👍 / 👎.
| static bool HasDisposedGuard(BlockSyntax body) => | ||
| body.Statements.OfType<IfStatementSyntax>().Any(ifs => | ||
| ifs.Condition.DescendantNodesAndSelf().OfType<IdentifierNameSyntax>() | ||
| .Any(id => id.Identifier.Text.Contains("ispos")) |
There was a problem hiding this comment.
The guard matcher is case-sensitive, so the documented canonical form if (IsDisposed) return; is not recognized because IsDisposed does not contain lowercase ispos. In codebases that expose disposal state through a common PascalCase property, the new field-UAF pass still emits OWN002 on guarded handlers, making the fixed pattern a false positive. Normalize the identifier or use an ordinal case-insensitive comparison.
Useful? React with 👍 / 👎.
| if (IsHandler(a.Right) && FieldName(a.Right) is { } hn) | ||
| { | ||
| if (a.IsKind(SyntaxKind.AddAssignmentExpression)) subscribed.Add(hn); | ||
| else if (a.IsKind(SyntaxKind.SubtractAssignmentExpression)) unsubscribed.Add(hn); |
There was a problem hiding this comment.
Recording only the handler name makes one -= suppress every subscription that uses that method. For example, if _bus1.Changed += OnChanged and _bus2.Changed += OnChanged, then only _bus1.Changed -= OnChanged, the handler is still live through _bus2, but ExceptWith drops OnChanged entirely and the disposed-field read in that still-live callback is missed. Keep the subscription source in the key, like the existing event-leak pass does with left|right, or otherwise track whether any matching subscription remains live.
Useful? React with 👍 / 👎.
Three precision fixes to the field-mediated use-after-dispose pass, each flagged by both reviewers: 1. Field identity (FP fix): match the disposed field by THIS-object receiver only — a bare `_f` or `this._f`, not `other._f`. Text-only matching could conflate a same-named field on a different receiver and emit a phantom OWN002. New `ThisFieldName` helper, used for both disposal discovery and the handler read. Kept syntactic (not symbol-bound): the corpus types do not resolve in the project-local compilation, so binding on the field's type is unreliable. 2. Guard recognition (`HasDisposedGuard` -> `DisposedGuardBefore`): the guard must (a) PRECEDE the disposed-field read — a guard only after the read does not protect it; (b) early-return IMMEDIATELY in its THEN branch, not via a `return` buried in a nested/`else` branch; and (c) match the flag name case-INsensitively so the PascalCase `if (IsDisposed) return;` form is recognised too. The handler loop now locates the first direct disposed-field read, then checks for a guard before that position. 3. Subscription liveness: key `+=`/`-=` by SOURCE|handler (like the event-leak pass's left|right) instead of by handler name. A handler `+=`'d to two sources and `-=`'d from only one stays live; a name-only set would drop it globally and miss the use-after-dispose in the still-live callback. Behaviour on the corpus is unchanged: before.cs still -> OWN002, the guarded after.cs still silent. Stricter matching only narrows firing, so the zero-FP audit holds. Python suite + metamorphic green; C# validated by CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
2 participants
What
A new extractor capability: a field-mediated cross-method use-after-dispose. An
IDisposablefield disposed in a class'sDispose()/DisposeAsync()and then directly read (_field.Member) inside a live subscription-target handler is a use-after-dispose — an external event source can still invoke the handler after the object is torn down (the very reason WPF handler leaks matter). The Roslyn extractor lowers it to a syntheticacquire/release/useflow (the same trick the MemoryPool slices use), so the existing OwnIR bridge raises OWN002 — no new diagnostic, no second checker.This is the field-UAF item that was sitting on the
corpus-benchmarkbacklog comment.How it stays low-FP (precise by construction)
The detector fires only when all hold:
.Dispose()/.DisposeAsync()inside aDispose/DisposeAsyncmethod), not an ad-hoc helper;+=or the argument of a.Subscribe(...)— whose subscription is not torn down by a matching-=(an unsubscribed callback cannot fire post-dispose, so it is exempt);if (_disposed) return;guard (the canonical fix, recognised byHasDisposedGuard); andI audited every CI-exercised
.cs(the whole corpus + the--flow-localssamples): no existingafter.csor sample trips the new detector (handlers are empty, touch non-disposed state, lack aDispose-of-field, or are unsubscribed), so specificity is preserved.Scope / honesty
This catches the direct
_field.Memberread. Its siblinghandler-use-after-disposereaches the disposed state indirectly (Refresh()touches subscription-backed state); the extractor does not follow that hop, so that case stays an honest extractor miss (a tracked recall gap, not a logic gap — its.ownreduction still fires OWN002). That indirect frontier, plus a view stored in a field and an injected-source region-escape, remain on the backlog.Validation
corpus/wpf/field-use-after-dispose/—before.cs→ OWN002 (caught), guardedafter.cs→ silent.case.own→[OWN002] use 'conn' after it was released [resource: disposable](validated bytests/test_wpf.py, 5/5).metamorphic.py --selftest8/8 (swept 42 corpus.own);benchmark.py --selftestOK.corpus-benchmarkjob confirmsbefore/afteron real C# under the .NET SDK in CI.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit