From d6c72306c8fc2ec0fd9d6e499c32f2bf52f0b8ba Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 3 Apr 2015 15:30:10 -0700 Subject: [PATCH 1/3] std: Set CLOEXEC for all fds opened on unix This commit starts to set the CLOEXEC flag for all files and sockets opened by the standard library by default on all unix platforms. There are a few points of note in this commit: * The implementation is not 100% satisfactory in the face of threads. File descriptors only have the `F_CLOEXEC` flag set *after* they are opened, allowing for a fork/exec to happen in the middle and leak the descriptor. Some platforms do support atomically opening a descriptor while setting the `CLOEXEC` flag, and it is left as a future extension to bind these apis as it is unclear how to do so nicely at this time. * The implementation does not offer a method of opting into the old behavior of not setting `CLOEXEC`. This will possibly be added in the future through extensions on `OpenOptions`, for example. * This change does not yet audit any Windows APIs to see if the handles are inherited by default by accident. This is a breaking change for users who call `fork` or `exec` outside of the standard library itself and expect file descriptors to be inherted. All file descriptors created by the standard library will no longer be inherited. [breaking-change] --- src/libstd/sys/unix/c.rs | 42 +++++++--------- src/libstd/sys/unix/fd.rs | 15 ++++++ src/libstd/sys/unix/fs2.rs | 4 +- src/libstd/sys/unix/net.rs | 15 ++++-- src/libstd/sys/unix/pipe2.rs | 4 +- src/test/run-pass/fds-are-cloexec.rs | 75 ++++++++++++++++++++++++++++ 6 files changed, 125 insertions(+), 30 deletions(-) create mode 100644 src/test/run-pass/fds-are-cloexec.rs diff --git a/src/libstd/sys/unix/c.rs b/src/libstd/sys/unix/c.rs index 5ae508e4610..282e5668e6e 100644 --- a/src/libstd/sys/unix/c.rs +++ b/src/libstd/sys/unix/c.rs @@ -26,39 +26,35 @@ use libc; target_os = "dragonfly", target_os = "bitrig", target_os = "openbsd"))] -pub const FIONBIO: libc::c_ulong = 0x8004667e; +mod consts { + use libc; + pub const FIONBIO: libc::c_ulong = 0x8004667e; + pub const FIOCLEX: libc::c_ulong = 0x20006601; + pub const FIONCLEX: libc::c_ulong = 0x20006602; +} #[cfg(any(all(target_os = "linux", any(target_arch = "x86", target_arch = "x86_64", target_arch = "arm", target_arch = "aarch64")), target_os = "android"))] -pub const FIONBIO: libc::c_ulong = 0x5421; +mod consts { + use libc; + pub const FIONBIO: libc::c_ulong = 0x5421; + pub const FIOCLEX: libc::c_ulong = 0x5451; + pub const FIONCLEX: libc::c_ulong = 0x5450; +} #[cfg(all(target_os = "linux", any(target_arch = "mips", target_arch = "mipsel", target_arch = "powerpc")))] -pub const FIONBIO: libc::c_ulong = 0x667e; - -#[cfg(any(target_os = "macos", - target_os = "ios", - target_os = "freebsd", - target_os = "dragonfly", - target_os = "bitrig", - target_os = "openbsd"))] -pub const FIOCLEX: libc::c_ulong = 0x20006601; -#[cfg(any(all(target_os = "linux", - any(target_arch = "x86", - target_arch = "x86_64", - target_arch = "arm", - target_arch = "aarch64")), - target_os = "android"))] -pub const FIOCLEX: libc::c_ulong = 0x5451; -#[cfg(all(target_os = "linux", - any(target_arch = "mips", - target_arch = "mipsel", - target_arch = "powerpc")))] -pub const FIOCLEX: libc::c_ulong = 0x6601; +mod consts { + use libc; + pub const FIONBIO: libc::c_ulong = 0x667e; + pub const FIOCLEX: libc::c_ulong = 0x6601; + pub const FIONCLEX: libc::c_ulong = 0x6600; +} +pub use self::consts::*; #[cfg(any(target_os = "macos", target_os = "ios", diff --git a/src/libstd/sys/unix/fd.rs b/src/libstd/sys/unix/fd.rs index f7c57c3f5e5..4ef09b91c25 100644 --- a/src/libstd/sys/unix/fd.rs +++ b/src/libstd/sys/unix/fd.rs @@ -13,6 +13,7 @@ use core::prelude::*; use io; use libc::{self, c_int, size_t, c_void}; use mem; +use sys::c; use sys::cvt; use sys_common::AsInner; @@ -51,6 +52,20 @@ impl FileDesc { })); Ok(ret as usize) } + + pub fn set_cloexec(&self) { + unsafe { + let ret = c::ioctl(self.fd, c::FIOCLEX); + 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 { diff --git a/src/libstd/sys/unix/fs2.rs b/src/libstd/sys/unix/fs2.rs index c0426af051b..20b1aac8f45 100644 --- a/src/libstd/sys/unix/fs2.rs +++ b/src/libstd/sys/unix/fs2.rs @@ -215,7 +215,9 @@ impl File { let fd = try!(cvt_r(|| unsafe { libc::open(path.as_ptr(), flags, opts.mode) })); - Ok(File(FileDesc::new(fd))) + let fd = FileDesc::new(fd); + fd.set_cloexec(); + Ok(File(fd)) } pub fn file_attr(&self) -> io::Result { diff --git a/src/libstd/sys/unix/net.rs b/src/libstd/sys/unix/net.rs index 908136a42ab..2e1cbb2a1e1 100644 --- a/src/libstd/sys/unix/net.rs +++ b/src/libstd/sys/unix/net.rs @@ -47,7 +47,9 @@ impl Socket { }; unsafe { let fd = try!(cvt(libc::socket(fam, ty, 0))); - Ok(Socket(FileDesc::new(fd))) + let fd = FileDesc::new(fd); + fd.set_cloexec(); + Ok(Socket(fd)) } } @@ -56,13 +58,16 @@ impl Socket { let fd = try!(cvt_r(|| unsafe { libc::accept(self.0.raw(), storage, len) })); - Ok(Socket(FileDesc::new(fd))) + let fd = FileDesc::new(fd); + fd.set_cloexec(); + Ok(Socket(fd)) } pub fn duplicate(&self) -> io::Result { - cvt(unsafe { libc::dup(self.0.raw()) }).map(|fd| { - Socket(FileDesc::new(fd)) - }) + let fd = try!(cvt(unsafe { libc::dup(self.0.raw()) })); + let fd = FileDesc::new(fd); + fd.set_cloexec(); + Ok(Socket(fd)) } pub fn read(&self, buf: &mut [u8]) -> io::Result { diff --git a/src/libstd/sys/unix/pipe2.rs b/src/libstd/sys/unix/pipe2.rs index 7af2c0f0b2a..2fd4d6dd311 100644 --- a/src/libstd/sys/unix/pipe2.rs +++ b/src/libstd/sys/unix/pipe2.rs @@ -32,7 +32,9 @@ pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { impl AnonPipe { pub fn from_fd(fd: libc::c_int) -> AnonPipe { - AnonPipe(FileDesc::new(fd)) + let fd = FileDesc::new(fd); + fd.set_cloexec(); + AnonPipe(fd) } pub fn read(&self, buf: &mut [u8]) -> io::Result { diff --git a/src/test/run-pass/fds-are-cloexec.rs b/src/test/run-pass/fds-are-cloexec.rs new file mode 100644 index 00000000000..ec59b4dc03d --- /dev/null +++ b/src/test/run-pass/fds-are-cloexec.rs @@ -0,0 +1,75 @@ +// Copyright 2015 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +// ignore-windows + +#![feature(libc)] + +extern crate libc; + +use std::env; +use std::fs::{self, File}; +use std::io; +use std::net::{TcpListener, TcpStream, UdpSocket}; +use std::os::unix::prelude::*; +use std::process::Command; +use std::thread; + +fn main() { + let args = env::args().collect::>(); + if args.len() == 1 { + parent() + } else { + child(&args) + } +} + +fn parent() { + let file = File::open("Makefile").unwrap(); + let _dir = fs::read_dir("/").unwrap(); + let tcp1 = TcpListener::bind("127.0.0.1:0").unwrap(); + assert_eq!(tcp1.as_raw_fd(), file.as_raw_fd() + 2); + let tcp2 = tcp1.try_clone().unwrap(); + let addr = tcp1.local_addr().unwrap(); + let t = thread::scoped(|| TcpStream::connect(addr).unwrap()); + let tcp3 = tcp1.accept().unwrap().0; + let tcp4 = t.join(); + let tcp5 = tcp3.try_clone().unwrap(); + let tcp6 = tcp4.try_clone().unwrap(); + let udp1 = UdpSocket::bind("127.0.0.1:0").unwrap(); + let udp2 = udp1.try_clone().unwrap(); + + let status = Command::new(env::args().next().unwrap()) + .arg(file.as_raw_fd().to_string()) + .arg((file.as_raw_fd() + 1).to_string()) + .arg(tcp1.as_raw_fd().to_string()) + .arg(tcp2.as_raw_fd().to_string()) + .arg(tcp3.as_raw_fd().to_string()) + .arg(tcp4.as_raw_fd().to_string()) + .arg(tcp5.as_raw_fd().to_string()) + .arg(tcp6.as_raw_fd().to_string()) + .arg(udp1.as_raw_fd().to_string()) + .arg(udp2.as_raw_fd().to_string()) + .status() + .unwrap(); + assert!(status.success()); +} + +fn child(args: &[String]) { + let mut b = [0u8; 2]; + for arg in &args[1..] { + let fd: libc::c_int = arg.parse().unwrap(); + unsafe { + assert_eq!(libc::read(fd, b.as_mut_ptr() as *mut _, 2), -1); + assert_eq!(io::Error::last_os_error().raw_os_error(), + Some(libc::EBADF)); + } + } +} From 33a2191d0b880242b3bf9a32477a6b432f931c80 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 3 Apr 2015 15:34:15 -0700 Subject: [PATCH 2/3] std: Clean up process spawn impl on unix * De-indent quite a bit by removing usage of FnOnce closures * Clearly separate code for the parent/child after the fork * Use `fs2::{File, OpenOptions}` instead of calling `open` manually * Use RAII to close I/O objects wherever possible * Remove loop for closing all file descriptors, all our own ones are now `CLOEXEC` by default so they cannot be inherited --- src/libstd/process.rs | 2 +- src/libstd/sys/unix/c.rs | 2 + src/libstd/sys/unix/fs2.rs | 8 +- src/libstd/sys/unix/pipe2.rs | 11 +- src/libstd/sys/unix/process2.rs | 423 ++++++++++++++------------------ src/libstd/sys/windows/pipe2.rs | 18 +- 6 files changed, 214 insertions(+), 250 deletions(-) diff --git a/src/libstd/process.rs b/src/libstd/process.rs index 52f5965db80..9929593ffcb 100644 --- a/src/libstd/process.rs +++ b/src/libstd/process.rs @@ -340,7 +340,7 @@ fn setup_io(io: &StdioImp, fd: libc::c_int, readable: bool) (Some(AnonPipe::from_fd(fd)), None) } Piped => { - let (reader, writer) = try!(unsafe { pipe2::anon_pipe() }); + let (reader, writer) = try!(pipe2::anon_pipe()); if readable { (Some(reader), Some(writer)) } else { diff --git a/src/libstd/sys/unix/c.rs b/src/libstd/sys/unix/c.rs index 282e5668e6e..aa4bf821207 100644 --- a/src/libstd/sys/unix/c.rs +++ b/src/libstd/sys/unix/c.rs @@ -159,6 +159,8 @@ extern { pub fn utimes(filename: *const libc::c_char, times: *const libc::timeval) -> libc::c_int; pub fn gai_strerror(errcode: libc::c_int) -> *const libc::c_char; + pub fn setgroups(ngroups: libc::c_int, + ptr: *const libc::c_void) -> libc::c_int; } #[cfg(any(target_os = "macos", target_os = "ios"))] diff --git a/src/libstd/sys/unix/fs2.rs b/src/libstd/sys/unix/fs2.rs index 20b1aac8f45..ac121f1c82e 100644 --- a/src/libstd/sys/unix/fs2.rs +++ b/src/libstd/sys/unix/fs2.rs @@ -205,13 +205,17 @@ impl OpenOptions { impl File { pub fn open(path: &Path, opts: &OpenOptions) -> io::Result { + let path = try!(cstr(path)); + File::open_c(&path, opts) + } + + pub fn open_c(path: &CStr, opts: &OpenOptions) -> io::Result { let flags = opts.flags | match (opts.read, opts.write) { (true, true) => libc::O_RDWR, (false, true) => libc::O_WRONLY, (true, false) | (false, false) => libc::O_RDONLY, }; - let path = try!(cstr(path)); let fd = try!(cvt_r(|| unsafe { libc::open(path.as_ptr(), flags, opts.mode) })); @@ -220,6 +224,8 @@ impl File { Ok(File(fd)) } + pub fn into_fd(self) -> FileDesc { self.0 } + pub fn file_attr(&self) -> io::Result { let mut stat: libc::stat = unsafe { mem::zeroed() }; try!(cvt(unsafe { libc::fstat(self.0.raw(), &mut stat) })); diff --git a/src/libstd/sys/unix/pipe2.rs b/src/libstd/sys/unix/pipe2.rs index 2fd4d6dd311..e9d8c69fefb 100644 --- a/src/libstd/sys/unix/pipe2.rs +++ b/src/libstd/sys/unix/pipe2.rs @@ -20,11 +20,10 @@ use libc; pub struct AnonPipe(FileDesc); -pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { +pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { let mut fds = [0; 2]; - if libc::pipe(fds.as_mut_ptr()) == 0 { - Ok((AnonPipe::from_fd(fds[0]), - AnonPipe::from_fd(fds[1]))) + if unsafe { libc::pipe(fds.as_mut_ptr()) == 0 } { + Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1]))) } else { Err(io::Error::last_os_error()) } @@ -45,7 +44,7 @@ impl AnonPipe { self.0.write(buf) } - pub fn raw(&self) -> libc::c_int { - self.0.raw() + pub fn into_fd(self) -> FileDesc { + self.0 } } diff --git a/src/libstd/sys/unix/process2.rs b/src/libstd/sys/unix/process2.rs index 60f00c80b4a..dc6897fec8e 100644 --- a/src/libstd/sys/unix/process2.rs +++ b/src/libstd/sys/unix/process2.rs @@ -13,14 +13,14 @@ use os::unix::prelude::*; use collections::HashMap; use env; -use ffi::{OsString, OsStr, CString}; +use ffi::{OsString, OsStr, CString, CStr}; use fmt; use io::{self, Error, ErrorKind}; use libc::{self, pid_t, c_void, c_int, gid_t, uid_t}; -use mem; use ptr; use sys::pipe2::AnonPipe; use sys::{self, retry, c, cvt}; +use sys::fs2::{File, OpenOptions}; //////////////////////////////////////////////////////////////////////////////// // Command @@ -128,221 +128,178 @@ impl Process { } pub fn spawn(cfg: &Command, - in_fd: Option, out_fd: Option, err_fd: Option) - -> io::Result - { - use libc::funcs::posix88::unistd::{fork, dup2, close, chdir, execvp}; + in_fd: Option, + out_fd: Option, + err_fd: Option) -> io::Result { + let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null()); - mod rustrt { - extern { - pub fn rust_unset_sigprocmask(); + let (envp, _a, _b) = make_envp(cfg.env.as_ref()); + let (argv, _a) = make_argv(&cfg.program, &cfg.args); + let (input, output) = try!(sys::pipe2::anon_pipe()); + + let pid = unsafe { + match libc::fork() { + 0 => { + drop(input); + Process::child_after_fork(cfg, output, argv, envp, dirp, + in_fd, out_fd, err_fd) + } + n if n < 0 => return Err(Error::last_os_error()), + n => n, + } + }; + + let p = Process{ pid: pid }; + drop(output); + let mut bytes = [0; 8]; + + // loop to handle EINTR + loop { + match input.read(&mut bytes) { + Ok(0) => return Ok(p), + Ok(8) => { + assert!(combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4.. 8]), + "Validation on the CLOEXEC pipe failed: {:?}", bytes); + let errno = combine(&bytes[0.. 4]); + assert!(p.wait().is_ok(), + "wait() should either return Ok or panic"); + return Err(Error::from_raw_os_error(errno)) + } + Err(ref e) if e.kind() == ErrorKind::Interrupted => {} + Err(e) => { + assert!(p.wait().is_ok(), + "wait() should either return Ok or panic"); + panic!("the CLOEXEC pipe failed: {:?}", e) + }, + Ok(..) => { // pipe I/O up to PIPE_BUF bytes should be atomic + assert!(p.wait().is_ok(), + "wait() should either return Ok or panic"); + panic!("short read on the CLOEXEC pipe") + } } } - unsafe fn set_cloexec(fd: c_int) { - let ret = c::ioctl(fd, c::FIOCLEX); - assert_eq!(ret, 0); + fn combine(arr: &[u8]) -> i32 { + let a = arr[0] as u32; + let b = arr[1] as u32; + let c = arr[2] as u32; + let d = arr[3] as u32; + + ((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32 + } + } + + // And at this point we've reached a special time in the life of the + // child. The child must now be considered hamstrung and unable to + // do anything other than syscalls really. Consider the following + // scenario: + // + // 1. Thread A of process 1 grabs the malloc() mutex + // 2. Thread B of process 1 forks(), creating thread C + // 3. Thread C of process 2 then attempts to malloc() + // 4. The memory of process 2 is the same as the memory of + // process 1, so the mutex is locked. + // + // This situation looks a lot like deadlock, right? It turns out + // that this is what pthread_atfork() takes care of, which is + // presumably implemented across platforms. The first thing that + // threads to *before* forking is to do things like grab the malloc + // mutex, and then after the fork they unlock it. + // + // Despite this information, libnative's spawn has been witnessed to + // deadlock on both OSX and FreeBSD. I'm not entirely sure why, but + // all collected backtraces point at malloc/free traffic in the + // child spawned process. + // + // For this reason, the block of code below should contain 0 + // invocations of either malloc of free (or their related friends). + // + // As an example of not having malloc/free traffic, we don't close + // this file descriptor by dropping the FileDesc (which contains an + // allocation). Instead we just close it manually. This will never + // have the drop glue anyway because this code never returns (the + // child will either exec() or invoke libc::exit) + unsafe fn child_after_fork(cfg: &Command, + mut output: AnonPipe, + 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) -> ! { + fn fail(output: &mut AnonPipe) -> ! { + let errno = sys::os::errno() as u32; + let bytes = [ + (errno >> 24) as u8, + (errno >> 16) as u8, + (errno >> 8) as u8, + (errno >> 0) as u8, + CLOEXEC_MSG_FOOTER[0], CLOEXEC_MSG_FOOTER[1], + CLOEXEC_MSG_FOOTER[2], CLOEXEC_MSG_FOOTER[3] + ]; + // pipe I/O up to PIPE_BUF bytes should be atomic, and then we want + // to be sure we *don't* run at_exit destructors as we're being torn + // down regardless + assert!(output.write(&bytes).is_ok()); + unsafe { libc::_exit(1) } } - #[cfg(all(target_os = "android", target_arch = "aarch64"))] - unsafe fn getdtablesize() -> c_int { - libc::sysconf(libc::consts::os::sysconf::_SC_OPEN_MAX) as c_int - } + // 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) + }; - #[cfg(not(all(target_os = "android", target_arch = "aarch64")))] - unsafe fn getdtablesize() -> c_int { - libc::funcs::bsd44::getdtablesize() - } + if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) } + if !setup(out_fd, libc::STDOUT_FILENO) { fail(&mut output) } + if !setup(err_fd, libc::STDERR_FILENO) { fail(&mut output) } - let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null()); - - with_envp(cfg.env.as_ref(), |envp: *const c_void| { - with_argv(&cfg.program, &cfg.args, |argv: *const *const libc::c_char| unsafe { - let (input, mut output) = try!(sys::pipe2::anon_pipe()); - - // We may use this in the child, so perform allocations before the - // fork - let devnull = b"/dev/null\0"; - - set_cloexec(output.raw()); - - let pid = fork(); - if pid < 0 { - return Err(Error::last_os_error()) - } else if pid > 0 { - #[inline] - fn combine(arr: &[u8]) -> i32 { - let a = arr[0] as u32; - let b = arr[1] as u32; - let c = arr[2] as u32; - let d = arr[3] as u32; - - ((a << 24) | (b << 16) | (c << 8) | (d << 0)) as i32 - } - - let p = Process{ pid: pid }; - drop(output); - let mut bytes = [0; 8]; - - // loop to handle EINTER - loop { - match input.read(&mut bytes) { - Ok(8) => { - assert!(combine(CLOEXEC_MSG_FOOTER) == combine(&bytes[4.. 8]), - "Validation on the CLOEXEC pipe failed: {:?}", bytes); - let errno = combine(&bytes[0.. 4]); - assert!(p.wait().is_ok(), - "wait() should either return Ok or panic"); - return Err(Error::from_raw_os_error(errno)) - } - Ok(0) => return Ok(p), - Err(ref e) if e.kind() == ErrorKind::Interrupted => {} - Err(e) => { - assert!(p.wait().is_ok(), - "wait() should either return Ok or panic"); - panic!("the CLOEXEC pipe failed: {:?}", e) - }, - Ok(..) => { // pipe I/O up to PIPE_BUF bytes should be atomic - assert!(p.wait().is_ok(), - "wait() should either return Ok or panic"); - panic!("short read on the CLOEXEC pipe") - } - } - } - } - - // And at this point we've reached a special time in the life of the - // child. The child must now be considered hamstrung and unable to - // do anything other than syscalls really. Consider the following - // scenario: - // - // 1. Thread A of process 1 grabs the malloc() mutex - // 2. Thread B of process 1 forks(), creating thread C - // 3. Thread C of process 2 then attempts to malloc() - // 4. The memory of process 2 is the same as the memory of - // process 1, so the mutex is locked. - // - // This situation looks a lot like deadlock, right? It turns out - // that this is what pthread_atfork() takes care of, which is - // presumably implemented across platforms. The first thing that - // threads to *before* forking is to do things like grab the malloc - // mutex, and then after the fork they unlock it. - // - // Despite this information, libnative's spawn has been witnessed to - // deadlock on both OSX and FreeBSD. I'm not entirely sure why, but - // all collected backtraces point at malloc/free traffic in the - // child spawned process. - // - // For this reason, the block of code below should contain 0 - // invocations of either malloc of free (or their related friends). - // - // As an example of not having malloc/free traffic, we don't close - // this file descriptor by dropping the FileDesc (which contains an - // allocation). Instead we just close it manually. This will never - // have the drop glue anyway because this code never returns (the - // child will either exec() or invoke libc::exit) - let _ = libc::close(input.raw()); - - fn fail(output: &mut AnonPipe) -> ! { - let errno = sys::os::errno() as u32; - let bytes = [ - (errno >> 24) as u8, - (errno >> 16) as u8, - (errno >> 8) as u8, - (errno >> 0) as u8, - CLOEXEC_MSG_FOOTER[0], CLOEXEC_MSG_FOOTER[1], - CLOEXEC_MSG_FOOTER[2], CLOEXEC_MSG_FOOTER[3] - ]; - // pipe I/O up to PIPE_BUF bytes should be atomic - assert!(output.write(&bytes).is_ok()); - unsafe { libc::_exit(1) } - } - - rustrt::rust_unset_sigprocmask(); - - // 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| { - let src = match src { - None => { - let flags = if dst == libc::STDIN_FILENO { - libc::O_RDONLY - } else { - libc::O_RDWR - }; - libc::open(devnull.as_ptr() as *const _, flags, 0) - } - Some(obj) => { - let fd = obj.raw(); - // Leak the memory and the file descriptor. We're in the - // child now an all our resources are going to be - // cleaned up very soon - mem::forget(obj); - fd - } - }; - src != -1 && retry(|| dup2(src, dst)) != -1 - }; - - if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) } - if !setup(out_fd, libc::STDOUT_FILENO) { fail(&mut output) } - if !setup(err_fd, libc::STDERR_FILENO) { fail(&mut output) } - - // close all other fds - for fd in (3..getdtablesize()).rev() { - if fd != output.raw() { - let _ = close(fd as c_int); - } - } - - match cfg.gid { - Some(u) => { - if libc::setgid(u as libc::gid_t) != 0 { - fail(&mut output); - } - } - None => {} - } - match cfg.uid { - Some(u) => { - // When dropping privileges from root, the `setgroups` call - // will remove any extraneous groups. If we don't call this, - // then even though our uid has dropped, we may still have - // groups that enable us to do super-user things. This will - // fail if we aren't root, so don't bother checking the - // return value, this is just done as an optimistic - // privilege dropping function. - extern { - fn setgroups(ngroups: libc::c_int, - ptr: *const libc::c_void) -> libc::c_int; - } - let _ = setgroups(0, ptr::null()); - - if libc::setuid(u as libc::uid_t) != 0 { - fail(&mut output); - } - } - None => {} - } - if cfg.detach { - // Don't check the error of setsid because it fails if we're the - // process leader already. We just forked so it shouldn't return - // error, but ignore it anyway. - let _ = libc::setsid(); - } - if !dirp.is_null() && chdir(dirp) == -1 { - fail(&mut output); - } - if !envp.is_null() { - *sys::os::environ() = envp as *const _; - } - let _ = execvp(*argv, argv as *mut _); + if let Some(u) = cfg.gid { + if libc::setgid(u as libc::gid_t) != 0 { fail(&mut output); - }) - }) + } + } + if let Some(u) = cfg.uid { + // When dropping privileges from root, the `setgroups` call + // will remove any extraneous groups. If we don't call this, + // then even though our uid has dropped, we may still have + // groups that enable us to do super-user things. This will + // fail if we aren't root, so don't bother checking the + // return value, this is just done as an optimistic + // privilege dropping function. + let _ = c::setgroups(0, ptr::null()); + + if libc::setuid(u as libc::uid_t) != 0 { + fail(&mut output); + } + } + if cfg.detach { + // Don't check the error of setsid because it fails if we're the + // process leader already. We just forked so it shouldn't return + // error, but ignore it anyway. + let _ = libc::setsid(); + } + if !dirp.is_null() && libc::chdir(dirp) == -1 { + fail(&mut output); + } + if !envp.is_null() { + *sys::os::environ() = envp as *const _; + } + let _ = libc::execvp(*argv, argv as *mut _); + fail(&mut output) } pub fn wait(&self) -> io::Result { @@ -364,8 +321,8 @@ impl Process { } } -fn with_argv(prog: &CString, args: &[CString], cb: F) -> T - where F : FnOnce(*const *const libc::c_char) -> T +fn make_argv(prog: &CString, args: &[CString]) + -> (*const *const libc::c_char, Vec<*const libc::c_char>) { let mut ptrs: Vec<*const libc::c_char> = Vec::with_capacity(args.len()+1); @@ -380,40 +337,38 @@ fn with_argv(prog: &CString, args: &[CString], cb: F) -> T // Add a terminating null pointer (required by libc). ptrs.push(ptr::null()); - cb(ptrs.as_ptr()) + (ptrs.as_ptr(), ptrs) } -fn with_envp(env: Option<&HashMap>, cb: F) -> T - where F : FnOnce(*const c_void) -> T +fn make_envp(env: Option<&HashMap>) + -> (*const c_void, Vec>, Vec<*const libc::c_char>) { // On posixy systems we can pass a char** for envp, which is a // null-terminated array of "k=v\0" strings. Since we must create // these strings locally, yet expose a raw pointer to them, we // create a temporary vector to own the CStrings that outlives the // call to cb. - match env { - Some(env) => { - let mut tmps = Vec::with_capacity(env.len()); + if let Some(env) = env { + let mut tmps = Vec::with_capacity(env.len()); - for pair in env { - let mut kv = Vec::new(); - kv.push_all(pair.0.as_bytes()); - kv.push('=' as u8); - kv.push_all(pair.1.as_bytes()); - kv.push(0); // terminating null - tmps.push(kv); - } - - // As with `with_argv`, this is unsafe, since cb could leak the pointers. - let mut ptrs: Vec<*const libc::c_char> = - tmps.iter() - .map(|tmp| tmp.as_ptr() as *const libc::c_char) - .collect(); - ptrs.push(ptr::null()); - - cb(ptrs.as_ptr() as *const c_void) + for pair in env { + let mut kv = Vec::new(); + kv.push_all(pair.0.as_bytes()); + kv.push('=' as u8); + kv.push_all(pair.1.as_bytes()); + kv.push(0); // terminating null + tmps.push(kv); } - _ => cb(ptr::null()) + + let mut ptrs: Vec<*const libc::c_char> = + tmps.iter() + .map(|tmp| tmp.as_ptr() as *const libc::c_char) + .collect(); + ptrs.push(ptr::null()); + + (ptrs.as_ptr() as *const _, tmps, ptrs) + } else { + (0 as *const _, Vec::new(), Vec::new()) } } diff --git a/src/libstd/sys/windows/pipe2.rs b/src/libstd/sys/windows/pipe2.rs index 229481e3d57..ed41c959782 100644 --- a/src/libstd/sys/windows/pipe2.rs +++ b/src/libstd/sys/windows/pipe2.rs @@ -22,22 +22,24 @@ pub struct AnonPipe { fd: c_int } -pub unsafe fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { +pub fn anon_pipe() -> io::Result<(AnonPipe, AnonPipe)> { // Windows pipes work subtly differently than unix pipes, and their // inheritance has to be handled in a different way that I do not // fully understand. Here we explicitly make the pipe non-inheritable, // which means to pass it to a subprocess they need to be duplicated // first, as in std::run. let mut fds = [0; 2]; - match libc::pipe(fds.as_mut_ptr(), 1024 as ::libc::c_uint, - (libc::O_BINARY | libc::O_NOINHERIT) as c_int) { - 0 => { - assert!(fds[0] != -1 && fds[0] != 0); - assert!(fds[1] != -1 && fds[1] != 0); + unsafe { + match libc::pipe(fds.as_mut_ptr(), 1024 as ::libc::c_uint, + (libc::O_BINARY | libc::O_NOINHERIT) as c_int) { + 0 => { + assert!(fds[0] != -1 && fds[0] != 0); + assert!(fds[1] != -1 && fds[1] != 0); - Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1]))) + Ok((AnonPipe::from_fd(fds[0]), AnonPipe::from_fd(fds[1]))) + } + _ => Err(io::Error::last_os_error()), } - _ => Err(io::Error::last_os_error()), } } From eadc3bcd676277d72c211bde6c20f7036339fd84 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Fri, 3 Apr 2015 15:44:14 -0700 Subject: [PATCH 3/3] 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)]