From 0f5ad405850444accf039531e63730c9a5a38a0c Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 23 May 2026 13:33:35 +0000 Subject: [PATCH 1/2] Keep BuildMessage wrappers GC-visible while building module error AggregateErrors process_fetch_log and Log::toJS accumulated the BuildMessage/ResolveMessage JS wrappers for a multi-error AggregateError in a heap Vec. The conservative GC scan only covers the native stack, so a GC triggered while creating the remaining wrappers could finalize the earlier ones and free their native BuildMessage before the AggregateError was constructed. The dead wrappers then ended up in the errors array and printing the unhandled rejection read the freed allocation (build_error.logged). Use the on-stack [JSValue; 256] array (same idiom as log_to_js in ast_jsc) so every wrapper stays reachable until the AggregateError owns it. --- src/jsc/VirtualMachine.rs | 20 +++++++-------- src/jsc/lib.rs | 12 ++++++--- test/js/bun/resolve/build-error.test.ts | 33 ++++++++++++++++++++++++- 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 8fd5af890e3f..632dc17495a9 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -2574,14 +2574,14 @@ pub fn process_fetch_log( } _ => { - // Spec caps at 256 (`var errors_stack: [256]JSValue`). PERF(port): - // was inline switch — Zig stack-allocated; we heap-allocate the - // exact `len` since `JSValue` is a thin u64 and 256 * 8 B = 2 KiB - // is fine either way, but `Vec` avoids the uninit-array dance. - let len = log.msgs.len().min(256); - let mut errors: alloc::vec::Vec = alloc::vec::Vec::with_capacity(len); - for msg in log.msgs.drain(..len) { - let v = match msg.metadata { + // Spec caps at 256 (`var errors_stack: [256]JSValue`). On-stack + // array: conservative GC stack scan keeps these JSValues alive + // (see PORTING.md §JSC) — creating later messages can trigger a + // GC, and a heap `Vec` is invisible to the scan. + let mut errors_stack: [JSValue; 256] = [JSValue::default(); 256]; + let len = log.msgs.len().min(errors_stack.len()); + for (i, msg) in log.msgs.drain(..len).enumerate() { + errors_stack[i] = match msg.metadata { bun_ast::Metadata::Build => take(BuildMessage::create(global_this, msg)), bun_ast::Metadata::Resolve(_) => take(ResolveMessage::create( global_this, @@ -2589,8 +2589,8 @@ pub fn process_fetch_log( referrer_utf8.slice(), )), }; - errors.push(v); } + let errors = &errors_stack[..len]; // C++ `Zig::toString` does `createWithoutCopying`, so the buffer // must outlive the AggregateError. Mark it global so JSC adopts it @@ -2604,7 +2604,7 @@ pub fn process_fetch_log( message.mark_global(); *ret = ErrorableResolvedSource::err( err, - take(global_this.create_aggregate_error(&errors, &message)), + take(global_this.create_aggregate_error(errors, &message)), ); } } diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index a23f5a105ecd..03e1f5653ebd 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -2143,12 +2143,16 @@ impl LogJsc for bun_ast::Log { 0 => Ok(JSValue::UNDEFINED), 1 => msg_to_js(&msgs[0], global), _ => { - let mut errors_stack: Vec = Vec::with_capacity(count); - for msg in &msgs[0..count] { - errors_stack.push(msg_to_js(msg, global)?); + // On-stack array: conservative GC stack scan keeps these + // JSValues alive (see PORTING.md §JSC) — creating later + // messages can trigger a GC, and a heap `Vec` is invisible + // to the scan. + let mut errors_stack: [JSValue; 256] = [JSValue::default(); 256]; + for (i, msg) in msgs[0..count].iter().enumerate() { + errors_stack[i] = msg_to_js(msg, global)?; } let out = bun_core::ZigString::init(message.as_bytes()); - global.create_aggregate_error(&errors_stack, &out) + global.create_aggregate_error(&errors_stack[..count], &out) } } } diff --git a/test/js/bun/resolve/build-error.test.ts b/test/js/bun/resolve/build-error.test.ts index a623aa505e9f..1784b4ab00f3 100644 --- a/test/js/bun/resolve/build-error.test.ts +++ b/test/js/bun/resolve/build-error.test.ts @@ -1,4 +1,4 @@ -import { tempDir } from "harness"; +import { bunEnv, bunExe, tempDir } from "harness"; import { join } from "node:path"; test("BuildError is modifiable", async () => { @@ -19,6 +19,37 @@ test("BuildError is modifiable", async () => { expect(error!.message).not.toBe(message); }); +test("importing a module with many build errors does not crash while reporting them", async () => { + // The AggregateError for a failed module build is assembled from one + // BuildMessage wrapper per log message. Those wrappers used to be collected + // only in a heap Vec (invisible to the conservative GC scan), so a GC during + // the loop could finalize earlier wrappers and free their native + // BuildMessage before the AggregateError was created, causing a + // use-after-free when the unhandled rejection was printed. + using dir = tempDir("build-error-many", { + // 40 declarations + 80 redeclarations -> ~80 build errors in one module + "bad.js": Array.from({ length: 120 }, (_, i) => `const x${i % 40} = 1;`).join("\n"), + "index.js": `import("./bad.js");`, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "index.js"], + cwd: String(dir), + env: { ...bunEnv, BUN_JSC_collectContinuously: "1" }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stderr, exitCode] = await Promise.all([proc.stderr.text(), proc.exited]); + + // Every error in the AggregateError should have been printed. + expect(stderr).toContain('"x0" has already been declared'); + expect(stderr).toContain('"x39" has already been declared'); + expect(stderr).not.toContain("AddressSanitizer"); + // Unhandled rejection -> clean exit with code 1, not a crash. + expect(exitCode).toBe(1); +}); + test("BuildMessage finalize frees with the same allocator it was created with", async () => { // BuildMessage.create() clones the message with the passed allocator // but finalize() was freeing it with bun.default_allocator and never From f16e0cd5ad43fe84cb349816b771443d8d5802bb Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Sat, 23 May 2026 13:49:33 +0000 Subject: [PATCH 2/2] Drop redundant AddressSanitizer assertion from build-error regression test --- test/js/bun/resolve/build-error.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/js/bun/resolve/build-error.test.ts b/test/js/bun/resolve/build-error.test.ts index 1784b4ab00f3..2b6073815544 100644 --- a/test/js/bun/resolve/build-error.test.ts +++ b/test/js/bun/resolve/build-error.test.ts @@ -45,7 +45,6 @@ test("importing a module with many build errors does not crash while reporting t // Every error in the AggregateError should have been printed. expect(stderr).toContain('"x0" has already been declared'); expect(stderr).toContain('"x39" has already been declared'); - expect(stderr).not.toContain("AddressSanitizer"); // Unhandled rejection -> clean exit with code 1, not a crash. expect(exitCode).toBe(1); });