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

std::env::remove_var() becomes unsafe even in Rust 2021 #125875

Closed
SteveLauC opened this issue Jun 2, 2024 · 13 comments · Fixed by #125925
Closed

std::env::remove_var() becomes unsafe even in Rust 2021 #125875

SteveLauC opened this issue Jun 2, 2024 · 13 comments · Fixed by #125925
Assignees
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@SteveLauC
Copy link
Contributor

SteveLauC commented Jun 2, 2024

I tried this code:

#![deny(unsafe_op_in_unsafe_fn)]

fn main() {
    unsafe { foo() };
}

unsafe fn foo() {
    std::env::remove_var("foo");
}
[package]
name = "remove_var_2021_reproduction"
version = "0.1.0"
edition = "2021"

[dependencies]

I expected to see this happen:

std::env::remove_var() will become unsafe in Rust 2024, the above code is using Rust 2021, so this should compile with no issue.

Instead, this happened:

$ cargo +nightly b
   Compiling remove_var_2021_reproduction v0.1.0 (remove_var_2021_reproduction)
error[E0133]: call to unsafe function `std::env::remove_var` is unsafe and requires unsafe block
 --> src/main.rs:8:5
  |
8 |     std::env::remove_var("foo");
  |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
  |
  = note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
  = note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
 --> src/main.rs:7:1
  |
7 | unsafe fn foo() {
  | ^^^^^^^^^^^^^^^
note: the lint level is defined here
 --> src/main.rs:1:9
  |
1 | #![deny(unsafe_op_in_unsafe_fn)]
  |         ^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0133`.
error: could not compile `remove_var_2021_reproduction` (bin "remove_var_2021_reproduction") due to 1 previous error

Meta

rustc --version --verbose:

$ rustc +nightly --version --verbose
rustc 1.80.0-nightly (ada5e2c7b 2024-05-31)
binary: rustc
commit-hash: ada5e2c7b5427a591e30baeeee2698a5eb6db0bd
commit-date: 2024-05-31
host: x86_64-unknown-linux-gnu
release: 1.80.0-nightly
LLVM version: 18.1.6
Backtrace

Not needed

Possible root cause

I think the root cause is the lint unsafe_op_in_unsafe_fn, commenting it out would make the code compile:

$ rg unsafe_op src/main.rs
1:// #![deny(unsafe_op_in_unsafe_fn)]

$ cargo +nightly b
   Compiling remove_var_2021_reproduction v0.1.0 (remove_var_2021_reproduction)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s
@SteveLauC SteveLauC added the C-bug Category: This is a bug. label Jun 2, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Jun 2, 2024
@RalfJung
Copy link
Member

RalfJung commented Jun 2, 2024

Thanks for the report! That should not happen. We did a crater run so I don't know how this is even possible...

@tbu- any ideas? This seems to be some bad interaction with unsafe_op_in_unsafe_fn.

@traviscross traviscross added L-unsafe_op_in_unsafe_fn Lint: unsafe_op_in_unsafe_fn and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Jun 2, 2024
@Noratrieb Noratrieb added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed L-unsafe_op_in_unsafe_fn Lint: unsafe_op_in_unsafe_fn labels Jun 2, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Jun 2, 2024
@tbu-
Copy link
Contributor

tbu- commented Jun 2, 2024

@rustbot claim

Hmmm, weird that crater run didn't catch this.

@tbu-
Copy link
Contributor

tbu- commented Jun 3, 2024

I posted a PR fixing this: #125925.

We did a crater run so I don't know how this is even possible...

Does crater maybe restrict lints?

tbu- added a commit to tbu-/rust that referenced this issue Jun 3, 2024
@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2024 via email

@SteveLauC
Copy link
Contributor Author

Is this code part of the "nix" crate?

If you are asking if this problem originates from the nix crate, then yes, see nix-rust/nix#2421 (comment)

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2024

I am wondering whether the nix crate as published on crates.io fails to build with latest nightly due to this issue.

@SteveLauC
Copy link
Contributor Author

I am wondering whether the nix crate as published on crates.io fails to build with latest nightly due to this issue.

I think yes, the related code has been in Nix for a long time.

One should be able to reproduce the build failure on a UNIX platform other than:

if #[cfg(any(linux_android,
                     target_os = "fuchsia",
                     target_os = "wasi",
                     target_env = "uclibc",
                     target_os = "emscripten"))] {

I am currently on Linux, will try reproducing it on my mac tomorrow.

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2024

Looks like crater tested nix 0.26.4 and that failed already before the remove_var/set_var change (logfile here). That's due to the deny(warnings) which makes the build fail even when there are just warnings. I guess we can add this to the list of why having deny(warnings) in the code is considered bad practice.

@tbu-
Copy link
Contributor

tbu- commented Jun 3, 2024

I created an issue and a PR against nix-rust.

@SteveLauC
Copy link
Contributor Author

I am currently on Linux, will try reproducing it on my mac tomorrow.

Yeah, we can reproduce this on macOS

$ uname -rs
Darwin 22.6.0

$ cargo +nightly --version
cargo 1.80.0-nightly (7a6fad098 2024-05-31)

$ cargo +nightly b --all-features -q
error[E0133]: call to unsafe function `std::env::remove_var` is unsafe and requires unsafe block
  --> src/env.rs:52:17
   |
52 |                 env::remove_var(name);
   |                 ^^^^^^^^^^^^^^^^^^^^^ call to unsafe function
   |
   = note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
   = note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
  --> src/env.rs:41:1
   |
41 | pub unsafe fn clearenv() -> std::result::Result<(), ClearEnvError> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: the lint level is defined here
  --> src/lib.rs:94:9
   |
94 | #![deny(unsafe_op_in_unsafe_fn)]
   |         ^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0133`.
error: could not compile `nix` (lib) due to 1 previous error

@RalfJung
Copy link
Member

RalfJung commented Jun 4, 2024

The reason this was missed is a combination of nix-rust/nix#2423 and rust-lang/crater#727.

@apiraino
Copy link
Contributor

apiraino commented Jun 5, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-high

@rustbot rustbot added P-high High priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jun 5, 2024
@tbu-
Copy link
Contributor

tbu- commented Jun 5, 2024

WG-prioritization assigning priority

Fix already exists, just needs a review: #125925.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 5, 2024
…safe_fn, r=Nadrieril

Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns

Fixes rust-lang#125875.

Tracking:

- rust-lang#124866
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Jun 6, 2024
…safe_fn, r=Nadrieril

Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns

Fixes rust-lang#125875.

Tracking:

- rust-lang#124866
@bors bors closed this as completed in bb901a1 Jun 6, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Jun 6, 2024
Rollup merge of rust-lang#125925 - tbu-:pr_unsafe_env_unsafe_op_in_unsafe_fn, r=Nadrieril

Don't trigger `unsafe_op_in_unsafe_fn` for deprecated safe fns

Fixes rust-lang#125875.

Tracking:

- rust-lang#124866
lcnr pushed a commit to lcnr/rust that referenced this issue Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. P-high High priority regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants