From 643aedaedbc8d39de96f0efeb006bc72d46d23fb Mon Sep 17 00:00:00 2001 From: Jesse Luehrs Date: Thu, 30 Dec 2021 01:14:50 -0500 Subject: better handling of file descriptors across fork --- src/blocking/command.rs | 7 +++---- src/command.rs | 6 +++--- src/sys.rs | 32 ++++++++++---------------------- 3 files changed, 16 insertions(+), 29 deletions(-) diff --git a/src/blocking/command.rs b/src/blocking/command.rs index 7f8d905..b2ca3e4 100644 --- a/src/blocking/command.rs +++ b/src/blocking/command.rs @@ -104,15 +104,14 @@ impl Command { pty: &crate::blocking::Pty, ) -> crate::Result { let (stdin, stdout, stderr, mut pre_exec) = - crate::sys::setup_subprocess(pty.pts(), Some(pty))?; + crate::sys::setup_subprocess(pty.pts())?; self.inner.stdin(self.stdin.take().unwrap_or(stdin)); self.inner.stdout(self.stdout.take().unwrap_or(stdout)); self.inner.stderr(self.stderr.take().unwrap_or(stderr)); - // safe because setsid() and close() are async-signal-safe functions - // and ioctl() is a raw syscall (which is inherently - // async-signal-safe). + // safe because setsid() is an async-signal-safe function and ioctl() + // is a raw syscall (which is inherently async-signal-safe). if let Some(mut custom) = self.pre_exec.take() { unsafe { self.inner.pre_exec(move || { diff --git a/src/command.rs b/src/command.rs index dfe9684..c50af30 100644 --- a/src/command.rs +++ b/src/command.rs @@ -104,14 +104,14 @@ impl Command { pty: &crate::Pty, ) -> crate::Result { let (stdin, stdout, stderr, mut pre_exec) = - crate::sys::setup_subprocess(pty.pts(), Some(pty))?; + crate::sys::setup_subprocess(pty.pts())?; self.inner.stdin(self.stdin.take().unwrap_or(stdin)); self.inner.stdout(self.stdout.take().unwrap_or(stdout)); self.inner.stderr(self.stderr.take().unwrap_or(stderr)); - // Safety: setsid() and close() are async-signal-safe functions and - // ioctl() is a raw syscall (which is inherently async-signal-safe). + // Safety: setsid() is an async-signal-safe function and ioctl() is a + // raw syscall (which is inherently async-signal-safe). if let Some(mut custom) = self.pre_exec.take() { unsafe { self.inner.pre_exec(move || { diff --git a/src/sys.rs b/src/sys.rs index ae789cd..b837949 100644 --- a/src/sys.rs +++ b/src/sys.rs @@ -2,7 +2,9 @@ use std::os::unix::io::{FromRawFd as _, IntoRawFd as _}; pub fn create_pt() -> nix::Result<(std::fs::File, std::path::PathBuf)> { let pt = nix::pty::posix_openpt( - nix::fcntl::OFlag::O_RDWR | nix::fcntl::OFlag::O_NOCTTY, + nix::fcntl::OFlag::O_RDWR + | nix::fcntl::OFlag::O_NOCTTY + | nix::fcntl::OFlag::O_CLOEXEC, )?; nix::pty::grantpt(&pt)?; nix::pty::unlockpt(&pt)?; @@ -40,19 +42,20 @@ pub fn set_term_size( pub fn setup_subprocess( pts: &impl std::os::unix::io::AsRawFd, - pt: Option<&impl std::os::unix::io::AsRawFd>, ) -> nix::Result<( std::process::Stdio, std::process::Stdio, std::process::Stdio, impl FnMut() -> std::io::Result<()>, )> { - let pt_fd = pt.map(std::os::unix::io::AsRawFd::as_raw_fd); let pts_fd = pts.as_raw_fd(); - let stdin = nix::unistd::dup(pts_fd)?; - let stdout = nix::unistd::dup(pts_fd)?; - let stderr = nix::unistd::dup(pts_fd)?; + let stdin = + nix::fcntl::fcntl(pts_fd, nix::fcntl::FcntlArg::F_DUPFD_CLOEXEC(0))?; + let stdout = + nix::fcntl::fcntl(pts_fd, nix::fcntl::FcntlArg::F_DUPFD_CLOEXEC(0))?; + let stderr = + nix::fcntl::fcntl(pts_fd, nix::fcntl::FcntlArg::F_DUPFD_CLOEXEC(0))?; // Safety: these file descriptors were all just returned from dup, so they // must be valid @@ -64,22 +67,7 @@ pub fn setup_subprocess( nix::unistd::setsid()?; set_controlling_terminal(pts_fd)?; - // in the parent, destructors will handle closing these file - // descriptors (other than pt, used by the parent to - // communicate with the child) when the function ends, but in - // the child, we end by calling exec(), which doesn't call - // destructors. - - if let Some(pt_fd) = pt_fd { - nix::unistd::close(pt_fd)?; - } - nix::unistd::close(pts_fd)?; - // at this point, stdin/stdout/stderr have already been - // reopened as fds 0/1/2 in the child, so we can (and should) - // close the originals - nix::unistd::close(stdin)?; - nix::unistd::close(stdout)?; - nix::unistd::close(stderr)?; + // no need to close anything, since we set cloexec everywhere Ok(()) }, -- cgit v1.2.3-54-g00ecf