From e1bfcf85db56f75a2f6d67143aad2da22c3d2240 Mon Sep 17 00:00:00 2001 From: Cole Miller Date: Thu, 11 Jun 2026 12:22:21 -0400 Subject: [PATCH] Fix file descriptor leak when process spawning failed on macOS (#59128) Wrap the fds returned by `create_pipe` and `open_dev_null` in `std::fs::File` earlier, so that they are closed if we return early due to an error. Includes a regression test. Self-Review Checklist: - [x] I've reviewed my own diff for quality, security, and reliability - [x] Unsafe blocks (if any) have justifying comments - [x] The content adheres to Zed's UI standards ([UX/UI](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist) and [icon](https://github.com/zed-industries/zed/blob/main/crates/icons/README.md) guidelines) - [x] Tests cover the new/changed behavior - [x] Performance impact has been considered and is acceptable Release Notes: - Fixed a file descriptor leak on macOS. --------- Co-authored-by: Marshall Bowers --- crates/util/src/command/darwin.rs | 78 +++++++++++++++++++------------ 1 file changed, 49 insertions(+), 29 deletions(-) diff --git a/crates/util/src/command/darwin.rs b/crates/util/src/command/darwin.rs index c9d5bc40ae..2c772d2c08 100644 --- a/crates/util/src/command/darwin.rs +++ b/crates/util/src/command/darwin.rs @@ -7,6 +7,7 @@ use smol::Unblock; use std::collections::BTreeMap; use std::ffi::{CString, OsStr, OsString}; use std::io; +use std::os::fd::AsRawFd; use std::os::unix::ffi::OsStrExt; use std::os::unix::io::FromRawFd; use std::os::unix::process::ExitStatusExt; @@ -445,10 +446,10 @@ fn spawn_posix_spawn( current_dir_cstr.as_ptr(), ))?; - if let Some(fd) = stdin_read { + if let Some(fd) = &stdin_read { cvt_nz(libc::posix_spawn_file_actions_adddup2( &mut file_actions, - fd, + fd.as_raw_fd(), libc::STDIN_FILENO, ))?; cvt_nz(posix_spawn_file_actions_addinherit_np( @@ -457,10 +458,10 @@ fn spawn_posix_spawn( ))?; } - if let Some(fd) = stdout_write { + if let Some(fd) = &stdout_write { cvt_nz(libc::posix_spawn_file_actions_adddup2( &mut file_actions, - fd, + fd.as_raw_fd(), libc::STDOUT_FILENO, ))?; cvt_nz(posix_spawn_file_actions_addinherit_np( @@ -469,10 +470,10 @@ fn spawn_posix_spawn( ))?; } - if let Some(fd) = stderr_write { + if let Some(fd) = &stderr_write { cvt_nz(libc::posix_spawn_file_actions_adddup2( &mut file_actions, - fd, + fd.as_raw_fd(), libc::STDERR_FILENO, ))?; cvt_nz(posix_spawn_file_actions_addinherit_np( @@ -499,30 +500,20 @@ fn spawn_posix_spawn( libc::posix_spawnattr_destroy(&mut attr); libc::posix_spawn_file_actions_destroy(&mut file_actions); - if let Some(fd) = stdin_read { - libc::close(fd); - } - if let Some(fd) = stdout_write { - libc::close(fd); - } - if let Some(fd) = stderr_write { - libc::close(fd); - } - cvt_nz(spawn_result)?; Ok(Child { pid, - stdin: stdin_write.map(|fd| Unblock::new(std::fs::File::from_raw_fd(fd))), - stdout: stdout_read.map(|fd| Unblock::new(std::fs::File::from_raw_fd(fd))), - stderr: stderr_read.map(|fd| Unblock::new(std::fs::File::from_raw_fd(fd))), + stdin: stdin_write.map(|fd| Unblock::new(fd)), + stdout: stdout_read.map(|fd| Unblock::new(fd)), + stderr: stderr_read.map(|fd| Unblock::new(fd)), kill_on_drop, status: None, }) } } -fn create_pipe() -> io::Result<(libc::c_int, libc::c_int)> { +fn create_pipe() -> io::Result<(std::fs::File, std::fs::File)> { let mut fds: [libc::c_int; 2] = [0; 2]; unsafe { let result = libc::pipe(fds.as_mut_ptr()); @@ -548,11 +539,14 @@ fn create_pipe() -> io::Result<(libc::c_int, libc::c_int)> { } } - Ok((fds[0], fds[1])) + Ok(( + std::fs::File::from_raw_fd(fds[0]), + std::fs::File::from_raw_fd(fds[1]), + )) } } -fn open_dev_null(flags: libc::c_int) -> io::Result { +fn open_dev_null(flags: libc::c_int) -> io::Result { // Set close-on-exec for this pipe, for the same reason as in `create_pipe`. let fd = unsafe { libc::open( @@ -563,7 +557,7 @@ fn open_dev_null(flags: libc::c_int) -> io::Result { if fd == -1 { return Err(io::Error::last_os_error()); } - Ok(fd) + Ok(unsafe { std::fs::File::from_raw_fd(fd) }) } /// Zero means `Ok()`, all other values are treated as raw OS errors. Does not look at `errno`. @@ -597,7 +591,9 @@ mod tests { // git's stdin write end open and deadlock the git child on `read()`. #[test] fn test_create_pipe_not_inherited_by_unrelated_spawn() { - let (read_fd, write_fd) = create_pipe().expect("create_pipe failed"); + let (read_file, write_file) = create_pipe().expect("create_pipe failed"); + let read_fd = read_file.as_raw_fd(); + let write_fd = write_file.as_raw_fd(); // Probe with the exact fds returned by `create_pipe` (no dup), since // duping with `F_DUPFD` would lose CLOEXEC and `F_DUPFD_CLOEXEC` would @@ -620,11 +616,6 @@ mod tests { let stdout = String::from_utf8_lossy(&output.stdout).into_owned(); - unsafe { - libc::close(read_fd); - libc::close(write_fd); - } - assert_eq!( stdout, format!("{read_fd} WAS NOT INHERITED\n{write_fd} WAS NOT INHERITED\nDONE\n") @@ -900,4 +891,33 @@ mod tests { assert_eq!(output.stdout, b"piped input"); }); } + + #[test] + fn test_stdio_fds_closed_on_error() { + fn count_open_fds() -> usize { + let limit = unsafe { libc::getdtablesize() }; + (0..limit) + .filter(|&fd| unsafe { libc::fcntl(fd, libc::F_GETFD) } != -1) + .count() + } + + const ATTEMPTS: usize = 100; + const SLACK: usize = 32; + + let before = count_open_fds(); + for _ in 0..ATTEMPTS { + Command::new("/bin/binarythatdoesnotexist") + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect_err("spawn should fail for a nonexistent binary"); + } + let after = count_open_fds(); + + assert!( + after <= before + SLACK, + "fd leak detected: {before} open fds before, {after} after {ATTEMPTS} failed spawns" + ); + } }