Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/install/PackageManager/PackageManagerEnqueue.rs
Original file line number Diff line number Diff line change
Expand Up @@ -519,13 +519,21 @@ pub fn enqueue_dependency_to_root(
// PORT NOTE: raw `*mut` (Zig `*PackageManager`) — `sleep_until`
// also receives this pointer, so `&mut` here would alias.
manager: *mut PackageManager,
// `sleep_until` ticks the JS event loop between polls; code
// run there (module transpile / nested resolve / AsyncModule)
// swaps `manager.log` and may restore it from a different
// source than the caller did, leaving it pointing at a dead
// stack `Log`. Snapshot the caller's log and re-assert it
// before every `run_tasks` so `log_mut()` never dangles.
log: *mut bun_ast::Log,
}
impl Closure {
fn is_done(&mut self) -> bool {
// SAFETY: `self.manager` is the raw provenance root set
// below; `sleep_until`/`tick_raw` hold no `&mut` across
// this callback, so this is the unique live borrow.
let manager = unsafe { &mut *self.manager };
manager.log = self.log;
if manager.pending_task_count() > 0 {
// Zig: `runTasks(void, {}, .{ .onExtract = {}, ... }, false, log_level)`
// — all callbacks `void`. `VoidRunTasksCallbacks` (below)
Expand Down Expand Up @@ -563,6 +571,7 @@ pub fn enqueue_dependency_to_root(
let mut closure = Closure {
err: None,
manager: mgr,
log: this.log,
};
// SAFETY: `mgr` derived from the live exclusive `this` borrow;
// `sleep_until` + `tick_raw` hold no `&mut PackageManager` across
Expand Down
5 changes: 5 additions & 0 deletions src/jsc/VirtualMachine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4369,8 +4369,10 @@ impl VirtualMachine {
// stores `*logger.Log`, always non-null), so the `Option` is purely a
// zeroed-init nicety; the `expect` is infallible.
let old_log: NonNull<bun_ast::Log> = jsc_vm.log.expect("vm.log set in init");
let old_transpiler_log: *mut bun_ast::Log = jsc_vm.transpiler.log;
let mut log = bun_ast::Log::default();
jsc_vm.log = NonNull::new(&raw mut log);
jsc_vm.transpiler.log = &raw mut log;
jsc_vm.transpiler.resolver.log = NonNull::from(&mut log);
jsc_vm.transpiler.linker.log = &raw mut log;
if let Some(pm) = jsc_vm.transpiler.resolver.package_manager {
Expand All @@ -4387,13 +4389,15 @@ impl VirtualMachine {
struct RestoreLog {
vm: bun_ptr::BackRef<VirtualMachine>,
old_log: NonNull<bun_ast::Log>,
old_transpiler_log: *mut bun_ast::Log,
}
impl Drop for RestoreLog {
fn drop(&mut self) {
// `vm` is the live per-thread VM (caller is on the JS
// thread); `old_log` outlives the VM (Box::leak in `init`).
let jsc_vm = self.vm.get().as_mut();
jsc_vm.log = Some(self.old_log);
jsc_vm.transpiler.log = self.old_transpiler_log;
jsc_vm.transpiler.resolver.log = self.old_log;
jsc_vm.transpiler.linker.log = self.old_log.as_ptr();
// `_resolve` may have lazily created the PM with
Expand All @@ -4411,6 +4415,7 @@ impl VirtualMachine {
let _restore = RestoreLog {
vm: bun_ptr::BackRef::from(NonNull::new(jsc_vm_ptr).expect("vm non-null")),
old_log,
old_transpiler_log,
};
// PORT NOTE: reshaped for borrowck — re-derive from raw so the unique
// borrow doesn't span the guard's drop.
Expand Down
3 changes: 3 additions & 0 deletions src/runtime/jsc_hooks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5012,10 +5012,12 @@ unsafe fn resolve_hook(
// `*logger.Log`, always non-null) — the `expect` is infallible.
let old_log: core::ptr::NonNull<bun_ast::Log> =
unsafe { &*vm }.log.expect("vm.log set in init");
let old_transpiler_log: *mut bun_ast::Log = unsafe { &*vm }.transpiler.log;
let log_nn: core::ptr::NonNull<bun_ast::Log> = core::ptr::NonNull::from(&mut log);
// SAFETY: `vm` is the live per-thread VM.
unsafe {
(*vm).log = Some(log_nn);
(*vm).transpiler.log = log_nn.as_ptr();
(*vm).transpiler.resolver.log = log_nn;
(*vm).transpiler.linker.log = log_nn.as_ptr();
if let Some(pm) = (*vm).transpiler.resolver.package_manager {
Expand All @@ -5030,6 +5032,7 @@ unsafe fn resolve_hook(
// restore it even if it was `None` at swap time.
unsafe {
(*vm).log = Some(old_log);
(*vm).transpiler.log = old_transpiler_log;
(*vm).transpiler.resolver.log = old_log;
(*vm).transpiler.linker.log = old_log.as_ptr();
if let Some(pm) = (*vm).transpiler.resolver.package_manager {
Expand Down
64 changes: 64 additions & 0 deletions test/js/bun/resolve/resolve-autoinstall-log-dangling.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,67 @@ test("repeated failing auto-install resolves at varying stack depth don't read a
expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);
});

// Auto-install's `sleep_until` ticks the JS event loop while waiting for the
// manifest response. JS that runs during that tick (module transpile, nested
// resolve) swaps `PackageManager.log` and restores it from a different save
// source than the outer resolve, which could leave it pointing at a popped
// stack `Log`. The next `run_tasks` poll then dereferenced dead stack memory
// when logging the 404.
test("JS running during auto-install's event-loop tick doesn't leave pm.log dangling", async () => {
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
let hits = 0;
await using server = Bun.serve({
port: 0,
fetch() {
hits++;
return new Response("Not Found", { status: 404 });
},
});

using dir = tempDir("resolve-autoinstall-log-tick", {
"index.js": `
const Module = require("module");
const fs = require("fs");
const path = require("path");

let n = 0;
function load() {
const f = path.join(import.meta.dir, "dyn" + n++ + ".cjs");
fs.writeFileSync(f, "module.exports = 0;");
try { require(f); } catch {}
try {
Module._resolveFilename("autoinstall-nested-pkg-" + n, { filename: import.meta.path });
} catch {}
}

for (let i = 0; i < 20; i++) {
setImmediate(load);
setImmediate(load);
try {
Module._resolveFilename("autoinstall-missing-pkg-" + i, { filename: import.meta.path });
} catch {}
}

console.log("ok " + n);
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "--install=force", "index.js"],
cwd: String(dir),
env: {
...bunEnv,
BUN_CONFIG_REGISTRY: server.url.href,
NPM_CONFIG_REGISTRY: server.url.href,
},
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

expect(stderr).toBe("");
expect(stdout).toContain("ok");
expect(hits).toBeGreaterThan(0);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
expect(exitCode).toBe(0);
});
Loading