crypto: pin StringOrBuffer inputs on the sync path so a later arg cannot detach them by robobun · Pull Request #34966 · oven-sh/bun

robobun · 2026-07-21T15:40:31Z

Repro

const crypto = require("node:crypto"), keep = [];
const mk = (n, b = 0x41) => { const x = Buffer.from(new ArrayBuffer(n)); x.fill(b); return x; };
const detachRecycle = v => {
  v.buffer.transfer(0); Bun.gc(true);
  for (let i = 0; i < 96; i++) { const x = new Uint8Array(v.byteLength || 1<<16); x.fill(0x5a); keep.push(x); }
  Bun.gc(true);
};
const pw = mk(1 << 16);
const got = crypto.scryptSync(pw, mk(64), 16, { get N() { detachRecycle(pw); return 1024; } }).toString("hex");
const recycled = crypto.scryptSync(mk(1 << 16, 0x5a), mk(64), 16, { N: 1024 }).toString("hex");
console.log("key derived from recycled foreign block:", got === recycled);  // true

The same shape reproduces on:

crypto.pbkdf2Sync(new StringSubclass(pw), saltBuf, ...): PBKDF2::from_js captures the salt slice first, then converts the password; a String-object password's toString() detaches the salt.
crypto.scryptSync(pwBuf, new StringSubclass(salt), ...): Scrypt::from_js captures the password slice first, then converts the salt.
Bun.password.verifySync(pwBuf, new StringSubclass(hash)): the hash's toString() detaches the password, so the comparison runs over freed bytes and returns false for the correct password.

Cause

StringOrBuffer::from_js_maybe_async only pins the backing JSC::ArrayBuffer on the async path. On the sync path it stores a raw (ptr, len) into the backing with no pin, and the caller then converts a later argument whose toString() or property getter can run user JS. That JS can transfer() the captured buffer's backing and recycle the pages, so the KDF / verify reads freed memory. The backing is bmalloc/libpas-owned, so ASAN does not catch the read.

Fix

Pin on the sync Buffer arm too, and release the pin in Drop for StringOrBuffer. While a pin is held, ArrayBuffer::transferTo() copies the bytes and leaves the source attached (see JSC__JSValue__pinArrayBuffer in bindings.cpp), so the captured slice stays valid across any subsequent argument conversion. unprotect() already clears pinned before unpinning, so the async path's accounting is unchanged. This is the same pin-then-Drop pattern PathLike already uses for buffer path arguments.

The will_be_async re-pin block in args::Write::from_js (src/runtime/node/node_fs.rs) and the PinnedView helper in src/runtime/api/MarkdownObject.rs are removed: StringOrBuffer::from_js already returns a pinned Buffer, so both were no-op pin/unpin round-trips. The two comments in fetch.rs/Blob.rs that asserted StringOrBuffer::Drop is a no-op for Buffer are reworded to reference the readFile-owned allocation instead.

Verification

New tests in scrypt.test.ts, pbkdf2.test.ts, and password.test.ts cover all four faces plus a pin-balance check (inputs are detachable again after scryptSync returns). All fail on the released binary and pass with this change; the full test/js/node/crypto and test/js/bun/util/password.test.ts suites pass.

[review] gate passed · iteration 1 · 8 files touched

fails on main (without fix)

ASAN without fix: 4 failed, 8 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/util/password.test.ts "test/js/node/crypto/pbkdf2.test.ts" test/js/node/crypto/scrypt.test.ts
bun test v1.4.0 (8cea940fe)

