Skip to content

Commit eef2e0c

Browse files
authored
Merge pull request #76 from PhysShell/claude/owner-dangle-and-indirect-uaf
feat(pool,wpf): bare-owner `using` dangle + one-hop indirect field-UAF → OWN002
2 parents 3b6dd4b + 734b9dc commit eef2e0c

11 files changed

Lines changed: 296 additions & 67 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -796,11 +796,15 @@ jobs:
796796
# leak / double-dispose ride the flow (POOL001/003 — `memorypool-double-dispose` -> OWN003), and
797797
# its `owner.Memory` / `owner.Memory.Span` view is a borrow lowered to a use of the OWNER
798798
# (`ViewOwner`), so reading it after Dispose trips OWN002 (POOL002 — `memorypool-view-after-
799-
# dispose`). A FIELD-mediated cross-method use-after-dispose is caught too: an IDisposable field
800-
# disposed in `Dispose()` and DIRECTLY read (`_field.Member`) in a live subscription-target handler
801-
# (RHS of a `+=` / arg of a `.Subscribe(...)`, not torn down, no `if (_disposed) return;` guard) is
802-
# lowered to a synthetic acquire/release/use flow -> OWN002 (`field-use-after-dispose`). Remaining
803-
# backlog: a view stored in a FIELD, an INDIRECT (helper-mediated) field use after dispose, and an
804-
# injected-source region-escape. A drop below the floor is a regression.
805-
run: python scripts/benchmark.py --min-recall 20
799+
# dispose`). Returning the BARE owner under `using` (`using owner = …; return owner;`) is the twin
800+
# of the returned-view dangle: the using-owner stays tracked through the bare return (a non-using
801+
# transfer does not) and its use is threaded after the scope-exit release -> OWN002 (`memorypool-
802+
# using-owner-escape`). A FIELD-mediated cross-method use-after-dispose is caught too: an IDisposable
803+
# field disposed in `Dispose()` and read in a live subscription-target handler (RHS of a `+=` / arg
804+
# of a `.Subscribe(...)`, not torn down, no `if (_disposed) return;` guard) — DIRECTLY
805+
# (`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
806810

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// AFTER (fixed). Ownership is TRANSFERRED to the caller: no `using`, so the owner is NOT disposed at
2+
// scope exit — the method hands back a live `IMemoryOwner<T>` and the caller owns its lifetime
3+
// (disposes it when done). A bare owner returned WITHOUT `using` is a genuine ownership transfer, so
4+
// the flow pass does not track it and the checker stays silent.
5+
using System;
6+
using System.Buffers;
7+
8+
static class MemoryPoolUsingOwnerEscape
9+
{
10+
static IMemoryOwner<byte> Borrow(int n)
11+
{
12+
IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n); // no `using` -> transfer
13+
return owner; // caller owns and disposes it
14+
}
15+
}
Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
// BEFORE (buggy). The bare-owner `using` dangle — the twin of `memorypool-using-view-escape`. An
2+
// `IMemoryOwner<T>` from `MemoryPool<T>` is held with a `using` declaration — so it is `Dispose()`d
3+
// at scope exit — but the method RETURNS THE OWNER ITSELF. The implicit dispose runs as the method
4+
// returns, so the caller receives an `IMemoryOwner<T>` already disposed (its pooled buffer handed
5+
// back to the pool): a dangling owner / use-after-free. `using owner = …; return owner;` is exactly
6+
// `try { return owner; } finally { owner.Dispose(); }`. The fix TRANSFERS ownership: drop the
7+
// `using` and let the caller own and dispose it (see after.cs).
8+
//
9+
// Wrapped in a class so the extractor's per-class flow pass visits it.
10+
using System;
11+
using System.Buffers;
12+
13+
static class MemoryPoolUsingOwnerEscape
14+
{
15+
static IMemoryOwner<byte> Borrow(int n)
16+
{
17+
using IMemoryOwner<byte> owner = MemoryPool<byte>.Shared.Rent(n);
18+
return owner; // <-- BUG: the `using` disposes owner as we return, so the caller gets an
19+
// IMemoryOwner already returned to the pool
20+
}
21+
}
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
// OwnLang model of the MemoryPool bare-owner `using` escape. `acquire` == MemoryPool.Rent,
2+
// `release` == the implicit `using` scope-exit Dispose. The method returns the OWNER itself, but
3+
// the `using` disposes it FIRST (the extractor desugars `using owner = …; return owner;` to
4+
// `acquire; release; use` — the returned owner is read by the caller AFTER Dispose), so the caller
5+
// holds an `IMemoryOwner` already returned to the pool: a use-after-release lowered to OWN002. The
6+
// bare-owner twin of `memorypool-using-view-escape` (which returns `owner.Memory`, a view of it).
7+
module Corpus
8+
resource MemoryOwner {
9+
acquire Rent
10+
release Dispose
11+
}
12+
fn lease(n: int) {
13+
let owner = acquire MemoryOwner(n); // using MemoryPool.Rent
14+
release owner; // implicit `using` Dispose at scope exit
15+
use owner; // return owner — read by the caller AFTER Dispose -> OWN002
16+
}
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
OWN002
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
# MemoryPool bare-owner `using` escape (`using owner = …; return owner;`)
2+
3+
**Pattern:** an `IMemoryOwner<T>` from `MemoryPool<T>` is held with a `using` declaration (so it is
4+
`Dispose()`d at scope exit), but the method **returns the owner itself**. The implicit dispose runs
5+
as the method returns, so the caller receives an `IMemoryOwner<T>` whose pooled buffer has already
6+
been handed back to the pool — a dangling owner / use-after-free. It is exactly
7+
`try { return owner; } finally { owner.Dispose(); }` — the **bare-owner** twin of the returned-view
8+
dangle (`memorypool-using-view-escape`, which returns `owner.Memory`). The fix is to **transfer
9+
ownership**: drop the `using` and return the live owner so the caller owns its lifetime.
10+
11+
**Why it was missed before (Codex follow-up on #74):** a local that is `return`ed is treated as an
12+
ownership transfer (the caller's to release) and dropped from the tracked set — so the bare-owner
13+
return escaped and was never analysed, even though the `using` makes it a dangle rather than a
14+
transfer. This slice keeps a **`using`-declared MemoryPool owner that is returned bare** tracked
15+
(only this shape is exempted from the return-escape — a non-`using` returned owner stays a genuine
16+
transfer, untracked, silent), and threads a use of the returned owner after the `using`-desugar's
17+
scope-exit release — reusing the exact return-chain insertion that already catches the returned
18+
**view** (`memorypool-using-view-escape`). So the caller's use of the owner lands after the release
19+
and trips OWN002.
20+
21+
**What the checker says:** the OwnLang model and the real `before.cs` both trip **OWN002**. The
22+
ownership-transfer fix in `after.cs` returns the owner with no `using`, so the escaped owner is
23+
untracked and the checker is silent.
24+
25+
**Honesty / scope.** `case.own` is a faithful hand reduction (not C# ingested by the checker);
26+
`before.cs` / `after.cs` are representative of the bug and its fix. One escape vector each: this
27+
catches the bare OWNER return; `memorypool-using-view-escape` catches the returned VIEW.
28+
29+
Reference: [P-007](../../../docs/proposals/P-007-arraypool-span.md); the view twin is
30+
`memorypool-using-view-escape` (#73/#74).
Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,19 @@
1-
// FIXED. The callback guards on the disposed flag (and/or the subscription is
2-
// disposed only after the dispatcher queue is drained), so nothing touches the
3-
// subscription-backed state after Dispose().
1+
// FIXED. The callback guards on the disposed flag BEFORE it calls the helper, so a late, already-
2+
// dispatched callback bails before anything reaches the connection. (Equivalently, drain the
3+
// dispatcher queue before disposing.) The extractor's field-UAF pass sees the opening
4+
// `if (_disposed) return;` guard precede the helper call and stays silent.
5+
using System;
6+
using System.Data.SqlClient;
7+
48
public sealed class CustomerViewModel : IDisposable
59
{
610
private readonly IDisposable _sub;
11+
private readonly SqlConnection _conn;
712
private bool _disposed;
813

914
public CustomerViewModel(IEventBus bus)
1015
{
16+
_conn = new SqlConnection("Server=.;Database=Customers");
1117
_sub = bus.Subscribe<CustomerChanged>(OnCustomerChanged);
1218
}
1319

@@ -17,11 +23,23 @@ private void OnCustomerChanged(CustomerChanged e)
1723
Refresh();
1824
}
1925

20-
private void Refresh() { /* ... */ }
26+
private void Refresh()
27+
{
28+
_conn.ChangeDatabase("customers");
29+
}
2130

2231
public void Dispose()
2332
{
2433
_disposed = true;
2534
_sub.Dispose();
35+
_conn.Dispose();
2636
}
2737
}
38+
39+
// Minimal in-file stand-ins so the reduction is self-contained.
40+
public interface IEventBus
41+
{
42+
IDisposable Subscribe<T>(Action<T> handler);
43+
}
44+
45+
public sealed class CustomerChanged { }
Lines changed: 28 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,29 +1,49 @@
1-
// BUGGY (representative WPF pattern, hand-reduced into case.own).
1+
// BUGGY (representative WPF pattern; the C# extractor now catches this end-to-end via its one-hop
2+
// indirect field-UAF pass).
23
//
3-
// The VM disposes its subscription on close, but a callback that was already
4-
// queued on the dispatcher still runs and touches the (now disposed) state. In
5-
// real code this surfaces as an ObjectDisposedException or a read of torn state.
4+
// The VM disposes its subscription (and its owned connection) on close, but a callback already
5+
// queued on the dispatcher still runs after Dispose() and reaches the disposed connection
6+
// INDIRECTLY: the handler calls a private `Refresh()` helper that reads `_conn` (disposed in
7+
// Dispose()). In real code this surfaces as an ObjectDisposedException or a read of torn state.
8+
// Unlike `field-use-after-dispose` (a DIRECT `_conn.X` read in the handler), here the disposed field
9+
// is one hop down, behind the helper — the extractor chases that single hop. The fix (after.cs)
10+
// guards the handler on a disposed flag before it calls the helper.
11+
using System;
12+
using System.Data.SqlClient;
13+
614
public sealed class CustomerViewModel : IDisposable
715
{
816
private readonly IDisposable _sub;
9-
private bool _disposed;
17+
private readonly SqlConnection _conn;
1018

1119
public CustomerViewModel(IEventBus bus)
1220
{
21+
_conn = new SqlConnection("Server=.;Database=Customers");
1322
_sub = bus.Subscribe<CustomerChanged>(OnCustomerChanged);
1423
}
1524

1625
private void OnCustomerChanged(CustomerChanged e)
1726
{
1827
// a late, already-dispatched callback: runs after Dispose()
19-
Refresh(); // touches subscription-backed state after it was disposed
28+
Refresh(); // reaches the disposed _conn INDIRECTLY (one hop, through the helper)
2029
}
2130

22-
private void Refresh() { /* reads disposed state */ }
31+
private void Refresh()
32+
{
33+
_conn.ChangeDatabase("customers"); // <-- reads the disposed connection
34+
}
2335

2436
public void Dispose()
2537
{
26-
_disposed = true;
2738
_sub.Dispose();
39+
_conn.Dispose();
2840
}
2941
}
42+
43+
// Minimal in-file stand-ins so the reduction is self-contained.
44+
public interface IEventBus
45+
{
46+
IDisposable Subscribe<T>(Action<T> handler);
47+
}
48+
49+
public sealed class CustomerChanged { }
Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,19 @@
1+
// OwnLang model of the field-mediated use-after-dispose reached INDIRECTLY (through a helper).
2+
// `acquire` == the owned connection's construction, `release` == its Dispose() in the ViewModel's
3+
// Dispose(), `use` == a late dispatcher callback (the subscribed handler) reaching the connection
4+
// one hop down — via a private Refresh() helper — AFTER Dispose(). Using a disposable after its
5+
// release is the generic OWN002, tagged with the resource kind. The indirect twin of
6+
// `field-use-after-dispose` (a DIRECT field read); the extractor now chases the single helper hop.
17
module WpfHandlerAfterDispose
28

3-
// Same subscription-token protocol, tagged with its kind.
4-
resource Subscription {
5-
acquire Subscribe
9+
resource Connection {
10+
acquire Open
611
release Dispose
7-
kind "subscription token"
12+
kind "disposable"
813
}
914

10-
// On window close the VM disposes (unsubscribes) its subscription, but a late
11-
// queued callback still touches it. Using a subscription after Dispose is the
12-
// generic use-after-release (OWN002), tagged with the resource kind.
13-
fn CloseHandler(bus: int) {
14-
let sub = acquire Subscription(bus);
15-
release sub; // unsubscribed / disposed on close
16-
use sub; // a late callback still touches it -> OWN002
15+
fn OnCustomerChanged(bus: int) {
16+
let conn = acquire Connection(bus);
17+
release conn; // ViewModel.Dispose() disposes the owned connection
18+
use conn; // a late callback reaches it via Refresh() after Dispose -> OWN002
1719
}
Lines changed: 27 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,32 @@
1-
# WPF subscription used after Dispose
1+
# WPF field-use-after-dispose reached INDIRECTLY (through a helper)
22

3-
**Pattern:** a ViewModel unsubscribes / disposes its subscription on close, but a
4-
callback that was already queued on the dispatcher still runs and touches the
5-
disposed, subscription-backed state. In real code this is an
6-
`ObjectDisposedException` or a read of torn state — the use-after-dispose cousin
3+
**Pattern:** a ViewModel owns an `IDisposable` (here a `SqlConnection`) and subscribes a handler to
4+
an event source. On teardown `Dispose()` disposes the connection (and the subscription token), but a
5+
callback already queued on the dispatcher still runs after `Dispose()` and reaches the disposed
6+
connection **indirectly** — the handler calls a private `Refresh()` helper that reads `_conn`. In
7+
real code this is an `ObjectDisposedException` or a read of torn state — the use-after-dispose cousin
78
of the zombie-ViewModel leak.
89

9-
**What the checker says:** using a resource after its `release` (Dispose) is the
10-
generic **OWN002** (use after release), carrying the resource-kind tag:
10+
**What's new — the extractor catches the one hop.** The Roslyn extractor's field-mediated
11+
use-after-dispose pass (under `--flow-locals`) already caught a **direct** `_field.Member` read in a
12+
live handler (`field-use-after-dispose`). This slice chases a **single hop**: a subscribed handler
13+
that calls a **private same-class helper** (`Refresh()` / `this.Refresh()`) which itself
14+
*unguardedly* reads a disposed field — with no `if (_disposed) return;` guard before the call — is
15+
lowered to a synthetic `acquire`/`release`/`use` flow → **OWN002**, via the existing OwnIR bridge
16+
(no new diagnostic). On the real C# the `corpus-benchmark` job scores `before.cs` as caught and
17+
`after.cs` (the guarded fix) as silent.
1118

12-
```text
13-
$ python -m ownlang check corpus/wpf/handler-use-after-dispose/case.own
14-
case.own:16:9: error: [OWN002] use 'sub' after it was released
15-
[resource: subscription token]
16-
16 | use sub;
17-
^
18-
```
19+
**Precision (why it stays low-FP).** One hop only — a deeper chain stays an honest miss. The helper
20+
must be a **private instance** method (not a public/virtual member with a broader contract); both the
21+
handler (before the call) and the helper must lack a disposed-guard; and the field read is a
22+
**direct** `this`-owned `_field.Member`. The guard exclusion is the canonical fix, so the guarded
23+
`after.cs` is silent.
1924

20-
**Honesty / scope.** `case.own` is a *hand reduction* of the C# pattern, not
21-
direct C# extractor output (the C# extractor in P-001 is narrow — event
22-
subscriptions only). It shows the ownership
23-
*logic* maps onto the real bug; it does not model the dispatcher queue or
24-
exception flow. `before.cs` / `after.cs` are representative, not a verbatim copy
25-
of one PR.
25+
**Honesty / scope.** This catches the **direct** read (`field-use-after-dispose`) and now the
26+
**one-hop indirect** read (this case). A two-plus-hop chain, or a read through a field/property
27+
indirection, remains an honest extractor miss — the `case.own` reduction still fires OWN002, showing
28+
the ownership logic maps onto the real bug. `before.cs` / `after.cs` are representative of the bug
29+
and its fix.
30+
31+
Reference: [P-007](../../../docs/proposals/P-007-arraypool-span.md); the direct twin is
32+
`field-use-after-dispose`; the late-callback framing matches `zombie-viewmodel`.

0 commit comments

Comments
 (0)