Auto merge of #29305 - alexcrichton:bad-getenv, r=brson

As discovered in #29298, `env::set_var("", "")` will panic, but it turns out
that it *also* deadlocks on Unix systems. This happens because if a panic
happens while holding the environment lock, we then go try to read
RUST_BACKTRACE, grabbing the environment lock, causing a deadlock.

Specifically, the changes made here are:

* The environment lock is pushed into `std::sys` instead of `std::env`. This
  also only puts it in the Unix implementation, not Windows where the functions
  are already threadsafe.
* The `std::sys` implementation now returns `io::Result` so panics are
  explicitly at the `std::env` level.

Author: bors

src/libstd/env.rs

@@ -23,7 +23,6 @@ use ffi::{OsStr, OsString};
 use fmt;
 use io;
 use path::{Path, PathBuf};
-use sync::StaticMutex;
 use sys::os as os_imp;
 
 /// Returns the current working directory as a `PathBuf`.
@@ -68,8 +67,6 @@ pub fn set_current_dir<P: AsRef<Path>>(p: P) -> io::Result<()> {
     os_imp::chdir(p.as_ref())
 }
 
-static ENV_LOCK: StaticMutex = StaticMutex::new();
-
 /// An iterator over a snapshot of the environment variables of this process.
 ///
 /// This iterator is created through `std::env::vars()` and yields `(String,
