Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change environment variable getters to error recoverably #86183

Merged
merged 4 commits into from
Aug 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 9 additions & 17 deletions library/std/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
/// 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`].
m-ou-se marked this conversation as resolved.
Show resolved Hide resolved
///
/// # Examples
///
Expand All @@ -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
///
/// ```
Expand All @@ -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.
Expand Down
9 changes: 2 additions & 7 deletions library/std/src/sys/hermit/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/sgx/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
9 changes: 4 additions & 5 deletions library/std/src/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/unsupported/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
9 changes: 4 additions & 5 deletions library/std/src/sys/wasi/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}

Expand Down
19 changes: 5 additions & 14 deletions library/std/src/sys/windows/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<()> {
Expand Down
10 changes: 10 additions & 0 deletions src/test/ui/macros/issue-86082-option-env-invalid-char.rs
Original file line number Diff line number Diff line change
@@ -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=");
}