From 8057586d3dca2f7bdda2096f4284c0323ed46b0b Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Wed, 18 Oct 2023 14:59:53 +0200 Subject: [PATCH 1/2] Add discussion that concurrent access to the environment is unsafe The bug report #27970 has existed for 8 years, the actual bug dates back to Rust pre-1.0. I documented it since it's in the interest of the user to be aware of it. The note can be removed once #27970 is fixed. --- library/std/src/env.rs | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index f67f6034d3412..53cbb80d3c042 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -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 +/// 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 `'='` @@ -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 From 2093d0c58e4508012439a55d6c5a52929d16748f Mon Sep 17 00:00:00 2001 From: Tobias Bucher Date: Wed, 13 Dec 2023 12:49:38 +0100 Subject: [PATCH 2/2] Reformulate `std::env::{set,remove}_env` as safety note --- library/std/src/env.rs | 60 +++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/library/std/src/env.rs b/library/std/src/env.rs index 53cbb80d3c042..30ac0512348ca 100644 --- a/library/std/src/env.rs +++ b/library/std/src/env.rs @@ -313,18 +313,24 @@ 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 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`]. +/// # Safety +/// +/// Even though this function is currently not marked as `unsafe`, it needs to +/// be because invoking it can cause undefined behaviour. The function will be +/// marked `unsafe` in a future version of Rust. This is tracked in +/// [rust#27970](https://github.com/rust-lang/rust/issues/27970). +/// +/// This function is safe to call in a single-threaded program. +/// +/// In multi-threaded programs, you must ensure that are no other threads +/// concurrently writing or *reading*(!) from the environment through functions +/// other than the ones in this module. You are responsible for figuring out +/// how to achieve this, but we strongly suggest not using `set_var` or +/// `remove_var` in multi-threaded programs at all. +/// +/// Most C libraries, including libc itself do not advertise which functions +/// read from the environment. Even functions from the Rust standard library do +/// that, e.g. for DNS lookups from [`std::net::ToSocketAddrs`]. /// /// Discussion of this unsafety on Unix may be found in: /// @@ -360,18 +366,24 @@ 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 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`]. +/// # Safety +/// +/// Even though this function is currently not marked as `unsafe`, it needs to +/// be because invoking it can cause undefined behaviour. The function will be +/// marked `unsafe` in a future version of Rust. This is tracked in +/// [rust#27970](https://github.com/rust-lang/rust/issues/27970). +/// +/// This function is safe to call in a single-threaded program. +/// +/// In multi-threaded programs, you must ensure that are no other threads +/// concurrently writing or *reading*(!) from the environment through functions +/// other than the ones in this module. You are responsible for figuring out +/// how to achieve this, but we strongly suggest not using `set_var` or +/// `remove_var` in multi-threaded programs at all. +/// +/// Most C libraries, including libc itself do not advertise which functions +/// read from the environment. Even functions from the Rust standard library do +/// that, e.g. for DNS lookups from [`std::net::ToSocketAddrs`]. /// /// Discussion of this unsafety on Unix may be found in: ///