Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,8 @@ jobs:
frontend/roslyn/samples/ViewOwnsVmSample.xaml.cs \
frontend/roslyn/samples/InjectedDcViewSample.xaml.cs \
frontend/roslyn/samples/ResolvedDisposableSample.cs \
frontend/roslyn/samples/FieldReleaseSample.cs \
frontend/roslyn/samples/StaticClassEscapeSample.cs \
-o "$RUNNER_TEMP/facts.json"
cat "$RUNNER_TEMP/facts.json"
- name: Check facts through the core
Expand Down Expand Up @@ -223,6 +225,20 @@ jobs:
if echo "$out" | grep -q "HolderWithDisposeOptional"; then
echo "FAIL: a dispose-optional (Task/DataTable) field was wrongly flagged"; exit 1
fi
# field release recognition (mined: ImageSharp). #2 null-conditional dispose
# `field?.Dispose()` must be recognized -> silent; the undisposed control still warns.
if echo "$out" | grep -q "DisposesViaConditional"; then
echo "FAIL: a field disposed via null-conditional field?.Dispose() was wrongly flagged"; exit 1
fi
echo "$out" | grep -q "NeverDisposesField" \
|| { echo "FAIL: an undisposed IDisposable field control must still warn"; exit 1; }
# #3 a pooled FIELD released cross-member (ctor rent + Dispose Return) must be silent;
# the rented-never-returned control still warns.
if echo "$out" | grep -q "pooled buffer 'returnedBuf'"; then
echo "FAIL: a pooled field returned in Dispose was wrongly flagged"; exit 1
fi
echo "$out" | grep -q "pooled buffer 'leakedBuf'" \
|| { echo "FAIL: a pooled field rented but never returned must still warn"; exit 1; }
# WPF004: an ignored `X.Subscribe(...)` result leaks; the captured+
# disposed one stays silent. "ignored" is unique to the WPF004 message.
echo "$out" | grep -q "MessengerViewModel.cs" \
Expand Down Expand Up @@ -306,6 +322,14 @@ jobs:
if echo "$out" | grep -q "CleanStaticEventViewModel"; then
echo "FAIL: an unsubscribed (released) static-event subscription was wrongly reported"; exit 1
fi
# P-004 robust static-handler exemption (mined: ImageSharp MemoryAllocatorValidator): a
# static-METHOD handler on a static event stores a null-target delegate -> no instance is
# retained -> OWN014 must NOT fire, even when the method-group symbol surfaces as a member
# group (now resolved via CandidateSymbols). (StaticEventEscapeViewModel above proves an
# INSTANCE handler on the same static event still escapes, so this stays scoped.)
if echo "$out" | grep -q "StaticAllocationCounter"; then
echo "FAIL: a static-method handler on a static event was wrongly reported as a region escape"; exit 1
fi
# P-004 process-lived-subscriber exemption (mined: ScreenToGif App +
# Translator): the WPF `App` singleton hooking the process-lived
# AppDomain.UnhandledException promotes nothing, so the static-source region
Expand Down
65 changes: 53 additions & 12 deletions frontend/roslyn/OwnSharp.Extractor/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -185,10 +185,23 @@
// Target is null, so no instance is retained — the subscription cannot leak a
// subscriber, however long-lived the source. Only method-group handlers
// (identifier / member access) are judged; lambdas and delegate-typed values may
// capture state and are left as leak candidates.
static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model) =>
IsHandler(right)
&& model.GetSymbolInfo(right).Symbol is IMethodSymbol { IsStatic: true };
// capture state and are left as leak candidates. A method group's symbol can surface
// as a MEMBER GROUP (Symbol == null, CandidateSymbols populated) instead of the bound
// method, so fall back to the candidates — else a genuinely static-method handler is
// missed and a static-source subscription that retains NO instance is mis-reported as
// a region escape (mined: ImageSharp MemoryAllocatorValidator's static MemoryDiagnostics
// handlers). When falling back, require ALL candidates static so an overload set that
// mixes a static and an instance method is not wrongly exempted.
static bool IsStaticHandler(ExpressionSyntax right, SemanticModel model)
{
if (!IsHandler(right))
return false;
var info = model.GetSymbolInfo(right);
if (info.Symbol is { } s)
return s is IMethodSymbol { IsStatic: true };
var cands = info.CandidateSymbols;
return cands.Length > 0 && cands.All(c => c is IMethodSymbol { IsStatic: true });
}

// P-004 process-lived-subscriber exemption: the WPF application object (`App`) is a
// process-lived singleton — exactly one instance, created at startup, alive until
Expand Down Expand Up @@ -1931,7 +1944,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 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check SARIF -> GitHub code scanning (dog-food)

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / C# leak extractor (Roslyn) -> OwnIR -> core

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.

Check warning on line 1947 in frontend/roslyn/OwnSharp.Extractor/Program.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

