StringOrBuffer: snapshot resizable ArrayBuffer inputs so a later arg cannot resize(0) through the borrow - #35821
StringOrBuffer: snapshot resizable ArrayBuffer inputs so a later arg cannot resize(0) through the borrow#35821robobun wants to merge 5 commits into
Conversation
…cannot resize(0) through the borrow ArrayBuffer.prototype.resize(0) mprotects trimmed pages PROT_NONE while a borrowed (ptr, len) still spans them; pin() only guards transfer(). A later argument's getter/toString (sync) or the JS thread (async) can therefore SIGSEGV any StringOrBuffer::from_js caller that reads the bytes after more user JS runs. scryptSync/pbkdf2Sync/etc. all hit this. Snapshot resizable non-shared inputs into an owned Buffer at capture time so variant dispatch (CryptoHasher output, fs.write) stays intact. Growable SharedArrayBuffer is left borrowed: it only grows in-place within the reserved max so reading the captured extent stays valid. - StringOrBuffer::Drop now destroys owned buffers (idempotent; to_js clears owns_buffer after transferring to JSC) - MarkedArrayBuffer::live_array_buffer re-reads the caller's live buffer for the five CryptoHasher digest-into-buffer sites - skip the fs.write async re-pin and the Markdown PinnedView re-borrow when the input is already an owned snapshot
WalkthroughChangesThe PR updates Resizable ArrayBuffer lifetime handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:34 PM PT - Jul 25th, 2026
❌ @robobun, your commit 7548050 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35821That installs a local version of the PR into your bun-35821 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I traced the new Drop for StringOrBuffer → buffer.destroy() through the other owns_buffer=true construction sites this could affect: the readFile/mkdtemp result paths (node_fs.rs:4727, node_fs.rs:6987) reach to_js with value == ZERO, so to_node_buffer adopts and the new owns_buffer = false prevents a double-free; the args::Write async re-pin at node_fs.rs:3894 correctly skips owned snapshots (without the guard, the assignment would drop the snapshot and re-borrow the resizable original); and the NodeHTTPResponse large-write path still hits its pre-existing resizable && !shared spill guard and reads from the owned snapshot before Drop frees it. live_array_buffer re-reads the caller's live view before any user JS runs between capture and the length check, so a shrunk output buffer is caught by the existing bytes_len < DIGEST guard rather than written past.
Extended reasoning...
The inline nit already covers the only concrete issue found. This note records the additional Drop-semantics audit so a later pass doesn't re-derive it.
…NodeHTTPResponse escape hatch, md/fs coverage - snapshot_resizable is #[cold] and out-of-line - ArrayBuffer::from_bytes / from_owned_bytes no longer round-trip through u32::try_from; len/byte_len are usize and a 4 GiB resizable input would otherwise panic before the caller's length validator ran - array_buffer_into takes snapshot_volatile so from_js_with_encoding_into (used only by NodeHTTPResponse::write_or_end, which resolves encoding/ callback before capturing and has its own resizable-tail spill) can opt out of the upfront copy - Drop now frees owned Buffer payloads, so the explicit destroy() after readFile in fetch.rs / Blob.rs is redundant - add fs.write async and Bun.markdown.html resizable-resize(0) coverage
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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: 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/crypto/scrypt.test.ts`:
- Around line 80-84: Remove the implementation-history comments from the
regression tests at test/js/node/crypto/scrypt.test.ts:80-84,
test/js/bun/md/md-render-callback.test.ts:432-435, and
test/js/node/fs/fs.test.ts:5644-5648, preserving only a confirmed issue URL
where applicable. Keep the existing test names and assertions unchanged.
🪄 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: 581e3a72-c57a-4e8d-b8f8-2659874b67cc
📒 Files selected for processing (10)
src/jsc/array_buffer.rssrc/runtime/api/MarkdownObject.rssrc/runtime/crypto/CryptoHasher.rssrc/runtime/node/node_fs.rssrc/runtime/node/types.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/fetch.rstest/js/bun/md/md-render-callback.test.tstest/js/node/crypto/scrypt.test.tstest/js/node/fs/fs.test.ts
There was a problem hiding this comment.
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/crypto/scrypt.test.ts (1)
147-158: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAssert the subprocess result before parsing stdout.
If the child crashes before writing JSON,
JSON.parse(stdout.trim())masks the native crash with a generic parse error. Assert the combined subprocess result first, includingstdout,stderr, andexitCode, then parse stdout only after confirming a successful exit.As per coding guidelines, crash-prone subprocess tests should preserve failure diagnostics rather than parse potentially empty stdout first. Based on learnings, crash/abort tests should assert combined subprocess details before interpreting stdout.
🤖 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/crypto/scrypt.test.ts` around lines 147 - 158, Update the subprocess assertions around proc.stdout, proc.stderr, and proc.exited to validate the combined result first, including stdout, stderr, and exitCode, and require a successful exit before parsing output. Keep the existing JSON expectations for syncPw, syncSalt, asyncPw, syncEmpty, sab, and hashOutput, but move JSON.parse(stdout.trim()) after the subprocess success assertion so crash diagnostics are preserved.Sources: Coding guidelines, Learnings
🤖 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.
Outside diff comments:
In `@test/js/node/crypto/scrypt.test.ts`:
- Around line 147-158: Update the subprocess assertions around proc.stdout,
proc.stderr, and proc.exited to validate the combined result first, including
stdout, stderr, and exitCode, and require a successful exit before parsing
output. Keep the existing JSON expectations for syncPw, syncSalt, asyncPw,
syncEmpty, sab, and hashOutput, but move JSON.parse(stdout.trim()) after the
subprocess success assertion so crash diagnostics are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a1cad940-bdeb-4dc7-bdfa-3057aa7b9c60
📒 Files selected for processing (3)
test/js/bun/md/md-render-callback.test.tstest/js/node/crypto/scrypt.test.tstest/js/node/fs/fs.test.ts
💤 Files with no reviewable changes (2)
- test/js/node/fs/fs.test.ts
- test/js/bun/md/md-render-callback.test.ts
There was a problem hiding this comment.
Beyond the inline note on PathLike, I also checked: the new Drop on StringOrBuffer::Buffer vs. to_js → to_node_buffer — owns_buffer is cleared after the ownership hand-off to JSC, so no double-free; and *out = Self::Buffer(...) in array_buffer_into — the previous value's Drop runs on assignment, so no leak of a prior owned snapshot.
Extended reasoning...
The PathLike finding is pre-existing and non-blocking. The two additional concerns I traced were the ones a Drop-semantics change on a shared type most obviously invites: (1) to_node_buffer calls JSValue::create_buffer, which installs MarkedArrayBuffer_deallocator and transfers ownership to JSC; the PR sets owns_buffer = false immediately after, so the subsequent Drop → destroy() is a no-op. (2) array_buffer_into writes *out by assignment, which runs the old value's Drop, so an owned snapshot already in out is released before the new one lands. Not approving because the ownership model change (owned snapshot + live value, live_array_buffer contract, snapshot_volatile opt-out) touches enough callers that a human should look.
|
CI status: the diff is green on every lane that ran the new tests. Build 81845 passed all debian/ubuntu/alpine/windows test-bun shards that got a runner; the three new tests ( Remaining red is unrelated to this diff:
Ready for review. |
1 participant
Repro
Any
StringOrBuffer::from_js*caller that captures a buffer and then evaluates a later argument (options getter,toString()on a boxed string,valueOf()on a numeric) is affected, on both the sync and async paths:crypto.scryptSync/scrypt,crypto.pbkdf2Sync/pbkdf2,Bun.password.*,Bun.zstdCompress/Decompress,Bun.Transpiler.transform,Bun.Markdown.*,fs.writeFile, and theBun.*CryptoHasher input paths.Cause
StringOrBuffer::from_js_maybe_async_intostores(ptr, len)into the caller's backing store. On the async path it pins; on the sync path it does not. Either way,pin()only clearsisDetachable()(sotransfer()/structuredClone/postMessagecopy instead of detaching). It does not guardArrayBuffer.prototype.resize(): JSC answers a shrink on a resizable buffer by mprotecting the trimmed pagesPROT_NONE. The captured slice still spans those pages, so the nextslice()read faults. The codebase already documents this interaction at theNodeHTTPResponselarge-write path.Node.js copies the password/salt bytes before handing them to the threadpool and does not crash.
Fix
When the input is backed by a resizable non-shared
ArrayBuffer, snapshot the bytes into an ownedMarkedArrayBufferat capture time (sync and async). The result stays aStringOrBuffer::Bufferso callers that dispatch on the variant (fs.write(fd, buffer, offset, length),CryptoHasherdigest-into-buffer) keep working;buffer.valuekeeps pointing at the caller's JS value soprotect()/unprotect()and return-the-input paths are unchanged. GrowableSharedArrayBufferis left borrowed: it can only grow in-place within the reserved max, so reading the captured extent stays valid.Supporting changes:
Drop for StringOrBuffernow releases an owned buffer.destroy()is idempotent andStringOrBuffer::to_jsclearsowns_bufferafter transferring the allocation to JSC, so the existing explicit.destroy()callers (fetch.rs,Blob.rs) and the readFile/mkdtemp result paths are unaffected.MarkedArrayBuffer::live_array_bufferre-reads the caller's live view fromvaluewhenselfis an owned snapshot; the fiveCryptoHasherdigest-into-buffer sites use it so a resizable output buffer still gets written into (not the private copy).args::Write::from_jsskips the async re-pin when the input is already an owned snapshot, andMarkdownObject::PinnedViewskips re-borrowing the original when theStringOrBufferalready owns its bytes.Fixed-length buffers (the common case) stay on the existing zero-copy / pin path.
Verification
test/js/node/crypto/scrypt.test.tsspawns a subprocess that exercises sync password, sync salt, async password, zero-length resizable, and growableSharedArrayBufferinputs, plus a regression guard thatBun.SHA256.hash(input, resizableOut)still writes into the caller's buffer. The subprocess segfaults on stockbunand passes with this change.test/js/node/crypto/,test/js/bun/util/bun-cryptohasher.test.ts,test/js/bun/util/password.test.ts, and thefs.writedispatch tests pass.PathLike::from_js_with_allocatorhas the same hole for buffer-typed paths and is intentionally left out here: itsDroplives inbun_jsc::node_pathand does not currentlydestroy(), so the snapshot needs that crate updated too; #32189 covers the async side.VectorArrayBuffer::from_js(fs.writev/fs.readv) has it too and is likewise left out: its iovec capture lives inBun__JSArray__collectBufferSpans(C++) and the type has no owned-buffer tracking; #34758 covers it. Related: #35757 and #34966 pin on the sync path fortransfer(); #34751 and #31645 snapshot on the async path only. This change covers theresize()hole on both paths without changing the variant the funnel returns.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.test.ts