OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

std: Unconditionally close all file descriptors

Browse Source 
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.
This commit is contained in:
Alex Crichton 2015-04-03 15:44:14 -07:00
parent 33a2191d0b
commit eadc3bcd67
5 changed files with 86 additions and 67 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

24
src/libstd/process.rs
View File

@ -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<Child> {
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<CommandImp> 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<AnonPipe>, Option<AnonPipe>)>
fn setup_io(io: &StdioImp, readable: bool)
-> io::Result<(StdioImp2, Option<AnonPipe>)>
{
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))
}
}
})

22
src/libstd/sys/unix/fd.rs
View File

@ -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<c_int> for FileDesc {
@ -74,14 +67,11 @@ impl AsInner<c_int> 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) };
}
}

59
src/libstd/sys/unix/process2.rs
View File

@ -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<AnonPipe>,
out_fd: Option<AnonPipe>,
err_fd: Option<AnonPipe>) -> io::Result<Process> {
in_fd: Stdio,
out_fd: Stdio,
err_fd: Stdio) -> io::Result<Process> {
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<AnonPipe>,
out_fd: Option<AnonPipe>,
err_fd: Option<AnonPipe>) -> ! {
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<AnonPipe>, 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) }

47
src/libstd/sys/windows/process2.rs
View File

@ -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<AnonPipe>, out_fd: Option<AnonPipe>, err_fd: Option<AnonPipe>)
-> io::Result<Process>
in_fd: Stdio,
out_fd: Stdio,
err_fd: Stdio) -> io::Result<Process>
{
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<AnonPipe>, 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);

1
src/test/run-pass/fds-are-cloexec.rs
View File

@ -9,6 +9,7 @@
// except according to those terms.
// ignore-windows
// ignore-android
#![feature(libc)]