feat(pool): POOL005 — full-length view of a pooled buffer over-reads its logical length → OWN025#71
Merged
Merged
Conversation
…its logical length → OWN025
A pooled array is oversized (ArrayPool.Rent(n) returns Length >= n), so a full-length
view of it — buf.AsSpan() / buf.AsMemory() / new Span<T>(buf) with NO length bound —
reaches past the requested length n into the stale [n, Length) tail a previous renter
left. Reading or copying through it processes that stale data: a correctness bug and a
potential information disclosure (P-007 POOL005). The fix is a bounded view,
buf.AsSpan(0, n).
New diagnostic OWN025, raised at the view site. The .own checker and the C# extractor
converge on ONE core flow op, so there is a single source of truth:
- .own DSL: a new `overspan b` statement (lexer/parser/AST) lowered to a CFG Overspan
instruction; analysis raises OWN025. It is a property of the view-creation site —
it fires regardless of the owner's flow state, beside the leak/use/release checks,
and changes no state. codegen emits the unbounded AsSpan for it (exhaustiveness).
- extractor: FullViewOwner recognises an unbounded AsSpan/AsMemory/new Span over a
Rent'd local (by the resolved System.MemoryExtensions / System.Span<T> BCL symbols,
like ViewOwner) and EmitFlowExpr emits an `overspan` fact, walking the whole
expression so a view nested in a call (Emit(buf.AsSpan())) or chain
(buf.AsSpan().ToArray()) is found; a bounded view (buf.AsSpan(0, n)) is not matched.
- bridge (ownir): the `overspan` fact lowers to the same Overspan op; the finding is
reported at the VIEW line (not the Rent site), tagged [resource: pooled buffer].
Corpus: arraypool-fullspan-overread (before.cs over-reads via Emit(buf.AsSpan());
after.cs bounds it to buf.AsSpan(0, n)). Benchmark recall floor 13 -> 14.
Tests: run_tests CASES (trigger / leak-coexist / branch / not-owned / undefined),
test_spec Buffer-B9, test_ownir overspan->OWN025 bridge check, gallery example 11.
Spec: Diagnostics OWN025, BufferPolicies B9, P-007 status.
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 (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds end-to-end detection of the POOL005 pattern (OWN025): an ChangesPOOL005/OWN025 overspan detection pipeline
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
Note over RoslynExtractor,OwnIRBridge: C# source → OwnIR facts
participant RoslynExtractor as OwnSharp.Extractor
participant OwnIRBridge as ownir.py (_lower_flow)
RoslynExtractor->>RoslynExtractor: FullViewOwner detects buf.AsSpan()
RoslynExtractor->>OwnIRBridge: emit overspan fact (owner, line)
end
rect rgba(144, 238, 144, 0.5)
Note over OwnIRBridge,Analyzer: OwnLang pipeline
participant Analyzer as analysis.py
participant Findings as OWN025 Finding
OwnIRBridge->>OwnIRBridge: op=="overspan" → A.Overspan node
OwnIRBridge->>OwnIRBridge: _Builder.lower_stmt → CFG Overspan instr
OwnIRBridge->>Analyzer: transfer Overspan(sym, line)
Analyzer->>Findings: report OWN025, kind="pooled buffer"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 |
…(<=100) The 11_overspan_full_view.own entry's analog description ran to 116 chars; trim it to keep the lint gate (ruff + mypy --strict) green. No behaviour change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![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: 5e1a68f18c
ℹ️ 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
+807
to
+811
| foreach (var node in expr.DescendantNodesAndSelf().OfType<ExpressionSyntax>()) | ||
| if (FullViewOwner(node, model) is { } owner && tracked.Contains(owner)) | ||
| overspanned.Add(owner); | ||
| foreach (var o in overspanned) | ||
| nodes.Add(new { op = "overspan", var = o, line = LineOf(expr) }); |
There was a problem hiding this comment.
When the over-copy is written as a local declaration initializer, e.g. var copy = buf.AsSpan().ToArray();, LowerFlowStmt takes the LocalDeclarationStatementSyntax branch and never calls EmitFlowExpr on non-acquire initializers, so this new descendant scan is never reached. That leaves a common POOL005 over-read silent even though the implementation comment calls out chains such as .ToArray(); please also scan declaration initializers before returning from that branch.
Useful? React with 👍 / 👎.
…l views (POOL005) Two follow-ups after PR #71's first CI run: 1. Metamorphic selftest (the failing job): the alpha-rename / reorder transforms in scripts/metamorphic.py did not know the new `overspan` statement, so renaming `buf` left `overspan buf` referencing the vanished name (OWN025 -> OWN030, non-invariant). Teach _sub_stmt / _all_names / _touches that `overspan` reads its var, exactly like `use`/`release`. Selftest back to 8/8. 2. Codex review (Program.cs): a full view written as a local-declaration initializer (`var copy = buf.AsSpan().ToArray();`) was missed — LowerFlowStmt's LocalDeclarationStatementSyntax branch never calls EmitFlowExpr on a non-acquire initializer. Refactor the overspan scan into a shared EmitOverspans helper and call it from the local-decl branch too, so the over-copy initializer AND a view-local declaration (`Span<T> s = buf.AsSpan();`) are both caught. The corpus before.cs now exercises both spellings; after.cs bounds both. Spec / notes / P-007 updated for expression + initializer coverage (only the `Array.Clear(buf, 0, buf.Length)` spelling remains a follow-up). The corpus benchmark already validated recall 14 (before.cs -> OWN025) on the prior run. 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
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
A pooled array is oversized:
ArrayPool<T>.Shared.Rent(n)returns an array ofLength >= n, not exactlyn. A full-length view of it —buf.AsSpan()/buf.AsMemory()/new Span<T>(buf)with no length bound — reaches past the requested lengthninto the stale[n, Length)tail a previous renter left behind. Reading or copying through that view processes the stale data: a correctness bug (wrong length) and a potential information disclosure. The fix is a bounded view,buf.AsSpan(0, n).This is P-007's POOL005 ("clear/copy past the logical length"), the last item on that profile's list. New diagnostic OWN025 in the buffer-policy family.
How — one core op, both front-ends converge on it
Unlike POOL002/004 (flow-sensitive use/escape), POOL005 is a property of the view-creation site — it fires regardless of the owner's flow state, beside the leak/use/release checks, and changes no state. Both the
.ownchecker and the C# extractor lower to a single new core op, so there is one source of truth:.ownDSL — a newoverspan bstatement (lexer → parser → AST → a CFGOverspaninstruction);analysisraises OWN025.codegenemits the unboundedAsSpanfor it (exhaustiveness).FullViewOwnerrecognises an unboundedAsSpan/AsMemory/new Spanover aRented local by the resolvedSystem.MemoryExtensions/System.Span<T>BCL symbols (likeViewOwner, so a look-alike isn't mistaken for it);EmitFlowExprwalks the whole expression so a view nested in a call (Emit(buf.AsSpan())) or chain (buf.AsSpan().ToArray()) is found. A bounded view (buf.AsSpan(0, n)) is not matched.ownir) — theoverspanfact lowers to the sameOverspanop; the finding is reported at the view line (not the Rent site), tagged[resource: pooled buffer].OWN025 only fires for pooled buffers: among
trackedlocals only aRented array is what the BCLAsSpan/Spansymbols bind to, so it never fires on a tracked IDisposable / factory result. Distinct from OWN024 (a sensitive buffer cleared too little) — this is reading too much.Corpus & tests
arraypool-fullspan-overread—before.csover-reads viaEmit(buf.AsSpan());after.csbounds it tobuf.AsSpan(0, n).case.ownreduces it (overspan buf→ OWN025). Benchmark recall floor 13 → 14.run_testsCASES — trigger, leak-coexistence (OWN025 + OWN001), in-branch, not-owned (OWN034), undefined (OWN030), bounded-view-clean.test_specBuffer-B9,test_owniroverspan→ OWN025 at the view line tagged pooled buffer, gallery example 11.Full local suite green (analysis 132/132, corpus 12/12, gallery 12/12, spec 23/23, ownir 117/117, codegen 43/43). The extractor (
Program.cs) is CI-validated.Scope (first slice)
Catches the unbounded view used in an expression. A view stored in a local first (
var s = buf.AsSpan(); Use(s);) and theArray.Clear(buf, 0, buf.Length)spelling are follow-ups noted in P-007.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
overspansupport and detection for unbounded full-length views from pooled buffers that can over-read stale tail bytes.overspan, gallery validation for OWN025, and conformance coverage for Buffer-B9.