From f545f3dfc89fe3a2d8685340190272b2a49d1bf4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 07:24:36 +0000 Subject: [PATCH 1/2] fix(extractor): SemaphoreSlim FIELD is dispose-optional unless AvailableWaitHandle is read (mined Npgsql) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sound, scoped version of the SemaphoreSlim FP (per review on #91 + the user's choice). SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when AvailableWaitHandle is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release leaks nothing and is dispose-optional. Mined: NpgsqlDataSource._setupMappingsSemaphore. Two guards keep it sound, both flagged by reviewers on the earlier attempt: - FIELD-scoped: the exemption lives in the field-disposable detector, NOT in the shared IsDisposeOptional — so the flow-locals detector and the deliberate method-bounded `semLeak` control (a prior ShareX decision: SemaphoreSlim stays tracked for locals) are left untouched (CodeRabbit). - AvailableWaitHandle-GATED: if `.AvailableWaitHandle` is read on the field, the wait handle exists and Dispose must release it, so the field STAYS tracked (Codex). The gate keys on a this/bare-receiver `.AvailableWaitHandle` access. Regression sample SemaphoreFieldSample.cs: OptionalSemaphore._optionalSem (Wait/ Release only) is SILENT; controls still warn — WaitHandleSemaphore._handleSem (reads AvailableWaitHandle) and HoldsCtsField._ctsControl (a non-SemaphoreSlim CTS). The existing FlowLocalsSample.semLeak local control is unaffected (separate detector path), asserted in the --flow-locals step. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 16 +++++++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 20 ++++++++ .../roslyn/samples/SemaphoreFieldSample.cs | 46 +++++++++++++++++++ 3 files changed, 82 insertions(+) create mode 100644 frontend/roslyn/samples/SemaphoreFieldSample.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a6361baa..000a2d7e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -145,6 +145,7 @@ jobs: frontend/roslyn/samples/EventSourceCountersSample.cs \ frontend/roslyn/samples/AppDomainShutdownSample.cs \ frontend/roslyn/samples/AliasDisposeSample.cs \ + frontend/roslyn/samples/SemaphoreFieldSample.cs \ -o "$RUNNER_TEMP/facts.json" cat "$RUNNER_TEMP/facts.json" - name: Check facts through the core @@ -261,6 +262,21 @@ jobs: # same-named local disposed in another method must NOT credit the field, so it still leaks. echo "$out" | grep -qE "AliasDisposeSample\.cs:[0-9]+:.*\[OWN001\].*'_scopedLeak'" \ || { echo "FAIL: a same-named local in another scope must not be miscredited (symbol-scoped aliases)"; exit 1; } + # P-004 SemaphoreSlim FIELD dispose-optional (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore): + # a SemaphoreSlim field used only for Wait/Release (AvailableWaitHandle never read) frees nothing on + # Dispose -> must be SILENT. + if echo "$out" | grep -q "'_optionalSem'"; then + echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is never read was wrongly reported (dispose-optional)"; exit 1 + fi + # gate control: a SemaphoreSlim field whose AvailableWaitHandle IS read allocates a handle Dispose + # must release -> it must STILL warn (proves the exemption is gated, not blanket — Codex). + echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_handleSem'" \ + || { echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is read must still warn"; exit 1; } + # type-scope control: a non-SemaphoreSlim owned IDisposable (CTS) never disposed must STILL warn. + echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_ctsControl'" \ + || { echo "FAIL: a non-SemaphoreSlim owned IDisposable field must still warn (exemption stays SemaphoreSlim-scoped)"; exit 1; } + # field-scoped: the existing method-bounded LOCAL SemaphoreSlim leak (FlowLocalsSample.semLeak) must + # be UNAFFECTED — checked in the --flow-locals step below; this exemption never touches IsDisposeOptional. # 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" \ diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 3f37f7e0..3f1202b8 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2296,6 +2296,19 @@ or ImplicitObjectCreationExpressionSyntax && ThisFieldName(a.Left) is { } cfn) // THIS instance's field only — `other._c` must not exempt our field (CodeRabbit) eventSourceCounters.Add(cfn); + // P-004 SemaphoreSlim field exemption (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore). + // SemaphoreSlim.Dispose() only frees a LAZILY-allocated wait handle — allocated solely when + // `.AvailableWaitHandle` is read — so a SemaphoreSlim field used purely for Wait/WaitAsync/Release + // leaks nothing and is dispose-optional. GATE: if `.AvailableWaitHandle` IS read on the field, + // that handle exists and Dispose must release it, so the field STAYS tracked (Codex). Collect the + // field names whose AvailableWaitHandle is read (this/bare receiver) so the loop below keeps them. + // Scoped to FIELDS only — the shared IsDisposeOptional (and the flow-locals detector / the + // deliberate method-bounded `semLeak` control) is intentionally left untouched (CodeRabbit). + var waitHandleSemaphores = new HashSet(StringComparer.Ordinal); + foreach (var ma in cls.DescendantNodes().OfType()) + if (ma.Name.Identifier.Text == "AvailableWaitHandle" && ThisFieldName(ma.Expression) is { } whf) + waitHandleSemaphores.Add(whf); + foreach (var fd in cls.Members.OfType()) { // a `static` IDisposable field is a process-lifetime singleton (a shared @@ -2323,6 +2336,13 @@ or ImplicitObjectCreationExpressionSyntax if (eventSourceCounters.Contains(v.Identifier.Text) && DerivesFromDiagnosticCounter(model.GetTypeInfo(fd.Declaration.Type).Type)) continue; + // a SemaphoreSlim field whose `.AvailableWaitHandle` is never read leaks nothing — + // Dispose() only frees that lazy handle — so it is dispose-optional and silent; if the + // handle IS read (in waitHandleSemaphores) it stays tracked. FIELD-scoped (Npgsql). + if (!waitHandleSemaphores.Contains(v.Identifier.Text) + && model.GetTypeInfo(fd.Declaration.Type).Type is { Name: "SemaphoreSlim" } st + && st.ContainingNamespace?.ToString() == "System.Threading") + continue; subs.Add(new { @event = v.Identifier.Text, diff --git a/frontend/roslyn/samples/SemaphoreFieldSample.cs b/frontend/roslyn/samples/SemaphoreFieldSample.cs new file mode 100644 index 00000000..970be43f --- /dev/null +++ b/frontend/roslyn/samples/SemaphoreFieldSample.cs @@ -0,0 +1,46 @@ +using System; +using System.Threading; +using System.Threading.Tasks; + +namespace Own.Samples; + +// P-004 SemaphoreSlim FIELD dispose-optional (mined: Npgsql NpgsqlDataSource._setupMappingsSemaphore). +// SemaphoreSlim.Dispose() only frees a lazily-allocated wait handle (allocated solely when +// AvailableWaitHandle is read), so a SemaphoreSlim field used purely for Wait/WaitAsync/Release leaks +// nothing and is dispose-optional. GATED on AvailableWaitHandle: if the field's AvailableWaitHandle is +// read, the handle exists and Dispose must release it -> the field STAYS tracked (Codex). Scoped to +// FIELDS only — method-bounded LOCAL SemaphoreSlims remain tracked via FlowLocalsSample.semLeak. + +// a SemaphoreSlim field used only for WaitAsync/Release, never disposed -> SILENT (dispose-optional). +public sealed class OptionalSemaphore +{ + private readonly SemaphoreSlim _optionalSem = new SemaphoreSlim(1, 1); + + public async Task RunAsync() + { + await _optionalSem.WaitAsync(); + try { /* critical section */ } + finally { _optionalSem.Release(); } + } +} + +// control (the gate): the field's AvailableWaitHandle IS read, so the wait handle is allocated and +// Dispose must release it -> the field must STILL warn OWN001. +public sealed class WaitHandleSemaphore +{ + private readonly SemaphoreSlim _handleSem = new SemaphoreSlim(0, 1); + + public void Block() + { + _handleSem.AvailableWaitHandle.WaitOne(); // reads AvailableWaitHandle -> stays tracked + } +} + +// control (type scope): a non-SemaphoreSlim owned IDisposable (CancellationTokenSource) never disposed +// must STILL warn — the exemption is SemaphoreSlim-specific, not a blanket "any field". +public sealed class HoldsCtsField +{ + private readonly CancellationTokenSource _ctsControl = new CancellationTokenSource(); + + public void Cancel() => _ctsControl.Cancel(); +} From 018b1f763e5ba155b709fe78bf34db555a652578 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 23 Jun 2026 07:30:25 +0000 Subject: [PATCH 2/2] =?UTF-8?q?fix(extractor):=20bind=20the=20AvailableWai?= =?UTF-8?q?tHandle=20gate=20by=20symbol=20=E2=80=94=20credit=20field=20+?= =?UTF-8?q?=20alias=20reads=20(Codex=20+=20CodeRabbit)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut keyed the AvailableWaitHandle gate by bare identifier text via ThisFieldName, which (a) missed a read through a field ALIAS — `var s = _sem; s.AvailableWaitHandle` recorded `s`, not `_sem`, so the field stayed wrongly exempt (Codex) — and (b) could conflate a shadowing local/parameter with a same-named field (CodeRabbit). Resolve the receiver by SYMBOL: credit the field's AvailableWaitHandle read only when the receiver binds to a real field symbol via a this/bare access (excludes `other._f` and shadowing locals), OR to a field-alias local (reusing the #90 aliasToField map). Anything else is ignored. Regression: SemaphoreFieldSample gains AliasedWaitHandleSemaphore._aliasedSem (AvailableWaitHandle read through `var s = _aliasedSem`) which must STILL warn — the field is tracked through the alias. The existing controls (_optionalSem silent, _handleSem direct-read warns, _ctsControl type-scope warns) are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED --- .github/workflows/ci.yml | 3 +++ frontend/roslyn/OwnSharp.Extractor/Program.cs | 14 ++++++++++++-- frontend/roslyn/samples/SemaphoreFieldSample.cs | 14 ++++++++++++++ 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 000a2d7e..622c7811 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -272,6 +272,9 @@ jobs: # must release -> it must STILL warn (proves the exemption is gated, not blanket — Codex). echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_handleSem'" \ || { echo "FAIL: a SemaphoreSlim field whose AvailableWaitHandle is read must still warn"; exit 1; } + # Codex control: an AvailableWaitHandle read THROUGH A FIELD ALIAS must credit the field -> still warn. + echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_aliasedSem'" \ + || { echo "FAIL: an aliased AvailableWaitHandle read must keep the field tracked (alias-aware gate)"; exit 1; } # type-scope control: a non-SemaphoreSlim owned IDisposable (CTS) never disposed must STILL warn. echo "$out" | grep -qE "SemaphoreFieldSample\.cs:[0-9]+:.*\[OWN001\].*'_ctsControl'" \ || { echo "FAIL: a non-SemaphoreSlim owned IDisposable field must still warn (exemption stays SemaphoreSlim-scoped)"; exit 1; } diff --git a/frontend/roslyn/OwnSharp.Extractor/Program.cs b/frontend/roslyn/OwnSharp.Extractor/Program.cs index 3f1202b8..ef6e58b2 100644 --- a/frontend/roslyn/OwnSharp.Extractor/Program.cs +++ b/frontend/roslyn/OwnSharp.Extractor/Program.cs @@ -2306,8 +2306,18 @@ or ImplicitObjectCreationExpressionSyntax // deliberate method-bounded `semLeak` control) is intentionally left untouched (CodeRabbit). var waitHandleSemaphores = new HashSet(StringComparer.Ordinal); foreach (var ma in cls.DescendantNodes().OfType()) - if (ma.Name.Identifier.Text == "AvailableWaitHandle" && ThisFieldName(ma.Expression) is { } whf) - waitHandleSemaphores.Add(whf); + if (ma.Name.Identifier.Text == "AvailableWaitHandle") + { + // credit the FIELD whose AvailableWaitHandle is read, by SYMBOL: a this/bare access bound + // to a real field symbol (so `other._f` and a shadowing local/param are NOT conflated — + // CodeRabbit), OR a field-ALIAS local (`var s = _sem; s.AvailableWaitHandle` -> `_sem` — + // Codex). Anything else (an unrelated local, another instance's field) is ignored. + var recv = model.GetSymbolInfo(ma.Expression).Symbol; + if (recv is IFieldSymbol && ThisFieldName(ma.Expression) is { } whf) + waitHandleSemaphores.Add(whf); + else if (recv is ILocalSymbol ls && aliasToField.TryGetValue(ls, out var fa)) + waitHandleSemaphores.Add(fa); + } foreach (var fd in cls.Members.OfType()) { diff --git a/frontend/roslyn/samples/SemaphoreFieldSample.cs b/frontend/roslyn/samples/SemaphoreFieldSample.cs index 970be43f..76b04623 100644 --- a/frontend/roslyn/samples/SemaphoreFieldSample.cs +++ b/frontend/roslyn/samples/SemaphoreFieldSample.cs @@ -36,6 +36,20 @@ public void Block() } } +// Codex control: AvailableWaitHandle read through a field ALIAS (`var s = _sem; s.AvailableWaitHandle`) +// must credit the FIELD — the handle is allocated, so the field must STILL warn OWN001 (not be exempted +// because the read went through the local name). +public sealed class AliasedWaitHandleSemaphore +{ + private readonly SemaphoreSlim _aliasedSem = new SemaphoreSlim(0, 1); + + public void Block() + { + var sem = _aliasedSem; + sem.AvailableWaitHandle.WaitOne(); // alias reads AvailableWaitHandle -> _aliasedSem stays tracked + } +} + // control (type scope): a non-SemaphoreSlim owned IDisposable (CancellationTokenSource) never disposed // must STILL warn — the exemption is SemaphoreSlim-specific, not a blanket "any field". public sealed class HoldsCtsField