Skip to content

Commit

Permalink
Auto merge of #55978 - alexcrichton:beta-next, r=Mark-Simulacrum
Browse files Browse the repository at this point in the history
[beta] std: Synchronize access to global env during `exec`

This is a beta backport of #55939
  • Loading branch information
bors committed Nov 15, 2018
2 parents a78c50c + 9609a91 commit 51be258
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 106 deletions.
20 changes: 12 additions & 8 deletions src/libstd/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,12 @@ use path::{self, PathBuf};
use ptr;
use slice;
use str;
use sys_common::mutex::Mutex;
use sys_common::mutex::{Mutex, MutexGuard};
use sys::cvt;
use sys::fd;
use vec;

const TMPBUF_SZ: usize = 128;
// We never call `ENV_LOCK.init()`, so it is UB to attempt to
// acquire this mutex reentrantly!
static ENV_LOCK: Mutex = Mutex::new();


extern {
Expand Down Expand Up @@ -408,11 +405,18 @@ pub unsafe fn environ() -> *mut *const *const c_char {
&mut environ
}

pub unsafe fn env_lock() -> MutexGuard<'static> {
// We never call `ENV_LOCK.init()`, so it is UB to attempt to
// acquire this mutex reentrantly!
static ENV_LOCK: Mutex = Mutex::new();
ENV_LOCK.lock()
}

