From b967d55c8e6df8464bb709efb5db1a8bf5401108 Mon Sep 17 00:00:00 2001 From: Alex Crichton Date: Tue, 13 Nov 2018 14:57:10 -0800 Subject: [PATCH] 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 --- src/libstd/sys/unix/os.rs | 20 +++++++------ src/libstd/sys/unix/process/process_unix.rs | 31 ++++++++++++++++++-- src/test/run-pass/command-exec.rs | 32 +++++++++++++++++++++ 3 files changed, 72 insertions(+), 11 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> { // 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..bbfdd6f1e7557 100644 --- a/src/libstd/sys/unix/process/process_unix.rs +++ b/src/libstd/sys/unix/process/process_unix.rs @@ -193,9 +193,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 +228,30 @@ impl Command { t!(callback()); } + // Note that we're accessing process-global state, `environ`, which + // means we need the rust-specific environment lock. 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 _lock = sys::os::env_lock(); + 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 +351,10 @@ impl Command { libc::POSIX_SPAWN_SETSIGMASK; cvt(libc::posix_spawnattr_setflags(&mut attrs.0, flags as _))?; + // We'e reading `sys::os::environ` below so make sure that we do so + // in a synchronized fashion via the rust-specific global + // environment lock. + let _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..0352161d7ddff 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,19 @@ 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"); + + let output = Command::new(&me).arg("exec-test6").output().unwrap(); + 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(); + assert!(output.status.success()); + assert!(output.stderr.is_empty()); + assert_eq!(output.stdout, b"passed\n"); }