Skip to content

Add discussion that concurrent access to the environment is unsafe #116888

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Dec 15, 2023
Merged

Add discussion that concurrent access to the environment is unsafe #116888

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8057586
Add discussion that concurrent access to the environment is unsafe
tbu- Oct 18, 2023
2093d0c
Reformulate `std::env::{set,remove}_env` as safety note
tbu- Dec 13, 2023
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
38 changes: 28 additions & 10 deletions library/std/src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,17 +313,26 @@ impl Error for VarError {
/// Sets the environment variable `key` to the value `value` for the currently running
/// process.
///
/// Note that while concurrent access to environment variables is safe in Rust,
/// some platforms only expose inherently unsafe non-threadsafe APIs for
/// inspecting the environment. As a result, extra care needs to be taken when
/// auditing calls to unsafe external FFI functions to ensure that any external
/// environment accesses are properly synchronized with accesses in Rust.
/// Note that while concurrent access to environment variables ought to be safe
/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs
/// for inspecting the environment. As a result, using `set_var` or
/// `remove_var` in a multi-threaded Rust program can lead to undefined
/// behavior, for example in combination with DNS lookups from
/// [`std::net::ToSocketAddrs`]. This is a bug
/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be
/// fixed in a future version of Rust. Additionally, extra care needs to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this makes it sound like rust will fix the concurrent access problem, but it cannot. it should say that this function will probably become unsafe in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also seems to me that get_var has the same problems, right? It's reading data that might be changed by concurrent C code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since getenv is usually considered safe in a multithreaded C environment, but setenv/putenv is not, I do not think that it requires an extra note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, C code designed for multithreaded use will almost never write to the env.

/// taken when auditing calls to unsafe external FFI functions to ensure that
/// any external environment accesses are properly synchronized with accesses
/// in Rust. Since Rust does not expose its environment lock directly, this
/// means that all accesses to the environment must go through Rust's [`var`].
Copy link
Member

@thomcc thomcc Oct 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is requesting something impossible in most cases. I'm not sure that's reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have a suggestion what to write there?

Yes, it's unreasonable, but that's the current state of the function AFAICT.

///
/// Discussion of this unsafety on Unix may be found in:
///
/// - [Austin Group Bugzilla](https://austingroupbugs.net/view.php?id=188)
/// - [GNU C library Bugzilla](https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2)
///
/// [`std::net::ToSocketAddrs`]: crate::net::ToSocketAddrs
///
/// # Panics
///
/// This function may panic if `key` is empty, contains an ASCII equals sign `'='`
Expand Down Expand Up @@ -351,17 +360,26 @@ fn _set_var(key: &OsStr, value: &OsStr) {

/// Removes an environment variable from the environment of the currently running process.
///
/// Note that while concurrent access to environment variables is safe in Rust,
/// some platforms only expose inherently unsafe non-threadsafe APIs for
/// inspecting the environment. As a result extra care needs to be taken when
/// auditing calls to unsafe external FFI functions to ensure that any external
/// environment accesses are properly synchronized with accesses in Rust.
/// Note that while concurrent access to environment variables ought to be safe
/// in Rust, some platforms only expose inherently unsafe non-threadsafe APIs
/// for inspecting the environment. As a result, using `set_var` or
/// `remove_var` in a multi-threaded Rust program can lead to undefined
/// behavior, for example in combination with DNS lookups from
/// [`std::net::ToSocketAddrs`]. This is a bug
/// ([rust#27970](https://github.com/rust-lang/rust/issues/27970)) and will be
/// fixed in a future version of Rust. Additionally, extra care needs to be
/// taken when auditing calls to unsafe external FFI functions to ensure that
/// any external environment accesses are properly synchronized with accesses
/// in Rust. Since Rust does not expose its environment lock directly, this
/// means that all accesses to the environment must go through Rust's [`var`].
///
/// Discussion of this unsafety on Unix may be found in:
///
/// - [Austin Group Bugzilla](https://austingroupbugs.net/view.php?id=188)
/// - [GNU C library Bugzilla](https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2)
///
/// [`std::net::ToSocketAddrs`]: crate::net::ToSocketAddrs
///
/// # Panics
///
/// This function may panic if `key` is empty, contains an ASCII equals sign
Expand Down