Skip to content

Commit 551e15d

Browse files
alexcrichtonalexcrichton
committed
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.
1 parent 33a2191 commit 551e15d

File tree

4 files changed

+85
-67
lines changed

4 files changed

+85
-67
lines changed

‎src/libstd/process.rs

+10-14
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ use io::prelude::*;
1919
use ffi::OsStr;
2020
use fmt;
2121
use io::{self, Error, ErrorKind};
22-
use libc;
2322
use path;
2423
use sync::mpsc::{channel, Receiver};
2524
use sys::pipe2::{self, AnonPipe};
2625
use sys::process2::Command as CommandImp;
2726
use sys::process2::Process as ProcessImp;
2827
use sys::process2::ExitStatus as ExitStatusImp;
28+
use sys::process2::Stdio as StdioImp2;
2929
use sys_common::{AsInner, AsInnerMut};
3030
use thread;
3131

@@ -229,13 +229,13 @@ impl Command {
229229

230230
fn spawn_inner(&self, default_io: StdioImp) -> io::Result<Child> {
231231
let (their_stdin, our_stdin) = try!(
232-
setup_io(self.stdin.as_ref().unwrap_or(&default_io), 0, true)
232+
setup_io(self.stdin.as_ref().unwrap_or(&default_io), true)
233233
);
234234
let (their_stdout, our_stdout) = try!(
235-
setup_io(self.stdout.as_ref().unwrap_or(&default_io), 1, false)
235+
setup_io(self.stdout.as_ref().unwrap_or(&default_io), false)
236236
);
237237
let (their_stderr, our_stderr) = try!(
238-
setup_io(self.stderr.as_ref().unwrap_or(&default_io), 2, false)
238+
setup_io(self.stderr.as_ref().unwrap_or(&default_io), false)
239239
);
240240

241241
match ProcessImp::spawn(&self.inner, their_stdin, their_stdout, their_stderr) {
@@ -328,23 +328,19 @@ impl AsInnerMut<CommandImp> for Command {
328328
fn as_inner_mut(&mut self) -> &mut CommandImp { &mut self.inner }
329329
}
330330

331-
fn setup_io(io: &StdioImp, fd: libc::c_int, readable: bool)
332-
-> io::Result<(Option<AnonPipe>, Option<AnonPipe>)>
331+
fn setup_io(io: &StdioImp, readable: bool)
332+
-> io::Result<(StdioImp2, Option<AnonPipe>)>
333333
{
334334
use self::StdioImp::*;
335335
Ok(match *io {
336-
Null => {
337-
(None, None)
338-
}
339-
Inherit => {
340-
(Some(AnonPipe::from_fd(fd)), None)
341-
}
336+
Null => (StdioImp2::None, None),
337+
Inherit => (StdioImp2::Inherit, None),
342338
Piped => {
343339
let (reader, writer) = try!(pipe2::anon_pipe());
344340
if readable {
345-
(Some(reader), Some(writer))
341+
(StdioImp2::Piped(reader), Some(writer))
346342
} else {
347-
(Some(writer), Some(reader))
343+
(StdioImp2::Piped(writer), Some(reader))
348344
}
349345
}
350346
})

‎src/libstd/sys/unix/fd.rs

+6-16
Original file line numberDiff line numberDiff line change
@@ -59,13 +59,6 @@ impl FileDesc {
5959
debug_assert_eq!(ret, 0);
6060
}
6161
}
62-
63-
pub fn unset_cloexec(&self) {
64-
unsafe {
65-
let ret = c::ioctl(self.fd, c::FIONCLEX);
66-
debug_assert_eq!(ret, 0);
67-
}
68-
}
6962
}
7063

7164
impl AsInner<c_int> for FileDesc {
@@ -74,14 +67,11 @@ impl AsInner<c_int> for FileDesc {
7467

7568
impl Drop for FileDesc {
7669
fn drop(&mut self) {
77-
// closing stdio file handles makes no sense, so never do it. Also, note
78-
// that errors are ignored when closing a file descriptor. The reason
79-
// for this is that if an error occurs we don't actually know if the
80-
// file descriptor was closed or not, and if we retried (for something
81-
// like EINTR), we might close another valid file descriptor (opened
82-
// after we closed ours.
83-
if self.fd > libc::STDERR_FILENO {
84-
let _ = unsafe { libc::close(self.fd) };
85-
}
70+
// Note that errors are ignored when closing a file descriptor. The
71+
// reason for this is that if an error occurs we don't actually know if
72+
// the file descriptor was closed or not, and if we retried (for
73+
// something like EINTR), we might close another valid file descriptor
74+
// (opened after we closed ours.
75+
let _ = unsafe { libc::close(self.fd) };
8676
}
8777
}

‎src/libstd/sys/unix/process2.rs

+36-23
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,12 @@ pub struct Process {
119119
pid: pid_t
120120
}
121121

122+
pub enum Stdio {
123+
Inherit,
124+
Piped(AnonPipe),
125+
None,
126+
}
127+
122128
const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX";
123129

124130
impl Process {
@@ -128,9 +134,9 @@ impl Process {
128134
}
129135

130136
pub fn spawn(cfg: &Command,
131-
in_fd: Option<AnonPipe>,
132-
out_fd: Option<AnonPipe>,
133-
err_fd: Option<AnonPipe>) -> io::Result<Process> {
137+
in_fd: Stdio,
138+
out_fd: Stdio,
139+
err_fd: Stdio) -> io::Result<Process> {
134140
let dirp = cfg.cwd.as_ref().map(|c| c.as_ptr()).unwrap_or(ptr::null());
135141

136142
let (envp, _a, _b) = make_envp(cfg.env.as_ref());
@@ -224,9 +230,9 @@ impl Process {
224230
argv: *const *const libc::c_char,
225231
envp: *const libc::c_void,
226232
dirp: *const libc::c_char,
227-
in_fd: Option<AnonPipe>,
228-
out_fd: Option<AnonPipe>,
229-
err_fd: Option<AnonPipe>) -> ! {
233+
in_fd: Stdio,
234+
out_fd: Stdio,
235+
err_fd: Stdio) -> ! {
230236
fn fail(output: &mut AnonPipe) -> ! {
231237
let errno = sys::os::errno() as u32;
232238
let bytes = [
@@ -244,23 +250,30 @@ impl Process {
244250
unsafe { libc::_exit(1) }
245251
}
246252

247-
// If a stdio file descriptor is set to be ignored, we don't
248-
// actually close it, but rather open up /dev/null into that
249-
// file descriptor. Otherwise, the first file descriptor opened
250-
// up in the child would be numbered as one of the stdio file
251-
// descriptors, which is likely to wreak havoc.
252-
let setup = |src: Option<AnonPipe>, dst: c_int| {
253-
src.map(|p| p.into_fd()).or_else(|| {
254-
let mut opts = OpenOptions::new();
255-
opts.read(dst == libc::STDIN_FILENO);
256-
opts.write(dst != libc::STDIN_FILENO);
257-
let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr()
258-
as *const _);
259-
File::open_c(devnull, &opts).ok().map(|f| f.into_fd())
260-
}).map(|fd| {
261-
fd.unset_cloexec();
262-
retry(|| libc::dup2(fd.raw(), dst)) != -1
263-
}).unwrap_or(false)
253+
let setup = |src: Stdio, dst: c_int| {
254+
let fd = match src {
255+
Stdio::Inherit => return true,
256+
Stdio::Piped(pipe) => pipe.into_fd(),
257+
258+
// If a stdio file descriptor is set to be ignored, we open up
259+
// /dev/null into that file descriptor. Otherwise, the first
260+
// file descriptor opened up in the child would be numbered as
261+
// one of the stdio file descriptors, which is likely to wreak
262+
// havoc.
263+
Stdio::None => {
264+
let mut opts = OpenOptions::new();
265+
opts.read(dst == libc::STDIN_FILENO);
266+
opts.write(dst != libc::STDIN_FILENO);
267+
let devnull = CStr::from_ptr(b"/dev/null\0".as_ptr()
268+
as *const _);
269+
if let Ok(f) = File::open_c(devnull, &opts) {
270+
f.into_fd()
271+
} else {
272+
return false
273+
}
274+
}
275+
};
276+
retry(|| libc::dup2(fd.raw(), dst)) != -1
264277
};
265278

266279
if !setup(in_fd, libc::STDIN_FILENO) { fail(&mut output) }

‎src/libstd/sys/windows/process2.rs

+33-14
Original file line numberDiff line numberDiff line change
@@ -105,11 +105,18 @@ pub struct Process {
105105
handle: Handle,
106106
}
107107

108+
pub enum Stdio {
109+
Inherit,
110+
Piped(AnonPipe),
111+
None,
112+
}
113+
108114
impl Process {
109115
#[allow(deprecated)]
110116
pub fn spawn(cfg: &Command,
111-
in_fd: Option<AnonPipe>, out_fd: Option<AnonPipe>, err_fd: Option<AnonPipe>)
112-
-> io::Result<Process>
117+
in_fd: Stdio,
118+
out_fd: Stdio,
119+
err_fd: Stdio) -> io::Result<Process>
113120
{
114121
use libc::types::os::arch::extra::{DWORD, HANDLE, STARTUPINFO};
115122
use libc::consts::os::extra::{
@@ -156,13 +163,16 @@ impl Process {
156163

157164
let cur_proc = GetCurrentProcess();
158165

159-
// Similarly to unix, we don't actually leave holes for the stdio file
160-
// descriptors, but rather open up /dev/null equivalents. These
161-
// equivalents are drawn from libuv's windows process spawning.
162-
let set_fd = |fd: &Option<AnonPipe>, slot: &mut HANDLE,
166+
let set_fd = |fd: &Stdio, slot: &mut HANDLE,
163167
is_stdin: bool| {
164168
match *fd {
165-
None => {
169+
Stdio::Inherit => {}
170+
171+
// Similarly to unix, we don't actually leave holes for the
172+
// stdio file descriptors, but rather open up /dev/null
173+
// equivalents. These equivalents are drawn from libuv's
174+
// windows process spawning.
175+
Stdio::None => {
166176
let access = if is_stdin {
167177
libc::FILE_GENERIC_READ
168178
} else {
@@ -188,11 +198,8 @@ impl Process {
188198
return Err(Error::last_os_error())
189199
}
190200
}
191-
Some(ref pipe) => {
201+
Stdio::Piped(ref pipe) => {
192202
let orig = pipe.raw();
193-
if orig == INVALID_HANDLE_VALUE {
194-
return Err(Error::last_os_error())
195-
}
196203
if DuplicateHandle(cur_proc, orig, cur_proc, slot,
197204
0, TRUE, DUPLICATE_SAME_ACCESS) == FALSE {
198205
return Err(Error::last_os_error())
@@ -235,9 +242,15 @@ impl Process {
235242
})
236243
});
237244

238-
assert!(CloseHandle(si.hStdInput) != 0);
239-
assert!(CloseHandle(si.hStdOutput) != 0);
240-
assert!(CloseHandle(si.hStdError) != 0);
245+
if !in_fd.inherited() {
246+
assert!(CloseHandle(si.hStdInput) != 0);
247+
}
248+
if !out_fd.inherited() {
249+
assert!(CloseHandle(si.hStdOutput) != 0);
250+
}
251+
if !err_fd.inherited() {
252+
assert!(CloseHandle(si.hStdError) != 0);
253+
}
241254

242255
match create_err {
243256
Some(err) => return Err(err),
@@ -296,6 +309,12 @@ impl Process {
296309
}
297310
}
298311

312+
impl Stdio {
313+
fn inherited(&self) -> bool {
314+
match *self { Stdio::Inherit => true, _ => false }
315+
}
316+
}
317+
299318
#[derive(PartialEq, Eq, Clone, Copy, Debug)]
300319
pub struct ExitStatus(i32);
301320

0 commit comments

Comments
 (0)