fix(extractor): suppress OWN014 region-escape FP on the process-lived App singleton (mined from ScreenToGif)#81
Merged
Conversation
Mining NickeManarin/ScreenToGif (WPF) surfaced two false positives in the subscription / region-escape detectors; both are suppressed soundly: - OWN014 on `App`: the WPF application object is a process-lived singleton, so hooking the process-lived AppDomain.CurrentDomain.UnhandledException promotes nothing — its "leaked" lifetime already equals the process. IsProcessLivedApplication recognises it syntactically (base `Application`, or the XAML-split `partial class App` whose `: Application` is in the generated partial we never see) since WPF does not resolve on the Linux runner. (ScreenToGif/App.xaml.cs:52 + Other/Translator/App.xaml.cs:14) - OWN001 on a static helper's subscription: a `+=` inside a static member has no enclosing `this`, so a static-method group or a lambda over locals retains no instance of the type — the "keeps <Type> alive" subscriber leak is structurally impossible. IsStaticContext skips it. (static ProcessHelper.RestartAsAdmin:83 does `process.Exited += (s,a) => comp.SetResult(...)` over method-locals) The undisposed `HttpClient` local in ScreenToGif.Test/Util/HttpHelper.cs (OWN001) is a TRUE positive and is intentionally left firing. Regression samples + wpf-extractor CI guards: AppLifetimeSample.cs (both App shapes stay silent), StaticContextSubscription.cs (static-context subs silent, the injected instance control still warns). The existing StaticEventEscapeViewModel (a non-App instance escape) must still raise OWN014, proving the exemptions are scoped and the detectors otherwise intact. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5179aac2a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+1992
to
+1993
| if (!isTimer && IsStaticContext(a)) | ||
| continue; |
There was a problem hiding this comment.
When a static helper registers an instance supplied through a parameter or local, e.g. publisher.Fired += subscriber.OnFired or a lambda that captures subscriber, the event source can still outlive and retain that instance. This continue runs before inspecting the handler target, so those true subscription leaks/region escapes are dropped solely because the enclosing member is static; the exemption should be limited to handlers that cannot retain any instance object.
Useful? React with 👍 / 👎.
📝 WalkthroughWalkthroughAdds ChangesApp-lifetime OWN014 suppression
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 239-244: The switch expression in the base type detection logic
(handling IdentifierNameSyntax and QualifiedNameSyntax) is missing a case for
AliasQualifiedNameSyntax, which prevents it from recognizing alias-qualified
base types like global::System.Windows.Application. Add a new case to the switch
pattern matching for AliasQualifiedNameSyntax that extracts the identifier text
from the right side of the alias-qualified name, similar to how the
QualifiedNameSyntax case works, so that WPF Application declarations using the
global:: prefix are properly detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e0c225de-5114-4ff6-88a1-a3b34b1a632a
📒 Files selected for processing (4)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/AppLifetimeSample.csfrontend/roslyn/samples/StaticContextSubscription.cs
…eep App-only Codex flagged the static-context skip as unsound (correctly): a `+=` inside a static member has no `this`, but the handler can still retain a NON-`this` instance — `publisher.Fired += subscriber.OnFired`, or a lambda capturing a parameter — which an outliving source leaks. "No enclosing `this`" does not imply "nothing can be retained". The ProcessHelper case is safe only because its source `process` is a FRESH, non-escaping method-local (`Process.Start(info)`), but there is no sound syntactic rule that admits a factory-fresh local while still excluding an aliased long-lived source (`var src = injectedBus; src.X += ...`) — only `new` proves freshness. A known soundness hole is worse than the FP under the zero-FN mandate, so the static-context exemption (helper + loop skip + StaticContextSubscription.cs + its CI assertions) is reverted in full. ProcessHelper's `process.Exited` subscription stays an honest "possible leak" warning; a proper fresh-source fix is a separate, deliberate change. This leaves PR #81 as the clean, uncontested App fix only: OWN014 region-escape is suppressed for the process-lived WPF `App` singleton. Also handle the alias-qualified base type `global::System.Windows.Application` (AliasQualifiedNameSyntax) in IsProcessLivedApplication, per CodeRabbit — without it `clsIsApp` would miss that declaration shape and re-enable the FP. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
changed the title
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/roslyn/OwnSharp.Extractor/Program.cs (1)
1976-1980:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the App exemption to OWN014 captures only (timers are being suppressed).
Line [1972] forces timer subscriptions to
source = "static", so the Line [1979] guard also skips timer findings inApp. That broadens the exemption beyond OWN014 and can hide real timer leaks.Suggested minimal fix
- if (source == "static" && clsIsApp) + if (!isTimer && source == "static" && clsIsApp) continue;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/roslyn/OwnSharp.Extractor/Program.cs` around lines 1976 - 1980, The condition at line 1979 that checks `source == "static" && clsIsApp` is too broad and skips processing for all findings matching these conditions, including timer subscriptions that were forced to `source = "static"` at line 1972. This exemption should only apply to OWN014 captures (region escape findings) not to timer leaks in App. Add an additional condition to the guard that specifically checks whether the current finding is an OWN014 capture before continuing, so that timer findings are not suppressed in App.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 1976-1980: The condition at line 1979 that checks `source ==
"static" && clsIsApp` is too broad and skips processing for all findings
matching these conditions, including timer subscriptions that were forced to
`source = "static"` at line 1972. This exemption should only apply to OWN014
captures (region escape findings) not to timer leaks in App. Add an additional
condition to the guard that specifically checks whether the current finding is
an OWN014 capture before continuing, so that timer findings are not suppressed
in App.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a45b9499-d937-4971-861e-19da8c40f74e
📒 Files selected for processing (2)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.cs
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mined from
NickeManarin/ScreenToGif(WPF, @27a49c3)The field-UAF / subscription / timer mining pass on a real WPF app surfaced 8 verdicts (210 OWN050 "type unresolved" notes dropped — WPF types don't resolve on the Linux runner). Audited each against source:
App.xaml.cs:52,Translator/App.xaml.cs:14httpClientHttpHelper.cs:10new HttpClient()local never disposed, doesn't escape. Left firing.ProcessHelper.cs:83VideoSource.xaml.csThe fix — OWN014 on the
AppsingletonThe WPF application object is a process-lived singleton. Hooking the process-lived
AppDomain.CurrentDomain.UnhandledException(the textbook global-exception handler) promotes nothing — App's lifetime already is the process.IsProcessLivedApplicationrecognises it syntactically (baseApplication/System.Windows.Application/global::…Application, or the XAML-splitpartial class Appwhose: Applicationlives in the generated partial the extractor never sees), since WPF doesn't resolve on the runner. Only the static-source escape is suppressed; an instance-field subscription leak insideAppwould still fire.Regression guard:
AppLifetimeSample.cs— both App shapes (name-basedpartial class App, base-based: Application) stay silent; the existingStaticEventEscapeViewModel(a non-App instance escape) must still raise OWN014, proving the exemption is scoped.Reverted: the static-context exemption (ProcessHelper)
The first cut also skipped subscriptions inside
staticmembers (ProcessHelper.RestartAsAdmin'sprocess.Exited += …). Codex correctly flagged this as unsound — a static helper can still leak a non-thisinstance (publisher.Fired += subscriber.OnFired, or a lambda capturing a parameter) when the source outlives it. The ProcessHelper case is safe only because its sourceprocessis a fresh, non-escaping method-local (Process.Start(…)), but there's no sound syntactic rule that admits a factory-fresh local while excluding an aliased long-lived one (var src = injectedBus; src.X += …) — onlynewproves freshness. A known false-negative hole is worse than the FP under the zero-FN mandate, so that exemption is reverted in full. ProcessHelper stays an honest "possible leak" warning; a proper fresh-source fix is a separate, deliberate change.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Summary by CodeRabbit
Bug Fixes
Tests