flow: model explicit throw as an abnormal exit (body-level dispose-not-called-on-throw)#94
Merged
Merged
Conversation
…-not-called-on-throw (no try)
An explicit `throw` statement hit the unmodelled `default` and bailed the WHOLE
method, so any leak in a method guarded by a top-level validation throw
(`if (x is null) throw …;`) was invisible to every flow detector. Model a `throw`
with no enclosing try (onThrow null && canEscape) as a bare CFG exit: a resource
owned there and disposed only later leaks on the throw path — exactly the
synthetic exit the injected may-throw edges already use.
Two wins, both sound (a throw with no enclosing try definitely exits, so an owned
resource definitely leaks — FP-free):
* the no-try slice of CodeQL's cs/dispose-not-called-on-throw: `acquire;
if (bad) throw; dispose;` leaks on the throw path (dotNoTry, partial-path);
* un-bailing every validation-throw-guarded method, lighting up all detectors
on the rest of the body (vtl, a later undisposed local, now caught).
Stays conservative inside a try: an explicit throw there may run a finally or be
caught (typed/catch-all), which needs the thrown-type-vs-catch match (not threaded
here), so it keeps bailing — no new false escape past a catch.
Samples (FlowLocalsSample): ValidatedThenLeaks (vtl, never-disposed, recall),
ThrowAfterAcquireLeaks (dotNoTry, partial-path), + ThrowAfterDisposeClean (release
before throw) and ValidatedThenClean (balanced) as silent controls. Core handling
of the {op:return} exit is unchanged (already pinned); the C# lowering is asserted
end-to-end in CI.
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://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public void ValidatedThenLeaks(object arg) | ||
| { | ||
| if (arg is null) throw new ArgumentNullException(nameof(arg)); | ||
| var vtl = new MemoryStream(); |
| // throw path -> OWN001 "may not be disposed on every path". The fix is `using`. | ||
| public void ThrowAfterAcquireLeaks(bool bad) | ||
| { | ||
| var dotNoTry = new MemoryStream(); |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesThrow-exit flow modeling and validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
Owner
Author
|
@codex yo bro |
Summary
Testing
|
Owner
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.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: 7f598b0043
ℹ️ 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
+965
to
+968
| if (canEscape && onThrow is null) | ||
| { | ||
| nodes.Add(new { op = "return", var = (string?)null, line = LineOf(thr) }); | ||
| return true; |
There was a problem hiding this comment.
This treats onThrow is null as proof that no enclosing finally must run, but finallyNodes are currently lowered with the default arguments, so a throw inside an inner finally also takes this branch. In a valid shape such as var s = new MemoryStream(); try { try { } finally { throw new InvalidOperationException(); } } finally { s.Dispose(); }, the outer finally disposes s, but the generated bare return terminates before the outer finally release and can report a false OWN001; either thread the enclosing continuation when lowering finally blocks or keep bailing on those throws.
Useful? React with 👍 / 👎.
…outer finally (Codex P2)
The explicit-throw exit keyed on `onThrow is null && canEscape`, but `finally`
bodies are lowered with the default (null) onThrow — so a `throw` INSIDE a finally
also took the bare-return branch, terminating before any ENCLOSING finally runs.
For `try { try {} finally { throw; } } finally { s.Dispose(); }` that bare exit
skips the outer `s.Dispose()` and falsely reports OWN001 on `s`.
Guard the body-level throw branch with `!IsInsideFinally(thr)`: a throw lexically
inside a finally keeps bailing the whole method (the sound honest-skip it had
before this feature), since its real continuation is the OUTER cleanup the bare
exit cannot run. Body-level throws (no enclosing try/finally) are unaffected.
Sample ThrowInFinallyBails pins the Codex repro as silent; the four body-level
throw samples keep their verdicts.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
…rehose (Codex P2) Symmetric to the explicit-throw fix on PR #94: under --body-throw-edges, a may-throw call lexically inside a `finally` is lowered with a null onThrow, so InjectThrowEdge synthesized a bare method exit for it — skipping any ENCLOSING finally/using cleanup and falsely flagging a resource the outer finally disposes (Codex P2: `using var o = ...; try {} finally { Log(); }`). Guard the synthesized body-level continuation with `!IsInsideFinally(st)` — the same predicate the explicit-throw path already uses. A finally-internal may-throw now gets no firehose edge (its real continuation is the outer cleanup), so the flag-on behaviour there matches flag-off. Body-level may-throws (not in a finally) unaffected. Sample MayThrowInFinallyClean ('mtf', disposed by the outer finally) pins it silent in both modes; CI asserts mtbd/mtf/adc silent in off-mode and adc/mtf silent under the flag. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
PhysShell
pushed a commit
that referenced
this pull request
Jun 23, 2026
… recall on Npgsql v8.0.9 Bring zen-pasteur's extractor/own-check/core current with main (PR #94/#95 — the explicit-throw model + the opt-in --body-throw-edges tier), plumb body_throw_edges= through oracle.yml (workflow_dispatch input + sentinel parse + own-check append), and flip it on for the Npgsql v8.0.9 oracle. Clean before/after vs the baseline (Own.NET 10 / CodeQL 19 / Infer# 73, Agree 0): measures how much body-level dispose-not-called-on-throw recall the firehose recovers (new Agreements) vs the CA2000 noise it adds (own-only jump) — both halves of the opt-in tradeoff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
10 tasks
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.
Why
The cross-tool oracle on Serilog and Npgsql both surfaced CodeQL's
cs/dispose-not-called-on-throwas oracle-only (a recall gap). Tracing it:dispose-not-called-on-throwis already modelled — but only inside atry(InjectThrowEdgefires only whenonThrow != null, which holds only in a try body). The gap is the body-level, no-tryslice.Worse, an explicit
throwstatement hit the unmodelleddefaultand bailed the WHOLE method — so any leak in a method guarded by a top-level validation throw (if (x is null) throw …;— everywhere in real .NET) was invisible to every flow detector.What
Model a
throwstatement with no enclosing try (canEscape && onThrow is null) as a bare CFG exit — the same synthetic{op:return}exit the injected may-throw edges already use. A resource owned there and disposed only later leaks on the throw path.Two wins, both sound / FP-free (a throw with no enclosing try definitely exits, so an owned resource definitely leaks):
dispose-not-called-on-throw—acquire; if (bad) throw; dispose;leaks on the throw path (dotNoTry, "may not be disposed on every path").throwused to hide is now caught (vtl, "is never disposed"), and so is everything else the rest of the body contains.Stays conservative inside a
try: an explicit throw there may run a finally or be caught (typed / catch-all), which needs the thrown-type-vs-catch match (not threaded here), so it keeps bailing — no new false escape past a catch.Tests
FlowLocalsSample.cs(asserted end-to-end in CI — real extractor → core):ValidatedThenLeaksvtlThrowAfterAcquireLeaksdotNoTryThrowAfterDisposeCleantdCleanValidatedThenCleanvtcCore handling of the
{op:return}exit is unchanged (already pinned by the existing return/leak fixtures, and re-validated against this IR shape locally); the C# lowering is the only new behaviour, covered by the CI flow step.Scope / posture
This is the sound default half of a tiered plan. The broader "any call may throw" at body level = CodeQL's full recall, but it's the CA2000 firehose (would flag harmless
MemoryStream/StringWriterdispose-on-throw, needs a fuzzy managed-only exemption) — so it'll land separately behind an opt-in flag (off by default), letting the oracle measure full recall without polluting the default. Default posture (lower FP than CA2000) is preserved here.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
New Features
throw, including cases with validation throws and throws that occur insidefinally.throwin switch sections.Tests
Samples