Argument of type 'IEnumerable<string?>' cannot be used for parameter 'collection' of type 'IEnumerable<string>' in 'HashSet<string>.HashSet(IEnumerable<string> collection, IEqualityComparer<string>? comparer)' due to differences in the nullability of reference types.
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
Expand Down Expand Up @@ -2128,6 +2141,16 @@
&& m.Name.Identifier.Text is "Dispose" or "DisposeAsync"
&& FieldName(m.Expression) is { } df)
disposed.Add(df);
// Also the NULL-CONDITIONAL form `field?.Dispose()` (a ConditionalAccess whose
// WhenNotNull is the `.Dispose()` invocation, NOT a plain MemberAccess) — the
// dominant disposal shape the match above misses. Mined as an FP across ImageSharp:
// `this.memoryStream?.Dispose()` (ZipExrCompressor/DeflateCompressor/IccDataWriter)
// and the BufferedStreams benchmark's `[GlobalCleanup]` `field?.Dispose()` calls.
foreach (var cae in cls.DescendantNodes().OfType<ConditionalAccessExpressionSyntax>())
if (FieldName(cae.Expression) is { } cdf
&& cae.WhenNotNull is InvocationExpressionSyntax { Expression: MemberBindingExpressionSyntax mb }
&& mb.Name.Identifier.Text is "Dispose" or "DisposeAsync")
disposed.Add(cdf);

foreach (var fd in cls.Members.OfType<FieldDeclarationSyntax>())
{
Expand Down Expand Up @@ -2355,25 +2378,41 @@
// tracks local declarations, so field/assignment-backed rents still need
// this syntactic pass; the local-declaration rents are skipped below to
// avoid double-reporting them (Codex).
// A FIELD-backed pooled buffer is legitimately rented in one member (the ctor) and
// Returned in another (Dispose), so for FIELDS the `pool.Return(field)` is searched
// CLASS-WIDE (a field name is unique to the class, so this cannot cross-mask — unlike
// same-named LOCALS in different methods, which keep the per-member scoping below).
// Mined: ImageSharp BufferedReadStream returns this.readBuffer in Dispose(bool). (An
// INDIRECT release via a lifetime-guard object — SharedArrayPoolBuffer — is left honest:
// a `new X(field)` is NOT assumed to own/return the buffer, since a non-owning view like
// `new ReadOnlyMemory<byte>(field)` would otherwise hide a real leak — Codex.)
var fieldReleased = new HashSet<string>();
foreach (var inv in cls.DescendantNodes().OfType<InvocationExpressionSyntax>())
if (inv.Expression is MemberAccessExpressionSyntax rm
&& rm.Name.Identifier.Text == "Return"
&& inv.ArgumentList.Arguments.Count > 0
&& model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol rfs)
Comment on lines +2391 to +2394

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Require ArrayPool.Return for field pool releases

When a field rented from ArrayPool<T> is assigned in one member, this class-wide release set treats any invocation named Return with that field as the first argument as a release, without verifying that the receiver is actually System.Buffers.ArrayPool<T>. In a class that calls something like someCache.Return(this._buf) or another unrelated Return(_buf), fieldReleased marks _buf released and the later pool fact is suppressed, so a real rented-but-never-returned buffer is missed. Please reuse the semantic ArrayPool check used for rents/PoolReturnBuffer before adding the field to this set.

Useful? React with 👍 / 👎.

fieldReleased.Add(rfs.Name);

foreach (var member in cls.Members)
{
var rented = new List<(string Name, int Line)>();
var rented = new List<(string Name, int Line, bool IsField)>();
foreach (var inv in member.DescendantNodes().OfType<InvocationExpressionSyntax>())
if (IsPoolRent(inv, model))
{
string? name = inv.Parent switch
(string? name, bool isField) = inv.Parent switch
{
// a local-declaration rent is the flow pass's job under
// --flow-locals; skip it here so it is not double-reported.
EqualsValueClauseSyntax { Parent: VariableDeclaratorSyntax vd }
=> flowLocals ? null : vd.Identifier.Text,
=> (flowLocals ? null : vd.Identifier.Text, false),
// a field/assignment rent (`_buf = pool.Rent(...)`) is NOT a
// flow candidate, so this pass keeps it in both modes.
AssignmentExpressionSyntax asg => FieldName(asg.Left),
_ => null,
AssignmentExpressionSyntax asg => (FieldName(asg.Left), true),
_ => ((string?)null, false),
};
if (name != null)
rented.Add((name, LineOf(inv)));
rented.Add((name, LineOf(inv), isField));
}
if (rented.Count == 0)
continue;
Expand All @@ -2384,12 +2423,14 @@
&& inv.ArgumentList.Arguments.Count > 0
&& FieldName(inv.ArgumentList.Arguments[0].Expression) is { } rn)
returned.Add(rn);
foreach (var (name, line) in rented)
foreach (var (name, line, isField) in rented)
subs.Add(new
{
@event = name,
line,
released = returned.Contains(name),
// locals stay per-member; a field is also released if returned/transferred
// anywhere in the class (cross-member ctor-rent + Dispose-return).
released = returned.Contains(name) || (isField && fieldReleased.Contains(name)),
resource = "pool",
});
}
Expand Down
48 changes: 48 additions & 0 deletions frontend/roslyn/samples/FieldReleaseSample.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
using System;
using System.Buffers;
using System.IO;