test/js/bun/util/password.test.ts:
(skip) does not leak > hashSync
(skip) does not leak > hash
(pass) hash > arguments parsing > no blank password allowed [3.91ms]
(pass) hash > arguments parsing > password is required [2.71ms]
(pass) hash > arguments parsing > invalid algorithm throws [18.87ms]
(pass) hash > arguments parsing > coercion throwing doesn't crash [5.38ms]
(pass) hash > arguments parsing > empty Uint8Array throws [2.70ms]
(pass) hash > arguments parsing > empty Uint16Array throws [0.65ms]
(pass) hash > arguments parsing > empty Uint32Array throws [0.49ms]
(pass) hash > arguments parsing > empty Int8Array throws [0.51ms]
(pass) hash > arguments parsing > empty Int16Array throws [0.50ms]
(pass) hash > arguments parsing > empty Int32Array throws [0.47ms]
(pass) hash > arguments parsing > empty Float16Array throws [0.53ms]
(pass) hash > arguments parsing > empty Float32Array throws [0.48ms]
(pa
... (truncated)

release without fix: all passed
bun test v1.4.0-canary.1 (aab95b44d)

test/js/bun/util/password.test.ts:
(pass) does not leak > hashSync [2165.87ms]
(pass) does not leak > hash [921.76ms]
(pass) hash > arguments parsing > no blank password allowed [0.15ms]
(pass) hash > arguments parsing > password is required [0.03ms]
(pass) hash > arguments parsing > invalid algorithm throws [0.20ms]
(pass) hash > arguments parsing > coercion throwing doesn't crash [0.06ms]
(pass) hash > arguments parsing > empty Uint8Array throws [0.03ms]
(pass) hash > arguments parsing > empty Uint16Array throws
(pass) hash > arguments parsing > empty Uint32Array throws
(pass) hash > arguments parsing > empty Int8Array throws
(pass) hash > arguments parsing > empty Int16Array throws
(pass) hash > arguments parsing > empty Int32Array throws
(pass) hash > arguments parsing > empty Float16Array throws
(pass) hash > arguments parsing > empty Float32Array throws
(pass) hash > arguments parsing > empty Float64Array throws
(pass) hash > arguments parsing > empty ArrayBuffer throws
(pass) hash > arguments parsing > no blank password allowed
(pass) hash > arguments parsing > password is required
(pass) hash > arguments parsing > invali
... (truncated)

passes on PR (with fix)

ASAN with fix: 8 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/bun/util/password.test.ts "test/js/node/crypto/pbkdf2.test.ts" test/js/node/crypto/scrypt.test.ts
bun test v1.4.0 (8cea940fe)

test/js/bun/util/password.test.ts:
(skip) does not leak > hashSync
(skip) does not leak > hash
(pass) hash > arguments parsing > no blank password allowed [3.90ms]
(pass) hash > arguments parsing > password is required [2.73ms]
(pass) hash > arguments parsing > invalid algorithm throws [20.18ms]
(pass) hash > arguments parsing > coercion throwing doesn't crash [5.23ms]
(pass) hash > arguments parsing > empty Uint8Array throws [2.73ms]
(pass) hash > arguments parsing > empty Uint16Array throws [0.62ms]
(pass) hash > arguments parsing > empty Uint32Array throws [0.49ms]
(pass) hash > arguments parsing > empty Int8Array throws [0.49ms]
(pass) hash > arguments parsing > empty Int16Array throws [0.48ms]
(pass) hash > arguments parsing > empty Int32Array throws [0.49ms]
(pass) hash > arguments parsing > empty Float16Array throws [0.52ms]
(pass) hash > arguments parsing > empty Float32Array throws [0.49ms]
(pa
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 693ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/6] gen generated_host_exports.rs
generated_host_exports.rs: 91 exports (host=3, lazy=10, generic=78, rust=0); 244 extern-C blocks audited
[1/6] cargo bun_bin → libbun_rust.a (--target x86_64-unknown-linux-gnu)

  nightly-2026-07-20-x86_64-unknown-linux-gnu unchanged - rustc 1.99.0-nightly (9f36de775 2026-07-19)

�[1m�[92m   Compiling�[0m bun_core v0.0.0 (/workspace/bun/src/bun_core)
�[1m�[92m   Compiling�[0m bun_errno v0.0.0 (/workspace/bun/src/errno)
�[1m�[92m   Compiling�[0m bun_ptr v0.0.0 (/workspace/bun/src/ptr)
�[1m�[92m   Compiling�[0m bun_boringssl_sys v0.0.0 (/workspace/bun/src/boringssl_sys)
�[1m�[92m   Compiling�[0m bun_safety v0.0.0 (/workspace/bun/src/safety)
�[1m�[92m   Compiling�[0m bun_zlib_sys v0.0.0 (/workspace/bun/src/zlib_sys)
�[1m�[92m   Compiling�[0m bun_cares_sys v0.0.0 (/workspace/bun/src/cares_sys)
�[1m�[92m   Compiling�[0m bun_zstd v0.0.0 (/workspace/bun/src/zstd)
�[1m�[92m   Compiling�[0m bun_picohttp v0.0.0 (/workspace/bun/src/picohttp)
�[1m�[92m   Compiling�[0m bun_output v
... (truncated)

diff hotspot

src/runtime/api/MarkdownObject.rs  | 52 +++------------------------
 src/runtime/node/node_fs.rs        |  9 -----
 src/runtime/node/types.rs          | 26 +++++++-------
 src/runtime/webcore/Blob.rs        |  4 +--
 src/runtime/webcore/fetch.rs       |  4 +--
 test/js/bun/util/password.test.ts  | 29 +++++++++++++++
 test/js/node/crypto/pbkdf2.test.ts | 30 ++++++++++++++++
 test/js/node/crypto/scrypt.test.ts | 74 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 155 insertions(+), 73 deletions(-)

gate history · 1 passed · 0 rejected · iteration 1

evidence per changed file

file                                reads  edits  tests
src/runtime/api/MarkdownObject.rs       2      3      0
src/runtime/node/node_fs.rs             2      1      0
src/runtime/node/types.rs               6      5      0
src/runtime/webcore/Blob.rs             1      1      0
src/runtime/webcore/fetch.rs            1      1      0
test/js/bun/util/password.test.ts       2      2      0
test/js/node/crypto/pbkdf2.test.ts      2      2      0
test/js/node/crypto/scrypt.test.ts      3      6      0

…not detach them scryptSync/pbkdf2Sync/Bun.password.verifySync capture a raw slice into a Buffer argument, then convert a later argument whose toString() or property getter can run user JS. That JS can transfer() the captured buffer's backing and recycle it, so the KDF runs over freed memory and verifySync compares the wrong bytes. The async Buffer arm already pins the backing (transfer() copies while a pin is held). Pin on the sync arm too and release in Drop, matching PathLike.

robobun · 2026-07-21T15:40:41Z

Reproduced all four faces on stock bun (scryptSync options getter, scryptSync String-object salt, pbkdf2Sync String-object password, Bun.password.verifySync String-object hash). With the fix applied, bun bd test passes all new tests plus the full test/js/node/crypto, test/js/bun/md, and test/js/bun/util/password.test.ts suites.

Follow-up cleanup: the now-redundant async re-pin in args::Write::from_js and the PinnedView helper in Bun.markdown are removed in this PR.

CI: builds 76956 and 76972 are green on every lane that ran except the :darwin: 14 x64 - test-bun lane, which is failing at git checkout on agent macOS-13-x64-1 (unlinkat test/node_modules/.bun/@types+nlcst@2.0.3: directory not empty) on both builds. Remaining failures are known Windows flakes (bun-install-registry, es-module-lexer, test-repl-close, compile-windows-metadata). Ready for review.

robobun · 2026-07-21T15:40:41Z

^{Updated 10:45 AM PT - Jul 21st, 2026}

❌ @robobun, your commit 8cea940 has some failures in Build #76972 (All Failures)

🧪 To try this PR locally:

bunx bun-pr 34966

That installs a local version of the PR into your bun-34966 executable, so you can run:

bun-34966 --bun

github-actions · 2026-07-21T15:44:55Z

Found 1 issue this PR may fix:

SCript Non-Deterministic in 1.3.11 #28607 - Reports scryptSync producing non-deterministic/wrong results in Bun 1.3.11, which matches the use-after-free symptom where buffer backing store is freed mid-call

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #28607

🤖 Generated with Claude Code

coderabbitai · 2026-07-21T15:47:04Z

Walkthrough

Changes

StringOrBuffer now pins JS-backed buffers during conversion and unpins them on drop. Related readFile handling is clarified, async write parsing is simplified, and synchronous crypto regression tests cover buffer retention and release.

Buffer pinning lifecycle

Layer / File(s)	Summary
Pin buffers during conversion and unpin on drop `src/runtime/node/types.rs`	`StringOrBuffer` pins array-buffer-like inputs during both conversion paths and unpins them when the buffer variant is dropped.
Clarify readFile buffer ownership `src/runtime/node/node_fs.rs`, `src/runtime/webcore/Blob.rs`, `src/runtime/webcore/fetch.rs`	Async write parsing no longer performs a separate pinning conversion, and comments clarify explicit cleanup of readFile-owned allocations.
Validate synchronous crypto buffer handling `test/js/node/crypto/scrypt.test.ts`, `test/js/node/crypto/pbkdf2.test.ts`, `test/js/bun/util/password.test.ts`	Regression tests cover call-time byte retention during coercion and garbage collection, correct derivation, and pin release after `scryptSync` returns.

Possibly related PRs

oven-sh/bun#34751: Both changes update StringOrBuffer handling for JS ArrayBuffer-backed data and pinning lifecycle behavior.

Suggested reviewers: alii

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately summarizes the main sync-path pinning fix.
Description check	✅ Passed	The description covers the bug, fix, and verification, but it uses custom sections instead of the template headings.

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

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 5-9: Remove the regression-narrative comments at
test/js/node/crypto/scrypt.test.ts lines 5-9, test/js/node/crypto/pbkdf2.test.ts
lines 167-170, and test/js/bun/util/password.test.ts lines 395-398; leave the
test names and assertions unchanged, and retain only an issue URL if one is
required by the regression-test guideline.

🪄 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: eda4b9fd-2141-4126-9212-d12ab12b1a01

📥 Commits

Reviewing files that changed from the base of the PR and between e550f2c and 2f7c64b.

📒 Files selected for processing (4)

src/runtime/node/types.rs
test/js/bun/util/password.test.ts
test/js/node/crypto/pbkdf2.test.ts
test/js/node/crypto/scrypt.test.ts

StringOrBuffer::from_js now pins on both paths, so the will_be_async re-pin in Write::from_js is a no-op round-trip. The two comments in fetch.rs/Blob.rs that asserted Drop is a no-op for Buffer now reference the owns_buffer allocation instead.

StringOrBuffer::from_js returns a pinned Buffer and releases it on Drop, so PinnedView was a no-op pin/unpin round-trip on every Bun.markdown call with a Buffer input.

robobun · 2026-07-25T17:02:04Z

Superseded by #35757, which consolidates all four sites into one change: StringOrBuffer::from_js now pins ArrayBuffer-backed inputs on the sync path (so every caller that goes through it is covered structurally), plus the two reorders for Bun.indexOfLine and Bun.randomUUIDv5 which snapshot the buffer directly.

robobun · 2026-08-04T16:59:39Z

The crypto/KDF/indexOfLine/verifySync sites this PR targets were fixed on main in #36165 (post-coercion buffer re-snapshot). The remaining sites (Bun.Transpiler, Bun.randomUUIDv5, Bun.RedisClient) are covered by #35757.

robobun · 2026-08-16T18:56:12Z

Closing: superseded by #36165, which landed while this was open.

#36165 fixes the same four entry points (pbkdf2Sync, scryptSync x2, Bun.password.verifySync) by re-reading the ArrayBuffer pointer after every argument has been coerced, so a transfer() from a later argument's getter or toString() is observed as a detached (empty) buffer instead of a dangling slice. Verified on a canary that includes it: all four faces from the report now derive from the empty buffer rather than recycled memory.

The approach here (pin at the StringOrBuffer funnel so transfer() copies instead of detaching) is memory-safe too, but it conflicts with the semantics #36165 chose and tested: its new tests assert salt.byteLength === 0 after the transfer, which a pin would prevent. Rebasing this on top would fail those tests, so rather than pick a different semantic in a rebase, closing in favor of the merged fix.

The two cleanups bundled here (args::Write re-pin, Bun.markdown PinnedView) were only redundant because of the funnel pin, so they go with it. If a funnel-level guard for the remaining sync StringOrBuffer::from_js callers is wanted as defense in depth, it should be a fresh PR that follows #36165's re-read semantics rather than pinning.

github-actions Bot added the claude label Jul 21, 2026

[autofix.ci] apply automated fixes

2f7c64b

coderabbitai Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread test/js/node/crypto/scrypt.test.ts Outdated

test: trim comments before the new detach tests

e75f705

claude Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread src/runtime/node/types.rs

Comment thread test/js/node/crypto/scrypt.test.ts Outdated

Comment thread src/runtime/node/types.rs

coderabbitai Bot mentioned this pull request Jul 21, 2026

Bun.Transpiler: coerce loader before capturing the code buffer #34970

Closed

claude Bot reviewed Jul 21, 2026

View reviewed changes

Comment thread src/runtime/node/types.rs

Comment thread src/runtime/node/types.rs

Comment thread src/runtime/node/types.rs

markdown: drop PinnedView now that StringOrBuffer::from_js pins

8cea940

StringOrBuffer::from_js returns a pinned Buffer and releases it on Drop, so PinnedView was a no-op pin/unpin round-trip on every Bun.markdown call with a Buffer input.

coderabbitai Bot mentioned this pull request Jul 22, 2026

crypto,util: coerce later arguments before capturing input buffers in Bun.* hash/UUID/indexOfLine/verifySync #34964

Open

robobun mentioned this pull request Jul 25, 2026

Coerce later arguments before capturing input buffers in Bun.Transpiler, Bun.randomUUIDv5 and Bun.RedisClient #35757

Open

This was referenced Jul 25, 2026

StringOrBuffer: snapshot resizable ArrayBuffer inputs so a later arg cannot resize(0) through the borrow #35821

Closed

StringOrBuffer: pin the backing ArrayBuffer lazily on first slice() #36568

Open

robobun closed this Aug 16, 2026

Navigation Menu

crypto: pin StringOrBuffer inputs on the sync path so a later arg cannot detach them - #34966

crypto: pin StringOrBuffer inputs on the sync path so a later arg cannot detach them#34966
robobun wants to merge 5 commits into
mainfrom
claude/farm/618118fd/sync-kdf-pin-buffer

robobun commented Jul 21, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 21, 2026 •

edited

Loading

Uh oh!

robobun commented Jul 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 25, 2026

Uh oh!

robobun commented Aug 4, 2026

Uh oh!

robobun commented Aug 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

robobun commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Repro

Cause

Fix

Verification

Uh oh!

robobun commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robobun commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robobun commented Jul 25, 2026

Uh oh!

robobun commented Aug 4, 2026

Uh oh!

robobun commented Aug 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

robobun commented Jul 21, 2026 •

edited

Loading

robobun commented Jul 21, 2026 •

edited

Loading

robobun commented Jul 21, 2026 •

edited

Loading

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading