-
Notifications
You must be signed in to change notification settings - Fork 0
feat(pool): POOL005 .Length spelling + MemoryPool tracking (POOL001/002/003) #72
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,20 @@ | ||
| // AFTER (fixed). The view is bound to the logical length `n` — `buf.AsSpan(0, n)` — | ||
| // so only the `n` valid payload bytes are read; the oversized `[n, Length)` tail is | ||
| // never touched. No `.Length`-spelled view remains, so the extractor emits no | ||
| // `overspan` fact and the checker is silent. | ||
| using System; | ||
| using System.Buffers; | ||
|
|
||
| static class PoolLengthOverread | ||
| { | ||
| static void Frame(int n) | ||
| { | ||
| byte[] buf = ArrayPool<byte>.Shared.Rent(n); | ||
| Fill(buf, n); | ||
| Emit(buf.AsSpan(0, n)); // bounded by the rented length n | ||
| ArrayPool<byte>.Shared.Return(buf); | ||
| } | ||
|
|
||
| static void Fill(byte[] b, int n) { for (int i = 0; i < n; i++) b[i] = (byte)i; } | ||
| static void Emit(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,29 @@ | ||
| // BEFORE (buggy). POOL005, the `.Length` spelling of the over-read. A rented array | ||
| // is OVERSIZED: `ArrayPool<T>.Shared.Rent(n)` returns an array of `Length >= n`. | ||
| // Viewing it with `buf.Length` — the oversized backing length — as the bound, | ||
| // `buf.AsSpan(0, buf.Length)`, spans the WHOLE array, so a consumer reads the `n` | ||
| // valid bytes PLUS the stale `[n, Length)` tail a previous renter left: a wrong- | ||
| // length read and an information disclosure. The fix is `buf.AsSpan(0, n)` (after.cs). | ||
| // | ||
| // Note: an over-CLEAR like `Array.Clear(buf, 0, buf.Length)` is deliberately NOT | ||
| // flagged — it only overwrites the pooled tail with zeros (a safe clear-before-Return | ||
| // idiom) and exposes nothing. The bug is READING/COPYING the tail, which the view | ||
| // spelling does. | ||
| // | ||
| // Wrapped in a class so the extractor's per-class flow pass visits it; helpers stubbed. | ||
| using System; | ||
| using System.Buffers; | ||
|
|
||
| static class PoolLengthOverread | ||
| { | ||
| static void Frame(int n) | ||
| { | ||
| byte[] buf = ArrayPool<byte>.Shared.Rent(n); | ||
| Fill(buf, n); // valid payload is buf[0..n] | ||
| Emit(buf.AsSpan(0, buf.Length)); // <-- BUG: length is buf.Length, not n -> reads the stale tail | ||
| ArrayPool<byte>.Shared.Return(buf); | ||
| } | ||
|
|
||
| static void Fill(byte[] b, int n) { for (int i = 0; i < n; i++) b[i] = (byte)i; } | ||
| static void Emit(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,17 @@ | ||
| // OwnLang model of the POOL005 `.Length` over-read. `acquire` == ArrayPool.Rent | ||
| // (oversized: Length >= n), `release` == ArrayPool.Return. `overspan buf` models a | ||
| // view whose length is the buffer's OWN oversized `.Length` (`buf.AsSpan(0, buf.Length)`), | ||
| // which spans the whole array — a consumer reads past the rented `n` into the stale | ||
| // tail. Bounding by `n` takes no full view, so the fix has no `overspan` and is silent. | ||
| // -> OWN025. The buffer is still returned, so there is no leak (OWN001). | ||
| module Corpus | ||
| resource Buffer { | ||
| acquire rent | ||
| release give | ||
| kind "pooled buffer" | ||
| } | ||
| fn frame(n: int) { | ||
| let buf = acquire Buffer(n); // ArrayPool.Rent (Length >= n) | ||
| overspan buf; // buf.AsSpan(0, buf.Length) — spans the whole array -> OWN025 | ||
| release buf; // ArrayPool.Return | ||
| } |
1 change: 1 addition & 0 deletions
1
corpus/real-world/arraypool-length-overread/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 @@ | ||
| OWN025 |
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,28 @@ | ||
| # ArrayPool over-read — the `.Length` spelling (POOL005) | ||
|
|
||
| **Pattern:** a pooled array from `ArrayPool<T>.Shared.Rent(n)` is *oversized* | ||
| (`Length >= n`). A view that uses `buf.Length` — the oversized backing length — as | ||
| its bound (`buf.AsSpan(0, buf.Length)`, `new Span<T>(buf, 0, buf.Length)`) spans the | ||
| whole array, so reading or copying through it processes the `n` valid bytes **plus** | ||
| the stale `[n, Length)` tail a previous renter left: a wrong-length read and a | ||
| potential information disclosure. The fix is to bound by the logical length: | ||
| `buf.AsSpan(0, n)`. | ||
|
|
||
| This is the `.Length` sibling of `arraypool-fullspan-overread` (which catches the | ||
| unbounded `buf.AsSpan()`): same OWN025, a different way of writing the oversized | ||
| length. The extractor recognises the `.Length`-as-length argument on the buffer's | ||
| own view (start `0`, resolved BCL symbols) and emits an `overspan` fact. | ||
|
|
||
| **Not flagged: the over-clear.** `Array.Clear(buf, 0, buf.Length)` (and the single-arg | ||
| `Array.Clear(buf)`) only *overwrite* the pooled tail with zeros — a safe | ||
| clear-before-`Return` idiom that exposes nothing. POOL005 is about *reading/copying* | ||
| too much, not *writing* too much (Codex review); only the **view** is flagged. | ||
|
|
||
| **What the checker says:** **OWN025** `[resource: pooled buffer]` at the view. The | ||
| buffer is still `Return`ed, so there is no OWN001 leak. | ||
|
|
||
| **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); replay target | ||
| AiDotNet.Tensors pooled-buffer over-read. |
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 @@ | ||
| // AFTER (fixed). A `using` declaration disposes the owner exactly once, at the end | ||
| // of the scope, on every path — no second Dispose. The pooled memory is released | ||
| // once and returned to the pool cleanly, so the checker is silent. | ||
| using System; | ||
| using System.Buffers; | ||
|
|
||
| static class MemoryPoolDoubleDispose | ||
| { | ||
| static void Run(int n) | ||
| { | ||
| using IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n); | ||
| Work(owner.Memory); | ||
| } | ||
|
|
||
| static void Work(Memory<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,30 @@ | ||
| // BEFORE (buggy). POOL003 for the OTHER pool: a `MemoryPool<T>` hands back an | ||
| // `IMemoryOwner<T>` released by `Dispose()` (there is no `Return`). Disposing the | ||
| // same owner twice — here on the success path AND again in `finally` — is a double | ||
| // release: the same memory can later be handed to two renters at once, exactly the | ||
| // corruption dotnet/runtime#33767 describes for ArrayPool double-return. The fix is | ||
| // to dispose exactly once (a `using` owner, see after.cs). | ||
| // | ||
| // 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 MemoryPoolDoubleDispose | ||
| { | ||
| static void Run(int n) | ||
| { | ||
| IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n); | ||
| try | ||
| { | ||
| Work(owner.Memory); | ||
| owner.Dispose(); // disposed here ... | ||
| } | ||
| finally | ||
| { | ||
| owner.Dispose(); // <-- ... and again here (double release) | ||
| } | ||
| } | ||
|
|
||
| static void Work(Memory<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,14 @@ | ||
| // OwnLang model of the MemoryPool double-dispose. A MemoryPool<T> owner is | ||
| // `acquire`d by Rent and `release`d by Dispose (there is no Return). Disposing it | ||
| // twice is a double release -> OWN003 — the same verdict as an ArrayPool | ||
| // double-return; the pool can then hand the same memory to two renters at once. | ||
| module Corpus | ||
| resource MemoryOwner { | ||
| acquire Rent | ||
| release Dispose | ||
| } | ||
| fn run(n: int) { | ||
| let owner = acquire MemoryOwner(n); // MemoryPool.Rent | ||
| release owner; // owner.Dispose() | ||
| release owner; // owner.Dispose() again -> OWN003 | ||
| } |
1 change: 1 addition & 0 deletions
1
corpus/real-world/memorypool-double-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 @@ | ||
| OWN003 |
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,26 @@ | ||
| # MemoryPool double-dispose (POOL003, the Dispose-released pool) | ||
|
|
||
| **Pattern:** `MemoryPool<T>.Shared.Rent(n)` returns an `IMemoryOwner<T>` whose | ||
| backing memory is returned to the pool by **`Dispose()`** (there is no `Return`). | ||
| Disposing the same owner twice — classically once on the normal path and again in a | ||
| `finally`/`Dispose`, or two explicit `Dispose()` calls — double-releases the memory, | ||
| so the pool can hand it to two renters simultaneously. This is the MemoryPool twin of | ||
| `arraypool-double-return` (dotnet/runtime#33767). | ||
|
|
||
| **What it adds:** the extractor now tracks `MemoryPool<T>.Shared.Rent` as an owned | ||
| resource (an `IMemoryOwner` released by `Dispose`, the IDisposable path — *not* a | ||
| `Return`-based pool buffer). With that one acquire recognised, the existing | ||
| flow-sensitive checker covers the whole MemoryPool family: **POOL001** (never | ||
| disposed → OWN001), **POOL002** (owner used after `Dispose` → OWN002), and | ||
| **POOL003** (this case — disposed twice → **OWN003**). | ||
|
|
||
| **What the checker says:** the OwnLang model and the real `before.cs` both trip | ||
| **OWN003** (double release). The `using` fix in `after.cs` disposes exactly once 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. The | ||
| `IMemoryOwner.Memory.Span` *view* tracking (a borrow of the owner) is a follow-up; | ||
| this slice covers the owner's own acquire / use / release lifecycle. | ||
|
|
||
| Reference: dotnet/runtime#33767; [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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.