Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regression from #13414 #14092

Merged
merged 5 commits into from
Sep 22, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
26 changes: 24 additions & 2 deletions src/bun.js/api/bun/subprocess.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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();
}

Expand Down Expand Up @@ -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 .{},
Expand All @@ -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| {
Expand Down
3 changes: 3 additions & 0 deletions test/js/bun/spawn/bad-fixture.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

38 changes: 38 additions & 0 deletions test/js/bun/spawn/spawn-stdin-destroy.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
Loading