From 0d6a1d3fdb93ee5444a2f998e085266ad443442a Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Thu, 23 Jan 2025 21:22:06 -0800 Subject: [PATCH] Prevent fd leaks to the running shell or command 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. --- build.zig.zon | 4 ++-- nix/zigCacheHash.nix | 2 +- src/os/cgroup.zig | 17 ++++++++++++++++- src/os/pipe.zig | 5 +++-- src/pty.zig | 26 ++++++++++++++++++++++---- src/termio/Exec.zig | 11 +++++++++++ 6 files changed, 55 insertions(+), 10 deletions(-) diff --git a/build.zig.zon b/build.zig.zon index 09dc9847e..9c00a4704 100644 --- a/build.zig.zon +++ b/build.zig.zon @@ -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", diff --git a/nix/zigCacheHash.nix b/nix/zigCacheHash.nix index dfc2e5f7f..c687a5a79 100644 --- a/nix/zigCacheHash.nix +++ b/nix/zigCacheHash.nix @@ -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=" diff --git a/src/os/cgroup.zig b/src/os/cgroup.zig index 0a66c5987..bef101acc 100644 --- a/src/os/cgroup.zig +++ b/src/os/cgroup.zig @@ -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| { diff --git a/src/os/pipe.zig b/src/os/pipe.zig index 392f72083..2cb7bd4a3 100644 --- a/src/os/pipe.zig +++ b/src/os/pipe.zig @@ -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; diff --git a/src/pty.zig b/src/pty.zig index c0d082411..1df09d79c 100644 --- a/src/pty.zig +++ b/src/pty.zig @@ -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, @@ -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; @@ -126,7 +147,7 @@ const PosixPty = struct { if (c.tcsetattr(master_fd, c.TCSANOW, &attrs) != 0) return error.OpenptyFailed; - return Pty{ + return .{ .master = master_fd, .slave = slave_fd, }; @@ -134,7 +155,6 @@ const PosixPty = struct { pub fn deinit(self: *Pty) void { _ = posix.system.close(self.master); - _ = posix.system.close(self.slave); self.* = undefined; } @@ -201,8 +221,6 @@ const PosixPty = struct { // Can close master/slave pair now posix.close(self.slave); posix.close(self.master); - - // TODO: reset signals } }; diff --git a/src/termio/Exec.zig b/src/termio/Exec.zig index e320152ec..c55e66729 100644 --- a/src/termio/Exec.zig +++ b/src/termio/Exec.zig @@ -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; } @@ -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 => .{