From 294a4cd2bf7d50e6ee8f00e9e2a25431385c6c60 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 09:24:35 +0000 Subject: [PATCH 1/2] fix(extractor): recognize WinForms Controls/IContainer disposal channels (#219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit WinForms child fields were false-flagged as OWN001 leaks because the field-disposal scan didn't model two designer-generated disposal channels: (a) a Control/ToolStripItem field added to THIS object's own Controls/Items collection is disposed transitively by Control.Dispose(bool) (the framework's contract); (b) a component built with `new T(components)` or registered via `components.Add(x)` is disposed by the designer's `components.Dispose()`. ~30 own-only FPs on a single ShareX scan; issue #201 sweep, field-notes 12-13. Fix (extractor-only, no OwnIR change): both channels credit a field's release ONLY when the class reaches the framework disposal root — a designer `Dispose(bool)` calling `base.Dispose(disposing)` (ClassReachesDisposalRoot). A class with no such Dispose (the ShareX HistoryItemManager true-positive) is never rooted, so its owned controls stay flagged. - (a) Collect `.Add/AddRange(this.child)` edges keyed by their container ("" = this, a field name, or null = foreign), then fixpoint from the rooted set so a ToolStrip added to this.Controls cascades to its Items. AddRange over an array initializer is flattened. The container is resolved SEMANTICALLY, so an add into a foreign (parameter/local) container yields no credit. - (b) The IContainer sink is a disposed field of type System.ComponentModel. IContainer; a field constructed with it (`new T(components)`) or passed to `components.Add(...)` is credited. A component not registered stays flagged. Precision-first, does not over-widen — negative controls all stay flagged: an add into a foreign container (lblForeign); a plain owner with no Dispose (cms/item); a component not container-registered (unregisteredIcon). Verified locally with the real extractor (.NET 8): WinFormsDisposalSample goes 10 findings -> 4 (six channel-disposed fields silent, four controls flagged); a full existing-sample diff in BOTH modes (49 samples) is byte-identical (zero regression). Gates: run_tests.py exit 0, test_ownir 276/276, ruff clean, mypy --strict green. Closes #219 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG --- .github/workflows/ci.yml | 26 +++- docs/notes/field-notes-patterns.md | 19 +++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 140 ++++++++++++++++++ .../roslyn/samples/WinFormsDisposalSample.cs | 135 +++++++++++++++++ 4 files changed, 319 insertions(+), 1 deletion(-) create mode 100644 frontend/roslyn/samples/WinFormsDisposalSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0e6f423d..62471713 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -213,6 +213,7 @@ jobs: frontend/roslyn/samples/TemplatePartLocalCaptureSample.cs \ frontend/roslyn/samples/EmptyDisposeSample.cs \ frontend/roslyn/samples/AppScopedSourceSample.cs \ + frontend/roslyn/samples/WinFormsDisposalSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -878,7 +879,30 @@ jobs: # curated initializer — the stale declaration binding must not exempt. echo "$out" | grep -qE "AppScopedSourceSample\.cs:[0-9]+: warning: \[OWN001\].*'ReassignApp'" \ || { echo "FAIL: expected OWN001 when the resolver-bound local is reassigned before the +="; exit 1; } - echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) at the C# location" + # issue #219 — WinForms disposal CHANNELS: (a) a Control/ToolStripItem field added to THIS's + # own Controls/Items collection is disposed transitively by base.Dispose(disposing); (b) a + # component built with `new T(components)` / registered via `components.Add(x)` is disposed by + # components.Dispose(). ~30 ShareX FPs. Both require the class to reach a disposal root. + # These fields must be SILENT: (a) direct Controls.Add, AddRange, a transitive ToolStrip + # Items add, and (b) both IContainer-registration shapes. + if echo "$out" | grep -qE "WinFormsDisposalSample\.cs.*'(lblStatus|btnOk|menu|menuItem|trayIcon|icon)'"; then + echo "FAIL: a WinForms Controls/Items-membership or IContainer-registered field was wrongly flagged (#219)"; exit 1 + fi + # ...and the negative controls MUST stay flagged: an add into a FOREIGN (param) container; + # a plain owner with NO Dispose at all (owned container never disposed); and a component NOT + # container-registered. + echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'lblForeign'" \ + || { echo "FAIL: #219 (a): a control added to a FOREIGN container must stay flagged"; exit 1; } + echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'item'" \ + || { echo "FAIL: #219 (a): a control in an owned container with NO Dispose must stay flagged"; exit 1; } + echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'unregisteredIcon'" \ + || { echo "FAIL: #219 (b): a component NOT registered in the IContainer must stay flagged"; exit 1; } + # exactly 4 WinFormsDisposalSample findings (lblForeign, cms, item, unregisteredIcon) — the + # six channel-disposed fields are all silent; any leak of them pushes the count past 4. + nwf=$(echo "$out" | grep -cE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\]") + [ "$nwf" = "4" ] \ + || { echo "FAIL: #219 expected exactly 4 WinFormsDisposalSample findings (controls only), got $nwf"; exit 1; } + echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) + #219 WinForms Controls/IContainer disposal channels (silent; controls flagged) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | # Path-sensitive flow analysis of local IDisposables — bugs the flat D1 diff --git a/docs/notes/field-notes-patterns.md b/docs/notes/field-notes-patterns.md index 30ede01f..9e021aa1 100644 --- a/docs/notes/field-notes-patterns.md +++ b/docs/notes/field-notes-patterns.md @@ -462,6 +462,25 @@ construction through (or explicit registration into) the designer's `IContainer` is a release, exactly like `Controls.Add` — the container's `Dispose()` is the real sink.** +**Shipped (issue #219 — covers both entries 12 and 13).** The field-disposal scan +now credits both channels, but **only when the class reaches the framework disposal +root** — a designer `Dispose(bool)` that calls `base.Dispose(disposing)` +(`ClassReachesDisposalRoot`). A class with no such Dispose (the ShareX +`HistoryItemManager` true-positive) is not rooted, so its owned controls stay +flagged. (a) A `Control`/`ToolStripItem` field added to **this object's own** +`Controls`/`Items` collection (`this.Controls.Add`/`AddRange`, or `this.. +Items.Add` where `` itself reaches the root) is released transitively — a +fixpoint so a `ToolStrip` added to `this.Controls` cascades to its items. An add +into a **foreign** container (a parameter/local — resolved semantically, not by +name) yields no credit and stays flagged. (b) A field **constructed with** the +designer `IContainer` (`new T(components)`) or explicitly `components.Add(x)` is +released by `components.Dispose()`; the sink is a disposed field of type +`System.ComponentModel.IContainer`. A component **not** registered (a bare +`new NotifyIcon()`) stays flagged. Extractor-only, no OwnIR change. Pinned by +`frontend/roslyn/samples/WinFormsDisposalSample.cs` (six channel-disposed fields +silent; `lblForeign`/`cms`/`item`/`unregisteredIcon` controls flagged) in the +`wpf-extractor` CI job. + ## 14. `using (field = new T())` — a field as the direct `using` acquisition target **Seen in:** ShareX `ShareX.HelpersLib/Cryptographic/HashChecker.cs:59`, diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 56ab8281..d60a6107 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1351,6 +1351,71 @@ t is not null || t.AllInterfaces.Any(i => i.Name == "IDisposable" && i.ContainingNamespace?.ToString() == "System")); +// --- Issue #219: WinForms disposal CHANNELS --- + +// The class reaches the framework disposal root via a designer `Dispose(bool)` that calls +// `base.Dispose(disposing)` — `Control.Dispose(bool)` recursively disposes its `Controls` collection +// and the `IContainer components`. Only such a class is treated as rooted; a class with no Dispose at +// all (the ShareX HistoryItemManager true-positive) is NOT, so its owned controls stay flagged. +static bool ClassReachesDisposalRoot(ClassDeclarationSyntax cls) +{ + foreach (var m in cls.Members.OfType()) + if (m.Identifier.Text == "Dispose") + foreach (var inv in m.DescendantNodes().OfType()) + if (inv.Expression is MemberAccessExpressionSyntax + { Expression: BaseExpressionSyntax, Name.Identifier.Text: "Dispose" }) + return true; + return false; +} + +// A type that is (or implements) System.ComponentModel.IContainer — the designer `components` sink. +static bool ImplementsIContainer(ITypeSymbol? t) => + t is not null + && ((t.Name == "IContainer" && t.ContainingNamespace?.ToString() == "System.ComponentModel") + || t.AllInterfaces.Any(i => i.Name == "IContainer" + && i.ContainingNamespace?.ToString() == "System.ComponentModel")); + +// The container an `X.Controls.Add(..)` / `X.Items.Add(..)` collection belongs to, given the +// collection expression `X.Controls` / `X.Items` (or a bare inherited `Controls`/`Items`): "" (a +// sentinel) for THIS object, the FIELD name for a `this.`/`` container, or null when it +// is not a Controls/Items collection, or its container is FOREIGN (a parameter / local / another +// instance) — an add into a foreign container is not this class's disposal channel. Semantic on the +// receiver so a param named like a field is not mistaken for one. +static string? CollectionContainerKey(ExpressionSyntax collExpr, SemanticModel model) +{ + if (collExpr is IdentifierNameSyntax { Identifier.Text: "Controls" or "Items" }) + return ""; // an inherited `Controls`/`Items` member accessed bare -> this object + if (collExpr is not MemberAccessExpressionSyntax { Name.Identifier.Text: "Controls" or "Items" } coll) + return null; + if (coll.Expression is ThisExpressionSyntax) + return ""; + return model.GetSymbolInfo(coll.Expression).Symbol is IFieldSymbol { IsStatic: false } f ? f.Name : null; +} + +// The THIS-field names added by an `.Add(this.child)` / `.AddRange(new T[]{ this.a, this.b })` call — +// flattening an array-creation initializer (the designer `AddRange` form). Semantic (IFieldSymbol) so +// only real fields of this object are credited. +static IEnumerable AddedFieldNames(ArgumentListSyntax args, SemanticModel model) +{ + foreach (var a in args.Arguments) + { + var items = a.Expression switch + { + ArrayCreationExpressionSyntax { Initializer: { } ai } => ai.Expressions, + ImplicitArrayCreationExpressionSyntax { Initializer: { } ii } => ii.Expressions, + _ => default, + }; + if (items.Count > 0) + { + foreach (var e in items) + if (model.GetSymbolInfo(e).Symbol is IFieldSymbol { IsStatic: false } af) + yield return af.Name; + } + else if (model.GetSymbolInfo(a.Expression).Symbol is IFieldSymbol { IsStatic: false } sf) + yield return sf.Name; + } +} + // The ignored result of a member `.Subscribe(...)` is a leakable IDisposable token (WPF004) only // when the call RETURNS an IDisposable — the Rx `IObservable.Subscribe()` shape. A RESOLVED void // / non-IDisposable return has no token to leak: StackExchange.Redis's `ISubscriber.Subscribe(channel, @@ -4370,6 +4435,81 @@ or ImplicitObjectCreationExpressionSyntax && ThisFieldName(uae.Left) is { } uf) disposed.Add(uf); + // Issue #219 — WinForms disposal CHANNELS. Only when the class reaches the framework disposal + // root (a designer `Dispose(bool)` -> `base.Dispose(disposing)`): a class with no such Dispose + // (the HistoryItemManager true-positive shape) is never rooted, so its owned controls stay + // flagged. + if (ClassReachesDisposalRoot(cls)) + { + // (b) IContainer registration. The sink is a field of type IContainer that is disposed in + // this class (already in `disposed` via `components.Dispose()` / `components?.Dispose()`). + // A field CONSTRUCTED with that sink (`new T(components)`) or explicitly `sink.Add(this.G)` + // is disposed by `components.Dispose()`. + var containerSinks = new HashSet(StringComparer.Ordinal); + foreach (var fdecl in cls.Members.OfType()) + foreach (var v in fdecl.Declaration.Variables) + if (disposed.Contains(v.Identifier.Text) + && model.GetDeclaredSymbol(v) is IFieldSymbol fsym + && ImplementsIContainer(fsym.Type)) + containerSinks.Add(v.Identifier.Text); + if (containerSinks.Count > 0) + { + bool CtorTakesSink(BaseObjectCreationExpressionSyntax oce) => + oce.ArgumentList is { } al && al.Arguments.Any( + a => model.GetSymbolInfo(a.Expression).Symbol is IFieldSymbol af + && containerSinks.Contains(af.Name)); + // `G = new T(..., components, ...)` — assignment form. + foreach (var a in assigns) + if (a.IsKind(SyntaxKind.SimpleAssignmentExpression) + && model.GetSymbolInfo(a.Left).Symbol is IFieldSymbol gf + && a.Right is BaseObjectCreationExpressionSyntax gnew && CtorTakesSink(gnew)) + disposed.Add(gf.Name); + // ...and the field-initializer form `T G = new T(..., components, ...)`. + foreach (var fdecl in cls.Members.OfType()) + foreach (var v in fdecl.Declaration.Variables) + if (v.Initializer?.Value is BaseObjectCreationExpressionSyntax inew + && CtorTakesSink(inew)) + disposed.Add(v.Identifier.Text); + // explicit `components.Add(this.G)`. + foreach (var inv in cls.DescendantNodes().OfType()) + if (inv.Expression is MemberAccessExpressionSyntax { Name.Identifier.Text: "Add" } addMa + && inv.ArgumentList.Arguments.Count == 1 + && model.GetSymbolInfo(addMa.Expression).Symbol is IFieldSymbol sinkf + && containerSinks.Contains(sinkf.Name) + && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol regf) + disposed.Add(regf.Name); + } + + // (a) Controls/Items membership. `Control.Dispose(bool)` disposes every child in its own + // `Controls` collection (and a ToolStrip disposes its `Items`). Collect each add edge + // `(containerKey, childField)`, then fixpoint from the rooted set — the child of a rooted + // container is itself rooted, so a ToolStrip added to `this.Controls` cascades to its items. + // An add into a FOREIGN container (param/local) yields no edge and stays flagged. + var edges = new List<(string container, string child)>(); + foreach (var inv in cls.DescendantNodes().OfType()) + if (inv.Expression is MemberAccessExpressionSyntax { Name.Identifier.Text: "Add" or "AddRange" } addColl + && CollectionContainerKey(addColl.Expression, model) is { } ckey) + foreach (var child in AddedFieldNames(inv.ArgumentList, model)) + edges.Add((ckey, child)); + if (edges.Count > 0) + { + // rooted = THIS ("") plus every field already released (direct dispose / registered above), + // any of which is a container whose Controls/Items disposal reaches the root. + var rooted = new HashSet(disposed, StringComparer.Ordinal) { "" }; + var changed = true; + while (changed) + { + changed = false; + foreach (var (container, child) in edges) + if (rooted.Contains(container) && disposed.Add(child)) + { + rooted.Add(child); // the child is now itself a rooted container + changed = true; + } + } + } + } + // P-004 EventSource counter exemption: inside an EventSource, a DiagnosticCounter field // constructed with `this` is registered to (and lifetime-owned by) the source — a // process-lived diagnostic the source never field-disposes (see IsEventSourceOwnedCounter). diff --git a/frontend/roslyn/samples/WinFormsDisposalSample.cs b/frontend/roslyn/samples/WinFormsDisposalSample.cs new file mode 100644 index 00000000..d7a261b0 --- /dev/null +++ b/frontend/roslyn/samples/WinFormsDisposalSample.cs @@ -0,0 +1,135 @@ +using System; +using System.ComponentModel; + +namespace Own.Samples; + +// Issue #219 — WinForms disposal CHANNELS the field-disposal scan didn't recognise: +// (a) a Control/ToolStripItem field added to THIS's own Controls/Items collection is disposed +// transitively by Control.Dispose(bool) (the framework's designer contract), when the class +// itself reaches a disposal root (a designer `Dispose(bool)` calling `base.Dispose(disposing)`); +// (b) a component constructed with the designer `IContainer` (`new T(components)`) or explicitly +// `components.Add(x)` is disposed by `components.Dispose()`. +// Confirmed FP in ShareX, ~30 sites. Negative controls prove the fix does not over-widen. +// +// Stand-ins for System.Windows.Forms (the extractor build has no WindowsDesktop ref) — same shapes. + +public sealed class ControlCollection +{ + public void Add(Control c) { } + public void AddRange(Control[] cs) { } +} + +public class Control : IDisposable +{ + public ControlCollection Controls { get; } = new ControlCollection(); + protected virtual void Dispose(bool disposing) { } + public void Dispose() { Dispose(true); } +} + +public sealed class Label : Control { } +public sealed class Button : Control { } + +public sealed class ToolStripItemCollection +{ + public void Add(ToolStripItem i) { } + public void AddRange(ToolStripItem[] items) { } +} + +public class ToolStripItem : IDisposable { public void Dispose() { } } +public sealed class ToolStripMenuItem : ToolStripItem { } + +public sealed class ContextMenuStrip : Control +{ + public ToolStripItemCollection Items { get; } = new ToolStripItemCollection(); +} + +public sealed class NotifyIcon : IDisposable +{ + public NotifyIcon() { } + public NotifyIcon(IContainer container) { } // the designer registration overload + public void Dispose() { } +} + +// ---- (a) Controls/Items membership + (b) IContainer registration — all SILENT ---- +// A designer-generated Form-like control: reaches a disposal root via `base.Dispose(disposing)`. +public sealed class MainForm : Control +{ + private readonly IContainer components = new Container(); + private Label lblStatus; + private Button btnOk; + private ContextMenuStrip menu; + private ToolStripMenuItem menuItem; + private NotifyIcon trayIcon; + private NotifyIcon unregisteredIcon; // NEGATIVE CONTROL (b): NOT container-registered + + public MainForm() + { + lblStatus = new Label(); + btnOk = new Button(); + menu = new ContextMenuStrip(); + menuItem = new ToolStripMenuItem(); + this.Controls.Add(this.lblStatus); // (a) direct -> SILENT + this.Controls.AddRange(new Control[] { this.btnOk }); // (a) AddRange -> SILENT + this.Controls.Add(this.menu); // menu reaches root (added to this.Controls) + this.menu.Items.Add(this.menuItem); // (a) transitive via a rooted container -> SILENT + trayIcon = new NotifyIcon(components); // (b) new T(components) -> SILENT + unregisteredIcon = new NotifyIcon(); // NOT registered -> LEAK (control b) + } + + protected override void Dispose(bool disposing) + { + if (disposing) components?.Dispose(); + base.Dispose(disposing); // reaches the disposal root: Control.Dispose(bool) disposes Controls + } +} + +// A component explicitly registered via components.Add(x) (the other (b) shape) -> SILENT. +public sealed class RegisteredComponentForm : Control +{ + private readonly Container components = new Container(); + private NotifyIcon icon; + + public RegisteredComponentForm() + { + icon = new NotifyIcon(); + components.Add(this.icon); // (b) explicit registration -> SILENT + } + + protected override void Dispose(bool disposing) + { + if (disposing) components.Dispose(); + base.Dispose(disposing); + } +} + +// ---- NEGATIVE CONTROLS ---- + +// (a) FOREIGN container: the child is added to a container passed in as a PARAMETER, not THIS's own +// Controls -> not a disposal channel for this class -> STAYS FLAGGED. +public sealed class ForeignContainerAdder : Control +{ + private Label lblForeign; + + public void AttachTo(Control other) + { + lblForeign = new Label(); + other.Controls.Add(this.lblForeign); // added to a FOREIGN (param) container -> LEAK + } + + protected override void Dispose(bool disposing) { base.Dispose(disposing); } +} + +// (a) NO disposal root: a plain owner (not framework-managed) builds a menu but has NO Dispose at +// all, so its owned container is never disposed -> both the container and its child STAY FLAGGED +// (the ShareX HistoryItemManager true-positive shape). +public sealed class MenuOwnerNoDispose +{ + private ContextMenuStrip cms = new ContextMenuStrip(); + private ToolStripMenuItem item; + + public MenuOwnerNoDispose() + { + item = new ToolStripMenuItem(); + cms.Items.Add(this.item); // owned container never disposed (no Dispose) -> LEAK + } +} From e3404affa9b06ec05e24beaec3bbbd443c3a6104 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 11 Jul 2026 09:33:46 +0000 Subject: [PATCH 2/2] fix(extractor): tighten WinForms Items channel + accept named IContainer.Add (#219) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Codex P2 catches on the #219 disposal-channel fix: 1. Items must be a ToolStripItemCollection. The (a) channel accepted ANY member named `Items`, but only a ToolStripItemCollection disposes its items — a ComboBox/ListBox `Items` (ObjectCollection) does not. `this.Controls.Add(combo); combo.Items.Add(this.stream);` would have wrongly credited a real owned disposable stored as a combo item. CollectionContainerKey now gates an `Items` add on the collection's type being a ToolStripItemCollection; `Controls` (ControlCollection) is always a channel. 2. Named IContainer.Add overload. The (b) channel only credited `components.Add(x)` (1 arg), missing the `Add(IComponent, string name)` overload (`components.Add(x, "name")`) — the first argument is the registered component in both, so credit it whenever the receiver is a container sink. Pinned by two new controls in WinFormsDisposalSample.cs: `ComboItemForm` (a disposable stored as a ComboBox item STAYS flagged) and `NamedRegistrationForm` (a named-registered component is SILENT). ImplementsIContainer now matches `IContainer` by simple name so the sample's stand-in and the real System.ComponentModel.IContainer both resolve. Verified locally: sample now 5 findings (comboItem added to the controls), the eight channel-disposed fields silent; full existing-sample diff still byte-identical in both modes (zero regression); gates green. Refs #219 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01Cg6DVXahkyY68Ruu7f76rG --- .github/workflows/ci.yml | 19 ++++-- frontend/roslyn/OwnSharp.Extractor/Program.cs | 44 ++++++++---- .../roslyn/samples/WinFormsDisposalSample.cs | 67 ++++++++++++++++++- 3 files changed, 108 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 62471713..8783715e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -885,23 +885,28 @@ jobs: # components.Dispose(). ~30 ShareX FPs. Both require the class to reach a disposal root. # These fields must be SILENT: (a) direct Controls.Add, AddRange, a transitive ToolStrip # Items add, and (b) both IContainer-registration shapes. - if echo "$out" | grep -qE "WinFormsDisposalSample\.cs.*'(lblStatus|btnOk|menu|menuItem|trayIcon|icon)'"; then + # SILENT: (a) direct Controls.Add, AddRange, a transitive ToolStrip Items add, a ComboBox + # added to Controls; (b) both IContainer-registration shapes incl. the named Add(x,"name"). + if echo "$out" | grep -qE "WinFormsDisposalSample\.cs.*'(lblStatus|btnOk|menu|menuItem|trayIcon|icon|namedIcon|combo)'"; then echo "FAIL: a WinForms Controls/Items-membership or IContainer-registered field was wrongly flagged (#219)"; exit 1 fi # ...and the negative controls MUST stay flagged: an add into a FOREIGN (param) container; - # a plain owner with NO Dispose at all (owned container never disposed); and a component NOT - # container-registered. + # a plain owner with NO Dispose at all (owned container never disposed); a component NOT + # container-registered; and a real disposable stored as a ComboBox item (ObjectCollection + # does not dispose its items — Codex). echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'lblForeign'" \ || { echo "FAIL: #219 (a): a control added to a FOREIGN container must stay flagged"; exit 1; } echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'item'" \ || { echo "FAIL: #219 (a): a control in an owned container with NO Dispose must stay flagged"; exit 1; } echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'unregisteredIcon'" \ || { echo "FAIL: #219 (b): a component NOT registered in the IContainer must stay flagged"; exit 1; } - # exactly 4 WinFormsDisposalSample findings (lblForeign, cms, item, unregisteredIcon) — the - # six channel-disposed fields are all silent; any leak of them pushes the count past 4. + echo "$out" | grep -qE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\].*'comboItem'" \ + || { echo "FAIL: #219 (a): a disposable stored as a ComboBox item (ObjectCollection) must stay flagged"; exit 1; } + # exactly 5 WinFormsDisposalSample findings (lblForeign, cms, item, unregisteredIcon, + # comboItem) — the eight channel-disposed fields are all silent; any leak pushes it past 5. nwf=$(echo "$out" | grep -cE "WinFormsDisposalSample\.cs:[0-9]+:.*\[OWN001\]") - [ "$nwf" = "4" ] \ - || { echo "FAIL: #219 expected exactly 4 WinFormsDisposalSample findings (controls only), got $nwf"; exit 1; } + [ "$nwf" = "5" ] \ + || { echo "FAIL: #219 expected exactly 5 WinFormsDisposalSample findings (controls only), got $nwf"; exit 1; } echo "OK: real C# -> facts -> OWN001 (subscription + timer + field + Subscribe + pool + local) + OWN014 (static-event region escape) + DI001 (captive dependency) + DI002 (scoped captured weakly) + DI003 (transient IDisposable captured by a singleton) + DI004 (transient IDisposable service-located from the root provider) + DI005 (scoped service cached from a created scope) + [OwnIgnore] suppression (silent-but-counted, SARIF suppressions) + #218 DP old->new subscription rotation (silent; controls flagged) + #225 empty-Dispose local exemption (silent; controls flagged) + #228 curated app-scoped source in App (silent; controls flagged) + #219 WinForms Controls/IContainer disposal channels (silent; controls flagged) at the C# location" - name: Flow-sensitive local IDisposables (--flow-locals, P-016 B0b/B2) run: | diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index d60a6107..123f4f08 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -1368,28 +1368,42 @@ static bool ClassReachesDisposalRoot(ClassDeclarationSyntax cls) return false; } -// A type that is (or implements) System.ComponentModel.IContainer — the designer `components` sink. +// A type that is (or implements) `IContainer` — the designer `components` sink. Matched by simple +// name (like the other WinForms type recognitions), so `System.ComponentModel.IContainer` and a +// stand-in of the same name both resolve. static bool ImplementsIContainer(ITypeSymbol? t) => t is not null - && ((t.Name == "IContainer" && t.ContainingNamespace?.ToString() == "System.ComponentModel") - || t.AllInterfaces.Any(i => i.Name == "IContainer" - && i.ContainingNamespace?.ToString() == "System.ComponentModel")); + && (t.Name == "IContainer" || t.AllInterfaces.Any(i => i.Name == "IContainer")); // The container an `X.Controls.Add(..)` / `X.Items.Add(..)` collection belongs to, given the // collection expression `X.Controls` / `X.Items` (or a bare inherited `Controls`/`Items`): "" (a // sentinel) for THIS object, the FIELD name for a `this.`/`` container, or null when it -// is not a Controls/Items collection, or its container is FOREIGN (a parameter / local / another -// instance) — an add into a foreign container is not this class's disposal channel. Semantic on the -// receiver so a param named like a field is not mistaken for one. +// is not a disposing collection, or its container is FOREIGN (a parameter / local / another +// instance). A `Controls` collection (Control.ControlCollection) disposes its children, and a +// ToolStrip's `Items` (ToolStripItemCollection) disposes its items — but a ComboBox/ListBox `Items` +// (ObjectCollection) does NOT dispose arbitrary items, so an `Items` add is a channel only when the +// collection's type is a ToolStripItemCollection (Codex). Semantic on the receiver so a parameter +// named like a field is not mistaken for one. static string? CollectionContainerKey(ExpressionSyntax collExpr, SemanticModel model) { - if (collExpr is IdentifierNameSyntax { Identifier.Text: "Controls" or "Items" }) - return ""; // an inherited `Controls`/`Items` member accessed bare -> this object - if (collExpr is not MemberAccessExpressionSyntax { Name.Identifier.Text: "Controls" or "Items" } coll) + string member; + ExpressionSyntax? containerObj; + switch (collExpr) + { + case IdentifierNameSyntax { Identifier.Text: "Controls" or "Items" } bare: + member = bare.Identifier.Text; containerObj = null; break; // inherited member -> this + case MemberAccessExpressionSyntax { Name.Identifier.Text: "Controls" or "Items" } ma: + member = ma.Name.Identifier.Text; containerObj = ma.Expression; break; + default: + return null; + } + // `Items` disposes its items only for a ToolStripItemCollection; a ComboBox/ListBox + // ObjectCollection does not, so it is not a disposal channel. + if (member == "Items" && model.GetTypeInfo(collExpr).Type?.Name != "ToolStripItemCollection") return null; - if (coll.Expression is ThisExpressionSyntax) + if (containerObj is null or ThisExpressionSyntax) return ""; - return model.GetSymbolInfo(coll.Expression).Symbol is IFieldSymbol { IsStatic: false } f ? f.Name : null; + return model.GetSymbolInfo(containerObj).Symbol is IFieldSymbol { IsStatic: false } f ? f.Name : null; } // The THIS-field names added by an `.Add(this.child)` / `.AddRange(new T[]{ this.a, this.b })` call — @@ -4470,10 +4484,12 @@ bool CtorTakesSink(BaseObjectCreationExpressionSyntax oce) => if (v.Initializer?.Value is BaseObjectCreationExpressionSyntax inew && CtorTakesSink(inew)) disposed.Add(v.Identifier.Text); - // explicit `components.Add(this.G)`. + // explicit `components.Add(this.G)` — and the named overload `Add(this.G, "name")` + // (IContainer.Add(IComponent, string)); the FIRST argument is the registered component + // in both, so credit it whenever the receiver is a container sink (Codex). foreach (var inv in cls.DescendantNodes().OfType()) if (inv.Expression is MemberAccessExpressionSyntax { Name.Identifier.Text: "Add" } addMa - && inv.ArgumentList.Arguments.Count == 1 + && inv.ArgumentList.Arguments.Count >= 1 && model.GetSymbolInfo(addMa.Expression).Symbol is IFieldSymbol sinkf && containerSinks.Contains(sinkf.Name) && model.GetSymbolInfo(inv.ArgumentList.Arguments[0].Expression).Symbol is IFieldSymbol regf) diff --git a/frontend/roslyn/samples/WinFormsDisposalSample.cs b/frontend/roslyn/samples/WinFormsDisposalSample.cs index d7a261b0..47242c68 100644 --- a/frontend/roslyn/samples/WinFormsDisposalSample.cs +++ b/frontend/roslyn/samples/WinFormsDisposalSample.cs @@ -1,5 +1,4 @@ using System; -using System.ComponentModel; namespace Own.Samples; @@ -29,6 +28,33 @@ protected virtual void Dispose(bool disposing) { } public sealed class Label : Control { } public sealed class Button : Control { } +// A ComboBox/ListBox `Items` is an ObjectCollection that does NOT dispose its items — used by a +// negative control so `Items` is only a disposal channel for a real ToolStripItemCollection. +public sealed class ObjectCollection +{ + public void Add(object item) { } +} + +public sealed class ComboBox : Control +{ + public ObjectCollection Items { get; } = new ObjectCollection(); +} + +// Stand-ins for System.ComponentModel.IContainer / Container (the designer `components` sink), +// matched by simple name. `Add` has the plain and the named (`Add(x, "name")`) overloads. +public interface IContainer : IDisposable +{ + void Add(object component); + void Add(object component, string name); +} + +public sealed class Container : IContainer +{ + public void Add(object component) { } + public void Add(object component, string name) { } + public void Dispose() { } +} + public sealed class ToolStripItemCollection { public void Add(ToolStripItem i) { } @@ -102,6 +128,26 @@ protected override void Dispose(bool disposing) } } +// A component registered through the NAMED IContainer overload `components.Add(x, "name")` is +// disposed by components.Dispose() just like the single-arg form -> SILENT. +public sealed class NamedRegistrationForm : Control +{ + private readonly Container components = new Container(); + private NotifyIcon namedIcon; + + public NamedRegistrationForm() + { + namedIcon = new NotifyIcon(); + components.Add(this.namedIcon, "tray"); // (b) named registration -> SILENT + } + + protected override void Dispose(bool disposing) + { + if (disposing) components.Dispose(); + base.Dispose(disposing); + } +} + // ---- NEGATIVE CONTROLS ---- // (a) FOREIGN container: the child is added to a container passed in as a PARAMETER, not THIS's own @@ -119,6 +165,25 @@ public void AttachTo(Control other) protected override void Dispose(bool disposing) { base.Dispose(disposing); } } +// (a) Codex P2: a ComboBox/ListBox `Items` is an ObjectCollection that does NOT dispose its items, +// so a real IDisposable stored as a combo item still leaks even though the combo itself is disposed +// (added to this.Controls). Only a ToolStripItemCollection's Items is a disposal channel. +public sealed class ComboItemForm : Control +{ + private ComboBox combo; + private NotifyIcon comboItem; // a real disposable stored as a combo item + + public ComboItemForm() + { + combo = new ComboBox(); + comboItem = new NotifyIcon(); + this.Controls.Add(this.combo); // combo IS disposed (Controls channel) -> silent + combo.Items.Add(this.comboItem); // ComboBox.Items does NOT dispose it -> LEAK + } + + protected override void Dispose(bool disposing) { base.Dispose(disposing); } +} + // (a) NO disposal root: a plain owner (not framework-managed) builds a menu but has NO Dispose at // all, so its owned container is never disposed -> both the container and its child STAY FLAGGED // (the ShareX HistoryItemManager true-positive shape).