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

Make std::env::{set_var, remove_var} unsafe in edition 2024 #124636

Merged
merged 4 commits into from
May 30, 2024
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
6 changes: 6 additions & 0 deletions compiler/rustc_feature/src/builtin_attrs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,12 @@ pub const BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
"rustc_allowed_through_unstable_modules special cases accidental stabilizations of stable items \
through unstable paths"
),
rustc_attr!(
rustc_deprecated_safe_2024, Normal, template!(Word), WarnFollowing,
EncodeCrossCrate::Yes,
"rustc_deprecated_safe_2024 is supposed to be used in libstd only",
),


// ==========================================================================
// Internal attributes: Type system related:
Expand Down
49 changes: 49 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ declare_lint_pass! {
DEPRECATED,
DEPRECATED_CFG_ATTR_CRATE_TYPE_NAME,
DEPRECATED_IN_FUTURE,
DEPRECATED_SAFE,
DEPRECATED_WHERE_CLAUSE_LOCATION,
DUPLICATE_MACRO_ATTRIBUTES,
ELIDED_LIFETIMES_IN_ASSOCIATED_CONSTANT,
Expand Down Expand Up @@ -4844,3 +4845,51 @@ declare_lint! {
reference: "issue #124559 <https://github.com/rust-lang/rust/issues/124559>",
};
}

declare_lint! {
/// The `deprecated_safe` lint detects unsafe functions being used as safe
/// functions.
///
/// ### Example
///
/// ```rust,edition2021,compile_fail
/// #![deny(deprecated_safe)]
/// // edition 2021
/// use std::env;
/// fn enable_backtrace() {
/// env::set_var("RUST_BACKTRACE", "1");
/// }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// Rust [editions] allow the language to evolve without breaking backward
/// compatibility. This lint catches code that uses `unsafe` functions that
/// were declared as safe (non-`unsafe`) in earlier editions. If you switch
/// the compiler to a new edition without updating the code, then it
/// will fail to compile if you are using a function previously marked as
/// safe.
///
/// You can audit the code to see if it suffices the preconditions of the
/// `unsafe` code, and if it does, you can wrap it in an `unsafe` block. If
/// you can't fulfill the preconditions, you probably need to switch to a
/// different way of doing what you want to achieve.
///
/// This lint can automatically wrap the calls in `unsafe` blocks, but this
/// obviously cannot verify that the preconditions of the `unsafe`
/// functions are fulfilled, so that is still up to the user.
///
/// The lint is currently "allow" by default, but that might change in the
/// future.
///
/// [editions]: https://doc.rust-lang.org/edition-guide/
pub DEPRECATED_SAFE,
Allow,
"detects unsafe functions being used as safe functions",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::EditionError(Edition::Edition2024),
reference: "issue #27970 <https://github.com/rust-lang/rust/issues/27970>",
};
}
6 changes: 6 additions & 0 deletions compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ mir_build_borrow_of_moved_value = borrow of moved value
.value_borrowed_label = value borrowed here after move
.suggestion = borrow this binding in the pattern to avoid moving the value

mir_build_call_to_deprecated_safe_fn_requires_unsafe =
call to deprecated safe function `{$function}` is unsafe and requires unsafe block
.note = consult the function's documentation for information on how to avoid undefined behavior
.label = call to unsafe function
.suggestion = you can wrap the call in an `unsafe` block if you can guarantee the code is only ever called from single-threaded code
Copy link
Member

Choose a reason for hiding this comment

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

This will be incorrect if we mark any other functions with this attribute where the unsafety is for a different reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it'll be fixed before #125970 is merged. This PR was intentionally minimal so it could definitely make it into the Rust 2024 edition.


mir_build_call_to_fn_with_requires_unsafe =
call to function `{$function}` with `#[target_feature]` is unsafe and requires unsafe block
.help = in order for the call to be safe, the context requires the following additional target {$missing_target_features_count ->
Expand Down
30 changes: 25 additions & 5 deletions compiler/rustc_mir_build/src/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc_middle::thir::visit::Visitor;
use rustc_middle::thir::*;
use rustc_middle::ty::print::with_no_trimmed_paths;
use rustc_middle::ty::{self, ParamEnv, Ty, TyCtxt};
use rustc_session::lint::builtin::{UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::builtin::{DEPRECATED_SAFE, UNSAFE_OP_IN_UNSAFE_FN, UNUSED_UNSAFE};
use rustc_session::lint::Level;
use rustc_span::def_id::{DefId, LocalDefId};
use rustc_span::symbol::Symbol;
Expand Down Expand Up @@ -110,14 +110,34 @@ impl<'tcx> UnsafetyVisitor<'_, 'tcx> {
);
self.suggest_unsafe_block = false;
}
SafetyContext::Safe => {
kind.emit_requires_unsafe_err(
SafetyContext::Safe => match kind {
// Allow calls to deprecated-safe unsafe functions if the
// caller is from an edition before 2024.
UnsafeOpKind::CallToUnsafeFunction(Some(id))
if !span.at_least_rust_2024()
&& self.tcx.has_attr(id, sym::rustc_deprecated_safe_2024) =>
{
self.tcx.emit_node_span_lint(
DEPRECATED_SAFE,
self.hir_context,
span,
CallToDeprecatedSafeFnRequiresUnsafe {
span,
function: with_no_trimmed_paths!(self.tcx.def_path_str(id)),
sub: CallToDeprecatedSafeFnRequiresUnsafeSub {
left: span.shrink_to_lo(),
right: span.shrink_to_hi(),
},
},
)
}
_ => kind.emit_requires_unsafe_err(
self.tcx,
span,
self.hir_context,
unsafe_op_in_unsafe_fn_allowed,
);
}
),
},
}
}