/// Returns a vector of (variable, value) byte-vector pairs for all the
/// environment variables of the current process.
pub fn env() -> Env {
unsafe {
let _guard = ENV_LOCK.lock();
let _guard = env_lock();
let mut environ = *environ();
let mut result = Vec::new();
while environ != ptr::null() && *environ != ptr::null() {
Expand Down Expand Up @@ -448,7 +452,7 @@ pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
// always None as well
let k = CString::new(k.as_bytes())?;
unsafe {
let _guard = ENV_LOCK.lock();
let _guard = env_lock();
let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
let ret = if s.is_null() {
None
Expand All @@ -464,7 +468,7 @@ pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
let v = CString::new(v.as_bytes())?;

unsafe {
let _guard = ENV_LOCK.lock();
let _guard = env_lock();
cvt(libc::setenv(k.as_ptr(), v.as_ptr(), 1)).map(|_| ())
}
}
Expand All @@ -473,7 +477,7 @@ pub fn unsetenv(n: &OsStr) -> io::Result<()> {
let nbuf = CString::new(n.as_bytes())?;

unsafe {
let _guard = ENV_LOCK.lock();
let _guard = env_lock();
cvt(libc::unsetenv(nbuf.as_ptr())).map(|_| ())
}
}
Expand Down
8 changes: 0 additions & 8 deletions src/libstd/sys/unix/process/process_common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,6 @@ impl Command {
pub fn get_argv(&self) -> &Vec<*const c_char> {
&self.argv.0
}
#[cfg(not(target_os = "fuchsia"))]
pub fn get_program(&self) -> &CString {
return &self.program;
}

#[allow(dead_code)]
pub fn get_cwd(&self) -> &Option<CString> {
Expand Down Expand Up @@ -248,10 +244,6 @@ impl CStringArray {
pub fn as_ptr(&self) -> *const *const c_char {
self.ptrs.as_ptr()
}
#[cfg(not(target_os = "fuchsia"))]
pub fn get_items(&self) -> &[CString] {
return &self.items;
}
}

fn construct_envp(env: BTreeMap<DefaultEnvKey, OsString>, saw_nul: &mut bool) -> CStringArray {
Expand Down
137 changes: 47 additions & 90 deletions src/libstd/sys/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,12 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use env;
use ffi::CString;
use io::{self, Error, ErrorKind};
use libc::{self, c_int, gid_t, pid_t, uid_t};
use ptr;

use sys::cvt;
use sys::process::process_common::*;
use sys;

////////////////////////////////////////////////////////////////////////////////
// Command
Expand All @@ -24,8 +22,6 @@ use sys::process::process_common::*;
impl Command {
pub fn spawn(&mut self, default: Stdio, needs_stdin: bool)
-> io::Result<(Process, StdioPipes)> {
use sys;

const CLOEXEC_MSG_FOOTER: &'static [u8] = b"NOEX";

let envp = self.capture_env();
Expand All @@ -41,15 +37,26 @@ impl Command {
return Ok((ret, ours))
}

let possible_paths = self.compute_possible_paths(envp.as_ref());

let (input, output) = sys::pipe::anon_pipe()?;

// Whatever happens after the fork is almost for sure going to touch or
// look at the environment in one way or another (PATH in `execvp` or
// accessing the `environ` pointer ourselves). Make sure no other thread
// is accessing the environment when we do the fork itself.
//
// Note that as soon as we're done with the fork there's no need to hold
// a lock any more because the parent won't do anything and the child is
// in its own process.
let result = unsafe {
let _env_lock = sys::os::env_lock();
cvt(libc::fork())?
};

let pid = unsafe {
match cvt(libc::fork())? {
match result {
0 => {
drop(input);
let err = self.do_exec(theirs, envp.as_ref(), possible_paths);
let err = self.do_exec(theirs, envp.as_ref());
let errno = err.raw_os_error().unwrap_or(libc::EINVAL) as u32;
let bytes = [
(errno >> 24) as u8,
Expand Down Expand Up @@ -117,46 +124,19 @@ impl Command {
"nul byte found in provided data")
}

let possible_paths = self.compute_possible_paths(envp.as_ref());
match self.setup_io(default, true) {
Ok((_, theirs)) => unsafe { self.do_exec(theirs, envp.as_ref(), possible_paths) },
Err(e) => e,
}
}
Ok((_, theirs)) => {
unsafe {
// Similar to when forking, we want to ensure that access to
// the environment is synchronized, so make sure to grab the
// environment lock before we try to exec.
let _lock = sys::os::env_lock();

fn compute_possible_paths(&self, maybe_envp: Option<&CStringArray>) -> Option<Vec<CString>> {
let program = self.get_program().as_bytes();
if program.contains(&b'/') {
return None;
}
// Outside the match so we can borrow it for the lifetime of the function.
let parent_path = env::var("PATH").ok();
let paths = match maybe_envp {
Some(envp) => {
match envp.get_items().iter().find(|var| var.as_bytes().starts_with(b"PATH=")) {
Some(p) => &p.as_bytes()[5..],
None => return None,
}
},
// maybe_envp is None if the process isn't changing the parent's env at all.
None => {
match parent_path.as_ref() {
Some(p) => p.as_bytes(),
None => return None,
self.do_exec(theirs, envp.as_ref())
}
},
};

let mut possible_paths = vec![];
for path in paths.split(|p| *p == b':') {
let mut binary_path = Vec::with_capacity(program.len() + path.len() + 1);
binary_path.extend_from_slice(path);
binary_path.push(b'/');
binary_path.extend_from_slice(program);
let c_binary_path = CString::new(binary_path).unwrap();
possible_paths.push(c_binary_path);
}
Err(e) => e,
}
return Some(possible_paths);
}

// And at this point we've reached a special time in the life of the
Expand Down Expand Up @@ -192,8 +172,7 @@ impl Command {
unsafe fn do_exec(
&mut self,
stdio: ChildPipes,
maybe_envp: Option<&CStringArray>,
maybe_possible_paths: Option<Vec<CString>>,
maybe_envp: Option<&CStringArray>
) -> io::Error {
use sys::{self, cvt_r};

Expand Down Expand Up @@ -269,53 +248,29 @@ impl Command {
t!(callback());
}

// If the program isn't an absolute path, and our environment contains a PATH var, then we
// implement the PATH traversal ourselves so that it honors the child's PATH instead of the
// parent's. This mirrors the logic that exists in glibc's execvpe, except using the
// child's env to fetch PATH.
match maybe_possible_paths {
Some(possible_paths) => {
let mut pending_error = None;
for path in possible_paths {
libc::execve(
path.as_ptr(),
self.get_argv().as_ptr(),
maybe_envp.map(|envp| envp.as_ptr()).unwrap_or_else(|| *sys::os::environ())
);
let err = io::Error::last_os_error();
match err.kind() {
io::ErrorKind::PermissionDenied => {
// If we saw a PermissionDenied, and none of the other entries in
// $PATH are successful, then we'll return the first EACCESS we see.
if pending_error.is_none() {
pending_error = Some(err);
}
},
// Errors which indicate we failed to find a file are ignored and we try
// the next entry in the path.
io::ErrorKind::NotFound | io::ErrorKind::TimedOut => {
continue
},
// Any other error means we found a file and couldn't execute it.
_ => {
return err;
}
// Although we're performing an exec here we may also return with an
// error from this function (without actually exec'ing) in which case we
// want to be sure to restore the global environment back to what it
// once was, ensuring that our temporary override, when free'd, doesn't
// corrupt our process's environment.
let mut _reset = None;
if let Some(envp) = maybe_envp {
struct Reset(*const *const libc::c_char);

impl Drop for Reset {
fn drop(&mut self) {
unsafe {
*sys::os::environ() = self.0;
}
}
if let Some(err) = pending_error {
return err;
}
return io::Error::from_raw_os_error(libc::ENOENT);
},
_ => {
libc::execve(
self.get_argv()[0],
self.get_argv().as_ptr(),
maybe_envp.map(|envp| envp.as_ptr()).unwrap_or_else(|| *sys::os::environ())
);
return io::Error::last_os_error()
}

_reset = Some(Reset(*sys::os::environ()));
*sys::os::environ() = envp.as_ptr();
}

libc::execvp(self.get_argv()[0], self.get_argv().as_ptr());
io::Error::last_os_error()
}

#[cfg(not(any(target_os = "macos", target_os = "freebsd",
Expand Down Expand Up @@ -413,6 +368,8 @@ impl Command {
libc::POSIX_SPAWN_SETSIGMASK;
cvt(libc::posix_spawnattr_setflags(&mut attrs.0, flags as _))?;

// Make sure we synchronize access to the global `environ` resource
let _env_lock = sys::os::env_lock();
let envp = envp.map(|c| c.as_ptr())
.unwrap_or_else(|| *sys::os::environ() as *const _);
let ret = libc::posix_spawnp(
Expand Down
24 changes: 24 additions & 0 deletions src/test/run-pass/command-exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,16 @@ fn main() {
println!("passed");
}

"exec-test6" => {
let err = Command::new("echo").arg("passed").env_clear().exec();
panic!("failed to spawn: {}", err);
}

"exec-test7" => {
let err = Command::new("echo").arg("passed").env_remove("PATH").exec();
panic!("failed to spawn: {}", err);
}

_ => panic!("unknown argument: {}", arg),
}
return
Expand Down Expand Up @@ -84,4 +94,18 @@ fn main() {
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");

if cfg!(target_os = "linux") {
let output = Command::new(&me).arg("exec-test6").output().unwrap();
println!("{:?}", output);
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");

let output = Command::new(&me).arg("exec-test7").output().unwrap();
println!("{:?}", output);
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");
}
}

0 comments on commit 51be258

Please sign in to comment.