From 2f3bf67bb4cc02a7b4ce9e810ccd521b494e0198 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 25 May 2026 19:45:59 +0000 Subject: [PATCH] Keep multi-error AggregateError children GC-visible while they are created process_fetch_log collected the per-message BuildMessage/ResolveMessage wrappers in a heap Vec before creating the AggregateError. The conservative GC scan only covers the stack and registers, so a collection triggered while later wrappers were still being created could sweep the earlier ones and free their native payloads; printing or inspecting the resulting AggregateError then read freed memory. Use a stack array for the staging buffer, matching VirtualMachine.zig's errors_stack, so the wrappers stay rooted until createAggregateError runs. --- src/jsc/VirtualMachine.rs | 24 ++++++----- test/js/bun/resolve/build-error.test.ts | 54 ++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index c5951dd147a..d216cf9a459 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -2558,14 +2558,18 @@ 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`). The array + // must stay on the stack like the Zig spec: until + // `create_aggregate_error` runs, these freshly created + // BuildMessage/ResolveMessage wrappers have no other GC-visible + // reference, and `create()` can trigger a collection mid-loop. + // JSC's conservative scan only covers the stack/registers, so a + // heap `Vec` would let earlier wrappers be swept (and + // their native payloads freed) before the AggregateError exists. + let mut errors_stack = [JSValue::ZERO; 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, @@ -2573,8 +2577,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 @@ -2588,7 +2592,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/test/js/bun/resolve/build-error.test.ts b/test/js/bun/resolve/build-error.test.ts index a623aa505e9..c35b9acc51e 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,58 @@ test("BuildError is modifiable", async () => { expect(error!.message).not.toBe(message); }); +test("AggregateError from a module with many build errors survives GC during its creation", async () => { + // A module that fails to load with multiple errors produces an + // AggregateError of BuildMessage objects. The wrappers used to be collected + // in a heap Vec the conservative GC scan cannot see, so a collection in the + // middle of creating them freed the native payloads and printing or + // inspecting the AggregateError afterwards was a use-after-free. + using dir = tempDir("aggregate-build-errors", { + "bad.js": Array.from({ length: 250 }, (_, i) => `let dup${i} = 1; let dup${i} = 2;`).join("\n"), + "index.js": ` + let aggregate; + try { + await import("./bad.js"); + throw new Error("expected import to fail"); + } catch (e) { + aggregate = e; + } + Bun.gc(true); + if (aggregate.constructor.name !== "AggregateError") { + throw new Error("expected AggregateError, got " + aggregate.constructor.name); + } + if (aggregate.errors.length < 2) { + throw new Error("expected multiple build errors, got " + aggregate.errors.length); + } + for (const sub of aggregate.errors) { + if (typeof sub.message !== "string") throw new Error("bad message"); + void sub.position; + } + console.error(aggregate); + console.log("OK", aggregate.errors.length); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "index.js"], + cwd: String(dir), + env: { + ...bunEnv, + // Make a collection land while the per-message error wrappers are being created. + BUN_JSC_collectContinuously: "true", + BUN_JSC_forceRAMSize: "1000000", + }, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).not.toContain("AddressSanitizer"); + expect(stdout).toContain("OK 250"); + expect(exitCode).toBe(0); +}); + 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