node:fs: keep ArrayBuffer storage alive across worker.terminate() during async write - #36818
node:fs: keep ArrayBuffer storage alive across worker.terminate() during async write#36818robobun wants to merge 7 commits into
Conversation
…ing async write When worker.terminate() lands while an async node:fs write (fs.write, fs.writev, fs.writeFile, FileHandle.write, createWriteStream) of a user Buffer is still inside write(2) on a pool thread, the worker's VM::~VM -> Heap::lastChanceToFinalize -> ~ArrayBufferContents frees the backing store while the kernel is still copying from it. The fd then receives freed-heap bytes (zero pages or another allocation's contents) instead of the bytes the writer held. Bun's off-thread buffer borrow (pinArrayBuffer / borrowBytesForOffThread / collectBufferSpans) previously relied on ArrayBuffer::pin() plus JSValue::protect(). pin() only blocks transfer() and protect() is a GC root, neither of which Heap::lastChanceToFinalize honours: it drops the heap's deferred flag on every ArrayBuffer and deletes at refcount 0. Take a native RefCounted +1 on the JSC::ArrayBuffer alongside the pin, and release it in unpinArrayBuffer. The ref survives lastChanceToFinalize (GCIncomingRefCountedSet::lastChanceToFinalize only clears the deferred bit; setIsDeferred(false) deletes only when the count is zero), so the pool thread's write(2) reads valid bytes for as long as the async task holds the pin. Both ref and deref run on the JS thread (DeferrableRefCounted is not atomic). This covers the node:fs Write/WriteFile/Read paths and the fs.writev/ readv iovec collector via their existing pin/unpin balance, and the other off-thread consumers (zlib, Bun.Image, shell stdio, MySQL blob binds, NodeHTTPResponse chunked writes) for free. The pool-thread completion-into-dead-VM crash that follows is the separate #34154/#2880 work; this change only addresses the buffer lifetime, so terminate() latency is unchanged.
WalkthroughThe change retains ArrayBuffer backing storage during native and off-thread borrows. New ASAN-gated FIFO tests cover worker termination during asynchronous filesystem reads and writes. ChangesArrayBuffer borrow lifetime
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for maintainer review. Bot reviews clean; CI green on this diff. Reproduced with: CI (build 88107): both new tests pass on every ASAN lane. Remaining red lanes are unrelated to this diff: |
![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.
My earlier concerns (vacuous test assertion, timeout justification) are resolved and I don't see anything further, but this is JSC DeferrableRefCounted lifetime management across VM teardown — worth a maintainer's eyes.
What was reviewed:
- ref/deref balance at all four pin sites vs the single
unpinArrayBufferrelease, including theisShared()branch (pin skipped, ref still taken — matches unpin side). collectBufferSpansmid-loop early returns (!view,!buf) leave earlier elements ref'd — same shape as the pre-existingpin(), so not a new leak class.- Intentional leak-on-terminate (nobody derefs once the worker VM is gone) is scoped to the #34154 completion path the PR already calls out.
Extended reasoning...
Overview
Adds a native ref() alongside every pin() in the four ArrayBuffer-borrow helpers in bindings.cpp (pinArrayBuffer, two borrowBytesForOffThread branches, collectBufferSpans) and the matching deref() in unpinArrayBuffer. Adds an ASAN-gated POSIX content-oracle test that terminates a worker mid-write(2) to a FIFO and asserts no freed-heap bytes reach the fd.
Security risks
None new. The change strictly extends buffer lifetime; the failure mode it removes (kernel copying from freed pages into a user fd) is itself a data-integrity/potential-info-leak hazard that this closes.
Level of scrutiny
High. This is native memory-lifetime management at the JSC GC boundary, interacting with GCIncomingRefCountedSet::lastChanceToFinalize during worker VM teardown, and it fans out to every off-thread borrow consumer (node:fs read/write/writev, zlib, Bun.Image, shell stdio, MySQL blob binds, NodeHTTPResponse). REVIEW.md flags native memory safety as the most-blocked category. The mechanism (DeferrableRefCounted count > 0 survives setIsDeferred(false)) checks out against the WebKit semantics described, and ~ArrayBufferContents needs no VM state, so a post-teardown deref() (or leak) is safe — but I'd want a maintainer to confirm the intentional leak-until-#34154 tradeoff and that no caller derefs from the pool thread.
Other factors
Both of my prior findings were addressed in 8cc9b96 (unconditional PASS reads=[1-9] assertion, negative-string check dropped) and the 30s timeout was justified with measured wall-clock. Fail-before/pass-after evidence is 9/9 → 9/9 on the revised assertions. The ref/deref pairing is symmetric across shared/non-shared branches. The pre-existing mid-loop-error pin leak in collectBufferSpans now also leaks a ref, but that's the same class, not new.
Read-direction coverage for the same pinArrayBuffer ref fix. The kernel's copy_to_user into the destination is invisible to ASAN, so the oracle is a direct address probe: the worker reports ptr(buf) before its fs.read/readv parks on an empty FIFO, the parent terminates it, and bun:ffi read.u8 at that address either sees the worker's fill byte (storage alive) or trips ASAN heap-use-after-free via GCIncomingRefCountedSet::lastChanceToFinalize (storage freed mid-read). Linux+ASAN only; the FIFO is never written so the pool thread stays in read(2) and the separate #34154 completion crash is not reached.
|
Added The kernel's Fail-before / pass-after |
![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/js/node/fs/fs-read-worker-terminate.test.ts`:
- Around line 81-85: Capture the resolved process exit value from the concurrent
Promise.all call in the subprocess test, while continuing to drain stdout,
stderr, and proc.exited together. Add an assertion that the captured exitCode
indicates successful termination, alongside the existing output and stage
checks.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c0525d6b-a565-4c9a-96a2-5a39ca51190f
📒 Files selected for processing (1)
test/js/node/fs/fs-read-worker-terminate.test.ts
… assert
Malloc=1 routes every WebKit allocation through system malloc so LSan
flags ~1 KiB of unrelated pre-existing worker-teardown leaks at
process.exit(), tripping the not.toContain('AddressSanitizer') check on
the release-asan lane. The oracle here is the ffi probe (PASS line or
heap-use-after-free abort), not leak accounting; detect_leaks=0 keeps
stderr clean and the unconditional toMatch(/^PASS.../m) carries the
fail-before signal on its own.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/node/fs/fs-read-worker-terminate.test.ts (1)
40-42: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftWait until the pool thread enters
read(2).
parentPort.postMessage()confirms only thatfs.read()orfs.readv()accepted the request. It does not confirm that the pool task started.w.terminate()can therefore run while the request is still queued.In that case, this test validates a queued borrow, not the lifetime while the kernel holds the destination address. Gate termination on an observable parked-read condition. For this Linux-only fixture, poll the fixture process's
/proc/self/task/*/wchanwith a bounded deadline until a thread is blocked in pipe read.As per coding guidelines, tests must await observable conditions and exercise actual production preconditions.
🤖 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 `@test/js/node/fs/fs-read-worker-terminate.test.ts` around lines 40 - 42, Update the worker-termination test around the read request and parentPort.postMessage flow to wait for an observable parked-read state before calling w.terminate(). For this Linux-only fixture, poll /proc/self/task/*/wchan with a bounded deadline until a thread is blocked in pipe read, then proceed with termination; do not treat the request callback or parentPort.postMessage as evidence that the pool task entered read(2).Source: Coding guidelines
🤖 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/js/node/fs/fs-read-worker-terminate.test.ts`:
- Line 85: Remove the explicit 20_000 timeout argument from the test
configuration in fs-read-worker-terminate.test.ts, allowing the repository
runner’s default timeout to apply. Do not add or retain any per-test timeout in
this Bun test.
---
Outside diff comments:
In `@test/js/node/fs/fs-read-worker-terminate.test.ts`:
- Around line 40-42: Update the worker-termination test around the read request
and parentPort.postMessage flow to wait for an observable parked-read state
before calling w.terminate(). For this Linux-only fixture, poll
/proc/self/task/*/wchan with a bounded deadline until a thread is blocked in
pipe read, then proceed with termination; do not treat the request callback or
parentPort.postMessage as evidence that the pool task entered read(2).
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8cd5c220-1a60-45d9-b612-59cf9126fc0d
📒 Files selected for processing (1)
test/js/node/fs/fs-read-worker-terminate.test.ts
There was a problem hiding this comment.
No issues found this pass, but this is a JSC GC-lifetime / cross-thread memory-safety change (DeferrableRefCounted ref across Heap::lastChanceToFinalize), so it's worth a human look before merge.
What was reviewed:
- ref/deref balance across all four pin sites and
unpinArrayBuffer:VectorArrayBuffer::release()derefs every appended view on both success and every error return ofcollectBufferSpans;borrowBytesForOffThread's return-1 (FastTypedArray) path takes no ref, matching callers that skip unpin. - Confirmed the terminated-worker path intentionally leaks the ref rather than deref'ing off-thread (
DeferrableRefCountedis non-atomic; unpin is JS-thread-only) — leak-over-UAF tradeoff, deferred to #34154. - Both new tests now carry unconditional positive assertions; earlier feedback on vacuous/negative-string checks was addressed in 8cc9b96 and ddbe327.
Extended reasoning...
Overview
The PR adds a native RefCounted +1 on JSC::ArrayBuffer at four existing pin sites in src/jsc/bindings/bindings.cpp (pinArrayBuffer, both branches of borrowBytesForOffThread, and the per-element loop in Bun__JSArray__collectBufferSpans), balanced by a deref() in unpinArrayBuffer. Two new ASAN-gated POSIX tests use FIFOs to park a pool-thread write(2)/read(2) mid-syscall, terminate the worker VM, and probe the buffer contents/address afterward.
Security risks
None beyond the memory-safety concern the PR itself fixes. The change adds a ref, not removes one; the failure mode of getting the balance wrong is a leak (over-ref) or the pre-existing UAF (under-ref), not a new attack surface.
Level of scrutiny
High. This is squarely in REVIEW.md's "most-blocked category": cross-thread buffer lifetime across a GC finalize boundary, with correctness depending on DeferrableRefCounted semantics under GCIncomingRefCountedSet::lastChanceToFinalize. The diff is ~5 production lines but the reasoning is subtle — pin() vs native ref, JS-thread-only deref, the intentional leak on the terminated-worker path pending #34154, and whether possiblySharedBuffer() returns the same ArrayBuffer* at pin and unpin time (it does; pin-time adoption is idempotent).
Other factors
I traced every caller of the four modified helpers (src/runtime/node/types.rs VectorArrayBuffer, src/jsc/array_buffer.rs, MySQLValue, Image, NodeHTTPResponse, node_zlib_binding) and each releases through unpinArrayBuffer on both success and error paths, including collectBufferSpans's partial-failure returns (Rust side sets pinned = true before checking status, then release() on every non-zero branch). All prior inline review threads on this PR are resolved. The bug-hunting pass found nothing new. Given the category, a maintainer sign-off on the lastChanceToFinalize reasoning and the leak-until-#34154 tradeoff is appropriate rather than a bot approval.
2 participants
Problem
worker.terminate()while an asyncnode:fswrite of a user Buffer (fs.write/writev/writeFile/fs.promises.writeFile/FileHandle.write/createWriteStream) is still insidewrite(2)on a pool thread:frees the backing store while the kernel is still copying from it. The fd then receives whatever those pages now hold instead of the bytes the writer actually held. ASAN cannot flag this (the reader is the kernel), so the test below uses a content oracle.
Cause
The async path's buffer borrow (
JSC__JSValue__pinArrayBuffer/borrowBytesForOffThread/Bun__JSArray__collectBufferSpans) relies onArrayBuffer::pin()plusJSValue::protect().pin()only blockstransfer(), andprotect()is a GC root;Heap::lastChanceToFinalizehonours neither. It runsGCIncomingRefCountedSet::lastChanceToFinalize, which drops the heap's deferred flag on everyArrayBufferand deletes at refcount 0.Fix
Take a native
RefCounted+1 on theJSC::ArrayBufferalongside the pin, and release it inunpinArrayBuffer. The ref surviveslastChanceToFinalize(setIsDeferred(false)deletes only when the count is zero), so the pool thread'swrite(2)reads valid bytes for as long as the async task holds the pin. Both ref and deref run on the JS thread (DeferrableRefCountedis not atomic).This covers the
node:fsWrite / WriteFile / AppendFile / Read args and thewritev/readviovec collector through their existing pin/unpin balance, and the other off-thread consumers of the same helpers (zlib,Bun.Image, shell stdio, MySQL blob binds,NodeHTTPResponsechunked writes) at no extra cost. Blocking worker shutdown on the pool job (thenode:zlibapproach in #35155) is not appropriate here: a write parked on a full pipe would maketerminate()wait as long as the peer stalls.The pool-thread completion-into-dead-VM crash that follows once the write drains is the separate #34154 work; this change only addresses the buffer lifetime, so
terminate()latency is unchanged.Test
test/js/node/fs/fs-write-worker-terminate.test.ts(POSIX, ASAN-gated): a worker fills an 8 MiB buffer with one known byte and streams it to a FIFO via each offs.write/fs.writev/fs.writeFile; the parent terminates the worker mid-write, drains, and flags any byte the writer never held.Malloc=1routes the ArrayBuffer backing through system malloc so ASAN free-fills it on release.Fail-before / pass-after
no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/fs/fs-read-worker-terminate.test.ts test/js/node/fs/fs-write-worker-terminate.test.ts