feat(pool): pooled-buffer view-in-a-field dangle → OWN002 (POOL005)#78
Conversation
…IMemoryOwner field read after Dispose → OWN002 Generalises the field-mediated use-after-dispose pass (#75/#76) to POOLED owners and their Memory views. An `IMemoryOwner<T>` field released in `Dispose()` is a released owner, and a `Memory`/`ReadOnlyMemory` field assigned `_owner.Memory` (recognised by ViewOwner, which returns the owner name for a bare-field receiver) is recorded as a VIEW of that owner. A read of the view field — or of the owner directly — in a live subscription-target handler (directly or one hop through a private helper), with no `if (_disposed) return;` guard before it, is lowered to a synthetic acquire/release/use flow on the owner → OWN002. The view-field aliases the owner, so reading it after the owner's release is a use of the owner after release. New `ReleasedOwner(field)` unifies the two: the field itself when it is a released owner, or the owner it aliases when it is a view field. `FirstUnguardedDisposedRead` and the handler trigger loop both route through it, so direct disposed-field reads (#75/#76) are unchanged and view-field reads are additive. Precision: same handler reachability that makes the disposed-field UAF sound (a live handler can fire after Dispose). The owner must be released in the dispose lifecycle; the view→owner alias is symbol-resolved (`IMemoryOwner<T>.Memory`); a guarded/unsubscribed handler never fires. Exposing such a view via a public getter is deliberately NOT flagged — `IMemoryOwner.Memory` is itself a valid "valid-until-Dispose" pattern, so only a provably-post-release read is a bug. New corpus case corpus/wpf/pooled-view-after-dispose (before → OWN002, guarded after → silent). Benchmark recall floor 22 -> 23. Scope: IMemoryOwner owner + member-access view read; ArrayPool byte[]-buffer-field views, bare-arg reads, and element-access reads are honest follow-ups. 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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml 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)
📝 WalkthroughWalkthroughExtends the OwnSharp extractor's ChangesIMemoryOwner View-Field Alias Detection and Corpus Coverage
Possibly related PRs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ 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: 243c4398a0
ℹ️ 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".
| continue; | ||
| if (!IsDisposableType(fd.Declaration.Type.ToString())) | ||
| var ftype = fd.Declaration.Type.ToString(); | ||
| if (!IsDisposableType(ftype) && !ftype.StartsWith("IMemoryOwner", StringComparison.Ordinal)) |
There was a problem hiding this comment.
When the owner field is written as System.Buffers.IMemoryOwner<byte> or through a type alias, fd.Declaration.Type.ToString() no longer starts with IMemoryOwner, so the field is never added to dispoFieldLine; releasedAt stays empty and the new pooled-view-after-dispose path is completely missed even though ViewOwner resolves _owner.Memory semantically. This makes the new checker depend on a using System.Buffers; spelling rather than the actual BCL type.
Useful? React with 👍 / 👎.
| var viewFieldOwner = new Dictionary<string, string>(StringComparer.Ordinal); | ||
| foreach (var a in assigns) | ||
| if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) | ||
| && FieldName(a.Left) is { } vf |
There was a problem hiding this comment.
For assignments like other._view = _owner.Memory, FieldName(a.Left) records _view as if this object's field aliases _owner. A later subscribed handler reading this instance's _view.Span will then emit OWN002 even though only another instance's field was populated from the owner. The view-field map should use the same bare/this. restriction as the release/read paths to avoid this false positive.
Useful? React with 👍 / 👎.
| foreach (var a in assigns) | ||
| if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) | ||
| && FieldName(a.Left) is { } vf | ||
| && ViewOwner(a.Right, model) is { } vo) |
There was a problem hiding this comment.
If the constructor uses the common spelling _view = this._owner.Memory (or this._view = this._owner.Memory), this call to ViewOwner returns null because ViewOwner only accepts an IdentifierNameSyntax receiver for owner.Memory. The owner still gets disposed and the handler still reads _view.Span, but the view-owner alias is never recorded, so the advertised pooled view-in-a-field dangle is missed for this-qualified field access.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 2025-2029: The current code in the foreach loop iterates through
assignments and records field-to-owner mappings using viewFieldOwner[vf] = vo
without verifying that the field being assigned belongs to the this instance.
This can incorrectly capture aliases from other object instances like
other._view. Add a check to ensure that a.Left refers to a field access on the
this instance before recording the alias in the viewFieldOwner dictionary,
filtering out assignments to fields on other objects.
- Around line 1994-1995: Replace the string-based check
`ftype.StartsWith("IMemoryOwner", StringComparison.Ordinal)` with semantic type
resolution. Instead of relying on the string representation of the field type
(which may include fully qualified names or aliases), use Roslyn's semantic
analysis to check if the actual type symbol represents IMemoryOwner<T>. This
will properly identify all valid IMemoryOwner declarations regardless of how
they are written in the source code, ensuring OWN002 cases are not skipped due
to naming variations.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b2daae83-7c58-448b-a6b5-165270fb0ff5
📒 Files selected for processing (7)
.github/workflows/ci.ymlcorpus/wpf/pooled-view-after-dispose/after.cscorpus/wpf/pooled-view-after-dispose/before.cscorpus/wpf/pooled-view-after-dispose/case.owncorpus/wpf/pooled-view-after-dispose/expected-diagnostics.txtcorpus/wpf/pooled-view-after-dispose/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…c IMemoryOwner, this-qualified owner (CodeRabbit + Codex)
Three precision/robustness fixes to the pooled view-in-a-field pass, each flagged
by review (the first two by BOTH CodeRabbit and Codex):
1. FP: the view-field alias map keyed the LHS with FieldName, so `other._view =
_owner.Memory` recorded THIS instance's `_view` as aliasing `_owner` — a later
handler read of this object's `_view.Span` would then falsely fire. Use
ThisFieldName for the LHS, matching the release/read paths (this/bare only).
2. Recall: an owner field written `System.Buffers.IMemoryOwner<byte>` (or an alias,
or a concrete type implementing IMemoryOwner) was missed by the
`StartsWith("IMemoryOwner")` string check, so releasedAt stayed empty and the
case was skipped. New `IsMemoryOwnerType` resolves the field's type SYMBOL
(interface or via AllInterfaces); the cheap StartsWith still short-circuits the
common unqualified spelling. IDisposable stays syntactic (unresolved customs).
3. Recall: `this._owner.Memory` was not recognised as a view (ViewOwner needs an
IdentifierNameSyntax receiver). New `FieldViewOwner` accepts a this/bare-field
receiver (ThisFieldName) for the `IMemoryOwner<T>.Memory` borrow, so both
`_owner.Memory` and `this._owner.Memory` alias, while `other._owner.Memory`
(another instance) does not.
Behaviour on the corpus is unchanged (the `_owner.Memory` view still aliases and
fires OWN002, the guarded after stays silent); the FP fix only narrows the map.
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
Generalises the field-mediated use-after-dispose pass (#75/#76) to pooled owners and their
Memoryviews — the sound, zero-FP form of "view-in-a-field" (POOL005).What
An
IMemoryOwner<T>field released inDispose()is a released owner, and aMemory/ReadOnlyMemoryfield assigned_owner.Memoryis recorded as a view of that owner. A read of the view field — or of the owner directly — in a live subscription-target handler (directly or one hop through a private helper), with noif (_disposed) return;guard before it, is lowered to a syntheticacquire/release/useflow on the owner → OWN002. The view field aliases the owner, so reading it after the owner's release is a use of the owner after release.ViewOwner(_owner.Memory)already returns the owner name for a bare-field receiver (the receiver is anIdentifierNameSyntax; theIMemoryOwner<T>.Memorysymbol resolves regardless), so no new view-recognition was needed. A newReleasedOwner(field)unifies the cases: the field itself when it is a released owner, or the owner it aliases when it is a view field. BothFirstUnguardedDisposedReadand the handler trigger loop route through it, so the existing disposed-field reads (#75/#76) are unchanged and view-field reads are purely additive.Why this scope (the FP finding)
The naive reading — flag a public getter that exposes a pooled-backed
Memory— is FP-dangerous:IMemoryOwner<T>.Memoryis itself a legitimate "valid-until-Dispose" pattern, so exposing such a view is sound; the bug is the caller reading it post-Dispose, which is cross-object and not type-locally provable. The only type-local, zero-FP proof of a post-release read is the handler reachability (a live handler can fire afterDispose()) — the same argument that makes #75/#76 sound. So this reuses that, and exposure is deliberately not flagged.Precision
IMemoryOwner<T>.Memory); a guarded or unsubscribed handler never fires.IMemoryOwnerin the existing corpus is a local (not a field) and there are no view-field assignments anywhere, sodispoFieldLine(now incl. IMemoryOwner) andviewFieldOwnerstay empty on all existing cases — zero new fires.Validation
case.own→[OWN002] ... [resource: pooled buffer](test_wpf 6/6, test_corpus green)..ownswept).corpus-benchmarkjob validates the real C# under the SDK in CI.Honest scope / follow-ups
IMemoryOwner owner + member-access view read (
_view.Span). Follow-ups: an ArrayPoolbyte[]buffer field returned inDispose()(interacts with the existing per-member pool-leak pass), a view passed as a bare argument (Consume(_view)), and element-access reads (_buf[i]).🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores