Fix four crash/correctness bugs: node:vm link(), Worker name, FFI threadsafe callbacks, sliced Bun.file - #34140
Conversation
…crashing link()'s length-parity and element-type checks were debug-only ASSERTs, so a holey array (or a specifiers/moduleNatives length mismatch) reached toWTFString/dynamicDowncast with an empty JSValue — which passes isCell() with a null cell — and segfaulted in release builds. Reject holes, non-string specifiers, and mismatched lengths with the matching ERR_* TypeErrors.
…ng heap options.name was stored as-is from the parent thread and later materialized as a worker-heap JSString, so both threads ref/deref'd one non-atomic StringImpl refcount — and for an atomized name (e.g. fn.name), the losing thread's final deref removes the impl from the wrong thread's atom table. isolatedCopy the name at option parsing (matching env/argv/execArgv) and again when building the worker-side threadName binding so neither heap shares an impl.
…ng thread
FFI_Callback_threadsafe_call captured Ref{wrapper} in the posted task but
ignored postTaskTo's return value: when the target context was gone or
terminating, the un-enqueued lambda died on the foreign thread, and if the JS
side had already dropped its ref, ~FFICallbackFunctionWrapper destroyed two
JSC::Strong handles off the JS thread. Take the ref inside postTaskTo's
found-live window (betweenLookupAndEnqueue) and release it via adoptRef inside
the task, so the failure path is ref-neutral and the last deref can only
happen on the JS thread.
…to EOF resolve_size()/resolved_size() got a "don't clobber a slice's concrete size" guard for Bytes-backed stores, but the File arm still unconditionally set size = file_size - offset, so Bun.file(p).slice(a, b) streamed to EOF, HEAD advertised the full remaining file as Content-Length, and serializing one widened the original in place. Apply the same guard to both functions. structuredClone of a file-backed slice also lost the window's end because the serialized record only carried the offset; bump the blob serialization format to v4 and carry the blob's (pre-resolve) size, keeping unknown sizes deferred to the receiver as before. v1-3 payloads still deserialize.
…ved_size The same unknown-vs-concrete size clamp appeared in all four store arms; move it into one helper. Also trim the worker-name stress test to 4 iterations.
|
Updated 5:37 AM PT - Jul 14th, 2026
❌ @autofix-ci[bot], your commit 4b9b39b has 1 failures in
Add 🧪 To try this PR locally: bunx bun-pr 34140That installs a local version of the PR into your bun-34140 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Caution Review failedAn error occurred during the review process. Please try again later. 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/webcore/Blob.rs (1)
4320-4335: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winv4 structured-clone size window isn't actually validated against EOF on the receiving side, and the new test can't detect it.
The root cause is that the new
file_sizewire field is written fromself.size.get()before it's resolved/clamped, and on deserialize it's applied directly with no working clamp for File stores (the fallback clamp is gated onstore.size() != MAX_SIZE, which is never true for a freshly-created, unstat'd File store). ABun.file(path).slice(a, b)created against an unresolved parent can have a concrete-but-unclampedsize(e.g..slice(5, 5000)on a 10-byte file →size = 4995), and that raw value now survives structuredClone/postMessage unchecked — a regression from v1-v3, where the receiver never trusted the sender's size and always resolved it locally.
src/runtime/webcore/Blob.rs#L4320-L4335: replace thestore.size()-gated manual clamp with an unconditionalblob.resolve_size()call (it already stats + applieswindow_size()correctly) so File stores get properly clamped on the receiver.src/runtime/webcore/Blob.rs#L772-L775: update/remove the misleading "resolve_size() clamps this... on first use" comment once the deserialize fix above makes it true, and note that the size written here is unresolved/unclamped by design.test/js/web/fetch/blob.test.ts#L681-L689: addexpect(clone.size).toBe(5);afterstructuredClone(s)so this regression is actually caught by the test suite.🤖 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 `@src/runtime/webcore/Blob.rs` around lines 4320 - 4335, The structured-clone receiver must validate File-backed blob size windows against EOF instead of trusting the serialized size. In src/runtime/webcore/Blob.rs:4320-4335, replace the store.size()-gated clamp with an unconditional blob.resolve_size() call; in src/runtime/webcore/Blob.rs:772-775, update the comment to state that the serialized size is intentionally unresolved/unclamped; in test/js/web/fetch/blob.test.ts:681-689, assert clone.size equals 5 after structuredClone(s).
🤖 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/worker_threads/worker_threads.test.ts`:
- Around line 749-751: Update the subprocess assertions around proc.stdout,
proc.stderr, and proc.exited to preserve stdout, stderr, exitCode, and
signalCode in failure diagnostics. Keep the success assertions focused on
stdout.trim() equaling "done" and exitCode equaling 0, while ensuring stderr and
signal information are included when those assertions fail.
In `@test/js/web/fetch/blob.test.ts`:
- Around line 681-689: Extend the “slice end beyond EOF clamps to the file size”
test after structured cloning to assert that clone.size equals 5. Keep the
existing clone.text() assertion and sender-size assertion unchanged, using the
clone created from s to verify EOF clamping on the receiving side.
---
Outside diff comments:
In `@src/runtime/webcore/Blob.rs`:
- Around line 4320-4335: The structured-clone receiver must validate File-backed
blob size windows against EOF instead of trusting the serialized size. In
src/runtime/webcore/Blob.rs:4320-4335, replace the store.size()-gated clamp with
an unconditional blob.resolve_size() call; in
src/runtime/webcore/Blob.rs:772-775, update the comment to state that the
serialized size is intentionally unresolved/unclamped; in
test/js/web/fetch/blob.test.ts:681-689, assert clone.size equals 5 after
structuredClone(s).
🪄 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: 7a3b1014-1898-4cdf-8e10-2dd5f94fc2f5
📒 Files selected for processing (8)
src/jsc/bindings/JSFFIFunction.cppsrc/jsc/bindings/NodeVMSourceTextModule.cppsrc/jsc/bindings/webcore/JSWorker.cppsrc/jsc/bindings/webcore/Worker.cppsrc/runtime/webcore/Blob.rstest/js/node/vm/vm.test.tstest/js/node/worker_threads/worker_threads.test.tstest/js/web/fetch/blob.test.ts
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| expect(stdout.trim()).toBe("done"); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Preserve crash diagnostics from the subprocess.
The test captures stderr but discards it, so an ASAN/JSC crash may surface only as a generic stdout or exit-code mismatch. Include stderr and signal information in the failure outcome while keeping the success assertion focused on "done" and exit code 0.
Based on learnings, worker-thread crash-detection tests should preserve stdout, stderr, exitCode, and signalCode together.
🤖 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/worker_threads/worker_threads.test.ts` around lines 749 - 751,
Update the subprocess assertions around proc.stdout, proc.stderr, and
proc.exited to preserve stdout, stderr, exitCode, and signalCode in failure
diagnostics. Keep the success assertions focused on stdout.trim() equaling
"done" and exitCode equaling 0, while ensuring stderr and signal information are
included when those assertions fail.
Source: Learnings
| test("slice end beyond EOF clamps to the file size", async () => { | ||
| using dir = tempDir("blob-file-slice-eof", { "data.txt": "0123456789" }); | ||
| const s = Bun.file(`${dir}/data.txt`).slice(5, 5000); | ||
| expect(await new Response(s).text()).toBe("56789"); | ||
| const clone = structuredClone(s); | ||
| expect(await clone.text()).toBe("56789"); | ||
| // Serializing resolves the original's size, clamping the window to EOF. | ||
| expect(s.size).toBe(5); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a clone.size assertion to catch the EOF-clamp regression on the receiving side.
This test only checks decoded content, which passes regardless of whether clone.size is correctly clamped (file reads stop at physical EOF either way). Add an explicit size assertion on the clone to actually verify the structured-clone receiver clamps the window to EOF, not just the sender.
✅ Suggested addition
const clone = structuredClone(s);
expect(await clone.text()).toBe("56789");
+ expect(clone.size).toBe(5);
// Serializing resolves the original's size, clamping the window to EOF.
expect(s.size).toBe(5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test("slice end beyond EOF clamps to the file size", async () => { | |
| using dir = tempDir("blob-file-slice-eof", { "data.txt": "0123456789" }); | |
| const s = Bun.file(`${dir}/data.txt`).slice(5, 5000); | |
| expect(await new Response(s).text()).toBe("56789"); | |
| const clone = structuredClone(s); | |
| expect(await clone.text()).toBe("56789"); | |
| // Serializing resolves the original's size, clamping the window to EOF. | |
| expect(s.size).toBe(5); | |
| }); | |
| test("slice end beyond EOF clamps to the file size", async () => { | |
| using dir = tempDir("blob-file-slice-eof", { "data.txt": "0123456789" }); | |
| const s = Bun.file(`${dir}/data.txt`).slice(5, 5000); | |
| expect(await new Response(s).text()).toBe("56789"); | |
| const clone = structuredClone(s); | |
| expect(await clone.text()).toBe("56789"); | |
| expect(clone.size).toBe(5); | |
| // Serializing resolves the original's size, clamping the window to EOF. | |
| expect(s.size).toBe(5); | |
| }); |
🤖 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/web/fetch/blob.test.ts` around lines 681 - 689, Extend the “slice end
beyond EOF clamps to the file size” test after structured cloning to assert that
clone.size equals 5. Keep the existing clone.text() assertion and sender-size
assertion unchanged, using the clone created from s to verify EOF clamping on
the receiving side.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this bundles four independent memory-/thread-safety fixes in native code (JSC Strong lifetime across threads in FFI, cross-thread StringImpl refcounting, null-cell validation, and a Blob serialization format bump) — worth a human look, and the flagged overlap with #32794 on the Blob fix should be reconciled.
Checked: the FFI change reuses the existing postTaskTo(id, betweenLookupAndEnqueue, task) overload the same way Worker::dispatchExit already does; ref/adoptRef balance on both the enqueued and not-enqueued paths.
Checked: window_size is applied to all four File/Bytes arms of resolve_size/resolved_size, and the v4 deserialize path leaves v1–v3 payloads at MAX_SIZE (unknown) as before.
Checked: the link() empty-JSValue guards cover both specifierValue and moduleNativeValue, and rejected attempts leave the module unlinked.
Extended reasoning...
Overview
Four independent native-code fixes: (1) NodeVMSourceTextModule::link() promotes debug-only ASSERTs to release-build validation for holey/mismatched argument arrays, (2) options.name in the Worker constructor and createNodeWorkerThreadsBinding gain .isolatedCopy() so parent and worker threads never share a StringImpl, (3) FFI_Callback_threadsafe_call moves its keep-alive ref inside postTaskTo's found-live window and adopts it in the task so ~FFICallbackFunctionWrapper (with two JSC::Strong members) can never run on a foreign thread, and (4) the File arm of Blob::resolve_size/resolved_size now uses the same slice-window clamp as the Bytes arm, with a serialization format bump (v3→v4) so a sliced Bun.file() keeps its window across structuredClone/postMessage. Tests are added for three of the four; the FFI teardown race is covered only by the existing cc.test.ts suite.
Security risks
None new. Fix 1 closes a user-reachable null-deref segfault via the kNative symbol on vm.SourceTextModule. The Blob v4 deserialize reads an extra u64 only when version >= 4 and the resulting size is later clamped by resolve_size, so a crafted payload can't over-read.
Level of scrutiny
High. Every fix sits in the CLAUDE.md "most-blocked category": cross-thread refcounts, JSC handle lifetime off the JS thread, and a wire-format version bump. The FFI ref/adoptRef ordering and the "failure path is ref-neutral" claim are exactly the kind of reasoning a maintainer should sign off on. The isolatedCopy additions and the window_size extraction are mechanical, but the serialization version bump is a one-way door.
Other factors
The FFI change follows the precise pattern already used in Worker::dispatchExit (same betweenLookupAndEnqueue overload, same rationale comment), which raises confidence. The window_size helper is a straight extraction of the existing Bytes-arm logic. A bot flagged #32794 as a possible duplicate of the Blob fix — a human should decide which lands. Given four unrelated subsystems in one PR, deferring.
3 participants
Four independent fixes, one commit each.
1.
node:vm:SourceTextModule.link()segfaults on holey or mismatched arraysThe native
link(specifiers, moduleNatives, scriptFetcher)binding is reachable from user code (the native handle is stored under an ordinary own symbol), but its length-parity and element-type checks were debug-onlyASSERTs. A hole in either array surfaces as an emptyJSValuefromgetDirectIndex, which passesisCell()with a null cell — sotoWTFString()/dynamicDowncast<NodeVMModule>()performed a member call on a nullJSC::JSCell*(UBSan abort in debug, null-deref segfault in release).Now rejects holes, non-string specifiers, and mismatched lengths with
ERR_INVALID_ARG_TYPE/ERR_INVALID_ARG_VALUEin release builds too.2.
worker_threads:options.nameshares a parent-heapStringImplwith the worker threadoptions.namewas stored from the parent thread withoutisolatedCopy()(unlike its siblingenv/argv/execArgvfields) and later materialized as a worker-heapJSStringforthreadName. Both threads then ref/deref'd one non-atomicStringImplrefcount, and for an atomized name — e.g.fn.nameor a property key — the losing thread's final deref removes the impl from the wrong thread'sAtomStringtable, leaving a dangling entry in the parent's.The name is now
isolatedCopy'd at option parsing and again when building the worker-sidethreadNamebinding, so neither heap shares an impl.3.
bun:ffi: threadsafeJSCallbackdestroysJSC::Stronghandles off the JS thread on teardownFFI_Callback_threadsafe_callcapturedRef { wrapper }in the task it posts to the JS thread, but ignoredpostTaskTo's return value. When the target context was gone or terminating (e.g. the owningWorkerwas terminated while a foreign thread was still invoking the callback), the never-enqueued lambda — and itsRef— was destroyed on the calling thread. If the JS side had already dropped its reference,~FFICallbackFunctionWrapperdestroyed twoJSC::Strongmembers against a dying VM from a foreign thread.The keep-alive ref is now taken inside
postTaskTo's found-live window (betweenLookupAndEnqueue) and released viaadoptRefinside the task, so the failure path is ref-neutral and the wrapper's last deref can only happen on the JS thread.4.
Blob: the File arm ofresolve_size()widens a slicedBun.file()to EOFThe "don't clobber a slice's concrete size" guard added for Bytes-backed stores was never applied to the File arm, which still unconditionally set
size = file_size - offset:slice(a, b)streamed fromato EOF via.stream()/Response(slice).body.Bun.file()HTTP response body advertised the full remaining file asContent-Length(HEAD and GET).structuredClone/postMessage) widened the original blob's size in place.Both
resolve_size()andresolved_size()now share the Bytes arm's clamp (extracted into awindow_sizehelper). Additionally,structuredCloneof a file-backed slice lost the window's end entirely because the serialized record only carried the offset — the blob serialization format is bumped to v4 to carry the blob's (pre-resolve) size; unknown sizes remain deferred to the receiver, and v1–v3 payloads still deserialize.Tests
vm.test.ts.worker_threads.test.ts.JSCallbacksuite incc.test.ts(delivery under GC churn, close-while-enqueued); the failure path itself is a teardown race with no deterministic hook.blob.test.ts(3 of 4 fail on the previous release).