Expand Down
19 changes: 19 additions & 0 deletions compiler/rustc_mir_build/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,25 @@ pub struct UnconditionalRecursion {
pub call_sites: Vec<Span>,
}

#[derive(LintDiagnostic)]
#[diag(mir_build_call_to_deprecated_safe_fn_requires_unsafe)]
pub struct CallToDeprecatedSafeFnRequiresUnsafe {
#[label]
pub span: Span,
pub function: String,
#[subdiagnostic]
pub sub: CallToDeprecatedSafeFnRequiresUnsafeSub,
}

#[derive(Subdiagnostic)]
#[multipart_suggestion(mir_build_suggestion, applicability = "machine-applicable")]
pub struct CallToDeprecatedSafeFnRequiresUnsafeSub {
#[suggestion_part(code = "unsafe {{ ")]
pub left: Span,
#[suggestion_part(code = " }}")]
pub right: Span,
}

#[derive(LintDiagnostic)]
#[diag(mir_build_unsafe_op_in_unsafe_fn_call_to_unsafe_fn_requires_unsafe, code = E0133)]
#[note]
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_span/src/symbol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1575,6 +1575,7 @@ symbols! {
rustc_def_path,
rustc_default_body_unstable,
rustc_deny_explicit_impl,
rustc_deprecated_safe_2024,
rustc_diagnostic_item,
rustc_diagnostic_macros,
rustc_dirty,
Expand Down
102 changes: 66 additions & 36 deletions library/std/src/env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,22 +318,25 @@ impl Error for VarError {
///
/// # 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`].
/// This function is also always safe to call on Windows, in single-threaded
/// and multi-threaded programs.
///
/// In multi-threaded programs on other operating systems, we strongly suggest
/// not using `set_var` or `remove_var` at all. The exact requirement is: you
/// must ensure that there are no other threads concurrently writing or
/// *reading*(!) the environment through functions or global variables other
/// than the ones in this module. The problem is that these operating systems
/// do not provide a thread-safe way to read the environment, and most C
/// libraries, including libc itself, do not advertise which functions read
/// from the environment. Even functions from the Rust standard library may
/// read the environment without going through this module, e.g. for DNS
/// lookups from [`std::net::ToSocketAddrs`]. No stable guarantee is made about
/// which functions may read from the environment in future versions of a
/// library. All this makes it not practically possible for you to guarantee
/// that no other thread will read the environment, so the only safe option is
/// to not use `set_var` or `remove_var` in multi-threaded programs at all.
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
///
/// Discussion of this unsafety on Unix may be found in:
///
Expand All @@ -353,15 +356,26 @@ impl Error for VarError {
/// use std::env;
///
/// let key = "KEY";
/// env::set_var(key, "VALUE");
/// unsafe {
/// env::set_var(key, "VALUE");
/// }
/// assert_eq!(env::var(key), Ok("VALUE".to_string()));
/// ```
#[cfg(not(bootstrap))]
#[rustc_deprecated_safe_2024]
petrochenkov marked this conversation as resolved.
Show resolved Hide resolved
#[stable(feature = "env", since = "1.0.0")]
pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
pub unsafe fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
_set_var(key.as_ref(), value.as_ref())
}

fn _set_var(key: &OsStr, value: &OsStr) {
#[cfg(bootstrap)]
tbu- marked this conversation as resolved.
Show resolved Hide resolved
#[allow(missing_docs)]
#[stable(feature = "env", since = "1.0.0")]
pub fn set_var<K: AsRef<OsStr>, V: AsRef<OsStr>>(key: K, value: V) {
unsafe { _set_var(key.as_ref(), value.as_ref()) }
}

unsafe fn _set_var(key: &OsStr, value: &OsStr) {
os_imp::setenv(key, value).unwrap_or_else(|e| {
panic!("failed to set environment variable `{key:?}` to `{value:?}`: {e}")
})
Expand All @@ -371,22 +385,25 @@ fn _set_var(key: &OsStr, value: &OsStr) {
///
/// # 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`].
/// This function is also always safe to call on Windows, in single-threaded
/// and multi-threaded programs.
///
/// In multi-threaded programs on other operating systems, we strongly suggest
/// not using `set_var` or `remove_var` at all. The exact requirement is: you
/// must ensure that there are no other threads concurrently writing or
/// *reading*(!) the environment through functions or global variables other
/// than the ones in this module. The problem is that these operating systems
/// do not provide a thread-safe way to read the environment, and most C
/// libraries, including libc itself, do not advertise which functions read
/// from the environment. Even functions from the Rust standard library may
/// read the environment without going through this module, e.g. for DNS
/// lookups from [`std::net::ToSocketAddrs`]. No stable guarantee is made about
/// which functions may read from the environment in future versions of a
/// library. All this makes it not practically possible for you to guarantee
/// that no other thread will read the environment, so the only safe option is
/// to not use `set_var` or `remove_var` in multi-threaded programs at all.
///
/// Discussion of this unsafety on Unix may be found in:
///
Expand All @@ -403,22 +420,35 @@ fn _set_var(key: &OsStr, value: &OsStr) {
///
/// # Examples
///
/// ```
/// ```no_run
/// use std::env;
///
/// let key = "KEY";
/// env::set_var(key, "VALUE");
/// unsafe {
/// env::set_var(key, "VALUE");
/// }
/// assert_eq!(env::var(key), Ok("VALUE".to_string()));
///
/// env::remove_var(key);
/// unsafe {
/// env::remove_var(key);
/// }
/// assert!(env::var(key).is_err());
/// ```
#[cfg(not(bootstrap))]
#[rustc_deprecated_safe_2024]
#[stable(feature = "env", since = "1.0.0")]
pub fn remove_var<K: AsRef<OsStr>>(key: K) {
pub unsafe fn remove_var<K: AsRef<OsStr>>(key: K) {
_remove_var(key.as_ref())
}

fn _remove_var(key: &OsStr) {
#[cfg(bootstrap)]
#[allow(missing_docs)]
#[stable(feature = "env", since = "1.0.0")]
pub fn remove_var<K: AsRef<OsStr>>(key: K) {
unsafe { _remove_var(key.as_ref()) }
}

unsafe fn _remove_var(key: &OsStr) {
os_imp::unsetenv(key)
.unwrap_or_else(|e| panic!("failed to remove environment variable `{key:?}`: {e}"))
}
Expand Down
14 changes: 5 additions & 9 deletions library/std/src/sys/pal/hermit/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,18 +172,14 @@ 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<()> {
unsafe {
let (k, v) = (k.to_owned(), v.to_owned());
ENV.as_ref().unwrap().lock().unwrap().insert(k, v);
}
pub unsafe fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
let (k, v) = (k.to_owned(), v.to_owned());
ENV.as_ref().unwrap().lock().unwrap().insert(k, v);
Ok(())
}

pub fn unsetenv(k: &OsStr) -> io::Result<()> {
unsafe {
ENV.as_ref().unwrap().lock().unwrap().remove(k);
}
pub unsafe fn unsetenv(k: &OsStr) -> io::Result<()> {
ENV.as_ref().unwrap().lock().unwrap().remove(k);
Ok(())
}

Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/pal/sgx/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,13 +157,13 @@ 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<()> {
pub unsafe fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
let (k, v) = (k.to_owned(), v.to_owned());
create_env_store().lock().unwrap().insert(k, v);
Ok(())
}

pub fn unsetenv(k: &OsStr) -> io::Result<()> {
pub unsafe fn unsetenv(k: &OsStr) -> io::Result<()> {
if let Some(env) = get_env_store() {
env.lock().unwrap().remove(k);
}
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/pal/solid/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ pub fn getenv(k: &OsStr) -> Option<OsString> {
.flatten()
}

pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
pub unsafe fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
run_with_cstr(k.as_bytes(), &|k| {
run_with_cstr(v.as_bytes(), &|v| {
let _guard = ENV_LOCK.write();
Expand All @@ -200,7 +200,7 @@ pub fn setenv(k: &OsStr, v: &OsStr) -> io::Result<()> {
})
}

pub fn unsetenv(n: &OsStr) -> io::Result<()> {
pub unsafe fn unsetenv(n: &OsStr) -> io::Result<()> {
run_with_cstr(n.as_bytes(), &|nbuf| {
let _guard = ENV_LOCK.write();
cvt_env(unsafe { libc::unsetenv(nbuf.as_ptr()) }).map(drop)
Expand Down
4 changes: 2 additions & 2 deletions library/std/src/sys/pal/teeos/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -109,11 +109,11 @@ pub fn getenv(_: &OsStr) -> Option<OsString> {
None
}

pub fn setenv(_: &OsStr, _: &OsStr) -> io::Result<()> {
pub unsafe fn setenv(_: &OsStr, _: &OsStr) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::Unsupported, "cannot set env vars on this platform"))
}

pub fn unsetenv(_: &OsStr) -> io::Result<()> {
pub unsafe fn unsetenv(_: &OsStr) -> io::Result<()> {
Err(io::Error::new(io::ErrorKind::Unsupported, "cannot unset env vars on this platform"))
}

Expand Down
Loading
Loading