From c1acb0b9a4868a1aa4fe024a9de9b756071f077f Mon Sep 17 00:00:00 2001 From: Dylan Conway Date: Wed, 17 Dec 2025 18:33:53 -0800 Subject: [PATCH] fix(shell): prevent double-close of fd when using &> redirect with builtins (#25568) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary - Fix double-close of file descriptor when using `&>` redirect with shell builtin commands - Add `dupeRef()` helper for cleaner reference counting semantics - Add tests for `&>` and `&>>` redirects with builtins ## Test plan - [x] Added tests in `test/js/bun/shell/file-io.test.ts` that reproduce the bug - [x] All file-io tests pass ## The Bug When using `&>` to redirect both stdout and stderr to the same file with a shell builtin command (e.g., `pwd &> file.txt`), the code was creating two separate `IOWriter` instances that shared the same file descriptor. When both `IOWriter`s were destroyed, they both tried to close the same fd, causing an `EBADF` (bad file descriptor) error. ```javascript import { $ } from "bun"; await $`pwd &> output.txt`; // Would crash with EBADF ``` ## The Fix 1. Share a single `IOWriter` between stdout and stderr when both are redirected to the same file, with proper reference counting 2. Rename `refSelf` to `dupeRef` for clarity across `IOReader`, `IOWriter`, `CowFd`, and add it to `Blob` for consistency 3. Fix the `Body.Value` blob case to also properly reference count when the same blob is assigned to multiple outputs 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Latest model --- src/shell/Builtin.zig | 39 ++++++++++++++++++++++++------- src/shell/IO.zig | 2 +- src/shell/IOReader.zig | 2 +- src/shell/IOWriter.zig | 2 +- src/shell/interpreter.zig | 2 +- src/shell/subproc.zig | 2 +- test/js/bun/shell/file-io.test.ts | 15 ++++++++++++ 7 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/shell/Builtin.zig b/src/shell/Builtin.zig index a5713ca804..b29ff24009 100644 --- a/src/shell/Builtin.zig +++ b/src/shell/Builtin.zig @@ -264,6 +264,11 @@ pub const BuiltinIO = struct { ref_count: RefCount, blob: bun.webcore.Blob, + fn dupeRef(this: *Blob) *Blob { + this.ref(); + return this; + } + fn deinit(this: *Blob) void { this.blob.deinit(); bun.destroy(this); @@ -340,16 +345,16 @@ pub fn init( io: *IO, ) ?Yield { const stdin: BuiltinIO.Input = switch (io.stdin) { - .fd => |fd| .{ .fd = fd.refSelf() }, + .fd => |fd| .{ .fd = fd.dupeRef() }, .ignore => .ignore, }; const stdout: BuiltinIO.Output = switch (io.stdout) { - .fd => |val| .{ .fd = .{ .writer = val.writer.refSelf(), .captured = val.captured } }, + .fd => |val| .{ .fd = .{ .writer = val.writer.dupeRef(), .captured = val.captured } }, .pipe => .{ .buf = std.array_list.Managed(u8).init(cmd.base.allocator()) }, .ignore => .ignore, }; const stderr: BuiltinIO.Output = switch (io.stderr) { - .fd => |val| .{ .fd = .{ .writer = val.writer.refSelf(), .captured = val.captured } }, + .fd => |val| .{ .fd = .{ .writer = val.writer.dupeRef(), .captured = val.captured } }, .pipe => .{ .buf = std.array_list.Managed(u8).init(cmd.base.allocator()) }, .ignore => .ignore, }; @@ -481,13 +486,26 @@ fn initRedirections( cmd.exec.bltn.stdin.deref(); cmd.exec.bltn.stdin = .{ .fd = IOReader.init(redirfd, cmd.base.eventLoop()) }; } + + if (!node.redirect.stdout and !node.redirect.stderr) { + return null; + } + + const redirect_writer: *IOWriter = .init( + redirfd, + .{ .pollable = is_pollable, .nonblocking = is_nonblocking, .is_socket = is_socket }, + cmd.base.eventLoop(), + ); + defer redirect_writer.deref(); + if (node.redirect.stdout) { cmd.exec.bltn.stdout.deref(); - cmd.exec.bltn.stdout = .{ .fd = .{ .writer = IOWriter.init(redirfd, .{ .pollable = is_pollable, .nonblocking = is_nonblocking, .is_socket = is_socket }, cmd.base.eventLoop()) } }; + cmd.exec.bltn.stdout = .{ .fd = .{ .writer = redirect_writer.dupeRef() } }; } + if (node.redirect.stderr) { cmd.exec.bltn.stderr.deref(); - cmd.exec.bltn.stderr = .{ .fd = .{ .writer = IOWriter.init(redirfd, .{ .pollable = is_pollable, .nonblocking = is_nonblocking, .is_socket = is_socket }, cmd.base.eventLoop()) } }; + cmd.exec.bltn.stderr = .{ .fd = .{ .writer = redirect_writer.dupeRef() } }; } }, .jsbuf => |val| { @@ -522,24 +540,29 @@ fn initRedirections( var original_blob = body.use(); defer original_blob.deinit(); + if (!node.redirect.stdin and !node.redirect.stdout and !node.redirect.stderr) { + return null; + } + const blob: *BuiltinIO.Blob = bun.new(BuiltinIO.Blob, .{ .ref_count = .init(), .blob = original_blob.dupe(), }); + defer blob.deref(); if (node.redirect.stdin) { cmd.exec.bltn.stdin.deref(); - cmd.exec.bltn.stdin = .{ .blob = blob }; + cmd.exec.bltn.stdin = .{ .blob = blob.dupeRef() }; } if (node.redirect.stdout) { cmd.exec.bltn.stdout.deref(); - cmd.exec.bltn.stdout = .{ .blob = blob }; + cmd.exec.bltn.stdout = .{ .blob = blob.dupeRef() }; } if (node.redirect.stderr) { cmd.exec.bltn.stderr.deref(); - cmd.exec.bltn.stderr = .{ .blob = blob }; + cmd.exec.bltn.stderr = .{ .blob = blob.dupeRef() }; } } else if (interpreter.jsobjs[file.jsbuf.idx].as(jsc.WebCore.Blob)) |blob| { if ((node.redirect.stdout or node.redirect.stderr) and !blob.needsToReadFile()) { diff --git a/src/shell/IO.zig b/src/shell/IO.zig index dc5730026f..b95520424a 100644 --- a/src/shell/IO.zig +++ b/src/shell/IO.zig @@ -170,7 +170,7 @@ pub const OutKind = union(enum) { fn to_subproc_stdio(this: OutKind, shellio: *?*shell.IOWriter) bun.shell.subproc.Stdio { return switch (this) { .fd => |val| brk: { - shellio.* = val.writer.refSelf(); + shellio.* = val.writer.dupeRef(); break :brk if (val.captured) |cap| .{ .capture = .{ .buf = cap, diff --git a/src/shell/IOReader.zig b/src/shell/IOReader.zig index 15ffb786e1..540b81cbc7 100644 --- a/src/shell/IOReader.zig +++ b/src/shell/IOReader.zig @@ -30,7 +30,7 @@ const InitFlags = packed struct(u8) { __unused: u5 = 0, }; -pub fn refSelf(this: *IOReader) *IOReader { +pub fn dupeRef(this: *IOReader) *IOReader { this.ref(); return this; } diff --git a/src/shell/IOWriter.zig b/src/shell/IOWriter.zig index 51aa360d00..868570ee9a 100644 --- a/src/shell/IOWriter.zig +++ b/src/shell/IOWriter.zig @@ -70,7 +70,7 @@ pub const Poll = WriterImpl; // pub fn __onClose(_: *IOWriter) void {} // pub fn __flush(_: *IOWriter) void {} -pub fn refSelf(this: *IOWriter) *IOWriter { +pub fn dupeRef(this: *IOWriter) *IOWriter { this.ref(); return this; } diff --git a/src/shell/interpreter.zig b/src/shell/interpreter.zig index 723b2c05cb..d4a1d6aa50 100644 --- a/src/shell/interpreter.zig +++ b/src/shell/interpreter.zig @@ -177,7 +177,7 @@ pub const CowFd = struct { this.refcount += 1; } - pub fn refSelf(this: *CowFd) *CowFd { + pub fn dupeRef(this: *CowFd) *CowFd { this.ref(); return this; } diff --git a/src/shell/subproc.zig b/src/shell/subproc.zig index b4e6bb08e4..0e28c43ec6 100644 --- a/src/shell/subproc.zig +++ b/src/shell/subproc.zig @@ -1121,7 +1121,7 @@ pub const PipeReader = struct { log("PipeReader(0x{x}, {s}) create()", .{ @intFromPtr(this), @tagName(this.out_type) }); if (capture) |cap| { - this.captured_writer.writer = cap.refSelf(); + this.captured_writer.writer = cap.dupeRef(); this.captured_writer.dead = false; } diff --git a/test/js/bun/shell/file-io.test.ts b/test/js/bun/shell/file-io.test.ts index 6c8f8e87e3..09d8e8c55f 100644 --- a/test/js/bun/shell/file-io.test.ts +++ b/test/js/bun/shell/file-io.test.ts @@ -140,4 +140,19 @@ describe("IOWriter file output redirection", () => { .fileEquals("pipe_output.txt", "pipe test\n") .runAsTest("pipe with file redirection"); }); + + describe("&> redirect (stdout and stderr to same file)", () => { + // This test verifies the fix for the bug where using &> with a builtin + // command caused the same file descriptor to be closed twice, resulting + // in an EBADF error. The issue was that two separate IOWriter instances + // were created for the same fd when both stdout and stderr were redirected. + TestBuilder.command`pwd &> pwd_output.txt`.exitCode(0).runAsTest("builtin pwd with &> redirect"); + + TestBuilder.command`echo "hello" &> echo_output.txt` + .exitCode(0) + .fileEquals("echo_output.txt", "hello\n") + .runAsTest("builtin echo with &> redirect"); + + TestBuilder.command`pwd &>> append_output.txt`.exitCode(0).runAsTest("builtin pwd with &>> append redirect"); + }); });