- Notifications
You must be signed in to change notification settings - Fork 0
feat(extractor): emit fresh-returning factory facts — D5.2 interprocedural leaks on real C##126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -560,6 +560,42 @@ | ||
| _ => "?", | ||
| }; | ||
| // P-005 D5.2: a FIRST-PARTY method call whose result is an owned IDisposable — the | ||
| // caller side of a fresh-returning factory. Because the callee is defined in source (not | ||
| // the BCL), the core can see its body and infer whether it returns `fresh`; if so, a | ||
| // `var r = Factory()` binding is an acquire and a caller that drops `r` leaks. The emitted | ||
| // `callee` matches the `functions[]` key `{TypeName}.{MethodName}`. A null/extern symbol, | ||
| // a void/non-disposable return, or a dispose-optional return is rejected (no claim); an | ||
| // overload (non-unique name) resolves to `unknown` in the core and is silently safe. | ||
| static bool IsFirstPartyDisposableFactory(ExpressionSyntax? expr, SemanticModel model, out string callee) | ||
| { | ||
| callee = ""; | ||
| if (expr is not InvocationExpressionSyntax inv) | ||
| return false; | ||
| if (model.GetSymbolInfo(inv).Symbol is not IMethodSymbol m) | ||
| return false; | ||
| if (m.ReturnsVoid || m.DeclaringSyntaxReferences.Length == 0) | ||
| return false; // void, or not first-party (no visible body to infer `fresh` from) | ||
| if (!ImplementsIDisposable(m.ReturnType) || IsDisposeOptional(m.ReturnType)) | ||
| return false; | ||
| // Fully-qualified key (namespace + containing-type chain) so the call resolves to the | ||
| // RIGHT summary: two `StreamFactory.Make` in different namespaces must not alias, or a | ||
| // call to a non-fresh one could pick up a fresh one's summary and fabricate OWN001 | ||
| // (Codex). Must match the `functions[]` name built by `FlowFunctionName`. | ||
| callee = $"{m.ContainingType.ToDisplayString()}.{m.Name}"; | ||
| return true; | ||
| } | ||
| // The fully-qualified `functions[]` key for a method — `{Namespace.Containing.Type}.{Name}` — | ||
| // used both as the flow-function name and as a D5.2 call callee, so the two always agree (a | ||
| // simple `{Type}.{Name}` would alias same-named types across namespaces). Falls back to the | ||
| // syntactic class name only if the symbol cannot be resolved. | ||
| static string FlowFunctionName(BaseMethodDeclarationSyntax method, string fallbackType, | ||
| SemanticModel model) => | ||
| model.GetDeclaredSymbol(method) is IMethodSymbol ms | ||
| ? $"{ms.ContainingType.ToDisplayString()}.{ms.Name}" | ||
| : $"{fallbackType}.{MethodName(method)}"; | ||
| // A `Dispose()`/`Close()`/`DisposeAsync()` call — through member access (`x.Dispose()`) | ||
| // or member binding (`x?.Dispose()`), and seen through a trailing `.ConfigureAwait(false)` | ||
| // (the idiomatic `await x.DisposeAsync().ConfigureAwait(false)` is the release, not a | ||
| @@ -758,6 +794,32 @@ | ||
| // — the flow path previously mislabelled a pool buffer leaked on a throw edge. | ||
| nodes.Add(new { op = "acquire", var = v.Identifier.Text, line = LineOf(v), | ||
| kind = IsPoolRent(v.Initializer?.Value, model) ? "pool" : "disposable" }); | ||
| // P-005 D5.2: `var r = FirstPartyFactory()` — emit a `call` op (NOT an | ||
| // acquire); the core mints the acquire only if it proves the callee returns | ||
| // `fresh`, so a non-fresh first-party call is never falsely owned. | ||
| else if (tracked.Contains(v.Identifier.Text) | ||
| && IsFirstPartyDisposableFactory(v.Initializer?.Value, model, out var fpCallee)) | ||
| { | ||
| // Preserve the call's TRACKED identifier args (CodeRabbit) so the core | ||
| // can apply the callee's per-argument ownership effects (consume/borrow) | ||
| // to a `var r = Wrap(stream)` — not just the fresh return. Untracked / | ||
| // non-identifier args are dropped (no local to attribute an effect to). | ||
| // Positional args only: the bridge applies the callee's effects by | ||
| // POSITION, so a NAMED argument (`Wrap(second: s2, first: s1)`) would | ||
| // mis-attribute if kept in syntactic order. Dropping named args | ||
| // under-claims (no effect on them) but never mis-aligns (CodeRabbit). | ||
| var fpArgs = v.Initializer?.Value is InvocationExpressionSyntax fpInv | ||
| ? fpInv.ArgumentList.Arguments | ||
| .Where(a => a.NameColon is null) | ||
| .Select(a => a.Expression) | ||
| .OfType<IdentifierNameSyntax>() | ||
| .Select(id => id.Identifier.Text) | ||
| .Where(tracked.Contains) | ||
| .ToArray() | ||
| : Array.Empty<string>(); | ||
| nodes.Add(new { op = "call", callee = fpCallee, args = fpArgs, | ||
| result = v.Identifier.Text, line = LineOf(v) }); | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| // POOL005: a full-length view in the initializer — `var copy = buf.AsSpan().ToArray();` | ||
| // — over-reads the pooled tail just as `Emit(buf.AsSpan());` does. EmitFlowExpr is not | ||
| // called on a non-acquire initializer, so scan it here for the overspan (Codex review). | ||
| @@ -849,7 +911,16 @@ | ||
| nodes.AddRange(chain); | ||
| } | ||
| else | ||
| nodes.Add(new { op = "return", var = (string?)null, line = LineOf(rs) }); | ||
| { | ||
| // P-005 D5.2: a tracked local returned BARE (outside any `finally`) is a | ||
| // fresh-factory transfer — emit it as the return's `var` so the core models the | ||
| // escape (a discharge: ownership moves to the caller) and classifies the method | ||
| // `returnsOwned: fresh`. A non-identifier / non-tracked return is a bare CFG exit. | ||
| var rvar = rs.Expression is IdentifierNameSyntax rid | ||
| && tracked.Contains(rid.Identifier.Text) | ||
| ? rid.Identifier.Text : (string?)null; | ||
| nodes.Add(new { op = "return", var = rvar, line = LineOf(rs) }); | ||
| } | ||
| return true; | ||
| } | ||
| case WhileStatementSyntax ws: | ||
| @@ -2383,7 +2454,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 2457 in frontend/roslyn/OwnSharp.Extractor/Program.cs
| ||
| 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 | ||
| @@ -3181,6 +3252,12 @@ | ||
| candidates.Add(v.Identifier.Text); | ||
| else if (IsMemoryPoolRent(v.Initializer?.Value, model)) // MemoryPool<T> IMemoryOwner (Dispose-released, NOT a poolBuffer) | ||
| candidates.Add(v.Identifier.Text); | ||
| else if (IsFirstPartyDisposableFactory(v.Initializer?.Value, model, out _)) | ||
| // P-005 D5.2: `var r = FirstPartyFactory()` — a candidate acquire | ||
| // IFF the core proves the callee returns `fresh` (it emits a `call` | ||
| // op, not an `acquire`; the core decides). Checked last so `new` / | ||
| // pool / BCL-factory initializers keep their existing classification. | ||
| candidates.Add(v.Identifier.Text); | ||
| } | ||
| // `using (IMemoryOwner owner = MemoryPool.Rent(...)) { … }` STATEMENT form: track the owner | ||
| // too, so its returned view dangles after the scope-exit dispose (the desugar mirrors the | ||
| @@ -3254,9 +3331,20 @@ | ||
| // use of the returned owner trips OWN002 — the bare-owner twin of the returned-view | ||
| // dangle. A NON-using returned owner stays a genuine transfer (escaped → untracked → | ||
| // silent), so this never fires on `var o = Rent(); return o;`. | ||
| if (idn.Parent is ReturnStatementSyntax) | ||
| if (idn.Parent is ReturnStatementSyntax rsp) | ||
| { | ||
| if (!usingMemoryOwners.Contains(nm)) | ||
| // P-005 D5.2: a `new`'d IDisposable returned BARE outside any `try` is a | ||
| // fresh-returning FACTORY — keep it tracked so the flow body emits | ||
| // `acquire …; return <var>`. The core then classifies the method | ||
| // `returnsOwned: fresh` (and the `return <var>` discharges it, so the | ||
| // factory itself stays silent), letting a caller that drops the result | ||
| // leak. A return INSIDE a try threads `finally` edges the fresh path does | ||
| // not model yet, so keep the old transfer (escape) there; a `using` owner | ||
| // also stays tracked (its scope-exit dispose dangles the returned value). | ||
| var freshFactory = newedDisposables.Contains(nm) | ||
| && !rsp.Ancestors().TakeWhile(a => a != mbody) | ||
| .OfType<TryStatementSyntax>().Any(); | ||
| if (!usingMemoryOwners.Contains(nm) && !freshFactory) | ||
| escapedLocals.Add(nm); | ||
| } | ||
| // A pooled buffer handed as an argument is normally a BORROW (the renter Returns it), | ||
| @@ -3300,7 +3388,7 @@ | ||
| statMethodsAnalysed++; | ||
| flowFunctions.Add(new | ||
| { | ||
| name = $"{cls.Identifier.Text}.{MethodName(method)}", | ||
| name = FlowFunctionName(method, cls.Identifier.Text, model), | ||
| file, | ||
| body = fbody, | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| using System.IO; | ||
| namespace Factories; | ||
| // P-005 D5.2: a fresh-returning factory + its callers. The factory `Make` creates and | ||
| // hands back a NEW owned stream; the core infers `returnsOwned: fresh` from its | ||
| // `acquire; return <var>` body. A caller that binds the result and drops it (`Leaks`) | ||
| // is then charged the leak at the call site — an INTERPROCEDURAL finding the flat, | ||
| // intra-procedural detectors cannot see. A caller that disposes the result (`Clean`) | ||
| // stays silent, and the factory itself stays silent (it transfers ownership out). | ||
| public static class StreamFactory | ||
| { | ||
| public static Stream Make() | ||
| { | ||
| var made = new MemoryStream(); // freshly owned, handed to the caller | ||
| return made; | ||
| } | ||
| } | ||
| public static class FactoryConsumers | ||
| { | ||
| // Drops the fresh factory result without disposing -> OWN001 at the call site. | ||
| public static void Leaks() | ||
| { | ||
| var factoryLeak = StreamFactory.Make(); | ||
| factoryLeak.WriteByte(1); | ||
| } | ||
| // Disposes the fresh factory result -> clean (silent). | ||
| public static void Clean() | ||
| { | ||
| var factoryOk = StreamFactory.Make(); | ||
| factoryOk.Dispose(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1075,6 +1075,11 @@ def _infer_return_skeleton(nodes: Any, param_names: set[str]) -> ReturnSkeleton: | ||
| ExternDecl("$borrow_mut", | ||
| [EffectParam(Effect.BORROW_MUT, "Disposable", 0)], None, 0), | ||
| ) | ||
| # The callee names a `call` op may resolve against in `lower_call` WITHOUT being a | ||
| # first-party function summary — the fixed sink externs. A call to any other callee | ||
| # that is not in the solved MOS is unresolvable (no signature) and must NOT be lowered | ||
| # to a `Call` (it would raise OWN040); see the `call` handler in `_lower_flow`. | ||
| _SINK_EXTERN_NAMES = frozenset(e.name for e in _OWNERSHIP_SINK_EXTERNS) | ||
| # A forward to a sink extern is a *known* transfer, so a skeleton can record the | ||
| # resolved path action directly — `$consume` is ownership leaving (a must-transfer), | ||
| @@ -1530,7 +1535,14 @@ def _lower_flow(nodes: list[Any], ffile: str, fname: str, | ||
| # extractor (it is an escape, surfaced separately). | ||
| callee = str(n.get("callee", "")) | ||
| raw_args = n.get("args", []) | ||
| if callee and isinstance(raw_args, list): | ||
| summ = mos.get(callee) if (mos is not None and callee) else None | ||
| # Only emit the `Call` when the callee is RESOLVABLE — a first-party function | ||
| # with a summary, or a fixed ownership-sink extern. A real extraction surfaces | ||
| # calls to callees we did not lower as functions (BCL / extension methods like | ||
| # `GetRequiredService`); those have no signature, so `lower_call` would raise | ||
| # OWN040. Drop them (no effect, no claim) — precision-safe, never a crash. | ||
| if (summ is not None or callee in _SINK_EXTERN_NAMES) \ | ||
| and callee and isinstance(raw_args, list): | ||
coderabbitai[bot] marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| arg_refs: list[Expr] = [VarRef(localmap.get(str(a), str(a)), line) | ||
| for a in raw_args] | ||
| body.append(Call(callee, arg_refs, line)) | ||
| @@ -1540,7 +1552,14 @@ def _lower_flow(nodes: list[Any], ffile: str, fname: str, | ||
| # above; this models the return). A non-fresh / unknown return makes no | ||
| # claim, so the result is never falsely owned (precision-first). | ||
| result = n.get("result") | ||
| summ = mos.get(callee) if (mos is not None and callee) else None | ||
| # Overwriting a tracked local KILLS its previous ownership binding: if the | ||
| # old handle was not released before this call, it leaks (the reference is | ||
| # lost). Drop the stale mapping before any optional fresh acquire, so | ||
| # `acquire x; x = Unknown(); release x` leaks the original x rather than | ||
| # reading as clean (CodeRabbit). A hoisted local keeps its single outer-scope | ||
| # handle (it is declared once and never re-bound), so leave it alone. | ||
| if isinstance(result, str) and result and result not in hoisted: | ||
| localmap.pop(result, None) | ||
| if (isinstance(result, str) and result and result not in hoisted | ||
| and summ is not None and getattr(summ, "returns", None) == "fresh"): | ||
| handle = f"loc_{loc[0]}" | ||
| @@ -1600,7 +1619,11 @@ def check_facts(facts: dict[str, Any]) -> list[Finding]: | ||
| # parameter's contract could not be inferred (an ambiguous pass-through | ||
| # stays plain) -- a "needs annotation / transitive inference" gap, not a | ||
| # leak. Surfacing these properly (with a subject) is a later step. | ||
| if d.code in ("OWN033", "OWN034", "OWN035", "OWN041"): | ||
| # - OWN040: a `call` to a callee the bridge did not lower as a function | ||
| # (an extension/BCL method surfaced by the extractor). The `call` handler | ||
| # already drops unresolvable callees, so this is belt-and-suspenders — a | ||
| # synthetic-call artifact, never a real C# bug (C# already binds the call). | ||
| if d.code in ("OWN033", "OWN034", "OWN035", "OWN040", "OWN041"): | ||
| continue | ||
| sub = handles.get(_handle_of(d) or "") | ||
| if sub is None: | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.