Skip to content

Commit ef5789f

Browse files
authored
Merge pull request #80 from PhysShell/claude/fp-pool-buffer-ctor-transfer
fix(extractor): pooled buffer passed to an escaping constructor is a transfer, not a leak (mined FP)
2 parents 0d462b5 + 56fc7df commit ef5789f

3 files changed

Lines changed: 50 additions & 2 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,10 @@ jobs:
518518
# case disposes (no default) -> last case is the tail, no phantom no-match leak.
519519
# `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release
520520
# (member-binding form), so it is disposed on the return path -> silent (Codex review).
521-
for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped defer; do
521+
# `ctorMoved`: a pooled buffer handed to a constructor whose result is RETURNED transfers
522+
# ownership to the returned wrapper -> escaped -> silent (mined FP on
523+
# Pipelines.Sockets.Unofficial: ArrayPoolBufferWriter.CreateNewSegment).
524+
for ok in clean looped esc exemptTask whileClean asyncDisposed asyncDisposedCfg tfClean tfCatch tfRet tfNull cda daci cif ctorLater lamPrior other doClean swAll ncf captured shaClean stopped defer ctorMoved; do
522525
if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi
523526
done
524527
echo "OK: flow-sensitive OWN001/002/003 on real C# (path-sensitive, loops via while/foreach/for, try/finally sequential, never-vs-every-path wording, dispose-optional exempt, beyond flat)"

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

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,21 @@ e is InvocationExpressionSyntax i
10371037
&& i.ArgumentList.Arguments[0].Expression is IdentifierNameSyntax buf
10381038
? buf.Identifier.Text : null;
10391039

1040+
// Is `idn` a pooled buffer passed as an argument to a CONSTRUCTOR whose result ESCAPES this method —
1041+
// `return new Wrapper(…, buf, …)` or `_field = new Wrapper(…, buf, …)`? Such a `new` hands the buffer
1042+
// to the constructed object, which becomes responsible for Return, so the buffer leaves the method
1043+
// inside the escaping object (an ownership transfer, not a borrow). One level only: the `new` must be
1044+
// the direct return value or assigned to a real FIELD — a `new` stored in a LOCAL stays a borrow (the
1045+
// local is method-scoped; a wrapper that never leaves the method and never Returns the buffer is a
1046+
// real leak, Codex), as does a `new` buried in another expression. The field check is symbol-based
1047+
// (`IFieldSymbol`) so an assignment to a same-named LOCAL is not mistaken for a field.
1048+
static bool PassedToEscapingCtor(IdentifierNameSyntax idn, SemanticModel model) =>
1049+
idn.Parent is ArgumentSyntax { Parent: ArgumentListSyntax
1050+
{ Parent: BaseObjectCreationExpressionSyntax oce } }
1051+
&& (oce.Parent is ReturnStatementSyntax
1052+
|| (oce.Parent is AssignmentExpressionSyntax a && a.Right == oce
1053+
&& model.GetSymbolInfo(a.Left).Symbol is IFieldSymbol));
1054+
10401055
// Is `t` the System.Buffers.MemoryPool<T> type — the Dispose-based pool. Mirrors IsArrayPoolType
10411056
// (checked on the resolved symbol, so an aliased/injected `MemoryPool<T>` receiver binds and a
10421057
// look-alike does not).
@@ -2405,8 +2420,16 @@ or ImplicitObjectCreationExpressionSyntax } init
24052420
if (!usingMemoryOwners.Contains(nm))
24062421
escapedLocals.Add(nm);
24072422
}
2423+
// A pooled buffer handed as an argument is normally a BORROW (the renter Returns it),
2424+
// NOT an escape — so `pool.Return(buf); Work(buf)` still trips use-after-return. But a
2425+
// pooled buffer passed to a CONSTRUCTOR whose result ESCAPES this method (`return new
2426+
// Wrapper(buf)` / `_field = new Wrapper(buf)`) transfers ownership to that object, which
2427+
// becomes responsible for Return — so the buffer is NOT leaked here. Treat that as an
2428+
// escape (mined FP on Pipelines.Sockets.Unofficial: ArrayPoolBufferWriter.CreateNewSegment
2429+
// returns `new ArrayPoolRefCountedSegment(pool, array, prev)`).
24082430
else if ((idn.Parent is AssignmentExpressionSyntax asg && asg.Right == idn)
2409-
|| (idn.Parent is ArgumentSyntax && !poolBuffers.Contains(nm) && !consumedArg))
2431+
|| (idn.Parent is ArgumentSyntax && !poolBuffers.Contains(nm) && !consumedArg)
2432+
|| (poolBuffers.Contains(nm) && PassedToEscapingCtor(idn, model)))
24102433
escapedLocals.Add(nm);
24112434
}
24122435
var tracked = new HashSet<string>(candidates);

‎frontend/roslyn/samples/FlowLocalsSample.cs‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System;
2+
using System.Buffers;
23
using System.Collections.Generic;
34
using System.IO;
45
using System.Net;
@@ -504,6 +505,27 @@ public void DeferredHandoffNoFalsePositive()
504505
defer.WriteByte(1); // must NOT trip OWN002 (it would, without the fix)
505506
defer.Dispose(); // disposed here -> balanced -> silent
506507
}
508+
509+
// NOT a leak (mined FP on Pipelines.Sockets.Unofficial — ArrayPoolBufferWriter.CreateNewSegment):
510+
// a pooled buffer handed to a constructor whose result is RETURNED transfers ownership to the
511+
// returned wrapper (which Returns the buffer on its own teardown), so this method does not leak it
512+
// even though it never calls Return -> silent ('ctorMoved'). A plain borrow `Work(buf)` still
513+
// leaks if not returned, so this is specifically the escaping-constructor transfer.
514+
public PooledHolder PooledIntoReturnedCtor(int n)
515+
{
516+
var ctorMoved = ArrayPool<byte>.Shared.Rent(n);
517+
return new PooledHolder(ctorMoved);
518+
}
519+
}
520+
521+
// Takes ownership of a pooled buffer (Returns it on teardown) — the wrapper that
522+
// PooledIntoReturnedCtor hands its rented buffer to. Models the ownership transfer through a
523+
// constructor argument that the escape analysis must recognise.
524+
internal sealed class PooledHolder
525+
{
526+
private readonly byte[] _buffer;
527+
public PooledHolder(byte[] buffer) => _buffer = buffer;
528+
public void Release() => ArrayPool<byte>.Shared.Return(_buffer);
507529
}
508530

509531
// A domain exception type literally named `Exception`, in a non-System namespace — the

0 commit comments

Comments
 (0)