Skip to content

Commit 70376da

Browse files
authored
Merge pull request #78 from PhysShell/claude/pooled-view-in-field
feat(pool): pooled-buffer view-in-a-field dangle → OWN002 (POOL005)
2 parents 5b31c36 + fa09b1c commit 70376da

7 files changed

Lines changed: 231 additions & 17 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -803,8 +803,10 @@ jobs:
803803
# field disposed in `Dispose()` and read in a live subscription-target handler (RHS of a `+=` / arg
804804
# of a `.Subscribe(...)`, not torn down, no `if (_disposed) return;` guard) — DIRECTLY
805805
# (`field-use-after-dispose`) or ONE hop down through a private helper (`handler-use-after-dispose`)
806-
# — lowered to a synthetic acquire/release/use flow -> OWN002. Remaining backlog: a view stored in a
807-
# FIELD, a TWO-plus-hop indirect field use, and an injected-source region-escape. A drop below the
808-
# floor is a regression.
809-
run: python scripts/benchmark.py --min-recall 22
806+
# — lowered to a synthetic acquire/release/use flow -> OWN002. That pass also covers POOLED owners:
807+
# an `IMemoryOwner<T>` field released in `Dispose()` and a `Memory` VIEW field of it (`_view =
808+
# _owner.Memory`) read in such a handler is the view-in-a-field dangle -> OWN002 (`pooled-view-after-
809+
# dispose`). Remaining backlog: an ArrayPool `byte[]`-buffer-field view, a TWO-plus-hop indirect field
810+
# use, and an injected-source region-escape. A drop below the floor is a regression.
811+
run: python scripts/benchmark.py --min-recall 23
810812

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
// FIXED. The callback guards on the disposed flag before it touches the view, so a late, already-
2+
// dispatched callback bails before reading a buffer that has been returned to the pool. (Equivalently,
3+
// drain the dispatcher queue before disposing.) The extractor sees the opening `if (_disposed) return;`
4+
// guard precede the view read and stays silent.
5+
using System;
6+
using System.Buffers;
7+
8+
public sealed class FrameDecoder : IDisposable
9+
{
10+
private readonly IMemoryOwner<byte> _owner;
11+
private readonly Memory<byte> _view;
12+
private readonly IDisposable _sub;
13+
private bool _disposed;
14+
15+
public FrameDecoder(int n, IEventBus bus)
16+
{
17+
_owner = MemoryPool<byte>.Shared.Rent(n);
18+
_view = _owner.Memory;
19+
_sub = bus.Subscribe<FrameReady>(OnFrameReady);
20+
}
21+
22+
private void OnFrameReady(FrameReady e)
23+
{
24+
if (_disposed) return; // do not touch the pooled buffer after it is returned
25+
Sink.Write(_view.Span);
26+
}
27+
28+
public void Dispose()
29+
{
30+
_disposed = true;
31+
_sub.Dispose();
32+
_owner.Dispose();
33+
}
34+
}
35+
36+
// Minimal in-file stand-ins so the reduction is self-contained.
37+
public interface IEventBus
38+
{
39+
IDisposable Subscribe<T>(Action<T> handler);
40+
}
41+
42+
public sealed class FrameReady { }
43+
44+
internal static class Sink
45+
{
46+
public static void Write(ReadOnlySpan<byte> data) { }
47+
}
Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
// BUGGY (representative pooled-buffer pattern; the C# extractor catches this end-to-end via its
2+
// field-mediated use-after-dispose pass extended to pooled owners and their Memory views).
3+
//
4+
// A type rents a pooled buffer (`IMemoryOwner<byte>` from `MemoryPool<byte>`), keeps a `Memory<byte>`
5+
// VIEW of it IN A FIELD, and subscribes a handler to an event source. On teardown Dispose() returns
6+
// the buffer to the pool (disposing the owner), but a callback already queued on the dispatcher still
7+
// runs after Dispose() and reads the field-held view — a `Memory` backed by a buffer already handed
8+
// back to the pool: a dangling borrow / use-after-free (an `ObjectDisposedException` or stale/torn
9+
// bytes from the next renter). The view field aliases the owner, so reading it after the owner's
10+
// Dispose is a use of the owner after release. The fix (after.cs) guards the handler on a disposed
11+
// flag before it touches the view.
12+
using System;
13+
using System.Buffers;
14+
15+
public sealed class FrameDecoder : IDisposable
16+
{
17+
private readonly IMemoryOwner<byte> _owner;
18+
private readonly Memory<byte> _view;
19+
private readonly IDisposable _sub;
20+
21+
public FrameDecoder(int n, IEventBus bus)
22+
{
23+
_owner = MemoryPool<byte>.Shared.Rent(n);
24+
_view = _owner.Memory; // a view of the pooled buffer, kept in a field
25+
_sub = bus.Subscribe<FrameReady>(OnFrameReady);
26+
}
27+
28+
private void OnFrameReady(FrameReady e)
29+
{
30+
// a late, already-dispatched callback: runs after Dispose()
31+
Sink.Write(_view.Span); // <-- reads a view of a buffer already returned to the pool
32+
}
33+
34+
public void Dispose()
35+
{
36+
_sub.Dispose();
37+
_owner.Dispose(); // pooled buffer returned; _view now dangles
38+
}
39+
}
40+
41+
// Minimal in-file stand-ins so the reduction is self-contained.
42+
public interface IEventBus
43+
{
44+
IDisposable Subscribe<T>(Action<T> handler);
45+
}
46+
47+
public sealed class FrameReady { }
48+
49+
internal static class Sink
50+
{
51+
public static void Write(ReadOnlySpan<byte> data) { }
52+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
// OwnLang model of the pooled-buffer view-in-a-field dangle. `acquire` == MemoryPool.Rent (the
2+
// IMemoryOwner), `release` == its Dispose() in the type's Dispose() (the pooled buffer goes back to
3+
// the pool), `use` == a late dispatcher callback (the subscribed handler) reading the owner's Memory
4+
// VIEW — held in a field — AFTER Dispose. The view field aliases the owner, so reading it after the
5+
// owner's release is a use of the owner after release, lowered to OWN002. (The C# bridge tags the
6+
// synthetic flow with the generic `disposable` kind; here it is named `pooled buffer` to describe the
7+
// modelled resource.)
8+
module PooledViewAfterDispose
9+
10+
resource PooledBuffer {
11+
acquire Rent
12+
release Dispose
13+
kind "pooled buffer"
14+
}
15+
16+
fn OnFrameReady(bus: int) {
17+
let owner = acquire PooledBuffer(bus);
18+
release owner; // the type's Dispose() returns the pooled buffer
19+
use owner; // a late callback reads the owner's Memory view (a field) after Dispose -> OWN002
20+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
OWN002
Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,40 @@
1+
# Pooled-buffer view-in-a-field, read after the owner is returned
2+
3+
**Pattern:** a type rents a pooled buffer (`IMemoryOwner<byte>` from `MemoryPool<byte>`), keeps a
4+
`Memory<byte>` **view of it in a field**, and subscribes a handler to an event source. On teardown
5+
`Dispose()` returns the buffer to the pool (disposing the owner), but a callback already queued on the
6+
dispatcher still runs after `Dispose()` and reads the field-held view — a `Memory` backed by a buffer
7+
already handed back to the pool: a dangling borrow / use-after-free (an `ObjectDisposedException`, or
8+
stale/torn bytes once the buffer is re-rented). It is the pooled-buffer, field-stored cousin of the
9+
local returned-view dangle (`memorypool-view-after-dispose`) and of the disposed-field UAF
10+
(`field-use-after-dispose`).
11+
12+
**What's new — the extractor follows the view-field to its owner.** The field-mediated
13+
use-after-dispose pass (#75/#76) is generalised to **pooled owners** and their **Memory views**: an
14+
`IMemoryOwner<T>` field released in `Dispose()` is a released owner, and a `Memory`/`ReadOnlyMemory`
15+
field assigned `_owner.Memory` (or `_buf.AsMemory(...)`) is recorded as a **view of that owner**. A
16+
read of the view field (or of the owner directly) in a live subscription-target handler — directly or
17+
one hop through a private helper — with no `if (_disposed) return;` guard before it, is lowered to a
18+
synthetic `acquire`/`release`/`use` flow on the owner → **OWN002**, via the existing OwnIR bridge (no
19+
new diagnostic). On the real C# the `corpus-benchmark` job scores `before.cs` as caught and the
20+
guarded `after.cs` as silent.
21+
22+
**Precision (why it stays low-FP).** The same reachability that makes the disposed-field UAF sound: a
23+
live (not unsubscribed) handler can fire *after* `Dispose()`, so reading the owner's buffer (through
24+
its view field) then is a use-after-release. The guard exclusion is the canonical fix; an unsubscribed
25+
or guarded handler never fires; the owner must be released in the dispose lifecycle; and the view→owner
26+
alias is recognised by the resolved `IMemoryOwner<T>.Memory` / `MemoryExtensions.AsMemory` symbols, not
27+
by name. Exposing such a view via a public getter is **not** flagged — `IMemoryOwner.Memory` is itself
28+
a legitimate "valid until Dispose" pattern, so the exposure is sound; only a provably-post-release read
29+
(the handler reachability) is a bug.
30+
31+
**Honesty / scope.** This slice covers an **`IMemoryOwner<T>`** owner field and its `Memory` view read
32+
via member access (`_view.Span`, `_view.Length`, …) in a handler/helper. Honest follow-ups: an
33+
**ArrayPool `byte[]` buffer field** returned in `Dispose()` (it interacts with the existing per-member
34+
pool-leak pass), the view passed as a **bare argument** (`Consume(_view)`) rather than through a member
35+
access, and **element-access** reads (`_buf[i]`). `case.own` is a faithful hand reduction of the
36+
ownership logic; `before.cs` / `after.cs` are representative of the bug and its fix.
37+
38+
Reference: [P-007](../../../docs/proposals/P-007-arraypool-span.md); the local twin is
39+
`memorypool-view-after-dispose`; the disposed-field twins are `field-use-after-dispose` /
40+
`handler-use-after-dispose`.

‎frontend/roslyn/OwnSharp.Extractor/Program.cs‎

Lines changed: 65 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -947,20 +947,31 @@ static bool DisposedGuardBefore(BlockSyntax body, int before) =>
947947
|| (ifs.Statement is BlockSyntax gb
948948
&& gb.Statements.FirstOrDefault() is ReturnStatementSyntax)));
949949

