Rollup merge of #86183 - inquisitivecrystal:env-nul, r=m-ou-se

Change environment variable getters to error recoverably

This PR changes the standard library environment variable getter functions to error recoverably (i.e. not panic) when given an invalid value.

On some platforms, it is invalid for environment variable names to contain `'\0'` or `'='`, or for their values to contain `'\0'`. Currently, the standard library panics when manipulating environment variables with names or values that violate these invariants. However, this behavior doesn't make a lot of sense, at least in the case of getters. If the environment variable is missing, the standard library just returns an error value, rather than panicking. It doesn't make sense to treat the case where the variable is invalid any differently from that. See the [internals thread](https://internals.rust-lang.org/t/why-should-std-var-panic/14847) for discussion. Thus, this PR changes the functions to error recoverably in this case as well.

If desired, I could change the functions that manipulate environment variables in other ways as well. I didn't do that here because it wasn't entirely clear what to change them to. Should they error silently or do something else? If someone tells me how to change them, I'm happy to implement the changes.

This fixes #86082, an ICE that arises from the current behavior. It also adds a regression test to make sure the ICE does not occur again in the future.

`@rustbot` label +T-libs
r? `@joshtriplett`

library/std/src/env.rs

@@ -185,15 +185,9 @@ impl fmt::Debug for VarsOs {
 ///
 /// # Errors
 ///
-/// Errors if the environment variable is not present.
-/// Errors if the environment variable is not valid Unicode. If this is not desired, consider using
-/// [`var_os`].
-///
-/// # Panics
-///
-/// This function may panic if `key` is empty, contains an ASCII equals sign
-/// `'='` or the NUL character `'\0'`, or when the value contains the NUL
-/// character.
+/// Returns `[None]` if the environment variable isn't set.
+/// Returns `[None]` if the environment variable is not valid Unicode. If this is not
+/// desired, consider using [`var_os`].
 ///
 /// # Examples
 ///
@@ -219,18 +213,17 @@ fn _var(key: &OsStr) -> Result<String, VarError> {
 }
 
 /// Fetches the environment variable `key` from the current process, returning
-/// [`None`] if the variable isn't set.
-///
-/// # Panics
-///
-/// This function may panic if `key` is empty, contains an ASCII equals sign
-/// `'='` or the NUL character `'\0'`, or when the value contains the NUL
-/// character.
+/// [`None`] if the variable isn't set or there's another error.
 ///
 /// Note that the method will not check if the environment variable
 /// is valid Unicode. If you want to have an error on invalid UTF-8,
 /// use the [`var`] function instead.
 ///
+/// # Errors
+///
+/// Returns `[None]` if the variable isn't set.
+/// May return `[None]` if the variable value contains the NUL character.
+///
 /// # Examples
 ///
 /// ```
@@ -249,7 +242,6 @@ pub fn var_os<K: AsRef<OsStr>>(key: K) -> Option<OsString> {
 
 fn _var_os(key: &OsStr) -> Option<OsString> {
     os_imp::getenv(key)
-        .unwrap_or_else(|e| panic!("failed to get environment variable `{:?}`: {}", key, e))
 }
 
 /// The error type for operations interacting with environment variables.

library/std/src/sys/hermit/os.rs

@@ -140,13 +140,8 @@ pub fn env() -> Env {
     }
 }
 
-pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
-    unsafe {
-        match ENV.as_ref().unwrap().lock().unwrap().get_mut(k) {
-            Some(value) => Ok(Some(value.clone())),
-            None => Ok(None),
-        }
-    }
+pub fn getenv(k: &OsStr) -> Option<OsString> {
+    unsafe { ENV.as_ref().unwrap().lock().unwrap().get_mut(k).cloned() }
 }
 
 pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {

library/std/src/sys/sgx/os.rs

@@ -106,8 +106,8 @@ pub fn env() -> Env {
     get_env_store().map(|env| clone_to_vec(&env.lock().unwrap())).unwrap_or_default().into_iter()
 }
 
-pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
-    Ok(get_env_store().and_then(|s| s.lock().unwrap().get(k).cloned()))
+pub fn getenv(k: &OsStr) -> Option<OsString> {
+    get_env_store().and_then(|s| s.lock().unwrap().get(k).cloned())
 }
 
 pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {

library/std/src/sys/unix/os.rs

@@ -532,19 +532,18 @@ pub fn env() -> Env {
     }
 }
 
-pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
+pub fn getenv(k: &OsStr) -> Option<OsString> {
     // environment variables with a nul byte can't be set, so their value is
     // always None as well
-    let k = CString::new(k.as_bytes())?;
+    let k = CString::new(k.as_bytes()).ok()?;
     unsafe {
         let _guard = env_read_lock();
         let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
-        let ret = if s.is_null() {
+        if s.is_null() {
             None
         } else {
             Some(OsStringExt::from_vec(CStr::from_ptr(s).to_bytes().to_vec()))
-        };
-        Ok(ret)
+        }
     }
 }
 

library/std/src/sys/unsupported/os.rs

@@ -76,8 +76,8 @@ pub fn env() -> Env {
     panic!("not supported on this platform")
 }
 
-pub fn getenv(_: &OsStr) -> io::Result<Option<OsString>> {
-    Ok(None)
+pub fn getenv(_: &OsStr) -> Option<OsString> {
+    None
 }
 
 pub fn setenv(_: &OsStr, _: &OsStr) -> io::Result<()> {

library/std/src/sys/wasi/os.rs

@@ -175,17 +175,16 @@ pub fn env() -> Env {
     }
 }
 
-pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
-    let k = CString::new(k.as_bytes())?;
+pub fn getenv(k: &OsStr) -> Option<OsString> {
+    let k = CString::new(k.as_bytes()).ok()?;
     unsafe {
         let _guard = env_lock();
         let s = libc::getenv(k.as_ptr()) as *const libc::c_char;
-        let ret = if s.is_null() {
+        if s.is_null() {
             None
         } else {
             Some(OsStringExt::from_vec(CStr::from_ptr(s).to_bytes().to_vec()))
-        };
-        Ok(ret)
+        }
     }
 }
 

library/std/src/sys/windows/os.rs

@@ -253,22 +253,13 @@ pub fn chdir(p: &path::Path) -> io::Result<()> {
     cvt(unsafe { c::SetCurrentDirectoryW(p.as_ptr()) }).map(drop)
 }
 
-pub fn getenv(k: &OsStr) -> io::Result<Option<OsString>> {
-    let k = to_u16s(k)?;
-    let res = super::fill_utf16_buf(
+pub fn getenv(k: &OsStr) -> Option<OsString> {
+    let k = to_u16s(k).ok()?;
+    super::fill_utf16_buf(
         |buf, sz| unsafe { c::GetEnvironmentVariableW(k.as_ptr(), buf, sz) },
         |buf| OsStringExt::from_wide(buf),
-    );
-    match res {
-        Ok(value) => Ok(Some(value)),
-        Err(e) => {
-            if e.raw_os_error() == Some(c::ERROR_ENVVAR_NOT_FOUND as i32) {
-                Ok(None)
-            } else {
-                Err(e)
-            }
-        }
-    }
+    )
+    .ok()
 }
 
 pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {

src/test/ui/macros/issue-86082-option-env-invalid-char.rs

@@ -0,0 +1,10 @@
+// check-pass
+//
+// Regression test for issue #86082
+//
+// Checks that option_env! does not panic on receiving an invalid
+// environment variable name.
+
+fn main() {
+    option_env!("\0=");
+}