fix(extractor): IMemoryOwner.Memory handoff escapes the owner (oracle-driven, CodeQL-validated)#87
Merged
Conversation
…he owner (mined ImageSharp; CodeQL-validated) Cross-tool oracle (scripts/oracle_compare.py) on SixLabors/ImageSharp surfaced this: our flow-locals detector flagged `Image.WrapMemory`'s `memoryManager` (a `ByteMemoryManager : IMemoryOwner`) as an undisposed-local leak, but CodeQL (interprocedural) does NOT — the manager's `.Memory` is handed to `MemoryGroup.Wrap(...)` and lives on in the returned `Image`. A `Memory<T>` keeps its `IMemoryOwner` alive (it IS the backing), so passing `owner.Memory` as an argument transfers the owner's lifetime to the consumer; disposing it at method scope would be premature, and not disposing it here is not a leak. The flow escape-exclusion now untracks a local when its `.Memory` view is passed as an argument, scoped tightly: only `IMemoryOwner.Memory` (not any `local.Member`, so a `FileStream` whose `.Length` is read still leaks), and only for non-pool / non-`using` owners (a MemoryPool/`using` owner keeps its dangling-borrow tracking — OWN002 — intact). Mirrors the existing #80 escaping-ctor transfer and the D5 "ambiguous handoff -> conservatively exclude" rule. Regression sample MemoryOwnerEscapeSample.cs (--flow-locals): an IMemoryOwner whose `.Memory` is handed to a consumer stays silent ('handedOwner'); one whose `.Memory` is only read locally and never disposed still warns ('leakedOwner'). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
|
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 (2)
📝 WalkthroughWalkthroughAdds a P-016 "escape-via-projection" escape rule to the flow-locals extractor: when a tracked ChangesP-016 IMemoryOwner escape-via-projection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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 |
![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: c7a24f431a
ℹ️ 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
+2589
to
+2593
| || (!poolBuffers.Contains(nm) && !usingMemoryOwners.Contains(nm) | ||
| && idn.Parent is MemberAccessExpressionSyntax { Name.Identifier.Text: "Memory" } projMem | ||
| && projMem.Expression == idn | ||
| && projMem.Parent is ArgumentSyntax | ||
| && IsMemoryOwnerType(model.GetTypeInfo(idn).Type)) |
There was a problem hiding this comment.
When the candidate is a normal MemoryPool<T>.Rent() local, this new escape path still fires: such locals are not in poolBuffers, and usingMemoryOwners only contains using-declared rentals. As a result, var owner = MemoryPool<byte>.Shared.Rent(16); owner.Dispose(); Store(owner.Memory); is removed from tracked, so the flow pass no longer emits the intended dangling-borrow/use-after-release OWN002 for passing owner.Memory after disposal, despite the comment saying MemoryPool owners keep that tracking.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
568-574: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression assertion for MemoryPool-owner projection to lock the rule boundary.
Current checks validate
handedOwner(customIMemoryOwner) andleakedOwner, but they don’t pin the “non-pool only” constraint. Add a sample/assertion whereMemoryPool<T>.Rentowner passes.Memoryas an argument and must not be silenced by projection escape.🤖 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 @.github/workflows/ci.yml around lines 568 - 574, The current validation checks for leakedOwner and handedOwner cases but does not have a regression test for MemoryPool-owner projection escape. Add a new assertion (similar to the existing OWN001 leakedOwner check) that validates when MemoryPool<T>.Rent owner passes its .Memory as an argument, it must NOT be silenced by projection escape, then add this new MemoryPool test case to the for loop starting with "for ok in" that lists all the valid test case scenarios like handedOwner, clean, looped, etc.
🤖 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 2589-2593: The guard condition at line 2589 that checks
!poolBuffers.Contains(nm) and !usingMemoryOwners.Contains(nm) is incomplete
because it still allows non-using MemoryPool owners to escape through the
owner.Memory projection path. Add another condition to the guard that also
excludes non-using MemoryPool owners, similar to how poolBuffers and
usingMemoryOwners are already excluded. This will ensure that only appropriately
scoped memory owners can proceed through this projection-escape path.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 568-574: The current validation checks for leakedOwner and
handedOwner cases but does not have a regression test for MemoryPool-owner
projection escape. Add a new assertion (similar to the existing OWN001
leakedOwner check) that validates when MemoryPool<T>.Rent owner passes its
.Memory as an argument, it must NOT be silenced by projection escape, then add
this new MemoryPool test case to the for loop starting with "for ok in" that
lists all the valid test case scenarios like handedOwner, clean, looped, etc.
🪄 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: 598a8b64-a7d6-41de-95bb-8023a0f91c45
📒 Files selected for processing (3)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/MemoryOwnerEscapeSample.cs
Comment on lines
+2589
to
+2593
| || (!poolBuffers.Contains(nm) && !usingMemoryOwners.Contains(nm) | ||
| && idn.Parent is MemberAccessExpressionSyntax { Name.Identifier.Text: "Memory" } projMem | ||
| && projMem.Expression == idn | ||
| && projMem.Parent is ArgumentSyntax | ||
| && IsMemoryOwnerType(model.GetTypeInfo(idn).Type)) |
There was a problem hiding this comment.
Exclude non-using MemoryPool<T>.Rent owners from this new projection-escape path.
Line 2589’s guard only excludes poolBuffers (ArrayPool) and usingMemoryOwners, so a non-using MemoryPool owner still escapes via owner.Memory argument at Line 2590. That conflicts with the stated non-pool/non-using scope and can hide valid owner-lifetime findings.
Proposed fix
- var usingMemoryOwners = new HashSet<string>(); // `using`-declared MemoryPool owners
+ var usingMemoryOwners = new HashSet<string>(); // `using`-declared MemoryPool owners
+ var memoryPoolOwners = new HashSet<string>(); // all MemoryPool<Rent> owners
@@
- else if (IsMemoryPoolRent(v.Initializer?.Value, model)) // MemoryPool<T> IMemoryOwner (Dispose-released, NOT a poolBuffer)
- candidates.Add(v.Identifier.Text);
+ else if (IsMemoryPoolRent(v.Initializer?.Value, model)) // MemoryPool<T> IMemoryOwner (Dispose-released, NOT a poolBuffer)
+ {
+ candidates.Add(v.Identifier.Text);
+ memoryPoolOwners.Add(v.Identifier.Text);
+ }
@@
- || (!poolBuffers.Contains(nm) && !usingMemoryOwners.Contains(nm)
+ || (!poolBuffers.Contains(nm) && !usingMemoryOwners.Contains(nm)
+ && !memoryPoolOwners.Contains(nm)
&& idn.Parent is MemberAccessExpressionSyntax { Name.Identifier.Text: "Memory" } projMem
&& projMem.Expression == idn
&& projMem.Parent is ArgumentSyntax
&& IsMemoryOwnerType(model.GetTypeInfo(idn).Type))🤖 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 2589 - 2593, The
guard condition at line 2589 that checks !poolBuffers.Contains(nm) and
!usingMemoryOwners.Contains(nm) is incomplete because it still allows non-using
MemoryPool owners to escape through the owner.Memory projection path. Add
another condition to the guard that also excludes non-using MemoryPool owners,
similar to how poolBuffers and usingMemoryOwners are already excluded. This will
ensure that only appropriately scoped memory owners can proceed through this
projection-escape path.
… pool-rental tracking (Codex/CodeRabbit P1) The first cut excluded only poolBuffers (ArrayPool) and usingMemoryOwners (using-declared MemoryPool), but a `var` MemoryPool.Rent owner is in neither — so its `.Memory` handoff wrongly untracked it, dropping the dangling-borrow / double-dispose findings (OWN002/OWN003). The corpus benchmark caught it: memorypool-double-dispose regressed (recall 23 -> 22). Scope the projection-escape to a new `newedDisposables` whitelist — locals created via `new` (the WrapMemory `new ByteMemoryManager(...)` shape). A pool rental of ANY kind (ArrayPool or MemoryPool, `var` or `using`) is never `new`'d, so it keeps its full use-after-dispose tracking; the renter owns the Return/Dispose. Stricter and more self-evident than enumerating pool sets. Sample/CI gain a MemoryPool-owner boundary case: `pooled.Dispose(); Store(pooled.Memory)` must still trip OWN002 (not be silenced). handedOwner stays silent, leakedOwner still warns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
| // owner.Memory only READ locally (a length); owner never disposed -> real leak -> must WARN. | ||
| public static int ReadOnlyLeak() | ||
| { | ||
| var leakedOwner = new PixelOwner(); |
| // after Dispose still trips OWN002 — the rule must NOT silence it (Codex/CodeRabbit P1). | ||
| public static void PoolOwnerNotEscaped() | ||
| { | ||
| var pooled = MemoryPool<byte>.Shared.Rent(16); |
PhysShell
pushed a commit
that referenced
this pull request
Jun 22, 2026
…handlers (Codex P1) Codex: the first cut keyed only off the EVENT (the AppDomain source), ignoring the handler — so it also dropped a real region escape, e.g. `AppDomain.CurrentDomain.ProcessExit += (_,_) => _field++` or an instance-method handler, whose delegate target is the subscriber and is pinned to the process until shutdown. The Npgsql case is safe only because its lambda is non-capturing (a static `ClearAll()` call). HandlerRetainsNoInstance now gates the exemption: a static method group (null target) or a lambda that captures neither `this` (explicit, or implicit via an instance member) nor an enclosing local/parameter. A capturing handler stays OWN014. Sample: ShutdownCleanup gains the 4th event (FirstChanceException, CodeRabbit) and a new CapturingShutdownSubscriber (`(_,_) => _count++`) that must STILL warn. CI: assert no OWN014 anywhere in the sample file (format-insensitive, CodeRabbit) for the exempt cases, and OWN014 for both the non-AppDomain and the capturing controls. (Rebased onto current main — #86/#87 landed; resolved the ci.yml sample-list conflict.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
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.
Oracle-driven precision fix (cross-tool, validated by CodeQL)
Running the cross-tool oracle (
scripts/oracle_compare.py→oracle.yml) on SixLabors/ImageSharp surfaced this directly. Our flow-locals detector flaggedImage.WrapMemory'smemoryManager(aByteMemoryManager : IMemoryOwner) as an undisposed-local leak ×4 — but CodeQL (interprocedural) does not, because the manager's.Memoryis handed toMemoryGroup.Wrap(...)and lives on inside the returnedImage.WrapMemory.memoryManager— own-only, CodeQL silentThe fix
A
Memory<T>keeps itsIMemoryOwneralive (it is the backing), so passingowner.Memoryas an argument transfers the owner's lifetime to the consumer — disposing it at method scope would be premature, and not disposing it here is not a leak. The flow escape-exclusion now untracks a local when its.Memoryview is passed as an argument, scoped tightly:IMemoryOwner.Memory— not anylocal.Member, so aFileStreamwhose.Lengthis read still leaks;usingowners — aMemoryPool/usingowner keeps its dangling-borrow (OWN002) tracking intact.This mirrors the existing #80 escaping-ctor transfer and the D5 "ambiguous handoff → conservatively exclude" rule.
Regression guard (
--flow-locals)MemoryOwnerEscapeSample.cs— anIMemoryOwnerwhose.Memoryis handed to a consumer stays silent (handedOwner); one whose.Memoryis only read locally and never disposed still warns (leakedOwner).Note
Independent of the still-to-land #2/#3/#4 (different code region — the flow escape logic, not the field/pool/subscription detectors), so it bases cleanly on
main. The other ImageSharp own-only remainders are left honest:SharedArrayPoolBuffer(guard-indirection — Codex stopped the unsound version), andMemoryStream(can't be made dispose-optional without gutting theFlowLocalsSamplesuite). CodeQL's 3 oracle-only (dispose-not-called-on-throw) are a separate, already-partially-modelled recall slice.🤖 Generated with Claude Code
https://claude.ai/code/session_01Rg8kSk1YT14x7A1vo5zgED
Generated by Claude Code
Summary by CodeRabbit
Release Notes
New Features
IMemoryOwnermemory can escape ownership tracking.Improvements
.Memoryhandoffs as ownership-escaping transfers, reducing incorrect tracking in downstream cases.Tests / Validation
Samples