Skip to content
Closed
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
47 changes: 47 additions & 0 deletions src/shell/Builtin.zig
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,10 @@ pub fn deinit(this: *Builtin) void {
// No need to free it because it belongs to the parent cmd
// _ = Syscall.close(this.cwd);

// Cancel any pending IOWriter chunks that reference this builtin
// to prevent use-after-free when the write completes.
this.cancelPendingChunks();

this.stdout.deref();
this.stderr.deref();
this.stdin.deref();
Expand All @@ -676,6 +680,49 @@ pub fn deinit(this: *Builtin) void {
// this.arena.deinit();
}

/// Cancel any pending IOWriter chunks that reference this builtin's impl.
/// This prevents use-after-free crashes when a write completes after
/// the builtin has been freed.
fn cancelPendingChunks(this: *Builtin) void {
const impl_ptr: usize = switch (this.kind) {
.cat => @intFromPtr(&this.impl.cat),
.touch => @intFromPtr(&this.impl.touch),
.mkdir => @intFromPtr(&this.impl.mkdir),
.@"export" => @intFromPtr(&this.impl.@"export"),
.echo => @intFromPtr(&this.impl.echo),
.cd => @intFromPtr(&this.impl.cd),
.which => @intFromPtr(&this.impl.which),
.rm => @intFromPtr(&this.impl.rm),
.pwd => @intFromPtr(&this.impl.pwd),
.mv => @intFromPtr(&this.impl.mv),
.ls => @intFromPtr(&this.impl.ls),
.exit => @intFromPtr(&this.impl.exit),
.true => @intFromPtr(&this.impl.true),
.false => @intFromPtr(&this.impl.false),
.yes => @intFromPtr(&this.impl.yes),
.seq => @intFromPtr(&this.impl.seq),
.dirname => @intFromPtr(&this.impl.dirname),
.basename => @intFromPtr(&this.impl.basename),
.cp => @intFromPtr(&this.impl.cp),
};

// Cancel chunks in stdout writer if it's an fd
switch (this.stdout) {
.fd => |fd| {
fd.writer.cancelChunksWithRawPtr(impl_ptr);
},
else => {},
}

// Cancel chunks in stderr writer if it's an fd
switch (this.stderr) {
.fd => |fd| {
fd.writer.cancelChunksWithRawPtr(impl_ptr);
},
else => {},
}
}

/// If the stdout/stderr is supposed to be captured then get the bytelist associated with that
pub fn stdBufferedBytelist(this: *Builtin, comptime io_kind: @Type(.enum_literal)) ?*bun.ByteList {
if (comptime io_kind != .stdout and io_kind != .stderr) {
Expand Down
7 changes: 6 additions & 1 deletion src/shell/IOWriter.zig
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,12 @@ pub fn cancelChunks(this: *IOWriter, ptr_: anytype) void {
ChildPtr => ptr_,
else => ChildPtr.init(ptr_),
};
const actual_ptr = ptr.ptr.repr._ptr;
this.cancelChunksWithRawPtr(ptr.ptr.repr._ptr);
}

/// Cancel chunks by raw pointer address. Used when we have an opaque
/// pointer and need to cancel chunks without type information.
pub fn cancelChunksWithRawPtr(this: *IOWriter, actual_ptr: usize) void {
if (this.writers.len() == 0) return;
const idx = this.writer_idx;
const slice: []Writer = this.writers.sliceMutable();
Expand Down
16 changes: 16 additions & 0 deletions src/shell/states/Cmd.zig
Original file line number Diff line number Diff line change
Expand Up @@ -726,11 +726,27 @@ pub fn deinit(this: *Cmd) void {
this.spawn_arena.deinit();
}

// Cancel any pending IOWriter chunks that reference this Cmd
// to prevent use-after-free when the write completes.
this.cancelPendingChunks();

this.io.deref();
this.base.endScope();
this.parent.destroy(this);
}

/// Cancel any pending IOWriter chunks that reference this Cmd.
fn cancelPendingChunks(this: *Cmd) void {
switch (this.io.stderr) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
switch (this.io.stdout) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
}

pub fn bufferedInputClose(this: *Cmd) void {
this.exec.subproc.buffered_closed.close(this, .stdin);
}
Expand Down
16 changes: 16 additions & 0 deletions src/shell/states/CondExpr.zig
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,10 @@ fn doStat(this: *CondExpr) Yield {
}

pub fn deinit(this: *CondExpr) void {
// Cancel any pending IOWriter chunks that reference this CondExpr
// to prevent use-after-free when the write completes.
this.cancelPendingChunks();

this.io.deinit();
for (this.args.items) |item| {
this.base.allocator().free(item);
Expand All @@ -220,6 +224,18 @@ pub fn deinit(this: *CondExpr) void {
this.parent.destroy(this);
}

/// Cancel any pending IOWriter chunks that reference this CondExpr.
fn cancelPendingChunks(this: *CondExpr) void {
switch (this.io.stderr) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
switch (this.io.stdout) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
}

pub fn childDone(this: *CondExpr, child: ChildPtr, exit_code: ExitCode) Yield {
if (child.ptr.is(Expansion)) {
if (exit_code != 0) {
Expand Down
17 changes: 17 additions & 0 deletions src/shell/states/Pipeline.zig
Original file line number Diff line number Diff line change
Expand Up @@ -274,11 +274,28 @@ pub fn deinit(this: *Pipeline) void {
if (this.cmds) |cmds| {
this.base.allocator().free(cmds);
}

// Cancel any pending IOWriter chunks that reference this Pipeline
// to prevent use-after-free when the write completes.
this.cancelPendingChunks();

this.io.deref();
this.base.endScope();
this.parent.destroy(this);
}

/// Cancel any pending IOWriter chunks that reference this Pipeline.
fn cancelPendingChunks(this: *Pipeline) void {
switch (this.io.stderr) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
switch (this.io.stdout) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
}

fn initializePipes(pipes: []Pipe, set_count: *u32) Maybe(void) {
for (pipes) |*pipe| {
if (bun.Environment.isWindows) {
Expand Down
17 changes: 17 additions & 0 deletions src/shell/states/Subshell.zig
Original file line number Diff line number Diff line change
Expand Up @@ -172,12 +172,29 @@ pub fn onIOWriterChunk(this: *Subshell, _: usize, err: ?jsc.SystemError) Yield {

pub fn deinit(this: *Subshell) void {
this.base.shell.deinit();

// Cancel any pending IOWriter chunks that reference this Subshell
// to prevent use-after-free when the write completes.
this.cancelPendingChunks();

this.io.deref();
this.redirection_file.deinit();
this.base.endScope();
this.parent.destroy(this);
}

/// Cancel any pending IOWriter chunks that reference this Subshell.
fn cancelPendingChunks(this: *Subshell) void {
switch (this.io.stderr) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
switch (this.io.stdout) {
.fd => |fd| fd.writer.cancelChunks(this),
else => {},
}
}

pub fn writeFailingError(this: *Subshell, comptime fmt: []const u8, args: anytype) Yield {
const handler = struct {
fn enqueueCb(ctx: *Subshell) void {
Expand Down
76 changes: 76 additions & 0 deletions test/regression/issue/26660.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import { expect, test } from "bun:test";
import { bunEnv, bunExe } from "harness";

// Test for issue #26660: Shell interpreter crash when spawning many concurrent shell commands
// The bug was that IOWriter could have pending chunks referencing freed memory after
// builtin/cmd cleanup, causing use-after-free crashes.
test("many concurrent shell commands should not crash", async () => {
// Spawn many shell commands concurrently (similar to what triggered the crash)
const script = `const promises = []; for (let i = 0; i < 60; i++) { promises.push(Bun.$\`echo "Hello from shell \${i}"\`.text().then(() => {})); } await Promise.all(promises); console.log("All shell commands completed successfully");`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});

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

expect(stdout).toContain("All shell commands completed successfully");
expect(stderr).toBe("");
expect(exitCode).toBe(0);
Comment on lines +11 to +22

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

鈿狅笍 Potential issue | 馃煛 Minor

Add an explicit failing-spawn case to cover error paths.
Right now only the success path is exercised. Add a small negative-case spawn (e.g., process.exit(1) or a throw) and assert non鈥憐ero exitCode + stderr.

As per coding guidelines: Always check exit codes and test error scenarios when spawning processes in tests.

馃 Prompt for AI Agents
In `@test/regression/issue/26660.test.ts` around lines 26 - 39, Add a negative
spawn case in the same test to exercise error paths: spawn another process
(using Bun.spawn like the existing proc) that runs a short script which either
throws or calls process.exit(1), then await its stdout/stderr and proc.exited
(use a different variable name than proc to avoid clobbering), and assert that
the returned exitCode is non鈥憐ero and stderr contains an error indicator (e.g.,
"Error" or the thrown message); ensure you mirror the existing pattern
(Bun.spawn, .stdout.text(), .stderr.text(), .exited) so the failure path is
covered alongside the current success assertions.

});

test("concurrent shell commands with output should not crash", async () => {
// Test with commands that produce output, which exercises IOWriter more
const script = `const promises = []; for (let i = 0; i < 40; i++) { promises.push(Bun.$\`echo "Line \${i}: test output"\`.text()); } const results = await Promise.all(promises); console.log("Collected", results.length, "results"); console.log("All outputs received correctly");`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});

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

expect(stdout).toContain("All outputs received correctly");
expect(stderr).toBe("");
expect(exitCode).toBe(0);
});

test("shell commands with mixed builtins should not crash", async () => {
// Test various builtins that use IOWriter
const script = `const promises = []; for (let i = 0; i < 20; i++) { promises.push(Bun.$\`echo "Echo \${i}"\`.text().then(() => {})); promises.push(Bun.$\`pwd\`.text().then(() => {})); promises.push(Bun.$\`true\`.text().then(() => {})); } await Promise.all(promises); console.log("All builtin commands completed");`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});

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

expect(stdout).toContain("All builtin commands completed");
expect(stderr).toBe("");
expect(exitCode).toBe(0);
});

test("concurrent shell commands with failing commands should handle errors", async () => {
// Test that failing commands are handled correctly and don't cause crashes
const script = `const promises = []; for (let i = 0; i < 20; i++) { if (i % 3 === 0) { promises.push(Bun.$\`nonexistent_command_\${i}\`.nothrow().text().catch(() => "failed")); } else { promises.push(Bun.$\`echo "Success \${i}"\`.text()); } } const results = await Promise.all(promises); console.log("Handled", results.length, "commands"); console.log("Error handling completed");`;

await using proc = Bun.spawn({
cmd: [bunExe(), "-e", script],
env: bunEnv,
stderr: "pipe",
stdout: "pipe",
});

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

expect(stdout).toContain("Error handling completed");
expect(exitCode).toBe(0);
});