mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
fix(shell): prevent double-close of fd when using &> redirect with builtins (#25568)
## 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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()) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user