950-
// The FIRST direct read of a disposed field (`_f.Member`, `_f`/`this._f` only) in `body` that is
951-
// NOT protected by an opening disposed-guard — the unit the field-UAF pass keys on. Returns the
952-
// member-access node and the field name, or null when the body has no such read (or its first
953-
// disposed-field read is already guarded, the canonical-fix shape). Shared by the direct handler
954-
// scan and the one-hop helper scan (an indirect use through a private helper).
950+
// The released OWNER field reached by a read of field `f`: `f` itself when it is a released owner
951+
// (an IDisposable or `IMemoryOwner<T>` field released in Dispose), or — when `f` is a `Memory` VIEW
952+
// field — the owner it aliases, provided that owner is released. Null when `f` reaches no released
953+
// owner. Unifies the direct disposed-field read (#75/#76) with the pooled-view-in-a-field dangle:
954+
// reading `_view` (a borrow of `_owner`) after `_owner`'s release is a use of `_owner` after release.
955+
static string? ReleasedOwner(string f, Dictionary<string, int> releasedAt,
956+
Dictionary<string, string> viewFieldOwner) =>
957+
releasedAt.ContainsKey(f) ? f
958+
: viewFieldOwner.TryGetValue(f, out var owner) && releasedAt.ContainsKey(owner) ? owner
959+
: null;
960+
961+
// The FIRST read of a released owner — directly (`_f.Member`) or through a Memory VIEW field that
962+
// aliases it — in `body` that is NOT protected by an opening disposed-guard. Returns the member-
963+
// access node and the OWNER field, or null when the body has no such read (or its first such read is
964+
// already guarded, the canonical-fix shape). Shared by the direct handler scan and the one-hop
965+
// helper scan (an indirect use through a private helper).
955966
static (MemberAccessExpressionSyntax Use, string Field)? FirstUnguardedDisposedRead(
956-
BlockSyntax body, Dictionary<string, int> releasedAt)
967+
BlockSyntax body, Dictionary<string, int> releasedAt, Dictionary<string, string> viewFieldOwner)
957968
{
958969
foreach (var ma in body.DescendantNodes().OfType<MemberAccessExpressionSyntax>())
959970
{
960971
if (ma.Name.Identifier.Text is "Dispose" or "DisposeAsync")
961972
continue;
962-
if (ThisFieldName(ma.Expression) is { } uf && releasedAt.ContainsKey(uf))
963-
return DisposedGuardBefore(body, ma.SpanStart) ? null : (ma, uf);
973+
if (ThisFieldName(ma.Expression) is { } f && ReleasedOwner(f, releasedAt, viewFieldOwner) is { } owner)
974+
return DisposedGuardBefore(body, ma.SpanStart) ? null : (ma, owner);
964975
}
965976
return null;
966977
}
@@ -1098,6 +1109,25 @@ e is InvocationExpressionSyntax i
10981109
return null;
10991110
}
11001111

