From f2c0f29248fa6f6d2c8d71fdfa2dfb86088dc4c7 Mon Sep 17 00:00:00 2001 From: Aris Merchant <22333129+inquisitivecrystal@users.noreply.github.com> Date: Wed, 9 Jun 2021 00:40:31 -0700 Subject: [PATCH 1/4] Change env var getters to error recoverably Before this, `std`'s env var getter functions would panic on receiving certain invalid inputs. This commit makes them return a `None` or `Err` instead. --- library/std/src/env.rs | 25 ++++++++++--------------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index 64f88c1aba68e..c0f9cc1f64e9f 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -188,12 +188,8 @@ impl fmt::Debug for VarsOs { /// 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. +/// May error if the `key` is empty, contains an ASCII equals sign `'='`, or contains the NUL character `'\0'`. +/// May error when the value contains the NUL character. /// /// # Examples /// @@ -219,18 +215,18 @@ fn _var(key: &OsStr) -> Result { } /// 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 +/// +/// Errors if the variable isn't set. +/// May error if the `key` is empty, contains an ASCII equals sign `'='`, or contains the NUL character `'\0'`. +/// May error when the value contains the NUL character. +/// /// # Examples /// /// ``` @@ -248,8 +244,7 @@ pub fn var_os>(key: K) -> Option { } fn _var_os(key: &OsStr) -> Option { - os_imp::getenv(key) - .unwrap_or_else(|e| panic!("failed to get environment variable `{:?}`: {}", key, e)) + os_imp::getenv(key).ok()? } /// The error type for operations interacting with environment variables. From d26e01e9df416304f7d0cc425bd0290560e12fae Mon Sep 17 00:00:00 2001 From: Aris Merchant <22333129+inquisitivecrystal@users.noreply.github.com> Date: Wed, 9 Jun 2021 01:44:06 -0700 Subject: [PATCH 2/4] Add test to prevent regression --- .../ui/macros/issue-86082-option-env-invalid-char.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 src/test/ui/macros/issue-86082-option-env-invalid-char.rs diff --git a/src/test/ui/macros/issue-86082-option-env-invalid-char.rs b/src/test/ui/macros/issue-86082-option-env-invalid-char.rs new file mode 100644 index 0000000000000..b556b24d6aa6a --- /dev/null +++ b/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="); +} From a12107afaaa634cd7352d3828caef89a975299bb Mon Sep 17 00:00:00 2001 From: Aris Merchant <22333129+inquisitivecrystal@users.noreply.github.com> Date: Thu, 24 Jun 2021 16:04:24 -0700 Subject: [PATCH 3/4] Make `getenv` return an Option instead of a Result --- library/std/src/env.rs | 2 +- library/std/src/sys/hermit/os.rs | 9 ++------- library/std/src/sys/sgx/os.rs | 4 ++-- library/std/src/sys/unix/os.rs | 9 ++++----- library/std/src/sys/unsupported/os.rs | 4 ++-- library/std/src/sys/wasi/os.rs | 9 ++++----- library/std/src/sys/windows/os.rs | 19 +++++-------------- 7 files changed, 20 insertions(+), 36 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index c0f9cc1f64e9f..a7b2748681114 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -244,7 +244,7 @@ pub fn var_os>(key: K) -> Option { } fn _var_os(key: &OsStr) -> Option { - os_imp::getenv(key).ok()? + os_imp::getenv(key) } /// The error type for operations interacting with environment variables. diff --git a/library/std/src/sys/hermit/os.rs b/library/std/src/sys/hermit/os.rs index eeb30a578c05b..8f927df85be5d 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> { - 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 { + 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 144248d60c9cf..5f8b8def7c670 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> { - Ok(get_env_store().and_then(|s| s.lock().unwrap().get(k).cloned())) +pub fn getenv(k: &OsStr) -> Option { + 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 71fec79347a87..1694656114a10 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> { +pub fn getenv(k: &OsStr) -> Option { // 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 e30395a0b1d92..2886ec1180e54 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> { - Ok(None) +pub fn getenv(_: &OsStr) -> Option { + 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 f129ee55a8391..c5229a188342a 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> { - let k = CString::new(k.as_bytes())?; +pub fn getenv(k: &OsStr) -> Option { + 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 77c378a66afd7..8db97ba50a81f 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> { - let k = to_u16s(k)?; - let res = super::fill_utf16_buf( +pub fn getenv(k: &OsStr) -> Option { + 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<()> { From d9752c7d843f3f93ed7f570b072aec8eb5127a96 Mon Sep 17 00:00:00 2001 From: Aris Merchant <22333129+inquisitivecrystal@users.noreply.github.com> Date: Mon, 5 Jul 2021 22:12:48 -0700 Subject: [PATCH 4/4] Improve env var getter docs --- library/std/src/env.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index a7b2748681114..a359ad3b55889 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -185,11 +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`]. -/// May error if the `key` is empty, contains an ASCII equals sign `'='`, or contains the NUL character `'\0'`. -/// May error 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 /// @@ -223,9 +221,8 @@ fn _var(key: &OsStr) -> Result { /// /// # Errors /// -/// Errors if the variable isn't set. -/// May error if the `key` is empty, contains an ASCII equals sign `'='`, or contains the NUL character `'\0'`. -/// May error when the value contains the NUL character. +/// Returns `[None]` if the variable isn't set. +/// May return `[None]` if the variable value contains the NUL character. /// /// # Examples ///