std: Synchronize access to global env during exec

This commit, after reverting rust-lang#55359, applies a different fix for rust-lang#46775
while also fixing rust-lang#55775. The basic idea was to go back to pre-rust-lang#55359
libstd, and then fix rust-lang#46775 in a way that doesn't expose rust-lang#55775.

The issue described in rust-lang#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 rust-lang#55775

Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>

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(|_| ())
     }
 }

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(

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");
 }