diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 670df2a3..ffd0c863 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -510,7 +510,10 @@ jobs: # case disposes (no default) -> last case is the tail, no phantom no-match leak. # `ncf`: `ncf?.Dispose()` (null-conditional) in a threaded finally IS a release # (member-binding form), so it is disposed on the return path -> silent (Codex review). - 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 + # `ctorMoved`: a pooled buffer handed to a constructor whose result is RETURNED transfers + # ownership to the returned wrapper -> escaped -> silent (mined FP on + # Pipelines.Sockets.Unofficial: ArrayPoolBufferWriter.CreateNewSegment). + 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 if echo "$out" | grep -q "'$ok'"; then echo "FAIL: silent/exempt case '$ok' was reported"; exit 1; fi done 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)" diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 44a48903..902d039e 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1037,6 +1037,21 @@ e is InvocationExpressionSyntax i && i.ArgumentList.Arguments[0].Expression is IdentifierNameSyntax buf ? buf.Identifier.Text : null; +// Is `idn` a pooled buffer passed as an argument to a CONSTRUCTOR whose result ESCAPES this method — +// `return new Wrapper(…, buf, …)` or `_field = new Wrapper(…, buf, …)`? Such a `new` hands the buffer +// to the constructed object, which becomes responsible for Return, so the buffer leaves the method +// inside the escaping object (an ownership transfer, not a borrow). One level only: the `new` must be +// the direct return value or assigned to a real FIELD — a `new` stored in a LOCAL stays a borrow (the +// local is method-scoped; a wrapper that never leaves the method and never Returns the buffer is a +// real leak, Codex), as does a `new` buried in another expression. The field check is symbol-based +// (`IFieldSymbol`) so an assignment to a same-named LOCAL is not mistaken for a field. +static bool PassedToEscapingCtor(IdentifierNameSyntax idn, SemanticModel model) => + idn.Parent is ArgumentSyntax { Parent: ArgumentListSyntax + { Parent: BaseObjectCreationExpressionSyntax oce } } + && (oce.Parent is ReturnStatementSyntax + || (oce.Parent is AssignmentExpressionSyntax a && a.Right == oce + && model.GetSymbolInfo(a.Left).Symbol is IFieldSymbol)); + // Is `t` the System.Buffers.MemoryPool type — the Dispose-based pool. Mirrors IsArrayPoolType // (checked on the resolved symbol, so an aliased/injected `MemoryPool` receiver binds and a // look-alike does not). @@ -2394,8 +2409,16 @@ or ImplicitObjectCreationExpressionSyntax } init if (!usingMemoryOwners.Contains(nm)) escapedLocals.Add(nm); } + // A pooled buffer handed as an argument is normally a BORROW (the renter Returns it), + // NOT an escape — so `pool.Return(buf); Work(buf)` still trips use-after-return. But a + // pooled buffer passed to a CONSTRUCTOR whose result ESCAPES this method (`return new + // Wrapper(buf)` / `_field = new Wrapper(buf)`) transfers ownership to that object, which + // becomes responsible for Return — so the buffer is NOT leaked here. Treat that as an + // escape (mined FP on Pipelines.Sockets.Unofficial: ArrayPoolBufferWriter.CreateNewSegment + // returns `new ArrayPoolRefCountedSegment(pool, array, prev)`). else if ((idn.Parent is AssignmentExpressionSyntax asg && asg.Right == idn) - || (idn.Parent is ArgumentSyntax && !poolBuffers.Contains(nm) && !consumedArg)) + || (idn.Parent is ArgumentSyntax && !poolBuffers.Contains(nm) && !consumedArg) + || (poolBuffers.Contains(nm) && PassedToEscapingCtor(idn, model))) escapedLocals.Add(nm); } var tracked = new HashSet(candidates); diff --git a/frontend/roslyn/samples/FlowLocalsSample.cs b/frontend/roslyn/samples/FlowLocalsSample.cs index 9dbe0132..464f9658 100644 --- a/frontend/roslyn/samples/FlowLocalsSample.cs +++ b/frontend/roslyn/samples/FlowLocalsSample.cs @@ -1,4 +1,5 @@ using System; +using System.Buffers; using System.Collections.Generic; using System.IO; using System.Net; @@ -504,6 +505,27 @@ public void DeferredHandoffNoFalsePositive() defer.WriteByte(1); // must NOT trip OWN002 (it would, without the fix) defer.Dispose(); // disposed here -> balanced -> silent } + + // NOT a leak (mined FP on Pipelines.Sockets.Unofficial — ArrayPoolBufferWriter.CreateNewSegment): + // a pooled buffer handed to a constructor whose result is RETURNED transfers ownership to the + // returned wrapper (which Returns the buffer on its own teardown), so this method does not leak it + // even though it never calls Return -> silent ('ctorMoved'). A plain borrow `Work(buf)` still + // leaks if not returned, so this is specifically the escaping-constructor transfer. + public PooledHolder PooledIntoReturnedCtor(int n) + { + var ctorMoved = ArrayPool.Shared.Rent(n); + return new PooledHolder(ctorMoved); + } +} + +// Takes ownership of a pooled buffer (Returns it on teardown) — the wrapper that +// PooledIntoReturnedCtor hands its rented buffer to. Models the ownership transfer through a +// constructor argument that the escape analysis must recognise. +internal sealed class PooledHolder +{ + private readonly byte[] _buffer; + public PooledHolder(byte[] buffer) => _buffer = buffer; + public void Release() => ArrayPool.Shared.Return(_buffer); } // A domain exception type literally named `Exception`, in a non-System namespace — the