From 5856797bda5b1cfecb486e8de439e958325a8a42 Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Tue, 13 Nov 2018 14:50:50 -0800
Subject: [PATCH 1/2] Revert "Fixes #46775 -- don't mutate the process's
environment in Command::exec"
This reverts commit 36fe3b605a7a7143a14565272140ba1b43c1b041.
---
src/libstd/sys/unix/process/process_common.rs | 8 --
src/libstd/sys/unix/process/process_unix.rs | 99 ++-----------------
src/test/run-pass/command-exec.rs | 12 ---
3 files changed, 8 insertions(+), 111 deletions(-)
diff --git a/src/libstd/sys/unix/process/process_common.rs b/src/libstd/sys/unix/process/process_common.rs
index 3d5920dfb69ac..77f125f3c5b56 100644
--- a/src/libstd/sys/unix/process/process_common.rs
+++ b/src/libstd/sys/unix/process/process_common.rs
@@ -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> {
@@ -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 {
diff --git a/src/libstd/sys/unix/process/process_unix.rs b/src/libstd/sys/unix/process/process_unix.rs
index f41bd2c20720a..7f1f9353c6d09 100644
--- a/src/libstd/sys/unix/process/process_unix.rs
+++ b/src/libstd/sys/unix/process/process_unix.rs
@@ -8,8 +8,6 @@
// 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;
@@ -41,15 +39,13 @@ impl Command {
return Ok((ret, ours))
}
- let possible_paths = self.compute_possible_paths(envp.as_ref());
-
let (input, output) = sys::pipe::anon_pipe()?;
let pid = unsafe {
match cvt(libc::fork())? {
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,
@@ -117,48 +113,12 @@ 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) },
+ Ok((_, theirs)) => unsafe { self.do_exec(theirs, envp.as_ref()) },
Err(e) => e,
}
}
- 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,
- }
- },
- };
-
- 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);
- }
- return Some(possible_paths);
- }
-
// 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
@@ -192,8 +152,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};
@@ -234,6 +193,9 @@ impl Command {
if let Some(ref cwd) = *self.get_cwd() {
t!(cvt(libc::chdir(cwd.as_ptr())));
}
+ if let Some(envp) = maybe_envp {
+ *sys::os::environ() = envp.as_ptr();
+ }
// emscripten has no signal support.
#[cfg(not(any(target_os = "emscripten")))]
@@ -269,53 +231,8 @@ 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;
- }
- }
- }
- 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()
- }
- }
+ 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",
diff --git a/src/test/run-pass/command-exec.rs b/src/test/run-pass/command-exec.rs
index 96f9da67790fc..46b409fb13a84 100644
--- a/src/test/run-pass/command-exec.rs
+++ b/src/test/run-pass/command-exec.rs
@@ -48,13 +48,6 @@ fn main() {
println!("passed");
}
- "exec-test5" => {
- env::set_var("VARIABLE", "ABC");
- Command::new("definitely-not-a-real-binary").env("VARIABLE", "XYZ").exec();
- assert_eq!(env::var("VARIABLE").unwrap(), "ABC");
- println!("passed");
- }
-
_ => panic!("unknown argument: {}", arg),
}
return
@@ -79,9 +72,4 @@ fn main() {
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");
-
- let output = Command::new(&me).arg("exec-test5").output().unwrap();
- assert!(output.status.success());
- assert!(output.stderr.is_empty());
- assert_eq!(output.stdout, b"passed\n");
}
From 4032b7a429462ece781629cab9f785742991ebef Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Tue, 13 Nov 2018 14:57:10 -0800
Subject: [PATCH 2/2] std: Synchronize access to global env during `exec`
This commit, after reverting #55359, applies a different fix for #46775
while also fixing #55775. The basic idea was to go back to pre-#55359
libstd, and then fix #46775 in a way that doesn't expose #55775.
The issue described in #46775 boils down to two problems:
* First, the global environment is reset during `exec` but, but if the
`exec` call fails then the global environment was a dangling pointer
into free'd memory as the block of memory was deallocated when
`Command` is dropped. This is fixed in this commit by installing a
`Drop` stack object which ensures that the `environ` pointer is
preserved on a failing `exec`.
* Second, the global environment was accessed in an unsynchronized
fashion during `exec`. This was fixed by ensuring that the
Rust-specific environment lock is acquired for these system-level
operations.
Thanks to Alex Gaynor for pioneering the solution here!
Closes #55775
Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
---
src/libstd/sys/unix/os.rs | 20 +++++---
src/libstd/sys/unix/process/process_unix.rs | 56 ++++++++++++++++++---
src/test/run-pass/command-exec.rs | 36 +++++++++++++
3 files changed, 96 insertions(+), 16 deletions(-)
diff --git a/src/libstd/sys/unix/os.rs b/src/libstd/sys/unix/os.rs
index 971e6501c2c2a..b387a8d59a56d 100644
--- a/src/libstd/sys/unix/os.rs
+++ b/src/libstd/sys/unix/os.rs
@@ -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 {
@@ -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() {
@@ -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
@@ -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(|_| ())
}
}
@@ -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(|_| ())
}
}
diff --git a/src/libstd/sys/unix/process/process_unix.rs b/src/libstd/sys/unix/process/process_unix.rs
index 7f1f9353c6d09..bfbf12f34ee37 100644
--- a/src/libstd/sys/unix/process/process_unix.rs
+++ b/src/libstd/sys/unix/process/process_unix.rs
@@ -11,9 +11,9 @@
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
@@ -22,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();
@@ -41,8 +39,21 @@ impl Command {
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());
@@ -114,7 +125,16 @@ impl Command {
}
match self.setup_io(default, true) {
- Ok((_, theirs)) => unsafe { self.do_exec(theirs, envp.as_ref()) },
+ 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();
+
+ self.do_exec(theirs, envp.as_ref())
+ }
+ }
Err(e) => e,
}
}
@@ -193,9 +213,6 @@ impl Command {
if let Some(ref cwd) = *self.get_cwd() {
t!(cvt(libc::chdir(cwd.as_ptr())));
}
- if let Some(envp) = maybe_envp {
- *sys::os::environ() = envp.as_ptr();
- }
// emscripten has no signal support.
#[cfg(not(any(target_os = "emscripten")))]
@@ -231,6 +248,27 @@ impl Command {
t!(callback());
}
+ // 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;
+ }
+ }
+ }
+
+ _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()
}
@@ -330,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(
diff --git a/src/test/run-pass/command-exec.rs b/src/test/run-pass/command-exec.rs
index 46b409fb13a84..f662945c0cf3d 100644
--- a/src/test/run-pass/command-exec.rs
+++ b/src/test/run-pass/command-exec.rs
@@ -48,6 +48,23 @@ fn main() {
println!("passed");
}
+ "exec-test5" => {
+ env::set_var("VARIABLE", "ABC");
+ Command::new("definitely-not-a-real-binary").env("VARIABLE", "XYZ").exec();
+ assert_eq!(env::var("VARIABLE").unwrap(), "ABC");
+ 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
@@ -72,4 +89,23 @@ fn main() {
assert!(output.status.success());
assert!(output.stderr.is_empty());
assert_eq!(output.stdout, b"passed\n");
+
+ let output = Command::new(&me).arg("exec-test5").output().unwrap();
+ 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");
+ }
}