From eadc3bcd676277d72c211bde6c20f7036339fd84 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 3 Apr 2015 15:44:14 -0700 Subject: [PATCH] std: Unconditionally close all file descriptors The logic for only closing file descriptors >= 3 was inherited from quite some time ago and ends up meaning that some internal APIs are less consistent than they should be. By unconditionally closing everything entering a `FileDesc` we ensure that we're consistent in our behavior as well as robustly handling the stdio case. --- src/libstd/process.rs | 24 +++++------ src/libstd/sys/unix/fd.rs | 22 +++-------- src/libstd/sys/unix/process2.rs | 59 +++++++++++++++++----------- src/libstd/sys/windows/process2.rs | 47 +++++++++++++++------- src/test/run-pass/fds-are-cloexec.rs | 1 + 5 files changed, 86 insertions(+), 67 deletions(-) diff --git a/src/libstd/process.rs b/src/libstd/process.rs index 9929593ffcb..90eabaee77b 100644 --- a/src/libstd/process.rs +++ b/src/libstd/process.rs @@ -19,13 +19,13 @@ use io::prelude::*; use ffi::OsStr; use fmt; use io::{self, Error, ErrorKind}; -use libc; use path; use sync::mpsc::{channel, Receiver}; use sys::pipe2::{self, AnonPipe}; use sys::process2::Command as CommandImp; use sys::process2::Process as ProcessImp; use sys::process2::ExitStatus as ExitStatusImp; +use sys::process2::Stdio as StdioImp2; use sys_common::{AsInner, AsInnerMut}; use thread; @@ -229,13 +229,13 @@ impl Command { fn spawn_inner(&self, default_io: StdioImp) -> io::Result { let (their_stdin, our_stdin) = try!( - setup_io(self.stdin.as_ref().unwrap_or(&default_io), 0, true) + setup_io(self.stdin.as_ref().unwrap_or(&default_io), true) ); let (their_stdout, our_stdout) = try!( - setup_io(self.stdout.as_ref().unwrap_or(&default_io), 1, false) + setup_io(self.stdout.as_ref().unwrap_or(&default_io), false) ); let (their_stderr, our_stderr) = try!( - setup_io(self.stderr.as_ref().unwrap_or(&default_io), 2, false) + setup_io(self.stderr.as_ref().unwrap_or(&default_io), false) ); match ProcessImp::spawn(&self.inner, their_stdin, their_stdout, their_stderr) { @@ -328,23 +328,19 @@ impl AsInnerMut for Command { fn as_inner_mut(&mut self) -> &mut CommandImp { &mut self.inner } } -fn setup_io(io: &StdioImp, fd: libc::c_int, readable: bool) - -> io::Result<(Option, Option)> +fn setup_io(io: &StdioImp, readable: bool) + -> io::Result<(StdioImp2, Option)> { use self::StdioImp::*; Ok(match *io { - Null => { - (None, None) - } - Inherit => { - (Some(AnonPipe::from_fd(fd)), None) - } + Null => (StdioImp2::None, None), + Inherit => (StdioImp2::Inherit, None), Piped => { let (reader, writer) = try!(pipe2::anon_pipe()); if readable { - (Some(reader), Some(writer)) + (StdioImp2::Piped(reader), Some(writer)) } else { - (Some(writer), Some(reader)) + (StdioImp2::Piped(writer), Some(reader)) } } }) diff --git a/src/libstd/sys/unix/fd.rs b/src/libstd/sys/unix/fd.rs index 4ef09b91c25..d86c77624e8 100644 --- a/src/libstd/sys/unix/fd.rs +++ b/src/libstd/sys/unix/fd.rs @@ -59,13 +59,6 @@ impl FileDesc { debug_assert_eq!(ret, 0); } } - - pub fn unset_cloexec(&self) { - unsafe { - let ret = c::ioctl(self.fd, c::FIONCLEX); - debug_assert_eq!(ret, 0); - } - } } impl AsInner for FileDesc { @@ -74,14 +67,11 @@ impl AsInner for FileDesc { impl Drop for FileDesc { fn drop(&mut self) { - // closing stdio file handles makes no sense, so never do it. Also, note - // that errors are ignored when closing a file descriptor. The reason - // for this is that if an error occurs we don't actually know if the - // file descriptor was closed or not, and if we retried (for something - // like EINTR), we might close another valid file descriptor (opened - // after we closed ours. - if self.fd > libc::STDERR_FILENO { - let _ = unsafe { libc::close(self.fd) }; - } + // Note that errors are ignored when closing a file descriptor. The + // reason for this is that if an error occurs we don't actually know if + // the file descriptor was closed or not, and if we retried (for + // something like EINTR), we might close another valid file descriptor + // (opened after we closed ours. + let _ = unsafe { libc::close(self.fd) }; } } diff --git a/src/libstd/sys/unix/process2.rs b/src/libstd/sys/unix/process2.rs index dc6897fec8e..0b4e871454d 100644 --- a/src/libstd/sys/unix/process2.rs +++ b/src/libstd/sys/unix/process2.rs @@ -119,6 +119,12 @@ pub struct Process { pid: pid_t } +pub enum Stdio { + Inherit, + Piped(AnonPipe), + None, +} + const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX"; impl Process { @@ -128,9 +134,9 @@ impl Process { } pub fn spawn(cfg: &Command, - in_fd: Option, - out_fd: Option, - err_fd: Option) -> io::Result { + in_fd: Stdio, + out_fd: Stdio, + err_fd: Stdio) -> io::Result { let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null()); let (envp, _a, _b) = make_envp(cfg.env.as_ref()); @@ -224,9 +230,9 @@ impl Process { argv: *const *const libc::c_char, envp: *const libc::c_void, dirp: *const libc::c_char, - in_fd: Option, - out_fd: Option, - err_fd: Option) -> ! { + in_fd: Stdio, + out_fd: Stdio, + err_fd: Stdio) -> ! { fn fail(output: &mut AnonPipe) -> ! { let errno = sys::os::errno() as u32; let bytes = [ @@ -244,23 +250,30 @@ impl Process { unsafe { libc::_exit(1) } } - // If a stdio file descriptor is set to be ignored, we don't - // actually close it, but rather open up /dev/null into that - // file descriptor. Otherwise, the first file descriptor opened - // up in the child would be numbered as one of the stdio file - // descriptors, which is likely to wreak havoc. - let setup = |src: Option, dst: c_int| { - src.map(|p| p.into_fd()).or_else(|| { - let mut opts = OpenOptions::new(); - opts.read(dst == libc::STDIN_FILENO); - opts.write(dst != libc::STDIN_FILENO); - let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr() - as *const _); - File::open_c(devnull, &opts).ok().map(|f| f.into_fd()) - }).map(|fd| { - fd.unset_cloexec(); - retry(|| libc::dup2(fd.raw(), dst)) != -1 - }).unwrap_or(false) + let setup = |src: Stdio, dst: c_int| { + let fd = match src { + Stdio::Inherit => return true, + Stdio::Piped(pipe) => pipe.into_fd(), + + // If a stdio file descriptor is set to be ignored, we open up + // /dev/null into that file descriptor. Otherwise, the first + // file descriptor opened up in the child would be numbered as + // one of the stdio file descriptors, which is likely to wreak + // havoc. + Stdio::None => { + let mut opts = OpenOptions::new(); + opts.read(dst == libc::STDIN_FILENO); + opts.write(dst != libc::STDIN_FILENO); + let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr() + as *const _); + if let Ok(f) = File::open_c(devnull, &opts) { + f.into_fd() + } else { + return false + } + } + }; + retry(|| libc::dup2(fd.raw(), dst)) != -1 }; if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) } diff --git a/src/libstd/sys/windows/process2.rs b/src/libstd/sys/windows/process2.rs index 7e832b6384d..74953921921 100644 --- a/src/libstd/sys/windows/process2.rs +++ b/src/libstd/sys/windows/process2.rs @@ -105,11 +105,18 @@ pub struct Process { handle: Handle, } +pub enum Stdio { + Inherit, + Piped(AnonPipe), + None, +} + impl Process { #[allow(deprecated)] pub fn spawn(cfg: &Command, - in_fd: Option, out_fd: Option, err_fd: Option) - -> io::Result + in_fd: Stdio, + out_fd: Stdio, + err_fd: Stdio) -> io::Result { use libc::types::os::arch::extra::{DWORD, HANDLE, STARTUPINFO}; use libc::consts::os::extra::{ @@ -156,13 +163,16 @@ impl Process { let cur_proc = GetCurrentProcess(); - // Similarly to unix, we don't actually leave holes for the stdio file - // descriptors, but rather open up /dev/null equivalents. These - // equivalents are drawn from libuv's windows process spawning. - let set_fd = |fd: &Option, slot: &mut HANDLE, + let set_fd = |fd: &Stdio, slot: &mut HANDLE, is_stdin: bool| { match *fd { - None => { + Stdio::Inherit => {} + + // Similarly to unix, we don't actually leave holes for the + // stdio file descriptors, but rather open up /dev/null + // equivalents. These equivalents are drawn from libuv's + // windows process spawning. + Stdio::None => { let access = if is_stdin { libc::FILE_GENERIC_READ } else { @@ -188,11 +198,8 @@ impl Process { return Err(Error::last_os_error()) } } - Some(ref pipe) => { + Stdio::Piped(ref pipe) => { let orig = pipe.raw(); - if orig == INVALID_HANDLE_VALUE { - return Err(Error::last_os_error()) - } if DuplicateHandle(cur_proc, orig, cur_proc, slot, 0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE { return Err(Error::last_os_error()) @@ -235,9 +242,15 @@ impl Process { }) }); - assert!(CloseHandle(si.hStdInput) != 0); - assert!(CloseHandle(si.hStdOutput) != 0); - assert!(CloseHandle(si.hStdError) != 0); + if !in_fd.inherited() { + assert!(CloseHandle(si.hStdInput) != 0); + } + if !out_fd.inherited() { + assert!(CloseHandle(si.hStdOutput) != 0); + } + if !err_fd.inherited() { + assert!(CloseHandle(si.hStdError) != 0); + } match create_err { Some(err) => return Err(err), @@ -296,6 +309,12 @@ impl Process { } } +impl Stdio { + fn inherited(&self) -> bool { + match *self { Stdio::Inherit => true, _ => false } + } +} + #[derive(PartialEq, Eq, Clone, Copy, Debug)] pub struct ExitStatus(i32); diff --git a/src/test/run-pass/fds-are-cloexec.rs b/src/test/run-pass/fds-are-cloexec.rs index ec59b4dc03d..cbf7830513a 100644 --- a/src/test/run-pass/fds-are-cloexec.rs +++ b/src/test/run-pass/fds-are-cloexec.rs @@ -9,6 +9,7 @@ // except according to those terms. // ignore-windows +// ignore-android #![feature(libc)]