Skip to content

feat(pool): MemoryPool view borrow — owner.Memory[.Span] used after Dispose → OWN002 (POOL002)#73

Merged
PhysShell merged 2 commits into
mainfrom
claude/memorypool-view-borrow
Jun 22, 2026
Merged

feat(pool): MemoryPool view borrow — owner.Memory[.Span] used after Dispose → OWN002 (POOL002)#73
PhysShell merged 2 commits into
mainfrom
claude/memorypool-view-borrow

Conversation

@PhysShell

@PhysShell PhysShell commented Jun 22, 2026

Copy link
Copy Markdown
Owner

What

#72 tracked the MemoryPool owner lifecycle (acquire/release → POOL001 leak, POOL003 double-dispose). This adds the view borrow: owner.Memory / owner.Memory.Span of a System.Buffers.IMemoryOwner<T> is a borrow of the owner — exactly like buf.AsSpan() is of a Rented array.

IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n);
Memory<byte> view = owner.Memory;   // a borrow of the owner's pooled buffer
owner.Dispose();                    // returns the memory to the pool ...
Consume(view.Span);                 // ❌ OWN002: ... read after Dispose (use-after-free)
// fix: read while alive, or `using IMemoryOwner<byte> owner = ...`

How

ViewOwner now resolves owner.Memory (and owner.Memory.Span) to the owner local, recognised by the resolved IMemoryOwner<T>.Memory property symbol (System.Buffers) — not by name, so a look-alike .Memory isn't mistaken for it. The existing borrow machinery does the rest: a use of the view lowers to a use of the owner, so a view-local read after an explicit owner.Dispose() is a use-after-release → OWN002.

The inline form (Consume(owner.Memory.Span)) already worked via the descendant owner; this closes the view-local gap (var v = owner.Memory; owner.Dispose(); Use(v)).

Zero core changes — it reuses the borrow lattice and OWN002. Extractor + corpus only.

Validation

  • Corpus: memorypool-view-after-dispose (the MemoryPool twin of arraypool-span-view-after-return) → OWN002; after.cs (a using owner read while alive) silent.
  • Corpus 15/15, suite EXIT 0, metamorphic 8/8 (39 files), ruff + mypy clean.
  • Benchmark recall floor 16 → 17 (the new case caught on real C#). The extractor is CI-validated.
  • P-007 status + ci.yml benchmark comment updated.

Scope

Delivers the use-after-explicit-Dispose POOL002 catch. The dangling-Memory escape via the idiomatic using owner; return owner.Memory; form is not yet caught (Codex review): using locals are skipped as non-leak candidates, so catching it needs the implicit scope-exit dispose modelled as a release on the return path (like the ArrayPool try/finally Memory escape in #70) — a follow-up. A POOL005 view stored in a FIELD is the other remaining backlog item.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED

Summary by CodeRabbit

  • New Features
    • Improved memory safety analysis to detect MemoryPool view-after-dispose issues via IMemoryOwner borrows from Memory/Span.
  • Documentation
    • Added guidance and examples explaining correct IMemoryOwner lifetime usage to avoid reading released pooled memory.
  • Tests
    • Added a new corpus scenario and expected diagnostic coverage for the view-after-dispose case.
  • Chores
    • Increased the CI benchmark gating threshold for the benchmark job.

…ispose → OWN002 (POOL002)

#72 tracked the MemoryPool OWNER lifecycle (acquire/release: POOL001 leak, POOL003
double-dispose). This adds the VIEW borrow: `owner.Memory` / `owner.Memory.Span` of a
System.Buffers.IMemoryOwner<T> is a borrow of the owner — exactly like `buf.AsSpan()` is
of a Rent'd array. ViewOwner now resolves it (by the IMemoryOwner<T>.Memory property
symbol), so a view-local read after `owner.Dispose()` is a use of the owner after release
-> OWN002 (POOL002), and a returned Memory view dangles. Reuses the whole borrow lattice
— no core changes, extractor + corpus only.

Corpus: memorypool-view-after-dispose (before: `Memory<byte> v = owner.Memory;
owner.Dispose(); Consume(v.Span)` -> OWN002; after: a `using` owner read while alive ->
silent) — the MemoryPool twin of arraypool-span-view-after-return. Benchmark recall floor
16 -> 17. P-007 status + ci.yml comment updated.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 03814b03-c1a3-4a1a-b769-6dcf546885e0

📥 Commits

Reviewing files that changed from the base of the PR and between 8284055 and c000057.

📒 Files selected for processing (1)
  • corpus/real-world/memorypool-view-after-dispose/notes.md
✅ Files skipped from review due to trivial changes (1)
  • corpus/real-world/memorypool-view-after-dispose/notes.md

📝 Walkthrough

Walkthrough

Extends the Roslyn extractor's ViewOwner function to recognize IMemoryOwner<T> view borrows via owner.Memory and owner.Memory.Span. Adds a complete memorypool-view-after-dispose corpus case (before/after C#, OwnLang model, expected diagnostic OWN002, notes). Updates the P-007 proposal doc and raises the CI benchmark recall gate from 16 to 17.