1112+
// Is `t` System.Buffers.IMemoryOwner<T> — the interface itself, or a concrete type that implements
1113+
// it? Resolved on the SYMBOL, so a fully-qualified `System.Buffers.IMemoryOwner<T>`, a type alias, or
1114+
// a concrete owner type is recognised — not only the bare `IMemoryOwner` spelling (CodeRabbit/Codex).
1115+
static bool IsMemoryOwnerType(ITypeSymbol? t) =>
1116+
t is INamedTypeSymbol nt
1117+
&& ((nt.Name == "IMemoryOwner" && IsInNamespace(nt, "System", "Buffers"))
1118+
|| nt.AllInterfaces.Any(i => i.Name == "IMemoryOwner" && IsInNamespace(i, "System", "Buffers")));
1119+
1120+
// The owner FIELD a `Memory` view assignment aliases — `_owner.Memory` / `this._owner.Memory` of an
1121+
// IMemoryOwner<T> field. Uses the this/bare receiver restriction (ThisFieldName), so `other._owner.
1122+
// Memory` (another instance's owner) is NOT recorded and the this-qualified spelling IS (Codex). The
1123+
// borrow is recognised by the resolved `IMemoryOwner<T>.Memory` property symbol, not by name.
1124+
static string? FieldViewOwner(ExpressionSyntax e, SemanticModel model) =>
1125+
e is MemberAccessExpressionSyntax { Name.Identifier.Text: "Memory" } mem
1126+
&& model.GetSymbolInfo(mem).Symbol is IPropertySymbol { ContainingType: { Name: "IMemoryOwner" } ict }
1127+
&& IsInNamespace(ict, "System", "Buffers")
1128+
? ThisFieldName(mem.Expression)
1129+
: null;
1130+
11011131
// The tracked owner buffer a FULL-LENGTH view spans, else null. POOL005: `buf.AsSpan()` /
11021132
// `buf.AsMemory()` with NO arguments span `[0, Length)` — the WHOLE backing array — and so does
11031133
// `buf.AsSpan(0, buf.Length)` / `new Span<T>(buf, 0, buf.Length)`, whose length argument is the
@@ -1972,13 +2002,22 @@ or ImplicitObjectCreationExpressionSyntax
19722002
// Gated on --flow-locals like the rest of the synthetic-flow emission.
19732003
if (flowLocals)
19742004
{
1975-
// IDisposable fields -> declaration line (the synthetic `acquire`).
2005+
// Owner fields -> declaration line (the synthetic `acquire`): IDisposable fields AND
2006+
// `IMemoryOwner<T>` MemoryPool rentals, whose pooled buffer is released by Dispose() — so a
2007+
// read after that Dispose (including through a Memory VIEW field of the owner) dangles.
19762008
var dispoFieldLine = new Dictionary<string, int>(StringComparer.Ordinal);
19772009
foreach (var fd in cls.Members.OfType<FieldDeclarationSyntax>())
19782010
{
19792011
if (fd.Modifiers.Any(mm => mm.IsKind(SyntaxKind.StaticKeyword)))
19802012
continue;
1981-
if (!IsDisposableType(fd.Declaration.Type.ToString()))
2013+
// IDisposable is matched syntactically (so a project's own unresolved `…Stream` etc.
2014+
// still counts); IMemoryOwner is matched on the resolved SYMBOL so a qualified
2015+
// `System.Buffers.IMemoryOwner<T>` / alias / concrete owner type counts too (CodeRabbit/
2016+
// Codex). The cheap `StartsWith` short-circuits the common unqualified spelling first.
2017+
var ftype = fd.Declaration.Type.ToString();
2018+
if (!IsDisposableType(ftype)
2019+
&& !ftype.StartsWith("IMemoryOwner", StringComparison.Ordinal)
2020+
&& !IsMemoryOwnerType(model.GetTypeInfo(fd.Declaration.Type).Type))
19822021
continue;
19832022
foreach (var v in fd.Declaration.Variables)
19842023
dispoFieldLine[v.Identifier.Text] = LineOf(v);
@@ -2002,6 +2041,18 @@ or ImplicitObjectCreationExpressionSyntax
20022041
}
20032042
if (releasedAt.Count > 0)
20042043
{
2044+
// a Memory/ReadOnlyMemory VIEW field that aliases an owner field — `_view = _owner.Memory`
2045+
// (an IMemoryOwner rental) or `_view = _buf.AsMemory(...)` — mapped to its OWNER, so a read
2046+
// of the VIEW field after the owner's release is a use of the released owner (the pooled-
2047+
// buffer view-in-a-field dangle). `ViewOwner` returns the owner name for a bare-field
2048+
// receiver; only owners actually released (in `releasedAt`) fire, checked at the read.
2049+
var viewFieldOwner = new Dictionary<string, string>(StringComparer.Ordinal);
2050+
foreach (var a in assigns)
2051+
if (a.IsKind(SyntaxKind.SimpleAssignmentExpression)
2052+
&& ThisFieldName(a.Left) is { } vf
2053+
&& FieldViewOwner(a.Right, model) is { } vo)
2054+
viewFieldOwner[vf] = vo;
2055+
20052056
// handler method names that are LIVE subscription targets. `+=` subscriptions are
20062057
// keyed by SOURCE|handler so a `-=` removes only the MATCHING one — a handler still
20072058
// `+=`'d to another live source stays live (a name-only set would let one `-=` drop
@@ -2039,7 +2090,7 @@ or ImplicitObjectCreationExpressionSyntax
20392090
&& hm.Identifier.Text is not ("Dispose" or "DisposeAsync")
20402091
&& IsPrivateInstanceHelper(hm)
20412092
&& model.GetDeclaredSymbol(hm) is { } hsym
2042-
&& FirstUnguardedDisposedRead(b, releasedAt) is { } r)
2093+
&& FirstUnguardedDisposedRead(b, releasedAt, viewFieldOwner) is { } r)
20432094
helperReads[hsym] = (r.Field, LineOf(r.Use));
20442095

20452096
foreach (var hm in cls.Members.OfType<MethodDeclarationSyntax>())
@@ -2062,9 +2113,10 @@ or ImplicitObjectCreationExpressionSyntax
20622113
{
20632114
if (node is MemberAccessExpressionSyntax ma
20642115
&& ma.Name.Identifier.Text is not ("Dispose" or "DisposeAsync")
2065-
&& ThisFieldName(ma.Expression) is { } uf && releasedAt.ContainsKey(uf))
2116+
&& ThisFieldName(ma.Expression) is { } uf
2117+
&& ReleasedOwner(uf, releasedAt, viewFieldOwner) is { } owner)
20662118
{
2067-
useField = uf; useLine = LineOf(ma); triggerPos = ma.SpanStart;
2119+
useField = owner; useLine = LineOf(ma); triggerPos = ma.SpanStart;
20682120
break;
20692121
}
20702122
if (node is InvocationExpressionSyntax call

0 commit comments

Comments
 (0)