namespace Own.Samples;

// Field release recognition (mined: ImageSharp). Two shapes the field detectors missed:
// #2 null-conditional disposal `field?.Dispose()`, and
// #3 a pooled FIELD released in a DIFFERENT member than the rent (ctor rent + Dispose
// return), or transferred into a field-stored guard that owns/returns it.

// #2: an IDisposable field disposed via the null-conditional `?.Dispose()` -> SILENT.
public sealed class DisposesViaConditional : IDisposable
{
private readonly MemoryStream stream = new();

public void Dispose() => this.stream?.Dispose(); // null-conditional -> now recognized
}

// #2 control: an IDisposable field the class new's but never disposes -> must WARN.
public sealed class NeverDisposesField
{
private readonly MemoryStream stream = new();

Check failure on line 23 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

IDisposable field 'stream' (type 'MemoryStream') is never disposed — its owner 'NeverDisposesField' leaks it (leak)

Check failure on line 23 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

OWN001

[OWN001] IDisposable field 'stream' (type 'MemoryStream') is never disposed — its owner 'NeverDisposesField' leaks it (leak) [resource: disposable field]

Check warning

Code scanning / Own.NET

owned resource not released on all paths (possible leak) Warning

IDisposable field 'stream' (type 'MemoryStream') is never disposed — its owner 'NeverDisposesField' leaks it (leak) [resource: disposable field]

Check failure on line 23 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

IDisposable field 'stream' (type 'MemoryStream') is never disposed — its owner 'NeverDisposesField' leaks it (leak)

Check failure on line 23 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

OWN001

[OWN001] IDisposable field 'stream' (type 'MemoryStream') is never disposed — its owner 'NeverDisposesField' leaks it (leak) [resource: disposable field]

public long Use() => this.stream.Length;
}

// #3: a pooled buffer FIELD rented in the ctor and Returned in Dispose (cross-member) -> SILENT.
public sealed class PoolFieldReturnedInDispose : IDisposable
{
private readonly byte[] returnedBuf;

public PoolFieldReturnedInDispose(int n) => this.returnedBuf = ArrayPool<byte>.Shared.Rent(n);

public void Dispose() => ArrayPool<byte>.Shared.Return(this.returnedBuf);

public byte First() => this.returnedBuf[0];
}

// #3 control: a pooled buffer FIELD rented but NEVER returned -> must WARN.
public sealed class PoolFieldLeaked
{
private readonly byte[] leakedBuf;

public PoolFieldLeaked(int n) => this.leakedBuf = ArrayPool<byte>.Shared.Rent(n);

Check failure on line 45 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

pooled buffer 'leakedBuf' is rented but never returned to the pool (leak)

Check failure on line 45 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

OWN001

[OWN001] pooled buffer 'leakedBuf' is rented but never returned to the pool (leak) [resource: pooled buffer]

Check warning

Code scanning / Own.NET

owned resource not released on all paths (possible leak) Warning

pooled buffer 'leakedBuf' is rented but never returned to the pool (leak) [resource: pooled buffer]

Check failure on line 45 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

pooled buffer 'leakedBuf' is rented but never returned to the pool (leak)

Check failure on line 45 in frontend/roslyn/samples/FieldReleaseSample.cs

View workflow job for this annotation

GitHub Actions / own-check repo scan (github + msbuild) + composite action

OWN001

[OWN001] pooled buffer 'leakedBuf' is rented but never returned to the pool (leak) [resource: pooled buffer]

public byte First() => this.leakedBuf[0];
}
29 changes: 29 additions & 0 deletions frontend/roslyn/samples/StaticClassEscapeSample.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
using System;

namespace Own.Samples;

// P-004 static-handler exemption, robustly (mined: ImageSharp MemoryAllocatorValidator).
//
// A static class whose static ctor hooks a process-lived STATIC event with a STATIC METHOD
// handler stores a delegate whose Target is null — no instance is retained, so OWN014 must NOT
// fire. The mined case slipped through because the method-group symbol can surface as a member
// group (Symbol == null), which IsStaticHandler now resolves via CandidateSymbols. Contrast:
// StaticEventEscapeViewModel — an INSTANCE handler on a static event — must still raise OWN014
// (capturing lambdas in a static class likewise still escape: the closure is retained).

public static class StaticDiagnosticsBus
{
public static event EventHandler? Allocated;

public static void Raise() => Allocated?.Invoke(null, EventArgs.Empty);
}

public static class StaticAllocationCounter
{
static StaticAllocationCounter()
{
StaticDiagnosticsBus.Allocated += OnAllocated; // static-method handler -> null target -> SILENT
}

private static void OnAllocated(object? sender, EventArgs e) { }
}
Loading