@@ -133,7 +130,6 @@ pub fn vars() -> Vars {
 /// ```
 #[stable(feature = "env", since = "1.0.0")]
 pub fn vars_os() -> VarsOs {
-    let _g = ENV_LOCK.lock();
     VarsOs { inner: os_imp::env() }
 }
 
@@ -204,8 +200,9 @@ pub fn var_os<K: AsRef<OsStr>>(key: K) -> Option<OsString> {
 }
 
 fn _var_os(key: &OsStr) -> Option<OsString> {
-    let _g = ENV_LOCK.lock();
-    os_imp::getenv(key)
+    os_imp::getenv(key).unwrap_or_else(|e| {
+        panic!("failed to get environment variable `{:?}`: {}", key, e)
+    })
 }
 
 /// Possible errors from the `env::var` method.
@@ -281,8 +278,10 @@ pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(k: K, v: V) {
 }
 
 fn _set_var(k: &OsStr, v: &OsStr) {
-    let _g = ENV_LOCK.lock();
-    os_imp::setenv(k, v)
+    os_imp::setenv(k, v).unwrap_or_else(|e| {
+        panic!("failed to set environment variable `{:?}` to `{:?}`: {}",
+               k, v, e)
+    })
 }
 
 /// Removes an environment variable from the environment of the currently running process.
@@ -322,8 +321,9 @@ pub fn remove_var<K: AsRef<OsStr>>(k: K) {
 }
 
 fn _remove_var(k: &OsStr) {
-    let _g = ENV_LOCK.lock();
-    os_imp::unsetenv(k)
+    os_imp::unsetenv(k).unwrap_or_else(|e| {
+        panic!("failed to remove environment variable `{:?}`: {}", k, e)
+    })
 }
 
 /// An iterator over `Path` instances for parsing an environment variable

src/libstd/sys/unix/os.rs

@@ -26,11 +26,14 @@ use path::{self, PathBuf};
 use ptr;
 use slice;
 use str;
+use sync::StaticMutex;
 use sys::c;
 use sys::fd;
+use sys::cvt;
 use vec;
 
 const TMPBUF_SZ: usize = 128;
+static ENV_LOCK: StaticMutex = StaticMutex::new();
 
 /// Returns the platform-specific value of errno
 pub fn errno() -> i32 {
@@ -378,6 +381,7 @@ pub unsafe fn environ() -> *mut *const *const c_char {
 /// Returns a vector of (variable, value) byte-vector pairs for all the
 /// environment variables of the current process.
 pub fn env() -> Env {
+    let _g = ENV_LOCK.lock();
     return unsafe {
         let mut environ = *environ();
         if environ as usize == 0 {
@@ -401,35 +405,36 @@ pub fn env() -> Env {
     }
 }
 
-pub fn getenv(k: &OsStr) -> Option<OsString> {
-    unsafe {
-        let s = k.to_cstring().unwrap();
-        let s = libc::getenv(s.as_ptr()) as *const _;
+pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
+    // environment variables with a nul byte can't be set, so their value is
+    // always None as well
+    let k = try!(CString::new(k.as_bytes()));
+    let _g = ENV_LOCK.lock();
+    Ok(unsafe {
+        let s = libc::getenv(k.as_ptr()) as *const _;
         if s.is_null() {
             None
         } else {
             Some(OsStringExt::from_vec(CStr::from_ptr(s).to_bytes().to_vec()))
         }
-    }
+    })
 }
 
-pub fn setenv(k: &OsStr, v: &OsStr) {
-    unsafe {
-        let k = k.to_cstring().unwrap();
-        let v = v.to_cstring().unwrap();
-        if libc::funcs::posix01::unistd::setenv(k.as_ptr(), v.as_ptr(), 1) != 0 {
-            panic!("failed setenv: {}", io::Error::last_os_error());
-        }
-    }
+pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
+    let k = try!(CString::new(k.as_bytes()));
+    let v = try!(CString::new(v.as_bytes()));
+    let _g = ENV_LOCK.lock();
+    cvt(unsafe {
+        libc::funcs::posix01::unistd::setenv(k.as_ptr(), v.as_ptr(), 1)
+    }).map(|_| ())
 }
 
-pub fn unsetenv(n: &OsStr) {
-    unsafe {
-        let nbuf = n.to_cstring().unwrap();
-        if libc::funcs::posix01::unistd::unsetenv(nbuf.as_ptr()) != 0 {
-            panic!("failed unsetenv: {}", io::Error::last_os_error());
-        }
-    }
+pub fn unsetenv(n: &OsStr) -> io::Result<()> {
+    let nbuf = try!(CString::new(n.as_bytes()));
+    let _g = ENV_LOCK.lock();
+    cvt(unsafe {
+        libc::funcs::posix01::unistd::unsetenv(nbuf.as_ptr())
+    }).map(|_| ())
 }
 
 pub fn page_size() -> usize {
@@ -439,7 +444,7 @@ pub fn page_size() -> usize {
 }
 
 pub fn temp_dir() -> PathBuf {
-    getenv("TMPDIR".as_ref()).map(PathBuf::from).unwrap_or_else(|| {
+    ::env::var_os("TMPDIR").map(PathBuf::from).unwrap_or_else(|| {
         if cfg!(target_os = "android") {
             PathBuf::from("/data/local/tmp")
         } else {
@@ -449,7 +454,7 @@ pub fn temp_dir() -> PathBuf {
 }
 
 pub fn home_dir() -> Option<PathBuf> {
-    return getenv("HOME".as_ref()).or_else(|| unsafe {
+    return ::env::var_os("HOME").or_else(|| unsafe {
         fallback()
     }).map(PathBuf::from);
 

src/libstd/sys/windows/c.rs

@@ -32,6 +32,7 @@ pub const WSASYS_STATUS_LEN: usize = 128;
 pub const FIONBIO: libc::c_long = 0x8004667e;
 pub const FD_SETSIZE: usize = 64;
 pub const MSG_DONTWAIT: libc::c_int = 0;
+pub const ERROR_ENVVAR_NOT_FOUND: libc::c_int = 203;
 pub const ERROR_ILLEGAL_CHARACTER: libc::c_int = 582;
 pub const ENABLE_ECHO_INPUT: libc::DWORD = 0x4;
 pub const ENABLE_EXTENDED_FLAGS: libc::DWORD = 0x80;

src/libstd/sys/windows/os.rs

@@ -26,7 +26,7 @@ use os::windows::ffi::EncodeWide;
 use path::{self, PathBuf};
 use ptr;
 use slice;
-use sys::c;
+use sys::{c, cvt};
 use sys::handle::Handle;
 
 use libc::funcs::extra::kernel32::{
@@ -248,41 +248,41 @@ pub fn chdir(p: &path::Path) -> io::Result<()> {
     let mut p = p.encode_wide().collect::<Vec<_>>();
     p.push(0);
 
-    unsafe {
-        match libc::SetCurrentDirectoryW(p.as_ptr()) != (0 as libc::BOOL) {
-            true => Ok(()),
-            false => Err(io::Error::last_os_error()),
-        }
-    }
+    cvt(unsafe {
+        libc::SetCurrentDirectoryW(p.as_ptr())
+    }).map(|_| ())
 }
 
-pub fn getenv(k: &OsStr) -> Option<OsString> {
+pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
     let k = super::to_utf16_os(k);
-    super::fill_utf16_buf(|buf, sz| unsafe {
+    let res = super::fill_utf16_buf(|buf, sz| unsafe {
         libc::GetEnvironmentVariableW(k.as_ptr(), buf, sz)
     }, |buf| {
         OsStringExt::from_wide(buf)
-    }).ok()
+    });
+    match res {
+        Ok(value) => Ok(Some(value)),
+        Err(ref e) if e.raw_os_error() == Some(c::ERROR_ENVVAR_NOT_FOUND) => {
+            Ok(None)
+        }
+        Err(e) => Err(e)
+    }
 }
 
-pub fn setenv(k: &OsStr, v: &OsStr) {
+pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
     let k = super::to_utf16_os(k);
     let v = super::to_utf16_os(v);
 
-    unsafe {
-        if libc::SetEnvironmentVariableW(k.as_ptr(), v.as_ptr()) == 0 {
-            panic!("failed to set env: {}", io::Error::last_os_error());
-        }
-    }
+    cvt(unsafe {
+        libc::SetEnvironmentVariableW(k.as_ptr(), v.as_ptr())
+    }).map(|_| ())
 }
 
-pub fn unsetenv(n: &OsStr) {
+pub fn unsetenv(n: &OsStr) -> io::Result<()> {
     let v = super::to_utf16_os(n);
-    unsafe {
-        if libc::SetEnvironmentVariableW(v.as_ptr(), ptr::null()) == 0 {
-            panic!("failed to unset env: {}", io::Error::last_os_error());
-        }
-    }
+    cvt(unsafe {
+        libc::SetEnvironmentVariableW(v.as_ptr(), ptr::null())
+    }).map(|_| ())
 }
 
 pub struct Args {
@@ -339,8 +339,8 @@ pub fn temp_dir() -> PathBuf {
 }
 
 pub fn home_dir() -> Option<PathBuf> {
-    getenv("HOME".as_ref()).or_else(|| {
-        getenv("USERPROFILE".as_ref())
+    ::env::var_os("HOME").or_else(|| {
+        ::env::var_os("USERPROFILE")
     }).map(PathBuf::from).or_else(|| unsafe {
         let me = c::GetCurrentProcess();
         let mut token = ptr::null_mut();