feat(pool,wpf): bare-owner using dangle + one-hop indirect field-UAF → OWN002#76
Merged
Merged
Conversation
…e a one-hop indirect field-UAF Two sibling "returns/uses a dead resource" capabilities, plus the corpus that closes the matching benchmark misses. 1. MemoryPool bare-owner `using` escape (`using owner = …; return owner;`) — the twin of the returned-view dangle (#74). A using-declared MemoryPool owner that is returned BARE stays tracked (only this shape is exempted from the return-escape; a non-using returned owner is still a genuine transfer, escaped and silent), and the return lowering threads a use of the returned owner after the using-desugar's scope-exit release — reusing the exact return-chain insert that already catches the returned VIEW. The caller's use lands after the release -> OWN002. New corpus case corpus/real-world/memorypool-using-owner-escape. 2. One-hop indirect field-UAF. The field-mediated use-after-dispose pass now chases a single hop: a live subscription-target handler that calls a PRIVATE same-class helper (`Refresh()` / `this.Refresh()`) which itself unguardedly reads a disposed field — with no `if (_disposed) return;` guard before the call — is lowered to a synthetic acquire/release/use flow -> OWN002. One hop only; deeper chains and field/property indirection stay honest misses. This closes the corpus/wpf/handler-use-after-dispose miss (before.cs now reads `_conn` through a helper; guarded after.cs stays silent). Precision: the helper must be a private instance method; both handler (before the call) and helper must lack a disposed-guard; the field read is a direct this-owned member access. Existing cases unchanged: the returned-VIEW escape and a non-using transfer are both preserved; stricter shapes only narrow firing. Benchmark recall floor 20 -> 22. Python suite + metamorphic + test_corpus 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
|
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)
📝 WalkthroughWalkthroughTwo new OWN002 detection patterns are added to the Roslyn extractor: (1) a ChangesMemoryPool bare-owner using escape
Indirect one-hop handler use-after-dispose
CI gate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: d940d2a08b
ℹ️ 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".
Comment on lines
+2065
to
+2067
| if (node is InvocationExpressionSyntax call | ||
| && SelfCallName(call) is { } callee | ||
| && helperReads.TryGetValue(callee, out var hr)) |
There was a problem hiding this comment.
When a class has overloaded private helpers, this lookup only uses the method name returned by SelfCallName, so a subscribed handler can be attributed the disposed-field read from a different overload. For example, if Refresh() reads _conn but Refresh(CustomerChanged e) is safe, OnCustomerChanged calling Refresh(e) will still hit helperReads["Refresh"] and emit a synthetic OWN002 even though that handler does not reach the disposed field. Include arity/signature or resolve the invoked method symbol before applying the one-hop helper result.
Useful? React with 👍 / 👎.
The one-hop helper lookup keyed `helperReads` by method name, so with overloaded private helpers a handler calling the SAFE overload (`Refresh(e)`) could be attributed the UNSAFE overload's disposed-field read (`Refresh()`) and emit a spurious OWN002 — a false positive. Key the map by the helper's method SYMBOL and resolve the invoked call's symbol, so an overload is matched exactly. Same-class methods are in-source, so their symbols resolve even when the field's type does not; `SelfCallName` stays as the this/bare self-receiver gate (a same-typed `other.Refresh()` resolves to the same symbol but is not a self call). Behaviour on the corpus is unchanged: the single-overload `Refresh()` resolves to the same symbol it is keyed under. C# validated by CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 2032-2039: The helperReads dictionary in the
MethodDeclarationSyntax processing loop uses only the method name
(hm.Identifier.Text) as the key, which causes overloaded private helper methods
to conflate and overwrite each other in the dictionary. To fix this, change the
dictionary key from a simple string name to include the method signature (such
as combining the method name with its parameter count or full parameter types)
so that each overload is tracked separately. Also update the corresponding
lookup logic (around line 2065-2069) where self-call matching occurs to use the
same signature-based key when retrieving from helperReads, ensuring the correct
overload is matched.
🪄 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: db051329-0fa3-4fc7-9348-61bfa63ddaf6
📒 Files selected for processing (11)
.github/workflows/ci.ymlcorpus/real-world/memorypool-using-owner-escape/after.cscorpus/real-world/memorypool-using-owner-escape/before.cscorpus/real-world/memorypool-using-owner-escape/case.owncorpus/real-world/memorypool-using-owner-escape/expected-diagnostics.txtcorpus/real-world/memorypool-using-owner-escape/notes.mdcorpus/wpf/handler-use-after-dispose/after.cscorpus/wpf/handler-use-after-dispose/before.cscorpus/wpf/handler-use-after-dispose/case.owncorpus/wpf/handler-use-after-dispose/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
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
2 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.
Two sibling "returns/uses a dead resource" capabilities, landing in one slice, each closing a matching benchmark miss.
1. MemoryPool bare-owner
usingescape → OWN002The twin of the returned-view dangle (#74).
using owner = …; return owner;returns the owner itself, but theusingdisposes it at scope exit, so the caller gets an already-disposedIMemoryOwner<T>.using-declared MemoryPool owner returned bare now stays tracked (only this shape is exempted from the return-escape; a non-usingreturned owner is still a genuine ownership transfer → escaped → silent).useof the returned owner after theusing-desugar's scope-exit release — reusing the exact return-chain insertion that already catches the returned view. The caller's use lands after the release → OWN002.corpus/real-world/memorypool-using-owner-escape/.2. One-hop indirect field-UAF → OWN002
The field-mediated use-after-dispose pass (#75) caught a direct
_field.Memberread in a live handler. It now chases a single hop: a live subscription-target handler that calls a private same-class helper (Refresh()/this.Refresh()) which itself unguardedly reads a disposed field — with noif (_disposed) return;guard before the call — is lowered to a syntheticacquire/release/useflow → OWN002.corpus/wpf/handler-use-after-dispose/miss:before.csnow reaches the disposed_connthrough aRefresh()helper (caught); the guardedafter.csstays silent.Precision (stays low-FP)
this-owned member access.using-declared MemoryPool owner is exempted from the return-escape — a non-usingtransfer (var o = Rent(); return o;) stays untracked/silent, and the existing returned-view escape is unchanged. I traced both directions: stricter shapes only narrow firing.Validation
case.ownfor both →[OWN002](test_wpf 5/5, test_corpus 18/18)..ownswept); benchmark selftest OK.handler-use-after-disposemiss→caught). Thecorpus-benchmarkjob validates the real C# under the SDK in CI.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
using-declaredMemoryPoolowners.OWN002diagnostics.