-
Notifications
You must be signed in to change notification settings - Fork 4.7k
runtime: single-owner reset for transpile_source_code_arena #31119
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -335,4 +335,47 @@ describe.concurrent("require.cache", () => { | |||||||||||||
| isWindows ? 60000 : 30000, | ||||||||||||||
| ); | ||||||||||||||
| }); | ||||||||||||||
|
|
||||||||||||||
| test("synchronous transpile arena retain across many distinct modules", async () => { | ||||||||||||||
| const N = 200; | ||||||||||||||
| const files: Record<string, string> = { | ||||||||||||||
| "entry.cjs": ` | ||||||||||||||
| const N = ${N}; | ||||||||||||||
| let mismatches = 0; | ||||||||||||||
| for (let i = 0; i < N; i++) { | ||||||||||||||
| const m = require("./mod" + i + ".cjs"); | ||||||||||||||
| if (m.id !== i) mismatches++; | ||||||||||||||
| if (m.tag !== "module-number-" + i) mismatches++; | ||||||||||||||
| if (m.values.length !== 8 || m.values[7] !== i * 7) mismatches++; | ||||||||||||||
| if (m.padded.length !== 4096 + String(i).length) mismatches++; | ||||||||||||||
| } | ||||||||||||||
| for (let i = 0; i < N; i++) { | ||||||||||||||
| const m = require("./mod" + i + ".cjs"); | ||||||||||||||
| if (m.tag !== "module-number-" + i) mismatches++; | ||||||||||||||
| } | ||||||||||||||
| console.log(JSON.stringify({ mismatches, rss: process.memoryUsage.rss() })); | ||||||||||||||
| `, | ||||||||||||||
| }; | ||||||||||||||
| for (let i = 0; i < N; i++) { | ||||||||||||||
| files[`mod${i}.cjs`] = | ||||||||||||||
| `// ${Buffer.alloc(2048, 120).toString()}\n` + | ||||||||||||||
| `exports.id = ${i};\n` + | ||||||||||||||
| `exports.tag = "module-number-" + ${i};\n` + | ||||||||||||||
| `exports.values = [${Array.from({ length: 8 }, (_, k) => i * k).join(", ")}];\n` + | ||||||||||||||
| `exports.padded = ${JSON.stringify(Buffer.alloc(4096, 46).toString() + i)};\n`; | ||||||||||||||
| } | ||||||||||||||
| const dir = tempDirWithFiles("transpile-arena-retain", files); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win Use On Line 378, please switch to As per coding guidelines: “for multi-file tests, create a temporary directory using 🤖 Prompt for AI Agents |
||||||||||||||
|
|
||||||||||||||
| await using proc = Bun.spawn({ | ||||||||||||||
| cmd: [bunExe(), "run", join(dir, "entry.cjs")], | ||||||||||||||
| env: { ...bunEnv, BUN_RUNTIME_TRANSPILER_CACHE_PATH: "0" }, | ||||||||||||||
| stdout: "pipe", | ||||||||||||||
| stderr: "pipe", | ||||||||||||||
| }); | ||||||||||||||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||||||||||||||
| expect(stderr).toBe(""); | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Filter known ASAN startup noise before asserting empty stderr. On Line 387, Suggested patch- expect(stderr).toBe("");
+ const stderrLines = stderr
+ .split(/\r?\n/)
+ .filter(line => line.length > 0)
+ .filter(line => !line.startsWith("WARNING: ASAN interferes"));
+ expect(stderrLines).toEqual([]);Based on learnings: when spawning subprocesses with 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| const out = JSON.parse(stdout.trim()); | ||||||||||||||
| expect(out.mismatches).toBe(0); | ||||||||||||||
| expect(exitCode).toBe(0); | ||||||||||||||
| }); | ||||||||||||||
| }); | ||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 When the slot is occupied, this drops the current frame's arena — but in the re-entrant + outer-ParseError case, that arena owns the source bytes the log spans point into, and
process_fetch_loghasn't run yet. The old!give_backbranch parked the arena unconditionally (overwriting any occupant) precisely to keep those bytes alive; withgive_back_arenaremoved, the outer arena is nowmi_heap_destroy'd here andtranspile_filereads freed memory at :4290. Fix: when the slot is occupied, swap (parkself.arena, drop the previous occupant) so the current frame's arena always survives for the caller'sArenaResetGuard.Extended reasoning...
What the bug is
ActiveTranspilerArena::dropparksself.arenaback intotranspile_source_code_arenaonly whenslot.is_none(); otherwise it lets theBoxdrop, which isMimallocArena::drop→mi_heap_destroy. The new comment at jsc_hooks.rs:2125-2134 claims this is safe because "the caller'sArenaResetGuard… runs afterprocess_fetch_log, so log spans pointing into arena-owned source bytes stay valid on the ParseError path" — but that statement only holds when the arena was actually parked. In the re-entrant case the slot is occupied, so the arena is destroyed instead, and the caller'sArenaResetGuardresets a different arena.This is a regression introduced by removing the
give_back_arenaflag. Compare the three versions:give_back_arena = false(set at :293 and :339) makes the entiredeferblock a no-op — the arena is neither parked nordeinit()'d. It is intentionally leaked so its bytes outliveprocessFetchLog. The:161-163destroy branch the new comment cites is only reachable whengive_back_arena == true.!give_back, did*slot = Some(arena); return;unconditionally — overwriting any occupant. The deleted comment explicitly documented why: "reads log entries whose spans point into arena-owned source bytes. Freeing here would be a use-after-free."give_backflag; the sameif slot.is_none() { park } else { drop }logic now applies to all paths including ParseError. This matches neither the spec nor the old port on the parse-error path.Code path that triggers it
transpile_source_code_inneris re-entrant — the spec says so explicitly at ModuleLoader.zig:130 ("This code is potentially re-entrant"). The concrete trigger is macro evaluation during parse:parse_maybe_return_file_only(jsc_hooks.rs:2468) → parser visits a macro call →MacroContext::call→vm.load_macro_entry_point→JSModuleLoader::load_and_evaluate_module→ JSC →Bun__transpileFile→transpile_file(creates anArenaResetGuardat jsc_hooks.rs:4210) →transpile_source_code_inneragain on the same VM/thread.The source bytes that log spans point into do live in this arena:
transpile_source_code_innercallsparse_maybe_return_file_only::<false>→parse_maybe_return_file_only_allow_shared_buffer::<_, false>(transpiler.rs:1390), soUSE_SHARED_BUFFER=falseand the file is read viaread_file_with_allocator(..., Some(arena))(transpiler.rs:1502-1508). The PORT NOTE at transpiler.rs:1528-1533 confirms: "bytes live … in this_parse.arena … bulk-freed by mi_heap_destroy when the per-call arena is recycled".Why existing code doesn't prevent it
The new comment at the
elsebranch says "drop the fresh Box (spec :161-163)", assuming the dropped Box is always the inner re-entrant frame's freshly-allocated arena. But re-entry is stack-shaped, and the inner frame returns first — it parks its fresh arena (slot wasNoneat that point) and itsArenaResetGuardresets it in place viaas_mut()(ModuleLoader.rs:68-75), leavingslot = Some(arena_Y). When the outer frame later returns with a ParseError, itsDropis the one that sees the occupied slot — and the Box it drops isarena_X, the outer frame's source-bearing arena, not a "fresh" one.Step-by-step proof
transpile_filecreates_reset_arena_0: ArenaResetGuard(jsc_hooks.rs:4210), callstranspile_source_code_inner. Ittake()sarena_Xfrom the slot (slot →None), buildsarena_guard_0 = ActiveTranspilerArena { arena: Some(arena_X) }, and reads the file intoarena_X.import()s a module →Bun__transpileFile→ level 1transpile_filecreates_reset_arena_1, callstranspile_source_code_inner. Slot isNone, so it allocates fresharena_Yand buildsarena_guard_1.arena_guard_1::dropseesslot.is_none()→ parksarena_Y(slot →Some(arena_Y))._reset_arena_1::dropcallsreset_arena, which doesslot.as_mut().reset_retain_with_limit(...)— slot staysSome(arena_Y).log.errors > 0at jsc_hooks.rs:2546) →return Err(err!("ParseError")).arena_guard_0::dropseesslot = Some(arena_Y), so theif slot.is_none()branch is false →arena_Xfalls through and isBox::drop'd →mi_heap_destroy(heap_X).transpile_file, theErrarm callsprocess_fetch_log(..., &mut log, ...)at jsc_hooks.rs:4290. This walkslog.msgsand callsBuildMessage::createper msg (VirtualMachine.rs:2553-2585), readingLocationline-text spans that point intoarena_X's now-destroyed heap. UAF._reset_arena_0::dropthen resetsarena_Y— the wrong arena.Impact
Use-after-free on the JS thread when a module that uses a bundle-time macro (or otherwise re-enters the transpiler during parse) subsequently fails to parse. The freed bytes are read to build the user-visible error message, so this would typically manifest as garbage in the diagnostic location/line-text or a crash, depending on whether mimalloc has unmapped the page. The trigger is narrow (macro re-entry + outer parse error) but reachable from user code without any special flags.
Fix
Restore the old
!give_backinvariant that the current frame's arena always survives for the caller'sprocess_fetch_log+ArenaResetGuard. The minimal change is to swap rather than drop when the slot is occupied:This preserves the PR's single-owner-reset goal (still no reset here) while matching the old port's parse-error semantics.