-
Notifications
You must be signed in to change notification settings - Fork 0
feat(pool): MemoryPool view borrow — owner.Memory[.Span] used after Dispose → OWN002 (POOL002) #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // AFTER (fixed). A `using` declaration owns the `IMemoryOwner` lifetime: the buffer is | ||
| // returned to the pool exactly once, at the end of the scope, AFTER the view has been | ||
| // read. The `owner.Memory.Span` borrow is consumed while the owner is still alive, so it | ||
| // is never read past the buffer's return. The checker is silent. | ||
| using System; | ||
| using System.Buffers; | ||
|
|
||
| static class MemoryPoolViewAfterDispose | ||
| { | ||
| static void Run(int n) | ||
| { | ||
| using IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n); | ||
| Consume(owner.Memory.Span); // read while the owner is still alive | ||
| } | ||
|
|
||
| static void Consume(ReadOnlySpan<byte> data) { } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // BEFORE (buggy). POOL002 for MemoryPool: an `IMemoryOwner<T>` from `MemoryPool<T>` | ||
| // exposes its pooled buffer as a `Memory<T>` via `owner.Memory`. That Memory (and the | ||
| // `Span` taken from it) is a BORROW of the owner — it is only valid while the owner is | ||
| // alive. Reading the view AFTER `owner.Dispose()` (which returns the memory to the pool) | ||
| // reads memory that may already have been handed to another renter: a use-after-free. | ||
| // The fix is to read the view BEFORE disposing — or let a `using` own the lifetime | ||
| // (see after.cs). The MemoryPool twin of `arraypool-span-view-after-return`. | ||
| // | ||
| // Wrapped in a class so the extractor's per-class flow pass visits it; the helper is | ||
| // stubbed so the reduction is self-contained. | ||
| using System; | ||
| using System.Buffers; | ||
|
|
||
| static class MemoryPoolViewAfterDispose | ||
| { | ||
| static void Run(int n) | ||
| { | ||
| 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); // <-- BUG: ... but the view is read AFTER (use-after-free) | ||
| } | ||
|
|
||
| static void Consume(ReadOnlySpan<byte> data) { } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // OwnLang model of the MemoryPool view-after-dispose. `acquire` == MemoryPool.Rent, | ||
| // `release` == owner.Dispose(). A `Memory<T>` view (`owner.Memory`, and the `Span` from | ||
| // it) is a BORROW of the owner — the extractor lowers a use of the view to a use of the | ||
| // OWNER, so reading it after Dispose is the generic use-after-release (OWN002), the same | ||
| // code reading the owner directly would give. The borrow is resolved to its owner in the | ||
| // extractor; the core sees a plain use-after-release. | ||
| module Corpus | ||
| resource MemoryOwner { | ||
| acquire Rent | ||
| release Dispose | ||
| } | ||
| fn run(n: int) { | ||
| let owner = acquire MemoryOwner(n); // MemoryPool.Rent | ||
| release owner; // owner.Dispose() <-- too early | ||
| use owner; // read the Memory view AFTER Dispose -> OWN002 | ||
| } |
1 change: 1 addition & 0 deletions
1
corpus/real-world/memorypool-view-after-dispose/expected-diagnostics.txt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| OWN002 |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # MemoryPool view used after dispose (POOL002, the Dispose-released pool) | ||
|
|
||
| **Pattern:** `MemoryPool<T>.Shared.Rent(n)` returns an `IMemoryOwner<T>` whose pooled | ||
| buffer is exposed as a `Memory<T>` via `owner.Memory` (and a `Span<T>` via | ||
| `owner.Memory.Span`). That view is a **borrow** of the owner — valid only while the | ||
| owner is alive. Reading it after `owner.Dispose()` (which returns the memory to the | ||
| pool) reads memory that may already belong to another renter: a use-after-free. The | ||
| fix is to read the view *before* disposing, or to let a `using` own the lifetime. | ||
|
|
||
| This is the MemoryPool twin of `arraypool-span-view-after-return`: there a `Span` | ||
| view of a `Rent`ed array is used after `Return`; here a `Memory`/`Span` view of an | ||
| `IMemoryOwner` is used after `Dispose`. Both lower the view to a use of the **owner** | ||
| (`ViewOwner` in the extractor), so the core sees a plain use-after-release. | ||
|
|
||
| **What it adds:** the extractor now recognises `owner.Memory` / `owner.Memory.Span` | ||
| (resolved via the `System.Buffers.IMemoryOwner<T>.Memory` property) as a borrow of the | ||
| owner — completing the MemoryPool story begun in #72 (which tracked the owner's | ||
| acquire / release lifecycle: POOL001 leak, POOL003 double-dispose). With the view | ||
| recognised, a view-local read after an explicit `owner.Dispose()` is | ||
| **POOL002 → OWN002**. (The *returned*-`Memory` dangle from the idiomatic `using` | ||
| owner — `using owner; return owner.Memory;`, where the implicit scope-exit dispose | ||
| hands a stale view to the caller — is a follow-up: `using` locals are skipped as | ||
| non-leak candidates, so that escape needs the scope-exit dispose modelled as a | ||
| release on the return path, like the ArrayPool try/finally `Memory` escape.) | ||
|
|
||
| **What the checker says:** the OwnLang model and the real `before.cs` both trip | ||
| **OWN002** (use after release). The `using` fix in `after.cs` reads the view while the | ||
| owner is alive and is silent. | ||
|
|
||
| **Honesty / scope.** `case.own` is a faithful hand reduction (not C# ingested by the | ||
| checker); `before.cs` / `after.cs` are representative of the bug and its fix. | ||
|
|
||
| Reference: [P-007](../../../docs/proposals/P-007-arraypool-span.md). |
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the owner is declared with a
usingdeclaration and the method returns its view (using IMemoryOwner<byte> owner = ...; return owner.Memory;), the candidate pass skipsusinglocals entirely, so this newowner.Memorymapping never seesownerintrackedand emits no post-dispose use. That return value is still delivered after the implicit dispose at scope exit, so the danglingMemory<T>escape that this change says it handles remains silent for the idiomaticusingcleanup form.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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-
DisposePOOL002 (the corpus case:var v = owner.Memory; owner.Dispose(); Use(v)→ OWN002), which is solid. You're right that the dangling-Memoryescape via the idiomaticusing owner; return owner.Memory;is silent: the candidate pass skipsusinglocals (they're correctly auto-disposed → not leak candidates), soownernever enterstrackedand 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
Memoryescape in #70) — a separate, non-trivial slice, since it changes howusinglocals are handled. I've scoped the claim innotes.mdaccordingly and noted it as the follow-up rather than leave it overstated. Thanks.Generated by Claude Code