From a8fa716d409410bd3a1743a1b2b79945e7068c35 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 22 Sep 2024 04:25:46 -0700 Subject: [PATCH 1/5] Fix crash introduced in https://github.com/oven-sh/bun/pull/13414 --- src/bun.js/api/bun/subprocess.zig | 18 +++++++- test/js/bun/spawn/bad-fixture.js | 3 ++ test/js/bun/spawn/spawn-stdin-destroy.test.ts | 42 +++++++++++++++++++ 3 files changed, 61 insertions(+), 2 deletions(-) create mode 100644 test/js/bun/spawn/bad-fixture.js create mode 100644 test/js/bun/spawn/spawn-stdin-destroy.test.ts diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 018b24f1ddf0d..2e41ff659c2d7 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,16 @@ 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, but we don't want that. + // since we never called ref() on it here. + bun.debugAssert(!subprocess.flags.deref_on_stdin_destroyed); pipe.onAttachedProcessExit(); 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(); } @@ -2157,6 +2168,9 @@ pub const Subprocess = struct { default_max_buffer_size, is_sync, ), + // 1. JavaScript. + // 2. Process. + .ref_count = if (is_sync) 1 else 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 +2191,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..f438722bbe37f --- /dev/null +++ b/test/js/bun/spawn/spawn-stdin-destroy.test.ts @@ -0,0 +1,42 @@ +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); + before = require("bun:jsc").heapStats().objectTypeCounts; + Bun.gc(true); + await Bun.sleep(50); + })(); + Bun.gc(true); + + const { FileSink = 0, Subprocess = 0 } = require("bun:jsc").heapStats().objectTypeCounts; + expect(FileSink).toBeLessThan(before.FileSink || 0); + expect(Subprocess).toBeLessThan(before.Subprocess || 0); +}); From 4a36a63531876848b5b68aa05024957036a5c0b6 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 22 Sep 2024 04:31:22 -0700 Subject: [PATCH 2/5] Update subprocess.zig --- src/bun.js/api/bun/subprocess.zig | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index 2e41ff659c2d7..cc4084aa2bc1f 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1352,10 +1352,17 @@ 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, but we don't want that. - // since we never called ref() on it here. + // 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; From 701af2ccc8cf5242e8b89fd5c166cde34e12646d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 22 Sep 2024 04:32:26 -0700 Subject: [PATCH 3/5] Update subprocess.zig --- src/bun.js/api/bun/subprocess.zig | 1 + 1 file changed, 1 insertion(+) diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index cc4084aa2bc1f..ea51c8dbc8fc0 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -1465,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(); } From 64fdcd2d1cf3e0cc8866c3f8ad231ffc241caf3d Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 22 Sep 2024 04:59:35 -0700 Subject: [PATCH 4/5] Update subprocess.zig --- src/bun.js/api/bun/subprocess.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bun.js/api/bun/subprocess.zig b/src/bun.js/api/bun/subprocess.zig index ea51c8dbc8fc0..693340adf547a 100644 --- a/src/bun.js/api/bun/subprocess.zig +++ b/src/bun.js/api/bun/subprocess.zig @@ -2178,7 +2178,7 @@ pub const Subprocess = struct { ), // 1. JavaScript. // 2. Process. - .ref_count = if (is_sync) 1 else 2, + .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 .{}, From c29ce1d93ca303036bb8773964eed66af8780612 Mon Sep 17 00:00:00 2001 From: Jarred Sumner Date: Sun, 22 Sep 2024 05:09:19 -0700 Subject: [PATCH 5/5] Update spawn-stdin-destroy.test.ts --- test/js/bun/spawn/spawn-stdin-destroy.test.ts | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/test/js/bun/spawn/spawn-stdin-destroy.test.ts b/test/js/bun/spawn/spawn-stdin-destroy.test.ts index f438722bbe37f..3025af43297e8 100644 --- a/test/js/bun/spawn/spawn-stdin-destroy.test.ts +++ b/test/js/bun/spawn/spawn-stdin-destroy.test.ts @@ -30,13 +30,9 @@ test("stdin destroy after exit crash", async () => { expect(out).toBe(""); expect(exited).toBe(1); - before = require("bun:jsc").heapStats().objectTypeCounts; + Bun.gc(true); await Bun.sleep(50); })(); Bun.gc(true); - - const { FileSink = 0, Subprocess = 0 } = require("bun:jsc").heapStats().objectTypeCounts; - expect(FileSink).toBeLessThan(before.FileSink || 0); - expect(Subprocess).toBeLessThan(before.Subprocess || 0); });