diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c27f4a41..670df2a3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -803,8 +803,10 @@ jobs: # field disposed in `Dispose()` and read in a live subscription-target handler (RHS of a `+=` / arg # of a `.Subscribe(...)`, not torn down, no `if (_disposed) return;` guard) — DIRECTLY # (`field-use-after-dispose`) or ONE hop down through a private helper (`handler-use-after-dispose`) - # — lowered to a synthetic acquire/release/use flow -> OWN002. Remaining backlog: a view stored in a - # FIELD, a TWO-plus-hop indirect field use, and an injected-source region-escape. A drop below the - # floor is a regression. - run: python scripts/benchmark.py --min-recall 22 + # — lowered to a synthetic acquire/release/use flow -> OWN002. That pass also covers POOLED owners: + # an `IMemoryOwner` field released in `Dispose()` and a `Memory` VIEW field of it (`_view = + # _owner.Memory`) read in such a handler is the view-in-a-field dangle -> OWN002 (`pooled-view-after- + # dispose`). Remaining backlog: an ArrayPool `byte[]`-buffer-field view, a TWO-plus-hop indirect field + # use, and an injected-source region-escape. A drop below the floor is a regression. + run: python scripts/benchmark.py --min-recall 23 diff --git a/corpus/wpf/pooled-view-after-dispose/after.cs b/corpus/wpf/pooled-view-after-dispose/after.cs new file mode 100644 index 00000000..b62ced3b --- /dev/null +++ b/corpus/wpf/pooled-view-after-dispose/after.cs @@ -0,0 +1,47 @@ +// FIXED. The callback guards on the disposed flag before it touches the view, so a late, already- +// dispatched callback bails before reading a buffer that has been returned to the pool. (Equivalently, +// drain the dispatcher queue before disposing.) The extractor sees the opening `if (_disposed) return;` +// guard precede the view read and stays silent. +using System; +using System.Buffers; + +public sealed class FrameDecoder : IDisposable +{ + private readonly IMemoryOwner _owner; + private readonly Memory _view; + private readonly IDisposable _sub; + private bool _disposed; + + public FrameDecoder(int n, IEventBus bus) + { + _owner = MemoryPool.Shared.Rent(n); + _view = _owner.Memory; + _sub = bus.Subscribe(OnFrameReady); + } + + private void OnFrameReady(FrameReady e) + { + if (_disposed) return; // do not touch the pooled buffer after it is returned + Sink.Write(_view.Span); + } + + public void Dispose() + { + _disposed = true; + _sub.Dispose(); + _owner.Dispose(); + } +} + +// Minimal in-file stand-ins so the reduction is self-contained. +public interface IEventBus +{ + IDisposable Subscribe(Action handler); +} + +public sealed class FrameReady { } + +internal static class Sink +{ + public static void Write(ReadOnlySpan data) { } +} diff --git a/corpus/wpf/pooled-view-after-dispose/before.cs b/corpus/wpf/pooled-view-after-dispose/before.cs new file mode 100644 index 00000000..4731249c --- /dev/null +++ b/corpus/wpf/pooled-view-after-dispose/before.cs @@ -0,0 +1,52 @@ +// BUGGY (representative pooled-buffer pattern; the C# extractor catches this end-to-end via its +// field-mediated use-after-dispose pass extended to pooled owners and their Memory views). +// +// A type rents a pooled buffer (`IMemoryOwner` from `MemoryPool`), keeps a `Memory` +// VIEW of it IN A FIELD, and subscribes a handler to an event source. On teardown Dispose() returns +// the buffer to the pool (disposing the owner), but a callback already queued on the dispatcher still +// runs after Dispose() and reads the field-held view — a `Memory` backed by a buffer already handed +// back to the pool: a dangling borrow / use-after-free (an `ObjectDisposedException` or stale/torn +// bytes from the next renter). The view field aliases the owner, so reading it after the owner's +// Dispose is a use of the owner after release. The fix (after.cs) guards the handler on a disposed +// flag before it touches the view. +using System; +using System.Buffers; + +public sealed class FrameDecoder : IDisposable +{ + private readonly IMemoryOwner _owner; + private readonly Memory _view; + private readonly IDisposable _sub; + + public FrameDecoder(int n, IEventBus bus) + { + _owner = MemoryPool.Shared.Rent(n); + _view = _owner.Memory; // a view of the pooled buffer, kept in a field + _sub = bus.Subscribe(OnFrameReady); + } + + private void OnFrameReady(FrameReady e) + { + // a late, already-dispatched callback: runs after Dispose() + Sink.Write(_view.Span); // <-- reads a view of a buffer already returned to the pool + } + + public void Dispose() + { + _sub.Dispose(); + _owner.Dispose(); // pooled buffer returned; _view now dangles + } +} + +// Minimal in-file stand-ins so the reduction is self-contained. +public interface IEventBus +{ + IDisposable Subscribe(Action handler); +} + +public sealed class FrameReady { } + +internal static class Sink +{ + public static void Write(ReadOnlySpan data) { } +} diff --git a/corpus/wpf/pooled-view-after-dispose/case.own b/corpus/wpf/pooled-view-after-dispose/case.own new file mode 100644 index 00000000..6ea8153e --- /dev/null +++ b/corpus/wpf/pooled-view-after-dispose/case.own @@ -0,0 +1,20 @@ +// OwnLang model of the pooled-buffer view-in-a-field dangle. `acquire` == MemoryPool.Rent (the +// IMemoryOwner), `release` == its Dispose() in the type's Dispose() (the pooled buffer goes back to +// the pool), `use` == a late dispatcher callback (the subscribed handler) reading the owner's Memory +// VIEW — held in a field — AFTER Dispose. The view field aliases the owner, so reading it after the +// owner's release is a use of the owner after release, lowered to OWN002. (The C# bridge tags the +// synthetic flow with the generic `disposable` kind; here it is named `pooled buffer` to describe the +// modelled resource.) +module PooledViewAfterDispose + +resource PooledBuffer { + acquire Rent + release Dispose + kind "pooled buffer" +} + +fn OnFrameReady(bus: int) { + let owner = acquire PooledBuffer(bus); + release owner; // the type's Dispose() returns the pooled buffer + use owner; // a late callback reads the owner's Memory view (a field) after Dispose -> OWN002 +} diff --git a/corpus/wpf/pooled-view-after-dispose/expected-diagnostics.txt b/corpus/wpf/pooled-view-after-dispose/expected-diagnostics.txt new file mode 100644 index 00000000..3a36fa92 --- /dev/null +++ b/corpus/wpf/pooled-view-after-dispose/expected-diagnostics.txt @@ -0,0 +1 @@ +OWN002 diff --git a/corpus/wpf/pooled-view-after-dispose/notes.md b/corpus/wpf/pooled-view-after-dispose/notes.md new file mode 100644 index 00000000..c960fc5b --- /dev/null +++ b/corpus/wpf/pooled-view-after-dispose/notes.md @@ -0,0 +1,40 @@ +# Pooled-buffer view-in-a-field, read after the owner is returned + +**Pattern:** a type rents a pooled buffer (`IMemoryOwner` from `MemoryPool`), keeps a +`Memory` **view of it in a field**, and subscribes a handler to an event source. On teardown +`Dispose()` returns the buffer to the pool (disposing the owner), but a callback already queued on the +dispatcher still runs after `Dispose()` and reads the field-held view — a `Memory` backed by a buffer +already handed back to the pool: a dangling borrow / use-after-free (an `ObjectDisposedException`, or +stale/torn bytes once the buffer is re-rented). It is the pooled-buffer, field-stored cousin of the +local returned-view dangle (`memorypool-view-after-dispose`) and of the disposed-field UAF +(`field-use-after-dispose`). + +**What's new — the extractor follows the view-field to its owner.** The field-mediated +use-after-dispose pass (#75/#76) is generalised to **pooled owners** and their **Memory views**: an +`IMemoryOwner` field released in `Dispose()` is a released owner, and a `Memory`/`ReadOnlyMemory` +field assigned `_owner.Memory` (or `_buf.AsMemory(...)`) is recorded as a **view of that owner**. A +read of the view field (or of the owner directly) in a live subscription-target handler — directly or +one hop through a private helper — with no `if (_disposed) return;` guard before it, is lowered to a +synthetic `acquire`/`release`/`use` flow on the owner → **OWN002**, via the existing OwnIR bridge (no +new diagnostic). On the real C# the `corpus-benchmark` job scores `before.cs` as caught and the +guarded `after.cs` as silent. + +**Precision (why it stays low-FP).** The same reachability that makes the disposed-field UAF sound: a +live (not unsubscribed) handler can fire *after* `Dispose()`, so reading the owner's buffer (through +its view field) then is a use-after-release. The guard exclusion is the canonical fix; an unsubscribed +or guarded handler never fires; the owner must be released in the dispose lifecycle; and the view→owner +alias is recognised by the resolved `IMemoryOwner.Memory` / `MemoryExtensions.AsMemory` symbols, not +by name. Exposing such a view via a public getter is **not** flagged — `IMemoryOwner.Memory` is itself +a legitimate "valid until Dispose" pattern, so the exposure is sound; only a provably-post-release read +(the handler reachability) is a bug. + +**Honesty / scope.** This slice covers an **`IMemoryOwner`** owner field and its `Memory` view read +via member access (`_view.Span`, `_view.Length`, …) in a handler/helper. Honest follow-ups: an +**ArrayPool `byte[]` buffer field** returned in `Dispose()` (it interacts with the existing per-member +pool-leak pass), the view passed as a **bare argument** (`Consume(_view)`) rather than through a member +access, and **element-access** reads (`_buf[i]`). `case.own` is a faithful hand reduction of the +ownership logic; `before.cs` / `after.cs` are representative of the bug and its fix. + +Reference: [P-007](../../../docs/proposals/P-007-arraypool-span.md); the local twin is +`memorypool-view-after-dispose`; the disposed-field twins are `field-use-after-dispose` / +`handler-use-after-dispose`. diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 85713b08..44a48903 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -947,20 +947,31 @@ static bool DisposedGuardBefore(BlockSyntax body, int before) => || (ifs.Statement is BlockSyntax gb && gb.Statements.FirstOrDefault() is ReturnStatementSyntax))); -// The FIRST direct read of a disposed field (`_f.Member`, `_f`/`this._f` only) in `body` that is -// NOT protected by an opening disposed-guard — the unit the field-UAF pass keys on. Returns the -// member-access node and the field name, or null when the body has no such read (or its first -// disposed-field read is already guarded, the canonical-fix shape). Shared by the direct handler -// scan and the one-hop helper scan (an indirect use through a private helper). +// The released OWNER field reached by a read of field `f`: `f` itself when it is a released owner +// (an IDisposable or `IMemoryOwner` field released in Dispose), or — when `f` is a `Memory` VIEW +// field — the owner it aliases, provided that owner is released. Null when `f` reaches no released +// owner. Unifies the direct disposed-field read (#75/#76) with the pooled-view-in-a-field dangle: +// reading `_view` (a borrow of `_owner`) after `_owner`'s release is a use of `_owner` after release. +static string? ReleasedOwner(string f, Dictionary releasedAt, + Dictionary viewFieldOwner) => + releasedAt.ContainsKey(f) ? f + : viewFieldOwner.TryGetValue(f, out var owner) && releasedAt.ContainsKey(owner) ? owner + : null; + +// The FIRST read of a released owner — directly (`_f.Member`) or through a Memory VIEW field that +// aliases it — in `body` that is NOT protected by an opening disposed-guard. Returns the member- +// access node and the OWNER field, or null when the body has no such read (or its first such read is +// already guarded, the canonical-fix shape). Shared by the direct handler scan and the one-hop +// helper scan (an indirect use through a private helper). static (MemberAccessExpressionSyntax Use, string Field)? FirstUnguardedDisposedRead( - BlockSyntax body, Dictionary releasedAt) + BlockSyntax body, Dictionary releasedAt, Dictionary viewFieldOwner) { foreach (var ma in body.DescendantNodes().OfType()) { if (ma.Name.Identifier.Text is "Dispose" or "DisposeAsync") continue; - if (ThisFieldName(ma.Expression) is { } uf && releasedAt.ContainsKey(uf)) - return DisposedGuardBefore(body, ma.SpanStart) ? null : (ma, uf); + if (ThisFieldName(ma.Expression) is { } f && ReleasedOwner(f, releasedAt, viewFieldOwner) is { } owner) + return DisposedGuardBefore(body, ma.SpanStart) ? null : (ma, owner); } return null; } @@ -1098,6 +1109,25 @@ e is InvocationExpressionSyntax i return null; } +// Is `t` System.Buffers.IMemoryOwner — the interface itself, or a concrete type that implements +// it? Resolved on the SYMBOL, so a fully-qualified `System.Buffers.IMemoryOwner`, a type alias, or +// a concrete owner type is recognised — not only the bare `IMemoryOwner` spelling (CodeRabbit/Codex). +static bool IsMemoryOwnerType(ITypeSymbol? t) => + t is INamedTypeSymbol nt + && ((nt.Name == "IMemoryOwner" && IsInNamespace(nt, "System", "Buffers")) + || nt.AllInterfaces.Any(i => i.Name == "IMemoryOwner" && IsInNamespace(i, "System", "Buffers"))); + +// The owner FIELD a `Memory` view assignment aliases — `_owner.Memory` / `this._owner.Memory` of an +// IMemoryOwner field. Uses the this/bare receiver restriction (ThisFieldName), so `other._owner. +// Memory` (another instance's owner) is NOT recorded and the this-qualified spelling IS (Codex). The +// borrow is recognised by the resolved `IMemoryOwner.Memory` property symbol, not by name. +static string? FieldViewOwner(ExpressionSyntax e, SemanticModel model) => + e is MemberAccessExpressionSyntax { Name.Identifier.Text: "Memory" } mem + && model.GetSymbolInfo(mem).Symbol is IPropertySymbol { ContainingType: { Name: "IMemoryOwner" } ict } + && IsInNamespace(ict, "System", "Buffers") + ? ThisFieldName(mem.Expression) + : null; + // The tracked owner buffer a FULL-LENGTH view spans, else null. POOL005: `buf.AsSpan()` / // `buf.AsMemory()` with NO arguments span `[0, Length)` — the WHOLE backing array — and so does // `buf.AsSpan(0, buf.Length)` / `new Span(buf, 0, buf.Length)`, whose length argument is the @@ -1972,13 +2002,22 @@ or ImplicitObjectCreationExpressionSyntax // Gated on --flow-locals like the rest of the synthetic-flow emission. if (flowLocals) { - // IDisposable fields -> declaration line (the synthetic `acquire`). + // Owner fields -> declaration line (the synthetic `acquire`): IDisposable fields AND + // `IMemoryOwner` MemoryPool rentals, whose pooled buffer is released by Dispose() — so a + // read after that Dispose (including through a Memory VIEW field of the owner) dangles. var dispoFieldLine = new Dictionary(StringComparer.Ordinal); foreach (var fd in cls.Members.OfType()) { if (fd.Modifiers.Any(mm => mm.IsKind(SyntaxKind.StaticKeyword))) continue; - if (!IsDisposableType(fd.Declaration.Type.ToString())) + // IDisposable is matched syntactically (so a project's own unresolved `…Stream` etc. + // still counts); IMemoryOwner is matched on the resolved SYMBOL so a qualified + // `System.Buffers.IMemoryOwner` / alias / concrete owner type counts too (CodeRabbit/ + // Codex). The cheap `StartsWith` short-circuits the common unqualified spelling first. + var ftype = fd.Declaration.Type.ToString(); + if (!IsDisposableType(ftype) + && !ftype.StartsWith("IMemoryOwner", StringComparison.Ordinal) + && !IsMemoryOwnerType(model.GetTypeInfo(fd.Declaration.Type).Type)) continue; foreach (var v in fd.Declaration.Variables) dispoFieldLine[v.Identifier.Text] = LineOf(v); @@ -2002,6 +2041,18 @@ or ImplicitObjectCreationExpressionSyntax } if (releasedAt.Count > 0) { + // a Memory/ReadOnlyMemory VIEW field that aliases an owner field — `_view = _owner.Memory` + // (an IMemoryOwner rental) or `_view = _buf.AsMemory(...)` — mapped to its OWNER, so a read + // of the VIEW field after the owner's release is a use of the released owner (the pooled- + // buffer view-in-a-field dangle). `ViewOwner` returns the owner name for a bare-field + // receiver; only owners actually released (in `releasedAt`) fire, checked at the read. + var viewFieldOwner = new Dictionary(StringComparer.Ordinal); + foreach (var a in assigns) + if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) + && ThisFieldName(a.Left) is { } vf + && FieldViewOwner(a.Right, model) is { } vo) + viewFieldOwner[vf] = vo; + // handler method names that are LIVE subscription targets. `+=` subscriptions are // keyed by SOURCE|handler so a `-=` removes only the MATCHING one — a handler still // `+=`'d to another live source stays live (a name-only set would let one `-=` drop @@ -2039,7 +2090,7 @@ or ImplicitObjectCreationExpressionSyntax && hm.Identifier.Text is not ("Dispose" or "DisposeAsync") && IsPrivateInstanceHelper(hm) && model.GetDeclaredSymbol(hm) is { } hsym - && FirstUnguardedDisposedRead(b, releasedAt) is { } r) + && FirstUnguardedDisposedRead(b, releasedAt, viewFieldOwner) is { } r) helperReads[hsym] = (r.Field, LineOf(r.Use)); foreach (var hm in cls.Members.OfType()) @@ -2062,9 +2113,10 @@ or ImplicitObjectCreationExpressionSyntax { if (node is MemberAccessExpressionSyntax ma && ma.Name.Identifier.Text is not ("Dispose" or "DisposeAsync") - && ThisFieldName(ma.Expression) is { } uf && releasedAt.ContainsKey(uf)) + && ThisFieldName(ma.Expression) is { } uf + && ReleasedOwner(uf, releasedAt, viewFieldOwner) is { } owner) { - useField = uf; useLine = LineOf(ma); triggerPos = ma.SpanStart; + useField = owner; useLine = LineOf(ma); triggerPos = ma.SpanStart; break; } if (node is InvocationExpressionSyntax call