Changes

MemoryPool view-after-dispose detection (OWN002)

Layer / File(s) Summary
ViewOwner: IMemoryOwner view borrow detection
frontend/roslyn/OwnSharp.Extractor/Program.cs
Two new pattern-matching branches added to ViewOwner: one matches owner.Memory when the receiver type is IMemoryOwner<T>, and one matches owner.Memory.Span by detecting a .Span access on a .Memory access of IMemoryOwner<T>. The doc comment above the function is updated to list the expanded recognized forms.
Corpus: memorypool-view-after-dispose test case
corpus/real-world/memorypool-view-after-dispose/before.cs, corpus/real-world/memorypool-view-after-dispose/after.cs, corpus/real-world/memorypool-view-after-dispose/case.own, corpus/real-world/memorypool-view-after-dispose/expected-diagnostics.txt, corpus/real-world/memorypool-view-after-dispose/notes.md
Adds before.cs modeling the use-after-dispose bug (rent, borrow owner.Memory, dispose, then read Span), after.cs showing the correct using-lifetime fix, case.own as an OwnLang model mapping Rent/Dispose to acquire/release with a use owner after dispose, expected-diagnostics.txt containing OWN002, and notes.md describing the POOL002 scenario and checker mapping.
Proposal doc update and CI recall threshold bump
docs/proposals/P-007-arraypool-span.md, .github/workflows/ci.yml
P-007 status text is revised to state that owner.Memory/owner.Memory.Span borrows are lowered to the OWNER so reading them after Dispose triggers POOL002 → OWN002. The corpus-benchmark CI job's --min-recall gate is raised from 16 to 17.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • PhysShell/Own.NET#50: Introduced the corpus-benchmark job and scripts/benchmark.py --min-recall gating that this PR's threshold bump directly modifies.
  • PhysShell/Own.NET#69: Extended the same ViewOwner borrow-resolution logic in Program.cs for ArrayPool/AsSpan+new Span views, which is the direct predecessor to this PR's IMemoryOwner/owner.Memory+.Span extension.
  • PhysShell/Own.NET#72: Added initial MemoryPool<T> Rent/IMemoryOwner<T> tracking in the extractor, on top of which this PR adds the view-borrow (owner.Memory/owner.Memory.Span) detection.

Poem

🐇 Hop, hop through memory lanes so wide,
A pool is rented, a span inside,
But dispose comes first — oh no, don't peek!
OWN002 fires for views that seek
the buffer back in the pool's embrace.
Now using keeps the lifetime in place! 🌿

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: detecting MemoryPool view-after-dispose (POOL002) as OWN002 violations, with proper technical specificity showing owner.Memory[.Span] access patterns and Dispose lifecycle.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/memorypool-view-borrow

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8284055bb1

ℹ️ 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 +934 to +938
if (e is MemberAccessExpressionSyntax mem
&& mem.Name.Identifier.Text == "Memory"
&& mem.Expression is IdentifierNameSyntax mo
&& model.GetSymbolInfo(mem).Symbol is IPropertySymbol { ContainingType: { Name: "IMemoryOwner" } ict }
&& IsInNamespace(ict, "System", "Buffers"))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Track using-owned MemoryPool views on escape

When the owner is declared with a using declaration and the method returns its view (using IMemoryOwner<byte> owner = ...; return owner.Memory;), the candidate pass skips using locals entirely, so this new owner.Memory mapping never sees owner in tracked and emits no post-dispose use. That return value is still delivered after the implicit dispose at scope exit, so the dangling Memory<T> escape that this change says it handles remains silent for the idiomatic using cleanup form.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the claim overstated. This PR delivers the use-after-explicit-Dispose POOL002 (the corpus case: var v = owner.Memory; owner.Dispose(); Use(v) → OWN002), which is solid. You're right that the dangling-Memory escape via the idiomatic using owner; return owner.Memory; is silent: the candidate pass skips using locals (they're correctly auto-disposed → not leak candidates), so owner never enters tracked and the returned view isn't mapped.

Catching that needs modelling the implicit scope-exit dispose as a release threaded on the return path (so the returned view's caller-use lands after it, like the ArrayPool try/finally Memory escape in #70) — a separate, non-trivial slice, since it changes how using locals are handled. I've scoped the claim in notes.md accordingly and noted it as the follow-up rather than leave it overstated. Thanks.


Generated by Claude Code

…ose (Codex review)

Codex P2: this PR catches a view-local read after an explicit owner.Dispose() (OWN002,
the corpus case) — but the dangling-Memory ESCAPE via the idiomatic `using owner;
return owner.Memory;` is NOT caught, because `using` locals are skipped as non-leak
candidates so the owner never enters `tracked`. Correct notes.md to not overstate: the
returned-view dangle from a `using` owner (modelling the implicit scope-exit dispose as
a release on the return path) is a follow-up, like the ArrayPool try/finally Memory
escape. The use-after-explicit-Dispose catch and the corpus case are unchanged.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants