From 7f7a059c4719bbff6c2859802bc50ab2fcaf249f Mon Sep 17 00:00:00 2001 From: Ivan Petkov Date: Sat, 19 Dec 2015 17:12:19 -0800 Subject: [PATCH] libstd: unix process spawning: fix bug with setting stdio * If the requested descriptors to inherit are stdio descriptors there are situations where they will not be set correctly * Example: parent's stdout --> child's stderr parent's stderr --> child's stdout * Solution: if the requested descriptors for the child are stdio descriptors, `dup` them before overwriting the child's stdio --- src/libstd/sys/unix/process.rs | 32 +++++++++ src/test/run-pass/issue-30490.rs | 107 +++++++++++++++++++++++++++++++ 2 files changed, 139 insertions(+) create mode 100644 src/test/run-pass/issue-30490.rs diff --git a/src/libstd/sys/unix/process.rs b/src/libstd/sys/unix/process.rs index 407fcb0a1b8f1..671cfbd361316 100644 --- a/src/libstd/sys/unix/process.rs +++ b/src/libstd/sys/unix/process.rs @@ -288,6 +288,32 @@ impl Process { unsafe { libc::_exit(1) } } + // Make sure that the source descriptors are not an stdio descriptor, + // otherwise the order which we set the child's descriptors may blow + // away a descriptor which we are hoping to save. For example, + // suppose we want the child's stderr to be the parent's stdout, and + // the child's stdout to be the parent's stderr. No matter which we + // dup first, the second will get overwritten prematurely. + let maybe_migrate = |src: Stdio, output: &mut AnonPipe| { + match src { + Stdio::Raw(fd @ libc::STDIN_FILENO) | + Stdio::Raw(fd @ libc::STDOUT_FILENO) | + Stdio::Raw(fd @ libc::STDERR_FILENO) => { + let fd = match cvt_r(|| libc::dup(fd)) { + Ok(fd) => fd, + Err(_) => fail(output), + }; + let fd = FileDesc::new(fd); + fd.set_cloexec(); + Stdio::Raw(fd.into_raw()) + }, + + s @ Stdio::None | + s @ Stdio::Inherit | + s @ Stdio::Raw(_) => s, + } + }; + let setup = |src: Stdio, dst: c_int| { match src { Stdio::Inherit => true, @@ -313,6 +339,12 @@ impl Process { } }; + // Make sure we migrate all source descriptors before + // we start overwriting them + let in_fd = maybe_migrate(in_fd, &mut output); + let out_fd = maybe_migrate(out_fd, &mut output); + let err_fd = maybe_migrate(err_fd, &mut output); + 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) } diff --git a/src/test/run-pass/issue-30490.rs b/src/test/run-pass/issue-30490.rs new file mode 100644 index 0000000000000..ea603fc665dda --- /dev/null +++ b/src/test/run-pass/issue-30490.rs @@ -0,0 +1,107 @@ +// 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. + +// Previously libstd would set stdio descriptors of a child process +// by `dup`ing the requested descriptors to inherit directly into the +// stdio descriptors. This, however, would incorrectly handle cases +// where the descriptors to inherit were already stdio descriptors. +// This test checks to avoid that regression. + +#![cfg_attr(unix, feature(libc))] +#![cfg_attr(windows, allow(unused_imports))] + +#[cfg(unix)] +extern crate libc; + +use std::fs::File; +use std::io::{Read, Write}; +use std::io::{stdout, stderr}; +use std::process::{Command, Stdio}; + +#[cfg(unix)] +use std::os::unix::io::FromRawFd; + +#[cfg(not(unix))] +fn main() { + // Bug not present in Windows +} + +#[cfg(unix)] +fn main() { + let mut args = std::env::args(); + let name = args.next().unwrap(); + let args: Vec = args.collect(); + if let Some("--child") = args.get(0).map(|s| &**s) { + return child(); + } else if !args.is_empty() { + panic!("unknown options"); + } + + let stdout_backup = unsafe { libc::dup(libc::STDOUT_FILENO) }; + let stderr_backup = unsafe { libc::dup(libc::STDERR_FILENO) }; + assert!(stdout_backup > -1); + assert!(stderr_backup > -1); + + let (stdout_reader, stdout_writer) = pipe(); + let (stderr_reader, stderr_writer) = pipe(); + assert!(unsafe { libc::dup2(stdout_writer, libc::STDOUT_FILENO) } > -1); + assert!(unsafe { libc::dup2(stderr_writer, libc::STDERR_FILENO) } > -1); + + // Make sure we close any duplicates of the writer end of the pipe, + // otherwise we can get stuck reading from the pipe which has open + // writers but no one supplying any input + assert_eq!(unsafe { libc::close(stdout_writer) }, 0); + assert_eq!(unsafe { libc::close(stderr_writer) }, 0); + + stdout().write_all("parent stdout\n".as_bytes()).expect("failed to write to stdout"); + stderr().write_all("parent stderr\n".as_bytes()).expect("failed to write to stderr"); + + let child = { + Command::new(name) + .arg("--child") + .stdin(Stdio::inherit()) + .stdout(unsafe { FromRawFd::from_raw_fd(libc::STDERR_FILENO) }) + .stderr(unsafe { FromRawFd::from_raw_fd(libc::STDOUT_FILENO) }) + .spawn() + }; + + // The Stdio passed into the Command took over (and closed) std{out, err} + // so we should restore them as they were. + assert!(unsafe { libc::dup2(stdout_backup, libc::STDOUT_FILENO) } > -1); + assert!(unsafe { libc::dup2(stderr_backup, libc::STDERR_FILENO) } > -1); + + // Using File as a shim around the descriptor + let mut read = String::new(); + let mut f: File = unsafe { FromRawFd::from_raw_fd(stdout_reader) }; + f.read_to_string(&mut read).expect("failed to read from stdout file"); + assert_eq!(read, "parent stdout\nchild stderr\n"); + + // Using File as a shim around the descriptor + read.clear(); + let mut f: File = unsafe { FromRawFd::from_raw_fd(stderr_reader) }; + f.read_to_string(&mut read).expect("failed to read from stderr file"); + assert_eq!(read, "parent stderr\nchild stdout\n"); + + assert!(child.expect("failed to execute child process").wait().unwrap().success()); +} + +#[cfg(unix)] +fn child() { + stdout().write_all("child stdout\n".as_bytes()).expect("child failed to write to stdout"); + stderr().write_all("child stderr\n".as_bytes()).expect("child failed to write to stderr"); +} + +#[cfg(unix)] +/// Returns a pipe (reader, writer combo) +fn pipe() -> (i32, i32) { + let mut fds = [0; 2]; + assert_eq!(unsafe { libc::pipe(fds.as_mut_ptr()) }, 0); + (fds[0], fds[1]) +}