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`

6 files changed, 19 insertions, 35 deletions

diff --git a/library/std/src/sys/hermit/os.rs b/library/std/src/sys/hermit/os.rs
index eeb30a578c0..8f927df85be 100644
--- a/library/std/src/sys/hermit/os.rs
+++ b/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<()> {
diff --git a/library/std/src/sys/sgx/os.rs b/library/std/src/sys/sgx/os.rs
index 144248d60c9..5f8b8def7c6 100644
--- a/library/std/src/sys/sgx/os.rs
+++ b/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<()> {
diff --git a/library/std/src/sys/unix/os.rs b/library/std/src/sys/unix/os.rs
index cc0802ed709..dfbd93c90e6 100644
--- a/library/std/src/sys/unix/os.rs
+++ b/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)
+        }
     }
 }
 
diff --git a/library/std/src/sys/unsupported/os.rs b/library/std/src/sys/unsupported/os.rs
index e30395a0b1d..2886ec1180e 100644
--- a/library/std/src/sys/unsupported/os.rs
+++ b/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<()> {
diff --git a/library/std/src/sys/wasi/os.rs b/library/std/src/sys/wasi/os.rs
index f129ee55a83..c5229a18834 100644
--- a/library/std/src/sys/wasi/os.rs
+++ b/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)
+        }
     }
 }
 
diff --git a/library/std/src/sys/windows/os.rs b/library/std/src/sys/windows/os.rs
index 77c378a66af..8db97ba50a8 100644
--- a/library/std/src/sys/windows/os.rs
+++ b/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<()> {