feat(pool): POOL005 over-read of a pooled byte[] FIELD viewed full-length#106
Merged
Conversation
ROADMAP.md still listed "POOL002 next" and P-007 as draft, but POOL002 (view-after-return -> OWN002), POOL003 (double-return) and POOL005 (full-length over-read) are built and pinned by corpus (arraypool-span-view-after-return et al.). Update the three ROADMAP spots and the proposals index to match the verified reality. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
…ngth The path-sensitive flow pass only tracks pooled buffers held in LOCALS, so a buffer Rent'ed into a FIELD (`_buf = ArrayPool<T>.Shared.Rent(n)`) and viewed full-length in a LATER member over-reads the oversized [n, Length) tail with no diagnostic. Add a class-level POOL005 field pass to the extractor: - collect the fields a class Rents (shared IsPoolRent — aliased pool receiver binds, a non-pool `.Rent` does not); - for each member that takes a full-length view of such a field — `_buf.AsSpan()` / `this._buf.AsMemory()` / `new Span<T>(_buf)` / the `.Length` spelling, the receiver resolved through the this/bare ThisFieldName shape and the over-read recognised by the same resolved System.MemoryExtensions / System.Span<T> BCL symbols as the local path — emit a synthetic acquire/overspan/release flow so the existing OwnIR bridge raises OWN025 at the view. No new diagnostic, no new core code (the synthetic-flow trick the field-UAF / MemoryPool slices use). A write/wipe (`Array.Clear(_buf, 0, _buf.Length)`) is not a view, so it is not flagged — only a read-capable VIEW is the over-read. Corpus `arraypool-field-fullspan-overread` (before/after/case.own/notes) pins it; case.own (the core OWN025 verdict) is validated by the Python-only test_corpus suite here, the field-vs-local extraction by the dotnet corpus-benchmark CI job. Bump --min-recall 23 -> 24 to pin the newly-caught case and strike the "ArrayPool byte[]-buffer-field view" item from the benchmark backlog. P-007 / ROADMAP updated; the remaining POOL005 frontier is a view stored INTO another field. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR extends pooled class-field full-length view detection in the extractor, emits synthetic flow facts for those views, adds a matching corpus case with expected OWN025 output, and updates related docs and the benchmark threshold. ChangesP-007 pooled-field over-read support
Sequence Diagram(s)sequenceDiagram
participant Program.cs
participant FullViewFieldOwner
participant OwnIR bridge
participant OWN025 diagnostic
Program.cs->>FullViewFieldOwner: identify pooled field full-length view
Program.cs->>OwnIR bridge: emit synthetic acquire / overspan / release facts
OwnIR bridge->>OWN025 diagnostic: report OWN025 at the view site
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
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: a4fbc1ec5d
ℹ️ 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".
| foreach (var inv in cls.DescendantNodes().OfType<InvocationExpressionSyntax>()) | ||
| if (IsPoolRent(inv, model) | ||
| && inv.Parent is AssignmentExpressionSyntax asg | ||
| && FieldName(asg.Left) is { } pf |
There was a problem hiding this comment.
When a method rents into anything other than this object's field, this still records the final name because FieldName accepts bare locals and arbitrary member receivers such as other._buf = ArrayPool<byte>.Shared.Rent(n). The later view check only looks for bare/this _buf, so a class that rents into another object's _buf (or a shadowing local) and separately full-spans its own non-pooled _buf will get a synthetic overspan and an OWN025 false positive. Use ThisFieldName or symbol binding here so only fields on the current instance enter pooledFieldRent.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Good catch — fixed in 6ea5adf. The collection now resolves the rent target through ThisFieldName (bare _buf / this._buf, never other._buf), the same this-instance shape FullViewFieldOwner reads the view through. A rent into another object's _buf can no longer seed pooledFieldRent and false-match this class's own same-named non-pooled field, so the cross-instance OWN025 false positive is gone.
Generated by Claude Code
…odex) `pooledFieldRent` collected the rent TARGET via `FieldName`, which accepts an arbitrary receiver (`other._buf = ArrayPool.Rent(n)`) and a bare local — so a class that rents into another object's `_buf` and separately full-spans its OWN same-named, non-pooled `_buf` would seed the set and trip a false OWN025. Switch to `ThisFieldName` (bare `_buf` / `this._buf`, never `other._buf`) — the same this-instance shape FullViewFieldOwner reads the view through, so the two halves agree and the cross-instance false positive is gone. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
![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
🧹 Nitpick comments (1)
corpus/real-world/arraypool-field-fullspan-overread/notes.md (1)
3-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one field-backed corpus variant for a non-
AsSpan()full-view shape.These notes claim the new field pass covers
_buf.AsMemory(),new Span<T>(_buf), and the.Lengthspelling too, but this directory only pins_buf.AsSpan(). One extra field-backed case for_buf.AsSpan(0, _buf.Length)or_buf.AsMemory()would keep those matcher branches under regression coverage.🤖 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 `@corpus/real-world/arraypool-field-fullspan-overread/notes.md` around lines 3 - 11, Add one additional field-backed corpus example that exercises a non-AsSpan full-view shape, since the current notes only pin _buf.AsSpan(). Extend the FIELD twin coverage in notes.md with a case using _buf.AsMemory() or _buf.AsSpan(0, _buf.Length) so the matcher branches for full-length views on the pooled field are covered, and keep the existing _buf/_n pattern consistent with the ArrayPool<T>.Shared.Rent(n) flow.
🤖 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 2788-2801: The POOL005 outer-class scan is walking into nested
types, so rents/views inside inner classes or structs can be incorrectly
attributed to the outer class. Update the traversal around cls.DescendantNodes()
and each member.DescendantNodes() in the POOL005 pass to skip any
BaseTypeDeclarationSyntax descendants, and let nested types be analyzed by their
own cls pass. Keep the pooledFieldRent and per-member flow synthesis limited to
direct members of the current class so Outer.member flows are not fabricated
from nested declarations.
- Around line 1523-1530: The pooled-field receiver check is still relying on
syntax-only name matching in ThisFieldName and IsFieldLengthOf, which can
misidentify shadowed locals or parameters as the pooled field and emit a false
OWN025. Update the receiver resolution in this analysis path to bind the
expression to an IFieldSymbol first, then only proceed when it matches the
actual pooled field symbol; keep the MemoryExtensions invocation flow in
Program.cs aligned with that symbol-based lookup.
- Around line 2788-2793: Update the pooled field tracking in Program.cs so
`pooledFieldRent` also captures field declaration initializers, not just
`AssignmentExpressionSyntax` rents, using the existing `IsPoolRent`,
`FieldName`, and `LineOf` flow in the class-level scan. Also scope the
`cls.DescendantNodes()`/`member.DescendantNodes()` traversal to the current type
only so nested type invocations are excluded from the outer class’s synthetic
POOL005 analysis.
---
Nitpick comments:
In `@corpus/real-world/arraypool-field-fullspan-overread/notes.md`:
- Around line 3-11: Add one additional field-backed corpus example that
exercises a non-AsSpan full-view shape, since the current notes only pin
_buf.AsSpan(). Extend the FIELD twin coverage in notes.md with a case using
_buf.AsMemory() or _buf.AsSpan(0, _buf.Length) so the matcher branches for
full-length views on the pooled field are covered, and keep the existing _buf/_n
pattern consistent with the ArrayPool<T>.Shared.Rent(n) flow.
🪄 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: b63e2599-0a3f-4bd9-8d86-2db122e4a48a
📒 Files selected for processing (10)
.github/workflows/ci.ymlcorpus/real-world/arraypool-field-fullspan-overread/after.cscorpus/real-world/arraypool-field-fullspan-overread/before.cscorpus/real-world/arraypool-field-fullspan-overread/case.owncorpus/real-world/arraypool-field-fullspan-overread/expected-diagnostics.txtcorpus/real-world/arraypool-field-fullspan-overread/notes.mddocs/ROADMAP.mddocs/proposals/P-007-arraypool-span.mddocs/proposals/README.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
…e (CodeRabbit) Two correctness improvements to the POOL005 field over-read pass: - Field-declaration initializers (`byte[] _buf = ArrayPool<T>.Shared.Rent(n);`) now seed `pooledFieldRent`, not just `_buf = Rent(...)` assignments — an idiomatic pooled-scratch-field shape that was previously missed (recall gap). - The pass is scoped to the class's OWN (direct) members: a `FirstAncestorOrSelf == cls` guard drops an assignment rent nested in an inner type, and the per-member loop skips `BaseTypeDeclarationSyntax` members, so a nested type's rents/views can no longer be mis-attributed to the outer class's synthetic flow. Not done: resolving the field receiver to an `IFieldSymbol` (CodeRabbit's symbol-binding suggestion). The codebase deliberately matches fields syntactically via `ThisFieldName` (documented at its definition — binding on the field's type is unreliable for the unresolved types the corpus depends on); `this._buf` is unambiguous and the bare-`_buf` residual matches every sibling field pass. Corpus `arraypool-field-fullspan-overread` now also exercises an initializer-rented field viewed via the `.Length` spelling (`_meta.AsSpan(0, _meta.Length)`), pinning both the new initializer seeding and the IsFieldLengthOf branch end to end in the dotnet benchmark; the fixed `after.cs` bounds both views and stays silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
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 `@corpus/real-world/arraypool-field-fullspan-overread/after.cs`:
- Around line 38-41: Dispose in the array-pool cleanup path can return a null
buffer if it runs before Capture initializes the fields. Update Dispose to guard
the _buf return in the same area as the ArrayPool<byte>.Shared.Return calls,
checking whether _buf has been captured before returning it to the pool, while
keeping the existing _meta return behavior intact.
🪄 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: 29a282b7-0108-404f-91fa-567dffc43d70
📒 Files selected for processing (4)
corpus/real-world/arraypool-field-fullspan-overread/after.cscorpus/real-world/arraypool-field-fullspan-overread/before.cscorpus/real-world/arraypool-field-fullspan-overread/notes.mdfrontend/roslyn/OwnSharp.Extractor/Program.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- corpus/real-world/arraypool-field-fullspan-overread/before.cs
- frontend/roslyn/OwnSharp.Extractor/Program.cs
`_buf` is null until Capture(), so a Dispose() before Capture() would call ArrayPool.Return(null) and throw. Guard the assignment-rented field's return with an `is not null` check in both before.cs/after.cs; `_meta` is initializer- rented (never null) and stays unguarded. The Return invocation is still present for the POOL001 field-release scan, and no view site changes, so before.cs still flags the two over-reads and after.cs stays silent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fmwk5ruExKy5fQ77CJ2NtF
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.
What
Extends POOL005 (P-007) to the field-backed pooled buffer. Today the over-read check only reaches buffers held in locals (the path-sensitive flow pass tracks local rents only). A buffer rented into a field —
— was silently missed. This is the "ArrayPool
byte[]-buffer-field view" item the benchmark backlog explicitly named.How
A class-level POOL005 field pass in the extractor (
OwnSharp.Extractor/Program.cs, gated on--flow-locals):Rents (sharedIsPoolRent— an aliased pool receiver binds, a non-pool.Rentdoes not);_buf.AsSpan()/this._buf.AsMemory()/new Span<T>(_buf)/ the.Lengthspelling_buf.AsSpan(0, _buf.Length), receiver resolved through thethis/bareThisFieldNameshape, the over-read recognised by the same resolvedSystem.MemoryExtensions/System.Span<T>BCL symbols as the local path — emits a syntheticacquire/overspan/releaseflow so the existing OwnIR bridge raises OWN025 at the view.No new diagnostic, no new core code — the synthetic-flow trick the field use-after-dispose and MemoryPool slices already use. A write/wipe (
Array.Clear(_buf, 0, _buf.Length)) is not a view, soFullViewFieldOwnerreturns null on it — only a read-capable VIEW is the over-read. Bounded views (_buf.AsSpan(0, _n)) are silent.Tests / validation
arraypool-field-fullspan-overread(before.cs / after.cs / case.own / expected-diagnostics / notes).case.own→ OWN025 via the Python-onlytest_corpussuite —19/19corpus cases green, full suite green (ownir 118/118, spec/wpf/lifetimes/loops all pass). The coreoverspan→OWN025 contract is already pinned by anownircheck.corpus-benchmarkjob scansbefore.cs(must be caught) /after.cs(must be silent). Bumped--min-recall 23 → 24to pin the newly-caught case and struck the field-view item from the benchmark backlog comment.Docs
P-007andROADMAP.mdupdated: POOL005 now covers local and pooledbyte[]FIELD; the remaining POOL005 frontier is a full-length view stored into another field.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Span/Memoryviews over pooled buffers stored in class fields, raising the expectedOWN025.Bug Fixes
corpus-benchmarkrecall gate (--min-recallincreased from 23 to 24).Documentation / Tests