Skip to content

Commit

Permalink
Prevent fd leaks to the running shell or command (ghostty-org#5341)
Browse files Browse the repository at this point in the history
Multiple fixes to prevent file descriptor leaks:

- libxev eventfd now uses CLOEXEC
- linux: cgroup clone now uses CLOEXEC for the cgroup fd
- termio pipe uses pipe2 with CLOEXEC
- pty master always sets CLOEXEC because the child doesn't need it
- termio exec now closes pty slave fd after fork

There still appear to be some fd leaks happening. They seem related to
GTK, they aren't things we're accessig directly. I still want to
investigate them but this at least cleans up the major sources of fd
leakage.
  • Loading branch information
mitchellh authored Jan 24, 2025
2 parents c0eb698 + 0d6a1d3 commit fd8caca
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 10 deletions.
4 changes: 2 additions & 2 deletions build.zig.zon
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
.dependencies = .{
// Zig libs
.libxev = .{
.url = "https://github.com/mitchellh/libxev/archive/db6a52bafadf00360e675fefa7926e8e6c0e9931.tar.gz",
.hash = "12206029de146b685739f69b10a6f08baee86b3d0a5f9a659fa2b2b66c9602078bbf",
.url = "https://github.com/mitchellh/libxev/archive/aceef3d11efacd9d237c91632f930ed13a2834bf.tar.gz",
.hash = "12205b2b47fe61a4cde3a45ee4b9cddee75897739dbc196d6396e117cb1ce28e1ad0",
},
.mach_glfw = .{
.url = "https://github.com/mitchellh/mach-glfw/archive/37c2995f31abcf7e8378fba68ddcf4a3faa02de0.tar.gz",
Expand Down
2 changes: 1 addition & 1 deletion nix/zigCacheHash.nix
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
# This file is auto-generated! check build-support/check-zig-cache-hash.sh for
# more details.
"sha256-H6o4Y09ATIylMUWuL9Y1fHwpuxSWyJ3Pl8fn4VeoDZo="
"sha256-AvfYl8vLxxsRnf/ERpw5jQIro5rVd98q63hwFsgQOvo="
17 changes: 16 additions & 1 deletion src/os/cgroup.zig
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,22 @@ pub fn cloneInto(cgroup: []const u8) !posix.pid_t {
// Get a file descriptor that refers to the cgroup directory in the cgroup
// sysfs to pass to the kernel in clone3.
const fd: linux.fd_t = fd: {
const rc = linux.open(path, linux.O{ .PATH = true, .DIRECTORY = true }, 0);
const rc = linux.open(
path,
.{
// Self-explanatory: we expect to open a directory, and
// we only need the path-level permissions.
.PATH = true,
.DIRECTORY = true,

// We don't want to leak this fd to the child process
// when we clone below since we're using this fd for
// a cgroup clone.
.CLOEXEC = true,
},
0,
);

switch (posix.errno(rc)) {
.SUCCESS => break :fd @as(linux.fd_t, @intCast(rc)),
else => |errno| {
Expand Down
5 changes: 3 additions & 2 deletions src/os/pipe.zig
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ const builtin = @import("builtin");
const windows = @import("windows.zig");
const posix = std.posix;

/// pipe() that works on Windows and POSIX.
/// pipe() that works on Windows and POSIX. For POSIX systems, this sets
/// CLOEXEC on the file descriptors.
pub fn pipe() ![2]posix.fd_t {
switch (builtin.os.tag) {
else => return try posix.pipe(),
else => return try posix.pipe2(.{ .CLOEXEC = true }),
.windows => {
var read: windows.HANDLE = undefined;
var write: windows.HANDLE = undefined;
Expand Down
26 changes: 22 additions & 4 deletions src/pty.zig
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ const PosixPty = struct {
};

/// The file descriptors for the master and slave side of the pty.
/// The slave side is never closed automatically by this struct
/// so the caller is responsible for closing it if things
/// go wrong.
master: Fd,
slave: Fd,

Expand All @@ -117,6 +120,24 @@ const PosixPty = struct {
_ = posix.system.close(slave_fd);
}

// Set CLOEXEC on the master fd, only the slave fd should be inherited
// by the child process (shell/command).
cloexec: {
const flags = std.posix.fcntl(master_fd, std.posix.F.GETFD, 0) catch |err| {
log.warn("error getting flags for master fd err={}", .{err});
break :cloexec;
};

_ = std.posix.fcntl(
master_fd,
std.posix.F.SETFD,
flags | std.posix.FD_CLOEXEC,
) catch |err| {
log.warn("error setting CLOEXEC on master fd err={}", .{err});
break :cloexec;
};
}

// Enable UTF-8 mode. I think this is on by default on Linux but it
// is NOT on by default on macOS so we ensure that it is always set.
var attrs: c.termios = undefined;
Expand All @@ -126,15 +147,14 @@ const PosixPty = struct {
if (c.tcsetattr(master_fd, c.TCSANOW, &attrs) != 0)
return error.OpenptyFailed;

return Pty{
return .{
.master = master_fd,
.slave = slave_fd,
};
}

pub fn deinit(self: *Pty) void {
_ = posix.system.close(self.master);
_ = posix.system.close(self.slave);
self.* = undefined;
}

Expand Down Expand Up @@ -201,8 +221,6 @@ const PosixPty = struct {
// Can close master/slave pair now
posix.close(self.slave);
posix.close(self.master);

// TODO: reset signals
}
};

Expand Down
11 changes: 11 additions & 0 deletions src/termio/Exec.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1098,6 +1098,10 @@ const Subprocess = struct {
});
self.pty = pty;
errdefer {
if (comptime builtin.os.tag != .windows) {
_ = posix.close(pty.slave);
}

pty.deinit();
self.pty = null;
}
Expand Down Expand Up @@ -1182,6 +1186,13 @@ const Subprocess = struct {
log.info("subcommand cgroup={s}", .{self.linux_cgroup orelse "-"});
}

if (comptime builtin.os.tag != .windows) {
// Once our subcommand is started we can close the slave
// side. This prevents the slave fd from being leaked to
// future children.
_ = posix.close(pty.slave);
}

self.command = cmd;
return switch (builtin.os.tag) {
.windows => .{
Expand Down

0 comments on commit fd8caca

Please sign in to comment.