Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>` 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

47 changes: 47 additions & 0 deletions corpus/wpf/pooled-view-after-dispose/after.cs
Original file line number Diff line number Diff line change
@@ -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<byte> _owner;
private readonly Memory<byte> _view;
private readonly IDisposable _sub;
private bool _disposed;

public FrameDecoder(int n, IEventBus bus)
{
_owner = MemoryPool<byte>.Shared.Rent(n);
_view = _owner.Memory;
_sub = bus.Subscribe<FrameReady>(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<T>(Action<T> handler);
}

public sealed class FrameReady { }

internal static class Sink
{
public static void Write(ReadOnlySpan<byte> data) { }
}
52 changes: 52 additions & 0 deletions corpus/wpf/pooled-view-after-dispose/before.cs
Original file line number Diff line number Diff line change
@@ -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<byte>` from `MemoryPool<byte>`), keeps a `Memory<byte>`
// 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<byte> _owner;
private readonly Memory<byte> _view;
private readonly IDisposable _sub;

public FrameDecoder(int n, IEventBus bus)
{
_owner = MemoryPool<byte>.Shared.Rent(n);
_view = _owner.Memory; // a view of the pooled buffer, kept in a field
_sub = bus.Subscribe<FrameReady>(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<T>(Action<T> handler);
}

public sealed class FrameReady { }

internal static class Sink
{
public static void Write(ReadOnlySpan<byte> data) { }
}
20 changes: 20 additions & 0 deletions corpus/wpf/pooled-view-after-dispose/case.own
Original file line number Diff line number Diff line change
@@ -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
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
OWN002
40 changes: 40 additions & 0 deletions corpus/wpf/pooled-view-after-dispose/notes.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# Pooled-buffer view-in-a-field, read after the owner is returned

**Pattern:** a type rents a pooled buffer (`IMemoryOwner<byte>` from `MemoryPool<byte>`), keeps a
`Memory<byte>` **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<T>` 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<T>.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<T>`** 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`.
78 changes: 65 additions & 13 deletions frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -947,20 +947,31 @@
|| (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<T>` 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<string, int> releasedAt,
Dictionary<string, string> 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<string, int> releasedAt)
BlockSyntax body, Dictionary<string, int> releasedAt, Dictionary<string, string> viewFieldOwner)
{
foreach (var ma in body.DescendantNodes().OfType<MemberAccessExpressionSyntax>())
{
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;
}
Expand Down Expand Up @@ -1098,6 +1109,25 @@
return null;
}

// Is `t` System.Buffers.IMemoryOwner<T> — the interface itself, or a concrete type that implements
// it? Resolved on the SYMBOL, so a fully-qualified `System.Buffers.IMemoryOwner<T>`, 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<T> 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<T>.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<T>(buf, 0, buf.Length)`, whose length argument is the
Expand Down Expand Up @@ -1752,7 +1782,7 @@
.Split(Path.PathSeparator, StringSplitOptions.RemoveEmptyEntries)
.Where(p => p.EndsWith(".dll", StringComparison.OrdinalIgnoreCase))
.ToList();
var refNames = new HashSet<string>(tpa.Select(Path.GetFileName), StringComparer.OrdinalIgnoreCase);

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1785 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.
var references = tpa.Select(p => (MetadataReference)MetadataReference.CreateFromFile(p)).ToList();
// P-004 WPF profile: widen the reference set with assemblies named by the
// OWN_EXTRA_REF_DIRS env var (colon-separated dirs) — e.g. the WindowsDesktop ref
Expand Down Expand Up @@ -1972,13 +2002,22 @@
// 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<T>` 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<string, int>(StringComparer.Ordinal);
foreach (var fd in cls.Members.OfType<FieldDeclarationSyntax>())
{
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<T>` / 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);
Expand All @@ -2002,6 +2041,18 @@
}
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<string, string>(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;
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// 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
Expand Down Expand Up @@ -2039,7 +2090,7 @@
&& 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<MethodDeclarationSyntax>())
Expand All @@ -2062,9 +2113,10 @@
{
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
Expand Down
Loading