extractor: close two oracle FP tails — using-statement local release + no-op-dispose BCL wrappers#159
Conversation
…+ no-op-dispose BCL wrappers Two precision fixes for the remaining open root-causes in oracle-known-fps.md. Tail 1 (root-cause #1, custom dispose-sink): release a tracked local used as the resource of a `using (existingLocal) { ... }` statement. The --flow-locals lowering already handled the `using (var x = ...)` declaration form and the MemoryPool owner statement form, but not the expression form over an already-acquired local (`var timer = new ...; using (timer) { ... }`) — it left the local looking live at method exit, a spurious OWN001. Now threads a scope-exit release onto the body's normal completion and its return/throw exits (no acquire — the local was acquired at its `new`). Clears protobuf-net's assorted/ Silverlight Page.xaml.cs `timer` (baseline entry marked fix-landed, pending a live-oracle re-run to confirm-and-delete). Tail 2 (root-cause #2, no-op Dispose): extend the StringWriter/StringReader field exemption to BCL pass-through wrappers. A StreamReader/StreamWriter/ BinaryReader/BinaryWriter field whose every construction wraps an in-memory backing (MemoryStream/StringWriter/StringReader) cascades disposal only to managed memory, so it is dispose-optional. Closed allowlist of those four adapters (GZipStream/CryptoStream own native/extra resource — stay flagged); a path building `new StreamReader(path)` (a real handle) keeps the field flagged. Does NOT clear Newtonsoft's _textWriter (a third-party JsonTextWriter wrapper whose Dispose we can't prove is pass-through without body analysis — the soundness wall); that stays baselined, with the reasoning recorded so it isn't unsoundly suppressed. Corpus fixtures: local-dispose-via-using-statement, field-noop-dispose-wrapper (each: before.cs caught / after.cs clean, proving the exemption stays narrow). Docs: new no-op-dispose-wrapper.md; oracle-known-fps.md root-causes #1/#2 and baseline annotations updated. Selftests: oracle 31/31, corpus 29/29, validate_contrib 16/16, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
Warning Review limit reached
More reviews will be available in 3 minutes and 43 seconds. Learn how PR review limits work. To continue reviewing without waiting, enable usage-based billing in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (14)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: e8cdaafc4d
ℹ️ 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".
| { | ||
| if (model.GetTypeInfo(oce).Type is not { } wt | ||
| || wt.ContainingNamespace?.ToString() != "System.IO" | ||
| || wt.Name is not ("StreamReader" or "StreamWriter" or "BinaryReader" or "BinaryWriter")) |
There was a problem hiding this comment.
Including StreamWriter in this allowlist suppresses OWN001 for fields such as new StreamWriter(new MemoryStream()), but disposing a StreamWriter is not a no-op: Microsoft documents Dispose(bool) as flushing buffered data to the underlying stream (https://learn.microsoft.com/en-us/dotnet/api/system.io.streamwriter.dispose). If the owner never disposes or flushes the field, the MemoryStream can be missing buffered characters/encoder state, so this exemption can hide real correctness bugs rather than just managed-memory-only leaks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 017c67a. Writers do flush on Dispose, so a never-disposed StreamWriter/BinaryWriter over an in-memory backing can silently drop buffered data — a real bug worth keeping the OWN001 for. The allowlist is now restricted to the read-only adapters StreamReader/BinaryReader (their Dispose only discards a read-ahead buffer and closes the in-memory backing). Docs/fixture notes updated to readers-only.
Generated by Claude Code
| bodyOnThrow.AddRange(onThrow ?? exit); | ||
| } | ||
| if (us.Statement is not null | ||
| && !LowerFlowStmt(us.Statement, tracked, model, nodes, canEscape, bodyOnThrow, bodyOnReturn)) |
There was a problem hiding this comment.
When a using (existingLocal) body contains an explicit throw, this call passes a non-null bodyOnThrow into LowerFlowStmt, but the ThrowStatementSyntax case returns false whenever onThrow is non-null. That causes the entire method's flow facts to be skipped; for example, var leaked = new Res(); using (r) { if (bad) throw ...; } will no longer report the unrelated leaked local. The using lowering should route explicit throws through the release continuation instead of bailing the method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 017c67a. You're right that the non-null bodyOnThrow makes the ThrowStatementSyntax case bail the whole method, losing unrelated leaks. The fully sound fix (route using-throws through the release continuation) needs an "onThrow is unconditional" signal threaded through the lowering to distinguish a using (no catch) from a try (a throw may be caught) — invasive, so deferred. For now the using-local release is gated on the body having no explicit escaping throw; when it does, we fall through to the plain lowering so the method stays analysed (status quo — r itself may still show undisposed on the throw path, no worse than before this fix). Documented in the fixture notes.
Generated by Claude Code
…ow bail) Two P2 review findings from Codex: 1. Exclude buffered writers from the no-op-dispose allowlist. StreamWriter / BinaryWriter FLUSH buffered output on Dispose, so a never-disposed writer field can leave even an in-memory backing missing buffered data — a real correctness bug, not a managed-memory-only no-op. IsNoOpDisposeWrapper now allows only the read-only adapters StreamReader / BinaryReader, whose Dispose just discards a read-ahead buffer and closes the (in-memory) backing. 2. Don't bail the whole method on an explicit throw inside a `using (existingLocal)` body. Threading a non-null bodyOnThrow makes the ThrowStatementSyntax case return false (it refuses a non-null onThrow — a throw inside a try might be caught), which skipped the method's entire flow analysis and lost UNRELATED leaks. The using-local release is now gated on the body having no explicit escaping throw; otherwise it falls through to the plain lowering (method stays analysed). Routing using-throws through the release would need invasive onThrow-is-unconditional threading — deferred and documented. Docs/fixtures updated to readers-only + the explicit-throw caveat. Selftests: oracle 31/31, corpus 29/29, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
@CodeRabbit reviw |
|
✅ Action performedReview finished.
|
…hrowDefinite) Follow-up to PR #159's deferred item. An explicit throw inside a using (existingLocal) body previously had to bail the whole method (the ThrowStatementSyntax case refuses a non-null onThrow, since a throw inside a try might be caught), losing detection of unrelated leaks; #159 worked around it by gating the using-release out when the body threw. This threads an `onThrowDefinite` flag alongside `onThrow` through the lowering (LowerFlowStmt / LowerFlowStatements / LowerSwitchSection). It is true when a throw that runs `onThrow` definitely leaves the method uncaught — a `using` or a finally-only `try` (neither has a catch) — and false for a `try` with any `catch` (the throw may be caught, a match this lowering does not model). The throw case then routes the throw through the release continuation when definite, and still bails otherwise. Throws inside a `finally` keep bailing (IsInsideFinally guard, unchanged). Net: the using-local release no longer needs the explicit-throw gate (removed); finally-only `try` bodies with an explicit throw are now analysed instead of bailed (sound bonus). try-with-catch is unchanged (still conservative). No existing golden sample has an explicit throw in a using/finally-only-try body, so no expected-output changes. Fixture: using-statement-throw-releases (before.cs: an unrelated leak is still caught despite a throw in the using body — a direct no-bail regression test; after.cs clean). Selftests: corpus 30/30, oracle 31/31, ruff clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
Закрываем два оставшихся открытых хвоста из
docs/notes/oracle-known-fps.md— обе чинки sound и узкие (помним урок со static-class):using (existingLocal) { ... }.--flow-localsуже умелusing (var x = ...)и MemoryPool-форму, но не expression-форму над ранее захваченным локалом (var timer = new ...; using (timer) { ... }) — локал «висел живым» к выходу из метода → ложный OWN001. Теперь release протягивается на normal-completion и return/throw выходы (без acquire — локал уже захвачен наnew). Чинит protobuf-netPage.xaml.cstimer.StringWriter/StringReaderна BCL pass-through обёртки. ПолеStreamReader/StreamWriter/BinaryReader/BinaryWriter, каждая конструкция которого оборачивает in-memory backing (MemoryStream/StringWriter/StringReader), каскадит Dispose только в managed-память → dispose-optional. Закрытый allowlist из 4 адаптеров (GZipStream/CryptoStreamдержат native-ресурс → остаются под флагом); путьnew StreamReader(path)(реальный handle) поле под флагом оставляет.Тип изменения
Как проверено
oracle_compare.py --selftest→ 31/31,validate_contrib.py --selftest→ 16/16python tests/test_corpus.py→ 29/29 (два новых фикстура)ruff check→ cleanСвязанные issue
Refs
docs/notes/oracle-known-fps.mdroot-causes #1, #2. Самостоятельной issue нет.Чеклист
local-dispose-via-using-statement,field-noop-dispose-wrapper— каждый: before.cs ловится / after.cs чисто, доказывая, что исключение остаётся узким)no-op-dispose-wrapper.md; root-causes Remove unnecessary .txt extensions from example and source files #1/Add buffer storage policies with mandatory logging #2 и аннотации baselineSoundness wall (честно)
Tail 2 не закрывает мотивирующую находку — Newtonsoft
TraceJsonReader._textWriter(JsonTextWriterнадStringWriter).JsonTextWriter— third-party обёртка, доказать что её Dispose pass-through нельзя без анализа тела; молча душить = тот же unsound over-reach, что и отклонённое static-class исключение. Поэтому она остаётся в baseline, с записанным обоснованием. Аналогично NLog_xmlSource/_reusable*Stream(тоже third-party обёртки) остаются видимыми. Запись protobufPage.xaml.csпомечена fix-landed и ждёт live-oracle прогона на confirm-and-delete.🤖 Generated with Claude Code
https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
Generated by Claude Code