JSSink: pass the sink to finalize as *mut instead of &mut - #37716
Conversation
`${Sink}__finalize` is called when a JS wrapper or controller cell gives
up its claim on the native sink. For ArrayBufferSink that frees the Box,
for FileSink it drops the wrapper's ref (the last one on a normal sweep),
and for FetchRequestBodySink it drops a tasklet ref whose deinit frees the
sink. The generated thunk, JSSink::js_finalize and JsSinkType::finalize
all took the sink as `&mut`, so the free happened while three reference
arguments to the allocation were still live, which the aliasing models
reject (the reference is protected for the duration of the call).
Make every frame of the chain take `*mut`: the thunk and js_finalize
forward the pointer, the freeing impls release through it without forming
a reference, and the impls that do not free (HTTPServerWritable,
NetworkSink, RewriterPipe) reborrow inside their own body. The inherent
ArrayBufferSink::finalize wrapper, whose only caller was the trait impl,
is folded into destroy.
Add a source lint that pins the shape of all three frames and of every
JsSinkType impl.
|
Updated 11:09 PM PT - Aug 11th, 2026
@Jarred-Sumner, your commit a9be37f is building: |
|
Status: ready for review. CI is green on head 0ae8483 (build 92764, 190/190 jobs; the three yellow entries are Verdaccio startup / inspect-error-leak timeouts that passed on retry and do not touch sinks). Reproduced as a source-level finding: with src/ at Review so far: the lint now also pins the inherent methods that perform the free (103ba76); the added comments were cut down to the |
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review 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 refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
WalkthroughSummarySink finalization now uses unsafe raw pointers across generated Rust thunks, the runtime trait, sink implementations, and source-lint validation. ChangesSink finalization contract
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.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 `@test/internal/source-lints/jssink-finalize-raw-ptr.test.ts`:
- Around line 100-116: Extend the FRAMES array with entries matching the
inherent free-performing methods FileSink::finalize,
FetchRequestBodySink::finalize, and ArrayBufferSink::destroy. Pin each method’s
raw-pointer receiver signature so the lint fails if any is changed back to &mut
self, while preserving the existing trait, forwarder, and generated-thunk
checks.
🪄 Autofix
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: ASSERTIVE
Plan: Pro
Run ID: 99524254-38c7-41d6-a205-dc265416b262
📒 Files selected for processing (8)
src/codegen/generate-jssink.tssrc/runtime/api/html_rewriter.rssrc/runtime/webcore/ArrayBufferSink.rssrc/runtime/webcore/FileSink.rssrc/runtime/webcore/Sink.rssrc/runtime/webcore/fetch/FetchRequestBodySink.rssrc/runtime/webcore/streams.rstest/internal/source-lints/jssink-finalize-raw-ptr.test.ts
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it rewires the unsafe finalize chain across every JSSink type (GC/heap-free paths in native code) and has stated allowlist interactions with sibling PRs (#37703/#37705), a human sign-off is still worthwhile.
What was reviewed:
- The
&mut→*mutsignature change through all four frames (thunk template,js_finalize, trait declaration, six impls) — ABI is unchanged, C++ callers untouched. FileSink::finalizebody restructuring:is_some_and+.replace(false)preserve the oldif let/get-then-set semantics; the twounsafeblocks perform the same operations in the same order.ArrayBufferSink: the deletedpub fn finalizewrapper had the trait impl as its sole caller;destroyis now called directly.HTTPServerWritable/NetworkSinkreborrows: verified the inherentfinalizeon each does not free*this(RequestContext / MultiPartUpload own those allocations), so the scoped&mutis sound.
Extended reasoning...
Overview
This PR changes the JSSink finalize call chain to pass the sink as *mut Self instead of &mut self/&mut T. The chain is: generated ${name}__finalize thunk (generate-jssink.ts) → JSSink::js_finalize (Sink.rs) → JsSinkType::finalize trait method → per-sink inherent finalize. Three of the six sink impls (ArrayBufferSink, FileSink, FetchRequestBodySink) free the allocation from inside this call, which is UB under Stacked/Tree Borrows when a &mut argument to any frame in the chain is still live (rustc marks reference arguments dereferenceable for the whole call). The fix carries the raw pointer end-to-end; each impl decides whether to reborrow. A new source-lint test pins every frame's signature.
Eight files touched: the codegen template, the trait/glue in Sink.rs, five sink impls (ArrayBufferSink, FileSink, FetchRequestBodySink, streams.rs for HTTPServerWritable/NetworkSink, html_rewriter.rs for RewriterPipe), and the new lint test.
Security risks
None in the traditional sense (no auth/crypto/input parsing). This is a memory-safety soundness fix — the risk is regression, not exploitation. The change is behavior-preserving by construction (same operations, same order; only the type the pointer travels as changes), and the ABI at the C++ boundary is identical (&mut T and *mut T are both a bare pointer in extern "C").
Level of scrutiny
High. This is unsafe Rust on the GC-sweep / heap-deallocation path for every JSSink type, called from C++ destructors and .close(). It's exactly the category REVIEW.md flags as most-blocked ("Native code: memory safety"). While the change is mechanical and part of an established sibling-PR family (#37672/#37681/#37685/#37693/#37705/#37551), and the bug hunter found nothing, this is not the kind of change the approval guidelines describe as auto-approvable ("simple, mechanical, or obvious … config tweaks, typo fixes").
Other factors
- All comment-cop and coderabbit threads are resolved (comments trimmed in 0ae8483; the lint extended to cover inherent freeing methods in 103ba76).
- The PR description explicitly notes NetworkSink's S3 tests couldn't run in the author's environment and are left to CI.
- The description documents allowlist-entry overlap with #37703/#37705 that whichever side lands second must reconcile — a human should be aware of the merge-order dependency.
- CI build #92764 was still running at the time of the last robobun status update.
- The
FileSink::finalizebody was restructured beyond a pure receiver swap (nestedif letcollapsed tois_some_and, get+set collapsed toreplace), which I verified is semantically identical but merits a second pair of eyes.
| /// `${abi}__finalize`: the JS cell holding `this` as `m_sinkPtr` is giving | ||
| /// up its claim on the sink. Raw pointer, not `&mut self`: for | ||
| /// `ArrayBufferSink`, `FileSink` and `FetchRequestBodySink` that releases | ||
| /// the allocation, and freeing under a live reference argument is UB. | ||
| /// | ||
| /// # Safety | ||
| /// `this` is the cell's live sink and must not be used after the call. | ||
| unsafe fn finalize(this: *mut Self); |
There was a problem hiding this comment.
🟡 The PR description's ### Tests section and the robobun status comment describe test/internal/source-lints/jssink-finalize-raw-ptr.test.ts as the shipped regression guard, but HEAD commit a9be37f deletes that file with no stated reason and the net diff contains no test — the description now overstates what ships (CLAUDE.md rule 11), and the "Overlap with the sibling lints" coordination paragraph is moot. Either restore the lint or update the description to drop the Tests section and explain why the ratchet was removed.
Extended reasoning...
What the issue is
The PR description dedicates a full ### Tests section to describing test/internal/source-lints/jssink-finalize-raw-ptr.test.ts — what it scans (every impl JsSinkType block's finalize item, the trait declaration, js_finalize, the codegen template, and the three inherent freeing methods), what it reports on main (an 11-line listing of impl sites plus the frame signatures), and how it self-checks its own patterns. The robobun status comment also cites it as the reproduction ("with src/ at main, test/internal/source-lints/jssink-finalize-raw-ptr.test.ts reports the six JsSinkType impls…"). The description's "Overlap with the sibling lints" paragraph then coordinates with #37685/#37693/#37703/#37705's allowlist entries on the assumption this lint exists ("Whichever side lands second deletes the entry").
However, the HEAD commit a9be37f deletes this test file entirely, with commit message "Delete test/internal/source-lints/jssink-finalize-raw-ptr.test.ts" and no stated reason. The net PR diff (7 files) contains no test file at all.
Step-by-step proof
git log --onelineon this branch shows the test was created in 9680e45 (this PR's first commit), extended in 103ba76 (adding FRAMES entries for the inherent freeing methods, per CodeRabbit's inline comment), then deleted in a9be37f (HEAD).ls test/internal/source-lints/jssink-finalize-raw-ptr.test.ts→ "No such file or directory".- The changed-files list is exactly the 7
src/files — notest/file. - Yet the PR description body (unchanged since the test existed) still contains the full "### Tests" section describing the file's behavior, its FRAMES entries, and its expected failure listing on
main. - The robobun status comment ("CI is green on head 0ae8483 … Reproduced as a source-level finding: … test/internal/source-lints/jssink-finalize-raw-ptr.test.ts reports …") describes the state as of 0ae8483, before the deletion commit — so it too is stale.
Why this is a real (if minor) problem
CLAUDE.md rule 11: "Be humble & honest — NEVER overstate what you got done or what actually works in commits, PRs or in messages to the user." The description promises a signature ratchet (a mechanical guard that any future &mut self reversion would trip in CI) that does not ship. A reviewer skimming the description would reasonably conclude the PR includes a regression guard when it does not.
The "Overlap with the sibling lints" paragraph is now actively misleading: it instructs whichever of #37685/#37693/#37703/#37705 lands second to delete allowlist entries in coordination with this lint, but there is no lint here to coordinate with.
Note that the REVIEW.md rule "Never silently … delete an existing test" is weaker here than the synthesis originally framed it: the test never existed on main — it was added and removed within this PR — so there is no pre-existing safety net being weakened. The issue is purely description-vs-reality staleness, not test deletion per se.
Impact
No runtime impact — the code change (moving the finalize chain from &mut to *mut) is correct and behavior-preserving, and the actual finalize paths are covered by the existing sink test suites listed under "### Verification" (arraybuffersink.test.ts, filesink.test.ts, body-stream.test.ts, etc.). Merging as-is causes no failure. The cost is (a) a misleading PR description in the permanent record and (b) confusion for the sibling-PR authors who read the coordination instructions.
How to fix
Either:
- Restore
test/internal/source-lints/jssink-finalize-raw-ptr.test.tsfrom 103ba76 (the ~17 sibling files intest/internal/source-lints/show the pattern is established in-tree, so it fits), or - Update the PR description: drop the "### Tests" section (or replace it with a note that the finalize behavior is covered by the existing sink suites under Verification), drop the "Overlap with the sibling lints" paragraph, and briefly say why the source-lint was removed (e.g. "signature-regex lints deemed too brittle" if that's the reason).
…e sink through its pointer The two pointers start_request_stream stores for later (the sink's back-pointer and the promise ctx) were made from the hop's &mut, so the last-ref releases made through them later used a borrow that accesses through the allocation pointer had since invalidated (Miri rejects that under both models). The hop now passes its allocation pointer down and those two stashes are made from it; the releases that happen inside the frame keep going through self. FetchRequestBodySink::end_from_stream takes the sink pointer, like NetworkSink's, since its release can free the tasklet and the sink. Lint: also match a bare self argument (the &mut -> *mut coercion), not counting fn definitions; allowlist the sites that surfaces; state the spellings the lint does not see; drop the FileSink entry (#37716 landed).
2 participants
Problem
The JSSink finalize chain frees the sink while reference arguments to it are still live. At
3fc747a7da:extern "C" fn ${name}__finalize(this: &mut ${name})(src/codegen/generate-jssink.ts), called from~JS${name},~JSReadable${name}Controllerand${name}__doCloseJSSink::js_finalize(this: &mut T)(src/runtime/webcore/Sink.rs)JsSinkType::finalize(&mut self)(src/runtime/webcore/Sink.rs), whose impls do the actual release:ArrayBufferSink(src/runtime/webcore/ArrayBufferSink.rs):Self::finalize(ptr::from_mut(self))->destroy->heap::take, unconditionally. The comment on the impl said the C export owned the free; this call is the free.FileSink(src/runtime/webcore/FileSink.rs): the inherentfinalize(&mut self)ends inFileSink::deref(ptr::from_mut(self)), which runsdeinit->heap::takewhenever the wrapper's +1 was the last ref, i.e. on an ordinary GC sweep of a sink nothing else holds. The header comment argued this was fine because the&mutcarries write provenance, which is true but is not the problem.FetchRequestBodySink(src/runtime/webcore/fetch/FetchRequestBodySink.rs): drops the tasklet ref taken instart_request_stream. The tasklet owns the sink allocation, so if that ref is the last one,FetchTasklet::deinit->clear_data->clear_sink->heap::take(sink)frees*selfinside the call. That is the fallback path for a pump that never settled; it is reachable at least on worker teardown: phase B ofVirtualMachineteardown releases the aborted fetch's other refs on the tasklet, and phase C then destroys the heap, sweeping the controller withm_sinkPtrstill set becauseJSSinkController__onClosedoes not run the detaching JS callback once termination is pending.HTTPServerWritable,NetworkSinkandRewriterPipedo not free anything here (their allocations are owned by theRequestContext, the S3 wrapper and the pipe's own refcount respectively).A reference passed as an argument has to stay dereferenceable until the call returns. Freeing it from inside the call is undefined behaviour under both aliasing models whether or not the reference is used again (Stacked Borrows:
deallocating while item is strongly protected; Tree Borrows, whichbun run rust:miriuses, rejects it the same way), and that protector is the model behind thedereferenceableattribute rustc puts on every&/&mutargument, so the optimizer may legitimately move a load through any of the three frames past the free. No crash is known from this; ASAN only has something to catch if the optimizer actually takes that liberty, which the unoptimized debug build never does, so it is not observable as a runtime test. Same family as #37672, #37681, #37685, #37693, #37705 and #37551; #37705's description leaves this chain out explicitly because it needs a change to the generated thunk.Fix
The whole chain takes the raw pointer, which is what the C++ side has anyway (
void* m_sinkPtr):pub unsafe extern "C" fn ${name}__finalize(this: *mut ${name})forwarding tojs_finalize; the ABI is unchanged, so JSSink.cpp is untouched.JSSink::js_finalize(this: *mut T)forwards to the trait.JsSinkType::finalizebecomesunsafe fn finalize(this: *mut Self), documented as "the cell is giving up its claim; this may free the sink", the same shape asHTTPServerWritable::abort(this: *mut Self)and the FileSink PipeWriter callbacks.ArrayBufferSinkcallsdestroydirectly (the inherentfinalizewrapper, whose only caller was the trait impl, is deleted);FileSink::finalize(this: *mut FileSink)keeps the same body with per-statement(*this).fieldaccess, likeon_closein the same file (the file header no longer claims the&mutversion was sound; the rationale lives once, on the trait method);FetchRequestBodySink::finalize(this: *mut Self)takestaskout through the pointer and does not touch it after the deref.HTTPServerWritableandNetworkSinkreborrow inside their own impl to call the unchanged inherentfinalize(&mut self); that borrow ends before the impl returns and nothing under it frees, which the SAFETY comments state.RewriterPipe's impl stays empty.Every impl performs the same operations in the same order as before; the only thing that moves is the type the pointer travels as.
js_controller_detached,js_closeandjs_end_with_sinkstill take&mut: nothing frees under them (thecontroller_detachedcontract on the trait already requires deferring a last-owner free for that reason).FileSink::assign_to_stream'sFileSinkRefguard also derefs from a&mut selfframe, but its ref is balanced against one it took itself and every caller (subprocess stdin setup) holds its own ref across the call, so it can never be the one that frees; left alone. Sites with the same shape outside this chain (S3UploadStreamWrapper::handle_{resolve,reject}_stream,FetchTasklet::write_end_request) are not sink frames and are reported separately.Tests
test/internal/source-lints/jssink-finalize-raw-ptr.test.ts scans every
impl ... JsSinkType for ...block for afinalizeitem and requiresunsafe fn finalize(<ident>: *mut Self), checks the other frames by signature (trait declaration,js_finalize, the codegen template, and the three inherent methods that perform the free, whichpubtells apart from the trait impls in the same files), and checks its own patterns against positive and negative spellings. With src/ restored tomainit reports:(
ArrayBufferSink::destroyalready took*mutonmain; its entry is a ratchet.)The behaviour itself is the existing coverage of each finalize path; see below.
Verification
Debug (ASAN) build on Linux:
cargo clippy -p bun_runtimeandrustfmt --checkon the touched files are clean; the generated thunks have the new signature. Passing: test/internal/source-lints/ (all 18 files), test/js/bun/util/arraybuffersink.test.ts and filesink.test.ts (wrapper sweep and prototype.close()for the two Box/refcount sinks), test/js/bun/spawn/spawn.test.ts (stdinFileSinkviaassign_to_stream), test/js/web/fetch/body-stream.test.ts, fetch-abort-stream-body.test.ts and fetch-stream-cancel-leak.test.ts (FetchRequestBodySink), test/js/bun/http/serve-response-stream-sink-leak, serve-direct-readable-stream, serve-stream-reject-flush-leak and serve-async-stream-client-abort (HTTPServerWritablecontroller teardown), test/js/web/fetch/server-response-stream-leak.test.ts, test/js/web/streams/streams.test.js, test/js/workerd/html-rewriter.test.js and html-rewriter-leak.test.ts (RewriterPipe), test/js/bun/s3/s3-stream-error-gc.test.ts and s3-argument-validation.test.ts. The S3 upload tests that would driveNetworkSink(s3.test.ts, s3-storage-class.test.ts) cannot connect from this environment and fail identically on the released binary, so that impl (a one-line forward to the unchanged inherent method) is left to CI.Overlap with the sibling lints, each of which documents these sites as tracked separately: #37685 / #37693 / #37705 add
self-receiver-teardown.test.tswithsrc/runtime/webcore/ArrayBufferSink.rs: 1allowlisted for theSelf::finalize(ptr::from_mut(self))line this PR removes, and #37703 addsself-receiver-release.test.tswithsrc/runtime/webcore/FileSink.rs: 2allowlisted for the two derefs inside the oldFileSink::finalize(&mut self)(running that lint against this branch reports FileSink.rs at 0). Whichever side lands second deletes the entry; nothing else conflicts (#37703's FetchRequestBodySink.rs hunk isend_from_stream, a different function). #34999 and #35528 edit the body ofFileSink::finalizetextually but keep the receiver.