Conversation
|
Updated 10:32 AM PT - Oct 2nd, 2026
✅ @robobun, your commit b0569563b0ef8f2b0802fce1acf39ad7bae09794 passed in 🧪 To try this PR locally: bunx bun-pr 32055That installs a local version of the PR into your bun-32055 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. Walkthrough
ChangesFFI typed-array pointer stability
Suggested reviewers: Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change stabilizes typed-array pointers for the view’s lifetime, and the regression test checks pointer stability after a hot loop. No concrete merge-blocking risk is supported by the reviewed evidence; the change is ready for normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Checked whether #31941 is the same bug before linking it: it isn't provably so, and the evidence leans against. The actual OpenCode guard (packages/tui/src/terminal-win32.ts, The crash signature in #31941 (frame inside an unsymbolized module above One honest caveat: with this PR, the first |
6bbdade to
1571a72
Compare
There was a problem hiding this comment.
I didn't find any issues — the fix is small and mirrors the existing borrowBytesForOffThread/pinArrayBuffer pattern — but since this is a memory-safety change at the FFI/JSC-JIT boundary, it's worth a human sanity check on the typed-array storage-mode assumptions.
Extended reasoning...
Overview
This PR fixes #32054: bun:ffi's ptr(typedArray) could return an address that goes stale once DFG tier-up relocates a FastTypedArray's backing vector. The fix adds a new C++ binding JSC__JSValue__ensureStableTypedArrayVector (bindings.cpp) plus a Rust wrapper (JSValue.rs), and calls it from ptr_() in FFIObject.rs before reading the vector address. A subprocess regression test in ffi.test.js forces early tier-up and asserts the captured address remains valid. Net new logic is ~9 lines; the rest is comments and the test.
Security risks
This is a memory-safety fix (eliminates a dangling pointer / write-to-abandoned-GC-allocation hazard). It does not introduce new attack surface — bun:ffi is already an inherently unsafe, trusted API. No injection/auth/data-exposure concerns. The only new failure mode is an OOM throw if possiblySharedBuffer() fails to allocate, which is the correct behavior.
Level of scrutiny
Medium-high. The diff is tiny and follows an established in-tree pattern (the adjacent pinArrayBuffer / borrowBytesForOffThread helpers in bindings.cpp do the same possiblySharedBuffer() materialization for the same reason, and the PR description explicitly cross-references them). I verified that the only live code path for ptr() is FFI__ptr__put → FFI__ptr__slowpathWrapper → ptr_() (the DOMJIT fastpath in ZigGeneratedCode.cpp is commented out), so the fix covers all current callers. That said, correctness here hinges on JSC-internals assumptions about typed-array storage-mode transitions and JIT watchpoint installation — knowledge I can verify against the comments but not against JSC source itself.
Other factors
No bugs were flagged by the bug-hunting system, no outstanding reviewer comments, no relevant CODEOWNERS entries. The root-cause analysis in the description is unusually thorough and the regression test is deterministic. I'm deferring rather than approving solely because FFI + JIT-internals + raw-pointer lifetime is exactly the kind of change where a human with JSC expertise should confirm the storage-mode reasoning (and note whether the dormant DOMJIT fastpath would need the same treatment if re-enabled).
|
Thanks for the careful read. Deferring to a human on the JSC storage-mode reasoning is fair, so here are the specifics a reviewer would want to confirm, plus a direct answer on the dormant DOMJIT fastpath. Dormant DOMJIT fastpath: good catch, and the answer is yes. Storage-mode assumptions to confirm:
|
|
Status: the diff is green and ready for review/merge. The red CI is entirely unrelated flakes and infra on lanes this change does not touch. This PR only adds a guard inside Failures seen on the CI runs for this branch, none of which execute the changed code path:
I have used my one CI re-roll on this PR. A maintainer re-run should clear the flaky/infra lanes. Happy to rebase again if needed. |
|
Leaving this open while closing the other pre-#35246 On The The test in this PR reproduces it without a native library ( |
## Summary - use `buffer` ABI parameters and pass typed-array owners directly for transient synchronous Bun FFI calls - lower portable `buffer` parameters to Node's stable `pointer` path while still passing owner objects directly - keep `ptr` only for nullable, mixed native-pointer, raw `ArrayBuffer`, callback, and retained-memory cases - stabilize retained text-memory views through `.buffer` before resolving their native address - preserve raw-pointer compatibility while allowing direct buffers in supersample, packed-buffer, matrix, and grayscale APIs - document the Bun 1.3.14 and Bun 1.4+ ownership rules in `AGENTS.md` ## Why A Bun `FastTypedArray` may store its data inline. Calling `ptr(view)` captures that address, but a later first access to `view.buffer` can move the data into separate `ArrayBuffer` storage and leave the native address stale. Passing the owner directly lets the FFI backend borrow the current storage for synchronous calls and allows Bun's `buffer` fast path to keep the owner visible to the JIT. This keeps nullable and true native-pointer parameters on `ptr`, where `buffer` cannot represent the ABI, while avoiding pre-resolved addresses for transient memory. Retained pointers explicitly materialize stable backing storage before `ptr(view)` and keep the view alive for the native lifetime. Node 26.4's Linux optimized `buffer` trampoline delivered a null pointer to a multi-argument audio call in CI. OpenTUI therefore maps its portable `buffer` descriptor to Node `pointer`, whose documented owner-borrowing path passes the same views correctly. Bun continues to receive the `buffer` descriptor. Related Bun investigation: oven-sh/bun#32054 and oven-sh/bun#32055. ## Testing - `bunx bun@1.3.14 test src/tests/ffi-borrowed-pointer-callsites.test.ts` (23 passed) - `bun test src/tests/ffi-borrowed-pointer-callsites.test.ts` on Bun 1.4 canary (23 passed) - `bun run test:js` (5,397 passed, 23 skipped) - `bun run test:js:node` with Node 26.4.0 (4,665 passed, 6 skipped) - `bun run test:dist` - `bun run build:lib` - `bun run fmt:check` - `bun run lint`
## Summary - use `buffer` ABI parameters and pass typed-array owners directly for transient synchronous Bun FFI calls - lower portable `buffer` parameters to Node's stable `pointer` path while still passing owner objects directly - keep `ptr` only for nullable, mixed native-pointer, raw `ArrayBuffer`, callback, and retained-memory cases - stabilize retained text-memory views through `.buffer` before resolving their native address - preserve raw-pointer compatibility while allowing direct buffers in supersample, packed-buffer, matrix, and grayscale APIs - document the Bun 1.3.14 and Bun 1.4+ ownership rules in `AGENTS.md` ## Why A Bun `FastTypedArray` may store its data inline. Calling `ptr(view)` captures that address, but a later first access to `view.buffer` can move the data into separate `ArrayBuffer` storage and leave the native address stale. Passing the owner directly lets the FFI backend borrow the current storage for synchronous calls and allows Bun's `buffer` fast path to keep the owner visible to the JIT. This keeps nullable and true native-pointer parameters on `ptr`, where `buffer` cannot represent the ABI, while avoiding pre-resolved addresses for transient memory. Retained pointers explicitly materialize stable backing storage before `ptr(view)` and keep the view alive for the native lifetime. Node 26.4's Linux optimized `buffer` trampoline delivered a null pointer to a multi-argument audio call in CI. OpenTUI therefore maps its portable `buffer` descriptor to Node `pointer`, whose documented owner-borrowing path passes the same views correctly. Bun continues to receive the `buffer` descriptor. Related Bun investigation: oven-sh/bun#32054 and oven-sh/bun#32055. ## Testing - `bunx bun@1.3.14 test src/tests/ffi-borrowed-pointer-callsites.test.ts` (23 passed) - `bun test src/tests/ffi-borrowed-pointer-callsites.test.ts` on Bun 1.4 canary (23 passed) - `bun run test:js` (5,397 passed, 23 skipped) - `bun run test:js:node` with Node 26.4.0 (4,665 passed, 6 skipped) - `bun run test:dist` - `bun run build:lib` - `bun run fmt:check` - `bun run lint`
|
#44404 carries this guard. It is the same change on top of the helper that #37154 added later ( Branch: https://github.com/oven-sh/bun/tree/robobun/79736f0f/buffer-from-string-in-place (commit 6add1a1 has the guard alone). One thing #44404 found in CI that applies here too: If #44404 merges, this PR is covered. |
…ress ptr(typedArray) returned the raw vector address of the view without forcing the view out of FastTypedArray mode. JSC relocates that vector when the view transitions to wasteful mode, and the engine triggers the transition on its own: DFG tier-up folds the view into compiled code and registers an ArrayBufferView watchpoint, whose installation calls possiblySharedBuffer() (slowDownAndWasteMemory copies the storage into a fresh ArrayBuffer and repoints m_vector). From that moment the captured pointer dangles: native writes land in the abandoned allocation while JS reads the new one. ptr() now forces the one-time fast-to-wasteful transition before reading the address, so the pointer it hands out can never be invalidated by the engine. Other storage modes already have immovable data and are left untouched. Fixes #32054
87174b5 to
51098c6
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I checked whether the new JSC__JSValue__ensureStableTypedArrayVector binding is needed at all: JSValue::materialize_array_buffer_view_buffer (src/jsc/JSValue.rs:2658, backed by Bun__JSValue__materializeArrayBufferViewBuffer at src/jsc/bindings/bindings.cpp:6567, already used by napi for the same pointer-stability purpose) calls possiblySharedBuffer() on any view and returns false on allocation failure, so ptr_ could call that existing helper and the new C++ function, extern decl, and Rust wrapper could be dropped. The only difference is the new helper's FastTypedArray mode check, which skips a no-copy in-place adoption for Oversize views and does not affect pointer stability.
Extended reasoning...
The change adds a one-time fast-to-wasteful transition in bun:ffi's ptr() before the data pointer is captured, plus a new C++ binding, Rust wrapper, and a spawned regression test; it touches no auth, crypto, or input-parsing surface. Inline findings are already posted, and the additional fact that an equivalent helper already exists in-tree is what this body adds.
…tr() validates its input Use JSValue::materialize_array_buffer_view_buffer, which napi already uses for the same pointer-stability reason, instead of a new binding. Run it after the type, length and byteOffset checks, then read the address again. The primitives test held no reference to the Buffer it took a pointer to, so the pointer dangled once the GC collected it. Keep the buffer alive.
Fixes #32054
Problem
ptr(typedArray)on a small view returns an address that goes stale once the caller is DFG-compiled. Native writes land in an abandoned block and JS reads return frozen values:FAIL: 82347/100000 stale typed-array reads.FastTypedArraywhose vector lives in the GC heap. DFG tier-up registers a watchpoint on the view, which callspossiblySharedBuffer(). That copies the vector into a freshArrayBufferand repoints the view.ptr_(src/runtime/ffi/FFIObject.rs) handed out the old address.Fix
ptr_calls the existingJSValue::materialize_array_buffer_view_buffer(napi uses it for the same reason) and reads the address again. The view now owns anArrayBufferthat JSC never moves.test/js/bun/ffi/ffi.test.js, testptr(typedArray) stays valid after DFG tier-up. Main fails it withMOVED: the view's storage was relocated after ptr() was taken. The issue's C repro passes 100000/100000.Background
ArrayBufferobject (wasteful). Only fast storage moves, once, when JSC materializes theArrayBuffer.FFI.h. It reads the vector at call time, so only aptr()address outlives the call.Downsides
ptr()now owns anArrayBuffer, once per view. For a fast view that is one malloc of up to 1000 elements plus one copy. For an oversize view JSC adopts the vector in place: oneArrayBufferobject, no copy, no double GC accounting. JSC does the same on.bufferaccess.ptr()call pays one extraas_array_bufferread (a type switch, no allocation).ptr()address after the view is collected sees freed memory sooner. That is a use-after-free on main too (a churn probe fails 4 of 50 runs there). Theprimitivestest did this and is fixed here.Notes
JSC__JSValue__ensureStableTypedArrayVectorbinding that checkedmode() == FastTypedArraybeforepossiblySharedBuffer(). Review pointed at the existingBun__JSValue__materializeArrayBufferViewBuffer(napi,src/jsc/bindings/bindings.cpp). The only difference is that the existing helper also materializes an oversize view, which adopts the vector in place with no copy.run ffi > primitiveson aarch64:new CString(ptr(Buffer.from([...])), 4, 2)read a freed buffer. TheBufferwas a temporary that nothing held. On main its bytes stay in the GC heap after collection, so the read happened to work. With the fix the bytes live in a malloc'dArrayBufferthat frees at collection. Probe on main (Linux x64, release):ptr(Buffer.from(...)),Bun.gc(true), allocate 2000 small arrays, then read: wrong 4 of 50 runs. The test now keeps the buffer referenced.ptr(view, 9)on an 8 byte view returnsNaNinstead of throwing, on main and here. Not touched.test/js/bun/ffi/ffi.test.js(152 pass, 2 timeouts at 5 s:ptr argument: ArrayBuffer cells through an FTL-compiled call siteandJSCallback tolerates worker.terminate() arriving inside the callback, both also slow on main on this machine and green in CI).Rebase notes
Rebased onto main (faac63e). The change is the same.
src/jsc/JSValue.rs:JSC__JSValue__pinArrayBufferreturnsu8on main. The new extern line sits after it.test/js/bun/ffi/ffi.test.js: the new test sits before thedescribe.skipIf(!FFI_FIXTURE_PATH)("run ffi")block of main.MOVED: the view's storage was relocated after ptr() was taken.ffi.test.jshas 146 pass and 3 timeouts at the 5 s limit on this machine. One of them (ptr argument: ArrayBuffer cells through an FTL-compiled call site) also times out on main. The other two (theJSCallbackworker teardown tests) take 4.2 s and 4.5 s on main.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/ffi/ffi.test.js