Skip to content

Commit dc1a1a7

Browse files
committed
fix(extractor): drop unsound pool guard-transfer; keep cross-member return only (Codex)
Codex P2: treating a rented field passed to ANY field-stored `new X(field)` as released wrongly suppresses a real leak for NON-owning wrappers (`_view = new ReadOnlyMemory<byte>(_buf)` and other cached views), where the array is never returned. Soundly distinguishing an owning guard (that Returns the buffer) from a non-owning view is not worth it for the single SharedArrayPoolBuffer FP, so the guard-transfer is removed entirely. Fix #3 keeps only the sound, high-value part: a pooled FIELD is released if `pool.Return(field)` appears anywhere in the class (cross-member ctor-rent + Dispose-return — BufferedReadStream). SharedArrayPoolBuffer's indirect release via its LifetimeGuard is left as an honest known limitation. Sample/CI drop the PoolFieldTransferredToGuard case accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
1 parent cec7c7e commit dc1a1a7

3 files changed

Lines changed: 12 additions & 56 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -231,10 +231,10 @@ jobs:
231231
fi
232232
echo "$out" | grep -q "NeverDisposesField" \
233233
|| { echo "FAIL: an undisposed IDisposable field control must still warn"; exit 1; }
234-
# #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) or transferred
235-
# into a field-stored guard must be silent; the rented-never-returned control warns.
236-
if echo "$out" | grep -qE "pooled buffer '(returnedBuf|guardedBuf)'"; then
237-
echo "FAIL: a pooled field returned in Dispose / transferred to a guard was wrongly flagged"; exit 1
234+
# #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) must be silent;
235+
# the rented-never-returned control still warns.
236+
if echo "$out" | grep -q "pooled buffer 'returnedBuf'"; then
237+
echo "FAIL: a pooled field returned in Dispose was wrongly flagged"; exit 1
238238
fi
239239
echo "$out" | grep -q "pooled buffer 'leakedBuf'" \
240240
|| { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; }

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

Lines changed: 7 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2366,37 +2366,20 @@ or ImplicitObjectCreationExpressionSyntax
23662366
// this syntactic pass; the local-declaration rents are skipped below to
23672367
// avoid double-reporting them (Codex).
23682368
// A FIELD-backed pooled buffer is legitimately rented in one member (the ctor) and
2369-
// released in another, so for FIELDS the release is searched CLASS-WIDE (a field name
2370-
// is unique to the class, so this cannot cross-mask — unlike same-named LOCALS in
2371-
// different methods, which keep the per-member scoping below). Released either by a
2372-
// direct `pool.Return(field)` ANYWHERE (mined: ImageSharp BufferedReadStream returns
2373-
// this.readBuffer in Dispose(bool)), or by TRANSFER of the buffer into a `new Guard(field)`
2374-
// that this object STORES in a field — a lifetime-owning wrapper that returns it (the #80
2375-
// escaping-ctor transfer at field level; mined: SharedArrayPoolBuffer's LifetimeGuard).
2369+
// Returned in another (Dispose), so for FIELDS the `pool.Return(field)` is searched
2370+
// CLASS-WIDE (a field name is unique to the class, so this cannot cross-mask — unlike
2371+
// same-named LOCALS in different methods, which keep the per-member scoping below).
2372+
// Mined: ImageSharp BufferedReadStream returns this.readBuffer in Dispose(bool). (An
2373+
// INDIRECT release via a lifetime-guard object — SharedArrayPoolBuffer — is left honest:
2374+
// a `new X(field)` is NOT assumed to own/return the buffer, since a non-owning view like
2375+
// `new ReadOnlyMemory<byte>(field)` would otherwise hide a real leak — Codex.)
23762376
var fieldReleased = new HashSet<string>();
23772377
foreach (var inv in cls.DescendantNodes().OfType<InvocationExpressionSyntax>())
23782378
if (inv.Expression is MemberAccessExpressionSyntax rm
23792379
&& rm.Name.Identifier.Text == "Return"
23802380
&& inv.ArgumentList.Arguments.Count > 0
23812381
&& model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs)
23822382
fieldReleased.Add(rfs.Name);
2383-
foreach (var oce in cls.DescendantNodes().OfType<BaseObjectCreationExpressionSyntax>())
2384-
{
2385-
// the `new Wrapper(field)` must itself be STORED in a field (kept by this object),
2386-
// not a throwaway local — mirrors #80's escape requirement, so a non-owning local
2387-
// view (`var v = new Span(field)`) is NOT mistaken for a transfer.
2388-
var storedInField =
2389-
(oce.Parent is AssignmentExpressionSyntax pa
2390-
&& model.GetSymbolInfo(pa.Left).Symbol is IFieldSymbol)
2391-
|| oce.Parent is EqualsValueClauseSyntax
2392-
{ Parent: VariableDeclaratorSyntax { Parent: VariableDeclarationSyntax
2393-
{ Parent: FieldDeclarationSyntax } } };
2394-
if (!storedInField || oce.ArgumentList is not { } al)
2395-
continue;
2396-
foreach (var arg in al.Arguments)
2397-
if (model.GetSymbolInfo(arg.Expression).Symbol is IFieldSymbol afs)
2398-
fieldReleased.Add(afs.Name);
2399-
}
24002383

24012384
foreach (var member in cls.Members)
24022385
{

‎frontend/roslyn/samples/FieldReleaseSample.cs‎

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -37,34 +37,7 @@ public sealed class PoolFieldReturnedInDispose : IDisposable
3737
public byte First() => this.returnedBuf[0];
3838
}
3939

40-
// #3: a pooled buffer FIELD transferred into a field-stored guard (#80 transfer at field
41-
// level) — the guard owns and returns it -> SILENT.
42-
public sealed class PoolFieldTransferredToGuard
43-
{
44-
private readonly byte[] guardedBuf;
45-
private readonly BufferGuard guard;
46-
47-
public PoolFieldTransferredToGuard(int n)
48-
{
49-
this.guardedBuf = ArrayPool<byte>.Shared.Rent(n);
50-
this.guard = new BufferGuard(this.guardedBuf); // ownership handed to the guard
51-
}
52-
}
53-
54-
public sealed class BufferGuard
55-
{
56-
private byte[]? held;
57-
58-
public BufferGuard(byte[] b) => this.held = b;
59-
60-
public void Release()
61-
{
62-
ArrayPool<byte>.Shared.Return(this.held!);
63-
this.held = null;
64-
}
65-
}
66-
67-
// #3 control: a pooled buffer FIELD rented but NEVER returned/transferred -> must WARN.
40+
// #3 control: a pooled buffer FIELD rented but NEVER returned -> must WARN.
6841
public sealed class PoolFieldLeaked
6942
{
7043
private readonly byte[] leakedBuf;

0 commit comments

Comments
 (0)