diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 018b24f1ddf0d..693340adf547a 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -211,6 +211,7 @@ pub const Subprocess = struct { killed: bool = false, has_stdin_destructor_called: bool = false, finalized: bool = false, + deref_on_stdin_destroyed: bool = false, }; pub const SignalCode = bun.SignalCode; @@ -703,9 +704,13 @@ pub const Subprocess = struct { } pub fn onStdinDestroyed(this: *Subprocess) void { + const must_deref = this.flags.deref_on_stdin_destroyed; + this.flags.deref_on_stdin_destroyed = false; + defer if (must_deref) this.deref(); + this.flags.has_stdin_destructor_called = true; this.weak_file_sink_stdin_ptr = null; - defer this.deref(); + if (!this.flags.finalized) { // otherwise update the pending activity flag this.updateHasPendingActivity(); @@ -1240,6 +1245,7 @@ pub const Subprocess = struct { pipe.writer.setParent(pipe); subprocess.weak_file_sink_stdin_ptr = pipe; subprocess.ref(); + subprocess.flags.deref_on_stdin_destroyed = true; subprocess.flags.has_stdin_destructor_called = false; return Writable{ @@ -1300,6 +1306,7 @@ pub const Subprocess = struct { subprocess.weak_file_sink_stdin_ptr = pipe; subprocess.ref(); subprocess.flags.has_stdin_destructor_called = false; + subprocess.flags.deref_on_stdin_destroyed = true; pipe.writer.handle.poll.flags.insert(.socket); @@ -1345,12 +1352,23 @@ pub const Subprocess = struct { .pipe => |pipe| { this.* = .{ .ignore = {} }; if (subprocess.process.hasExited() and !subprocess.flags.has_stdin_destructor_called) { + // onAttachedProcessExit() can call deref on the + // subprocess. Since we never called ref(), it would be + // unbalanced to do so, leading to a use-after-free. + // So, let's not do that. + // https://github.com/oven-sh/bun/pull/14092 + bun.debugAssert(!subprocess.flags.deref_on_stdin_destroyed); + const debug_ref_count: if (Environment.isDebug) u32 else u0 = if (Environment.isDebug) subprocess.ref_count else 0; pipe.onAttachedProcessExit(); + if (comptime Environment.isDebug) { + bun.debugAssert(subprocess.ref_count == debug_ref_count); + } return pipe.toJS(globalThis); } else { subprocess.flags.has_stdin_destructor_called = false; subprocess.weak_file_sink_stdin_ptr = pipe; subprocess.ref(); + subprocess.flags.deref_on_stdin_destroyed = true; if (@intFromPtr(pipe.signal.ptr) == @intFromPtr(subprocess)) { pipe.signal.clear(); } @@ -1447,6 +1465,7 @@ pub const Subprocess = struct { if (stdin) |pipe| { this.weak_file_sink_stdin_ptr = null; this.flags.has_stdin_destructor_called = true; + // It is okay if it does call deref() here, as in that case it was truly ref'd. pipe.onAttachedProcessExit(); } @@ -2157,6 +2176,9 @@ pub const Subprocess = struct { default_max_buffer_size, is_sync, ), + // 1. JavaScript. + // 2. Process. + .ref_count = 2, .stdio_pipes = spawned.extra_pipes.moveToUnmanaged(), .on_exit_callback = if (on_exit_callback != .zero) JSC.Strong.create(on_exit_callback, globalThis) else .{}, .on_disconnect_callback = if (on_disconnect_callback != .zero) JSC.Strong.create(on_disconnect_callback, globalThis) else .{}, @@ -2177,7 +2199,7 @@ pub const Subprocess = struct { .is_sync = is_sync, }, }; - subprocess.ref(); // + one ref for the process + subprocess.process.setExitHandler(subprocess); if (subprocess.ipc_data) |*ipc_data| { diff --git a/test/js/bun/spawn/bad-fixture.js b/test/js/bun/spawn/bad-fixture.js new file mode 100644 index 0000000000000..3e9cd5bb3b423 --- /dev/null +++ b/test/js/bun/spawn/bad-fixture.js @@ -0,0 +1,3 @@ +process.stdin.read(); + +throw new Error("test"); diff --git a/test/js/bun/spawn/spawn-stdin-destroy.test.ts b/test/js/bun/spawn/spawn-stdin-destroy.test.ts new file mode 100644 index 0000000000000..3025af43297e8 --- /dev/null +++ b/test/js/bun/spawn/spawn-stdin-destroy.test.ts @@ -0,0 +1,38 @@ +import { bunEnv, bunExe } from "harness"; +import path from "path"; + +test("stdin destroy after exit crash", async () => { + let before; + await (async () => { + const child = Bun.spawn({ + cmd: [bunExe(), path.join(import.meta.dir, "bad-fixture.js")], + env: bunEnv, + stdout: "pipe", + stdin: "pipe", + }); + + await Bun.sleep(80); + await child.stdin.write("dylan\n"); + await child.stdin.write("999\n"); + await child.stdin.flush(); + await child.stdin.end(); + + async function read() { + var out = ""; + for await (const chunk of child.stdout) { + out += new TextDecoder().decode(chunk); + } + return out; + } + + // This bug manifested as child.exited rejecting with an error code of "TODO" + const [out, exited] = await Promise.all([read(), child.exited]); + + expect(out).toBe(""); + expect(exited).toBe(1); + + Bun.gc(true); + await Bun.sleep(50); + })(); + Bun.gc(true); +});