Repository navigation
Conversation
MarkedArrayBuffer now stores its ArrayBuffer descriptor behind a Cell and defers pinning: from_js captures only the JSValue, and the first bytes() or slice() call pins the backing JSC::ArrayBuffer, reads vector()/ byteLength() once, and caches the result. Drop releases the pin; Unprotect clears the flag first so a ThreadSafe<T> dropped on the JS thread leaves nothing for an off-thread Drop to do. to_thread_safe() forces the pin+snapshot before handing off. This makes argument-coercion order irrelevant for StringOrBuffer::Buffer without an ad-hoc re-snapshot at each call site: a toString()/valueOf() on a later argument can transfer or resize the buffer, and the first slice() observes the post-coercion state (detached -> empty). The six buffer.buffer = ArrayBuffer::from_typed_array(...) re-derive lines in CryptoHasher/PBKDF2/PasswordObject/scrypt/Bun.sha are removed, as are the manual pin-at-end blocks in fs.write/fs.read args (bytes() now does that on first access). hash_to_bytes and friends take &Buffer instead of a raw ArrayBuffer copy and use bytes() for the writable span.
WalkthroughThis PR reworks ChangesArrayBuffer lifecycle and Buffer consumers
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Covers the lazy-pin guarantee on a call site that has no re-snapshot on main: js_valkey_functions::set converts the key (Buffer) then the value (StringObject -> toString), then serializes via .slice(). On main the key's 19 stale bytes are written to the wire; with lazy pin the first slice() observes the detached buffer and serializes a zero-length key. Also trims the longest new doc comments flagged by comment-cop.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@src/jsc/bindings/bindings.cpp`:
- Around line 3215-3236: Harden JSC__JSValue__arrayBufferLiveBytes in
src/jsc/bindings/bindings.cpp:3215-3236 by guarding non-cells/null cells,
initializing outputs to (nullptr, 0), and using dynamicDowncast for
JSArrayBuffer and JSArrayBufferView, including ObjectType/FinalObjectType cases;
update MarkedArrayBuffer::slice in src/jsc/array_buffer.rs:966-981 to return
cached (ab.ptr, ab.byte_len) when pin_array_buffer() fails and set self.pinned
only after successful pinning.
In `@src/jsc/node_path.rs`:
- Around line 203-204: The repeated Buffer pin/protect and unpin/unprotect
sequences should use shared Buffer helpers. Add the helper methods to the
Buffer/MarkedArrayBuffer API, then replace both directions of the pairing in
src/jsc/node_path.rs at lines 203-204 and 218-219, src/runtime/node/types.rs at
lines 302-303 and 319-320, and src/runtime/node/node_fs.rs at lines 3916-3924;
preserve each existing to_thread_safe and Unprotect flow while routing these
operations through the helpers.
In `@src/runtime/crypto/CryptoHasher.rs`:
- Around line 388-396: Update all five direct Buffer output capacity errors in
src/runtime/crypto/CryptoHasher.rs at lines 388-396, 717-724, 990-997,
1320-1326, and 1483-1489, including the supplied bytes_len, required digest
size, and guidance to provide a larger TypedArray. Preserve each existing error
path while ensuring messages identify the output resource, rejected capacity,
minimum constraint, and remedy.
- Around line 714-727: Update the output-buffer validation in the CryptoHasher
finalization path to derive the required digest length from the active
CryptoHasher variant instead of EVP_MAX_MD_SIZE_USIZE. Reject buffers smaller
than that selected digest length, and create output_digest_slice using the same
required length while preserving the existing error behavior.
- Around line 1327-1329: The digest pointer casts in both sites at
src/runtime/crypto/CryptoHasher.rs lines 1327-1329 and 1490-1492 rely on an
alignment guarantee not enforced by the StaticHasher trait. Seal StaticHasher so
all implementations are constrained to byte-aligned digest types, or replace
both typed mutable casts with &mut [u8] while preserving the existing length and
writable-buffer 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: 72f82b3f-cd49-41fe-8791-e16855bfc84e
📒 Files selected for processing (14)
src/jsc/JSValue.rssrc/jsc/array_buffer.rssrc/jsc/bindings/bindings.cppsrc/jsc/bindings/headers.hsrc/jsc/node_path.rssrc/runtime/api/BunObject.rssrc/runtime/api/MarkdownObject.rssrc/runtime/api/bun/subprocess/Readable.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/crypto/PBKDF2.rssrc/runtime/crypto/PasswordObject.rssrc/runtime/node/node_crypto_binding.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rs
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/valkey/valkey-gc.test.ts`:
- Around line 641-655: Wrap the Redis client connection, key setup, and set
operation in a try/finally block, moving client.close() and server.close() into
finally so both resources are released even when connect() or set() rejects.
Preserve the existing test behavior and failure propagation.
🪄 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: 6b6cf5ed-b6cd-4857-aba1-6cff3845ab63
📒 Files selected for processing (4)
src/jsc/array_buffer.rssrc/jsc/bindings/bindings.cppsrc/runtime/node/node_fs.rstest/js/valkey/valkey-gc.test.ts
💤 Files with no reviewable changes (1)
- src/runtime/node/node_fs.rs
bindings.cpp: use dynamicDowncast and initialize outputs so an
ObjectType/FinalObjectType buffer cell (matched by as_array_buffer's
fallback arm) is handled correctly instead of reaching an
uncheckedDowncast on the wrong shape.
array_buffer.rs: add MarkedArrayBuffer::{to_thread_safe, unprotect}
and route the five pin+protect / unpin+unprotect pairings in
node_path.rs, types.rs, node_fs.rs through them.
test: close the redis client/server in finally.
bytes() now only pins a view that already has a backing ArrayBuffer (hasArrayBuffer()); FastTypedArray and OversizeTypedArray have nothing to transfer() so there is nothing to pin against, and pinning them forces slowDownAndWasteMemory()/createAdopted which shows up in vm.heap.arrayBufferSize() (test-memory-usage.js). to_thread_safe() passes force_pin=true so off-thread borrows still promote. Also: route from_js/from_js_pinned through from_unpinned/from_pinned; add signalCode assertion to the valkey test.
There was a problem hiding this comment.
No bugs found this round, and the earlier nits (dead constructors/accessors, stale SAFETY refs, test consistency) are all addressed as of f714d93. Deferring to a human because this redesigns MarkedArrayBuffer's pinning model — new Drop, Cell<> interior mutability, a new C++ pin-and-read FFI, and changed to_thread_safe/unprotect semantics across the crypto/fs/PathLike hand-off paths — which is more design surface than I should sign off on alone.
What was reviewed: the bytes_() settled/pinned state machine and FORCE_PIN interaction with FastTypedArray/SharedArrayBuffer; Drop↔Unprotect ordering so off-thread ThreadSafe<T> drop is a no-op; that every removed from_typed_array re-snapshot is covered by a bytes()/slice() at the read site; and JSC__JSValue__pinAndReadArrayBufferBytes for non-cell / detached / shared inputs.
Extended reasoning...
Overview
This PR reworks MarkedArrayBuffer (the payload behind StringOrBuffer::Buffer / PathLike::Buffer) from eager to lazy pinning: fields become private Cell<>s, a settled flag caches the first vector()/byteLength() read, bytes()/slice() pin on first access via a new JSC__JSValue__pinAndReadArrayBufferBytes C++ entry point, and a new Drop releases the pin. to_thread_safe()/unprotect() become inherent methods that force-pin (promoting FastTypedArray) before threadpool hand-off. Seven ad-hoc ArrayBuffer::from_typed_array(.., buffer.value) re-snapshot workarounds are removed across CryptoHasher, Bun.sha, PBKDF2, scrypt, PasswordObject, and fs.read/fs.write; the crypto *_to_bytes helpers switch from raw ArrayBuffer to &Buffer + bytes().
Security risks
The change is itself a memory-safety hardening (centralising the "later argument's coercion detaches an earlier argument's buffer" UAF class). The risks are in the redesign: pin/unpin balance across every terminal path (early-return, ThreadSafe<T> drop on the JS thread vs. off-thread, borrow() clones taking their own pin), the new Drop firing on every scope exit including inside arenas or ManuallyDrop wrappers, and the Cell<> model's single-JS-thread assumption holding at every bytes() call site. I traced these and did not find an imbalance, but the blast radius (every StringOrBuffer/PathLike consumer) is large.
Level of scrutiny
High. This is unsafe Rust + C++ FFI touching JSC ArrayBuffer lifetime, GC rooting, and cross-thread hand-off — squarely in REVIEW.md's most-blocked category. The abstraction is used pervasively, so a subtle mistake in the pin lifecycle would surface as a rare UAF far from the change. That warrants a maintainer's eyes on the design (lazy-pin vs. the pin-at-construction approach in the referenced #34966/#35757), not just line-level review.
Other factors
Four prior review rounds surfaced only nits (dead from_pinned/from_js_pinned/accessors, an always-false new() param, a stale SAFETY parenthetical, missing signalCode assertion), all fixed. The existing detach-during-coercion regression suites (bun-cryptohasher, pbkdf2, scrypt, password, fs.test.ts) plus the new redis.set lazy-pin test give reasonable coverage of the removed workarounds. CodeRabbit's concerns were withdrawn or confirmed addressed. Nothing outstanding blocks merge from my side; the deferral is purely about design-level sign-off.
There was a problem hiding this comment.
No bugs found this pass; earlier nits (dead from_pinned/from_js_pinned, always-false new(.., pinned) param, unused accessors, stale SAFETY comment, missing signalCode assertion) are all addressed in ba0290f…f714d93b. Deferring to a human — this reworks MarkedArrayBuffer's pin lifecycle (lazy pin-on-first-bytes(), Cell<> interior mutability, new Drop that calls into JSC, to_thread_safe/unprotect pairing) and removes the per-call-site UAF re-snapshots across crypto/fs/PathLike; that's a core memory-safety contract change worth maintainer eyes.
What was reviewed:
bytes_<FORCE_PIN>state machine:settled/pinnedinteraction, FastTypedArray left unpinned on the sync path but promoted viaforce_pininto_thread_safe(); detached →(null, 0).Drop/Unprotectordering:unprotect()clearspinnedfirst so an off-threadThreadSafe<T>drop is a no-op;PathLike::DropandStringOrBuffer::Dropno longer touch the pin directly.- Removed re-derives (
CryptoHasher,PBKDF2, scrypt,PasswordObject,Bun.sha,fs.read/write) — each now reachesbytes()after the last coercion; the new&Bufferoutput-arg path throws on a detached destination instead of writing through a stale pointer. JSC__JSValue__pinAndReadArrayBufferBytes: non-cell /JSArrayBufferView/JSArrayBuffer/ shared / detached arms; out-params zeroed unconditionally.
Extended reasoning...
Overview
Refactors MarkedArrayBuffer (aliased as Buffer in StringOrBuffer/PathLike) from an eager (ptr, byte_len) snapshot to a lazy pin-and-read on the first bytes()/slice() call. Fields go private behind Cell<>; a new Drop impl releases the pin; to_thread_safe()/unprotect() centralise the threadpool hand-off pairing. A new C++ binding JSC__JSValue__pinAndReadArrayBufferBytes does the pin + vector()/byteLength() read atomically, with a force_pin flag to promote FastTypedArrays for off-thread borrows. ~10 call sites that previously re-derived the ArrayBuffer descriptor after argument coercion (the ad-hoc UAF workaround) are deleted, and the fs Read/Write per-args pinning blocks are removed. 14 files, +344/−274.
Security risks
This is memory-safety infrastructure at the JS↔native boundary — the exact class REVIEW.md flags as most-blocked. The lazy-pin design is more defensive than the old shape (a later argument's toString() detaching an earlier buffer now yields (null, 0) instead of a stale pointer), and the digest-output paths now throw on a too-short/detached destination rather than writing through output_buf.ptr. But the correctness depends on: every consumer reaching bytes() only after the last JS coercion; Drop running only on the JS thread (or unprotect() having cleared pinned first); the settled && (!FORCE_PIN || pinned) short-circuit not double-pinning or skipping a required promotion. I traced these through the changed call sites and they hold, but the invariant is now spread across the type rather than open-coded per site.
Level of scrutiny
High. This changes the ownership/lifetime contract of a type used by every buffer-accepting native API (node:fs, node:crypto, Bun.password, Bun.CryptoHasher, valkey, PathLike, subprocess Readable). It adds a Drop that calls into JSC, introduces interior mutability, and removes safety workarounds from ~8 hot paths on the assumption the new abstraction subsumes them. A maintainer should sign off on the design (lazy vs. eager pin) and the thread-affinity story for Drop.
Other factors
All prior review-round findings (mine and CodeRabbit's) are resolved and the threads marked so. The regression suites listed in the PR description pass, and the new valkey test demonstrates fails-without/passes-with under ASAN. The mechanical struct-literal → named-constructor conversions and the .buffer.value → .value() accessor swaps are straightforward. No outstanding unresolved comments.
|
CI on 0429346: 193/196 lanes green. The one red is |
This branch has not been deployed
2 participants
What
Makes
MarkedArrayBuffer(the payload ofStringOrBuffer::Buffer/PathLike::Buffer) defer pinning its backingJSC::ArrayBufferuntil the firstslice()/bytes()call, then cache that singlevector()/byteLength()read. Until then only theJSValueis meaningful, so a later argument'stoString()/valueOf()/option getter can freelytransfer()or resize the buffer and the first read observes the post-coercion state.Dropreleases the pin;Unprotectclears the flag first so aThreadSafe<T>released on the JS thread leaves nothing for an off-threadDrop.to_thread_safe()forces the pin+snapshot before a threadpool hand-off.Why
The sync crypto/hash paths each patched the same use-after-free with an ad-hoc
buffer.buffer = ArrayBuffer::from_typed_array(global, buffer.buffer.value)re-snapshot after argument coercion. Root cause: theArrayBufferdescriptor captures a raw(ptr, byte_len)at conversion time, and any later JS coercion can transfer or resize the backing store under it.With the lazy pin the per-call-site workaround is no longer needed. Removed:
CryptoHasher::hash(2 sites),Bun.shaPBKDF2::from_js,PasswordObject::verifySync, scrypt syncfrom_jsfs.write/fs.readargs (same effect viabytes()on first access)hash_to_bytes/digest_to_bytes/hash_by_name_inner_to_bytesnow take&Bufferand usebytes()for the writable span instead of reading the rawArrayBuffer.ptrfield, so a detached output buffer throws "TypedArray must be at least N bytes" instead of writing through a stale pointer.Verification
The detach/resize-during-coercion regression tests keep passing unchanged:
No
ArrayBuffer::from_typed_array(.., buffer.buffer.value)re-derive remains:(the one remaining call is an initial construction, not a re-derive)
Related: #34966 and #35757 take the pin-at-construction approach; this defers the pin so a later argument can still detach the buffer, which is what the existing regression tests in
bun-cryptohasher.test.ts/pbkdf2.test.ts/scrypt.test.tsassert.[review] gate passed · iteration 3 · 14 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 3
evidence per changed file