Conversation
transformSync(), scan(), and scanImports() called StringOrBuffer::from_js on the code argument (caching the ArrayBuffer's ptr/len) before calling loader_from_js on the loader argument. loader_from_js accepts String objects (is_string() is is_string_like()) and runs their toString(), so a hostile String-object loader could transfer() the code ArrayBuffer inside toString(), free its backing store, and leave the parser reading whatever heap block replaced it. The emitted output / import list is then derived from foreign heap contents. Move the loader coercion ahead of the code-buffer capture in all three functions. Async transform() was already safe: it copies the code bytes into an owned slice before coercing the loader.
|
Status Reproduced with Passes with Existing transpiler suites ( Review nits addressed in ba8b645 (trimmed header comment, combined-object assertion). |
WalkthroughChangesBun.Transpiler now coerces loaders before extracting code buffers in Transpiler loader lifetime handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:16 AM PT - Jul 21st, 2026
✅ @robobun, your commit ba8b645a2206fdcd4e9b0df9399165130e301087 passed in 🧪 To try this PR locally: bunx bun-pr 34970That installs a local version of the PR into your bun-34970 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the two inline style nits, I verified the reordering itself: code_arg is captured as a stack-held JSValue before args.eat() in all three functions, so reading it after loader_from_js runs is sound; the third-arg (js_ctx_value) path in transformSync only calls .is_object() (no user code) so it can't re-detach the buffer post-capture; and relative drop order of arena / code_holder / _restore is preserved. Deferring to a human because this is a native UAF fix.
Extended reasoning...
The reordering is mechanically correct and the test demonstrably fails on system bun / passes on the debug build. The only observable behavior change is that an invalid loader now throws before an invalid code type, which is the expected trade-off. Deferring because native memory-safety fixes in src/runtime/api/ warrant a maintainer sign-off even when the diff is a pure statement reorder.
There was a problem hiding this comment.
Both earlier nits are addressed (2-line header, combined-object assertion) and the reorder looks correct to me — loader_from_js now runs before StringOrBuffer::from_js in all three sync paths, and argument iteration order is preserved. Deferring to a maintainer only because this is a native memory-safety fix touching raw-pointer lifetime invariants in JSTranspiler.rs.
What was reviewed
- Verified
args.eat()placement keeps arg indices identical inscan/transform_sync/scan_importsafter the reorder. - Checked no remaining user-code entry point sits between
StringOrBuffer::from_js(code)and parse (arg-2 ctx read intransform_syncisis_object()only, no coercion). - Confirmed async
transform()copies bytes into an owned slice beforeloader_from_js, so it was already safe as the PR states.
Extended reasoning...
Overview
Reorders three host functions in src/runtime/api/JSTranspiler.rs (scan, transform_sync, scan_imports) so that loader_from_js(global, arg) — which can invoke a user-supplied toString() on a String object — runs before StringOrBuffer::from_js(global, code_arg) captures the code ArrayBuffer's ptr/len. Adds a spawned-subprocess regression test that heap-sprays after detaching the buffer inside toString() and asserts the transpiler sees a zero-length input rather than recycled heap.
Security risks
The PR closes a use-after-free where the parser reads from a freed/recycled ArrayBuffer backing store. The fix itself introduces no new unsafe blocks, allocations, or control flow — it is a pure statement reorder plus relocated args.eat() calls. I traced argument iteration in all three functions and the indices consumed are unchanged. In transform_sync, arena creation moved from before to after loader coercion, which is inert (loader_from_js doesn't touch the arena). After the reorder, the only code between buffer capture and parse in transform_sync is the arg-2 ctx read, which calls is_object() without coercion, so no user JS can run in that window.
Level of scrutiny
High. JSTranspiler.rs is native code dense with unsafe, detach_lifetime_ref, and RAII guards over raw pointers into stack-local arenas. REVIEW.md flags memory safety as the most-blocked category, and the specific rule this fix implements ("do all coercions first while holding no raw pointers") is one where a maintainer confirming completeness — that no other coercion site was missed — is worth the extra look. The change is mechanically small, but the invariants it interacts with are not.
Other factors
Both nits from the prior review pass were addressed in ba8b645 and the threads are resolved. The test was verified to fail with USE_SYSTEM_BUN=1 and pass on the debug build, and existing transpiler suites pass unchanged. The bug-hunting system found no issues on the current revision. I'm not approving solely because native memory-safety changes in this repo warrant a human sign-off; the diff itself reads as correct.
|
Superseded by #35757, which consolidates all four sites into one change: |
|
Closing: #35757 carries this same Transpiler fix and its test, together with the randomUUIDv5 and RedisClient sites. It was rebased onto main today and is mergeable. |
1 participant
What does this PR do?
transformSync(),scan(), andscanImports()read thecodeargument's backing bytes before coercing theloaderargument.JSValue::is_string()isis_string_like(), so aStringobject passes the check andloader_from_jscallstoString()on it. A hostiletoString()cantransfer()the code ArrayBuffer, free its backing store, and have the allocator recycle the block before the parser reads from the stale pointer. The result is the transpiler emitting / scanning whatever foreign heap block now lives at that address.Before:
After: empty output (the buffer is detached before its bytes are read, so the parser sees a zero-length input).
The fix moves the
loader_from_jscall ahead ofStringOrBuffer::from_js(code)in all three functions, so any usertoString()runs before the code buffer's ptr/len are captured. Asynctransform()was already safe because it copies the bytes into an owned slice before coercing the loader.Related: #34966 pins the backing buffer inside
StringOrBuffer::from_json the sync path, which closes the same class of bug at a lower layer for thenode:cryptocall sites. The two changes touch different files and are complementary.How did you verify your code works?
test/js/bun/transpiler/transpiler-loader-uaf.test.tsspawns a subprocess that exercisestransformSync,scan, andscanImportswith a hostile String-object loader. It fails on the released binary (USE_SYSTEM_BUN=1 bun test ...emitstransformSync emitted recycled heap: "import\"recycled-mod\";...") and passes on the debug build.test/js/bun/transpiler/andtest/bundler/transpiler/transpiler.test.jspass unchanged.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file