diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index 999122f4b47..e3b1787ad7b 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -2510,13 +2510,16 @@ pub fn process_fetch_log( } _ => { - // Caps at 256. We heap-allocate the - // exact `len` since `JSValue` is a thin u64 and 256 * 8 B = 2 KiB - // is fine either way, and `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 { + // On-stack array: the conservative GC stack scan is the only + // thing keeping these wrappers alive until + // `create_aggregate_error` stores them. A heap `Vec` is invisible + // to that scan, so a GC triggered by a later `create` could sweep + // the earlier cells and free their native + // `BuildMessage`/`ResolveMessage` out from under us. + 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, @@ -2524,14 +2527,13 @@ pub fn process_fetch_log( referrer_utf8.slice(), )), }; - errors.push(v); } // C++ `Zig::toString` does `createWithoutCopying`, so the buffer // must outlive the AggregateError. Mark it global so JSC adopts it // as an ExternalStringImpl and frees it via `free_global_string`. let message_text: &'static mut [u8] = bun_core::heap::release( - format!("{} errors building \"{specifier}\"", errors.len()) + format!("{len} errors building \"{specifier}\"") .into_bytes() .into_boxed_slice(), ); @@ -2539,7 +2541,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_stack[..len], &message)), ); } } diff --git a/src/jsc/lib.rs b/src/jsc/lib.rs index e1e607847f0..e86e796f0b7 100644 --- a/src/jsc/lib.rs +++ b/src/jsc/lib.rs @@ -1947,12 +1947,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 until `create_aggregate_error` stores them; + // a heap `Vec` is invisible to the scan, so a GC triggered by + // a later `msg_to_js` could sweep the earlier wrappers. + 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/src/runtime/bake/production.rs b/src/runtime/bake/production.rs index 5b0f4d695b9..f61de35ad56 100644 --- a/src/runtime/bake/production.rs +++ b/src/runtime/bake/production.rs @@ -942,25 +942,24 @@ pub(super) fn build_with_vm( )); } - let mut css_chunk_js_strings: Vec = vec![JSValue::ZERO; css_chunks_count]; - debug_assert_eq!( - pt.bundled_outputs[css_chunks_first..][..css_chunks_count].len(), - css_chunk_js_strings.len() - ); - for (output_file, str) in pt.bundled_outputs[css_chunks_first..][..css_chunks_count] - .iter() - .zip(css_chunk_js_strings.iter_mut()) - { + // These strings live only in this heap Vec until the route arrays below + // store them; the conservative GC stack scan cannot see a heap buffer, so + // keep each one protected (module evaluation below can trigger GC). + let mut css_chunk_js_strings: Vec = Vec::with_capacity(css_chunks_count); + for output_file in pt.bundled_outputs[css_chunks_first..][..css_chunks_count].iter() { debug_assert!(output_file.dest_path[0] != b'.'); // CSS chunks must be in contiguous order!! debug_assert!(output_file.loader.is_css()); - *str = BunString::create_format(format_args!( - "{}{}", - BStr::new(public_path), - BStr::new(&output_file.dest_path), - )) - .to_js(global) - .map_err(js_err)?; + css_chunk_js_strings.push( + BunString::create_format(format_args!( + "{}{}", + BStr::new(public_path), + BStr::new(&output_file.dest_path), + )) + .to_js(global) + .map_err(js_err)? + .protected(), + ); } // Route URL patterns with parameter placeholders. @@ -1075,7 +1074,7 @@ pub(super) fn build_with_vm( .put_index( global, css_file_count, - css_chunk_js_strings[r#ref.get() as usize - css_chunks_first], + css_chunk_js_strings[r#ref.get() as usize - css_chunks_first].value(), ) .map_err(js_err)?; css_file_count += 1; @@ -1093,7 +1092,7 @@ pub(super) fn build_with_vm( .put_index( global, css_file_count, - css_chunk_js_strings[r#ref.get() as usize - css_chunks_first], + css_chunk_js_strings[r#ref.get() as usize - css_chunks_first].value(), ) .map_err(js_err)?; css_file_count += 1; @@ -1116,7 +1115,7 @@ pub(super) fn build_with_vm( .put_index( global, css_file_count, - css_chunk_js_strings[r#ref.get() as usize - css_chunks_first], + css_chunk_js_strings[r#ref.get() as usize - css_chunks_first].value(), ) .map_err(js_err)?; css_file_count += 1; diff --git a/test/js/bun/resolve/build-error.test.ts b/test/js/bun/resolve/build-error.test.ts index a623aa505e9..4fa4b098d46 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,55 @@ test("BuildError is modifiable", async () => { expect(error!.message).not.toBe(message); }); +test("import with many build errors keeps AggregateError entries alive across GC", async () => { + // process_fetch_log accumulated the BuildMessage wrappers in a heap Vec + // while creating the next ones; the conservative GC stack scan never saw + // them, so a collection triggered mid-loop swept the earlier cells and + // freed their native BuildMessage (use-after-free found by fuzzing). + // 257 duplicate declarations produce 256 log messages, maximizing the + // number of allocations between the first wrapper and the AggregateError. + const dupes = Array.from({ length: 257 }, (_, i) => `const dup = ${i};`).join("\n"); + using dir = tempDir("build-error-gc", { + "bad.js": dupes, + "main.js": ` + const jobs = []; + for (let i = 0; i < 16; i++) { + jobs.push( + import("./bad.js?v=" + i).then( + () => { + throw new Error("expected rejection"); + }, + e => { + let n = 0; + for (const err of e.errors ?? []) { + if (typeof err.message === "string") n++; + } + return n; + }, + ), + ); + } + const counts = await Promise.all(jobs); + console.log(JSON.stringify(counts)); + `, + }); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "main.js"], + env: { ...bunEnv, BUN_JSC_slowPathAllocsBetweenGCs: "100" }, + cwd: String(dir), + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + // stderr carries the crash report when the child dies; surface it in the + // failure diff but don't assert on it (debug builds emit benign noise). + if (exitCode !== 0) console.error(stderr); + expect(stdout.trim()).toBe(JSON.stringify(Array.from({ length: 16 }, () => 256))); + 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 diff --git a/test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts b/test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts new file mode 100644 index 00000000000..6f7973eba06 --- /dev/null +++ b/test/js/bun/transpiler/transpiler-error-gc-uaf.test.ts @@ -0,0 +1,61 @@ +// Log.to_js (used by Bun.Transpiler().transform/transformSync when rejecting +// with parse errors, and by the module loader via process_fetch_log) builds +// an AggregateError by allocating one BuildMessage JS cell per log entry. The +// Rust port collected those cells in a heap Vec, which the +// conservative GC scan does not see, so an earlier cell could be swept while +// allocating a later one and the AggregateError would reference a zapped +// StructureID. +// +// useZombieMode scribbles 0xbadbeef0 over swept cells so the dangling access +// manifests deterministically; collectContinuously races the collector against +// the allocation loop so it reliably sweeps mid-loop. +import { expect, test } from "bun:test"; +import { bunEnv, bunExe, isWindows, tempDir } from "harness"; + +const fixture = ` +const src = Array.from({ length: 300 }, () => "a b").join("\\n"); +const t = new Bun.Transpiler(); +for (let i = 0; i < 20; i++) { + let err; + try { t.transformSync(src); } catch (e) { err = e; } + if (!(err instanceof AggregateError)) throw new Error("not AggregateError: " + err); + if (err.errors.length !== 256) throw new Error("wrong count: " + err.errors.length); + for (const m of err.errors) { + const msg = m.message; + if (msg !== 'Expected ";" but found "b"') { + throw new Error("corrupt BuildMessage: " + JSON.stringify(typeof msg) + " " + String(msg).slice(0, 80)); + } + } +} +console.log("OK"); +`; + +test("Log.to_js roots BuildMessage cells across allocation", async () => { + using dir = tempDir("log-to-js-gc-root", { + "fixture.js": fixture, + }); + + // Windows + collectContinuously is prohibitively slow in CI and the code + // path is platform-agnostic, so rely on zombie mode alone there. + const gcEnv: Record = { + ...bunEnv, + BUN_JSC_useZombieMode: "1", + }; + if (!isWindows) gcEnv.BUN_JSC_collectContinuously = "1"; + + await using proc = Bun.spawn({ + cmd: [bunExe(), "fixture.js"], + env: gcEnv, + cwd: String(dir), + stderr: "pipe", + stdout: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + if (exitCode !== 0) { + expect(stderr).toBe(""); + } + expect(stdout.trim()).toBe("OK"); + expect(exitCode).toBe(0); +}, 60_000);