Tracking Issue for Rust 2024: Make std::env::{set_var, remove_var} unsafe

[traviscross]

This is a tracking issue for making std::env::{set_var, remove_var} unsafe to call in Rust 2024.

• Add discussion that concurrent access to the environment is unsafe #116888 (comment)

About tracking issues

Tracking issues are used to record the overall progress of implementation. They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions. A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature. Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Implement in nightly Rust 2024 for std::env::{set_var, remove_var}.
  • Make std::env::{set_var, remove_var} unsafe in edition 2024 #124636
• Implement migration lints for rustc_deprecated_safe_2024.
  • Make std::env::{set_var, remove_var} unsafe in edition 2024 #124636
  • Rename deprecated_safe lint to deprecated_safe_2024 #125990
• Implement in nightly Rust 2024 for CommandExt::before_exec.
  • CommandExt::before_exec: deprecate safety in edition 2024 #125970
• N/A: Add documentation to the dev guide.
  • See the instructions.
• N/A: Add documentation to the reference.
  • See the instructions.
• Add documentation to the edition guide.
  • Add 2024 unsafe functions edition-guide#304
• Ensure ready for Rust 2024 stabilization.

Unresolved Questions

TODO.

Related

• Make std::env::{set_var, remove_var} unsafe in edition 2024 #124636
• set_var/remove_var are unsound in combination with C code that reads the environment #27970
• Consider deprecating and/or modifying behavior of std::env::set_var #90308
• Don't trigger unsafe_op_in_unsafe_fn for deprecated safe fns #125925
• Remove lock guarantee from std::env::{set_var, remove_var} #125937
• CommandExt::before_exec: deprecate safety in edition 2024 #125970
• Rename deprecated_safe lint to deprecated_safe_2024 #125990
• Add FIXME comment to unsafe env modification #126019

Implementation history

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

cc @tbu @Amanieu @rust-lang/libs-api

[traviscross]

@rustbot assign @tbu

[joboet]

I'm really not a big fan of this, because unsafe is completely unnecessary on well-behaved platforms like Windows, which provide a good API with proper synchronization, and on the platforms where this is an issue, we could easily make the API sound by properly enforcing that setenv is only called on the main thread (our current behaviour is technically unspecified, see MT-safe env). (not correct) it adds a very subtle safety condition that is almost impossible to reliable satisfy (quite some C library function access the environment internally and you'd have to guarantee that none of them is currently running).

I think the best solution would be to contribute new API to glibc/musl... that behaves correctly (e.g. by copying the GetEnvironmentVariable behaviour of copying into a user-provided buffer).

[BurntSushi]

@joboet The nooks and crannies of this issue have been discussed at length already. Your suggestion to "add a new API" to glibc/musl has been brought up before and it isn't viable. I would suggest at least skimming through that thread, and pay special attention to richfelker's comments if you think the solution to this problem can come from libc. (He is the maintainer of musl.)

I'm really not a big fan of this, because unsafe is completely unnecessary on well-behaved platforms like Windows, which provide a good API with proper synchronization

I don't think anyone is a big fan of this. The fact that Windows has a safe API for this isn't sufficient criteria for marking a platform independent API safe that is unsafe on some set of supported tier1 platforms. The answer here is to probably to publish a crate that exposes the safe Windows specific API.

[RalfJung]

I think the best solution would be to contribute new API to glibc/musl... that behaves correctly (e.g. by copying the GetEnvironmentVariable behaviour of copying into a user-provided buffer).

I agree. I hope someone will pursue this.

However, there was, as far as I can tell, basically zero progress on this in the almost 9 years (!) since #27970 was filed. I see no reason to believe that a fix for this will materialize medium-term (let's say, ≤ 5 years). Even on a 10-year scale I am not optimistic that anything will happen. Saying we have to wait until Linux and macOS get their APIs fixed is, at this point, basically equivalent to saying we're okay with this just not being fixed, and std being perpetually unsound when programs combine Rust code and C code.

Given that the best solution is just unrealistic, we should pursue the second-best solution, and that's what we are doing right now. I'm very happy to finally see progress on this.

it adds a very subtle safety condition that is almost impossible to reliable satisfy (quite some C library function access the environment internally and you'd have to guarantee that none of them is currently running).

The safety constraint is basically, don't mutate the environment if another thread may exist. That's the only way we can be sure there's no concurrently running C code. This is sad, but two of our Tier 1 OSes do not support anything better than that, and we can either accept or deny this fact but it remains a fact.

This hopefully pushes the Rust ecosystem towards APIs that avoid the need for mutating the environment.

[tbu-]

@rustbot claim

[ehuss]

It's a bit unclear to me why the migration lint was named deprecated_safe, since it is specific to the 2024 edition. @tbu- Would it be possible to rename it to something more specific for its purpose? Perhaps something like deprecated_safe_2024 to mirror keyword_idents_2024¹?

Footnotes

1. Unfortunately it looks like there is inconsistency in the naming conventions, with most lints using rust-2021-xxxx but keyword idents split into keyword-idents-20xx. I'm guessing deprecated_safe might follow in similar footsteps. 🤷 ↩

[SteveLauC]

Hi, is this considered a breaking change? The patch for this is available in Rust nightly 1.80 2024-05-31, and Nix encountered this even though we are using Rust 2021.

[ehuss]

@SteveLauC This should only fire in 2024. If you are encountering a problem, please open a new issue with a reproduction.

[SteveLauC]

@SteveLauC This should only fire in 2024. If you are encountering a problem, please open a new issue with a reproduction.

Hi, thanks for your reply! issue filed.

[tbu-]

Would it be possible to rename it to something more specific for its purpose? Perhaps something like deprecated_safe_2024 to mirror keyword_idents_2024?

Of course it's possible to rename the lint, if we want to do that.

It's a bit unclear to me why the migration lint was named deprecated_safe, since it is specific to the 2024 edition.

My thinking was that the deprecated_safe mechanism might be properly extended to more editions or third-party crates in the future and didn't want to give it a too specific name. I guess a deprecated_safe lint group could be added containing only the deprecated_safe_2024 lint, but I was thinking that this could also be done in the future, when we split it into multiple different lints.

[ehuss]

Yea, I think it would be best. The lint can't change after the edition is released, and it helps with documentation to make it clearer it is for the edition.

[tbu-]

Only addressing this particular sub-point:

The lint can't change after the edition is released

Why can the lint not change after the edition? It seems to be that it could be renamed to deprecated_safe_2024 if a lint group deprecated_safe containing it is created at the same time.

[ehuss]

The behavior of the lint can't change. It could be renamed, but that causes hassle with updating documentation, bug reports and older documentation no longer being accurate, etc.

[tbu-]

Created a PR: #125990.

The updated documentation, bug reports, older documentation make sense.

Why can the behavior of a lint not change? I didn't think this was part of our stability guarantees.

[workingjubilee]

Why can the behavior of a lint not change? I didn't think this was part of our stability guarantees.

It is explicitly not.

[workingjubilee]

RFC 1122 Underspecified language semantics:

There are a number of areas where the precise language semantics are currently somewhat underspecified. Over time, we expect to be fully defining the semantics of all of these areas. This may cause some existing code – and in particular existing unsafe code – to break or become invalid. Changes of this nature should be treated as soundness changes, meaning that we should attempt to mitigate the impact and ease the transition wherever possible.

Known areas where change is expected include the following:

• [...]
• Lints will evolve over time (both the lints that are enabled and the precise cases that lints catch). We expect to introduce a means to limit the effect of these changes on dependencies.

For those who don't have time to reread the entire RFC, all soundness changes are considered permitted breaking changes, especially including

• Cases where the user is able to produce Undefined Behavior (UB) purely from safe code.

It is a permitted breaking change to simply delete these functions. It is recommended by the same RFC to introduce mitigations so that the breakage is minimized. This change is thus effectively one of the recommended mitigation strategies. As it is a lint, its behavior itself is allowed to evolve going forward.

[ehuss]

Why can the behavior of a lint not change?

In this particular case, it's because the set of unsafe functions that need to be migrated for 2024 will be fixed once 2024 is stabilized. The lint machinery itself does not support a single lint spanning multiple editions (and that is the reason why keyword_idents was split up), so deprecated_safe would not be able to add new functions that would otherwise hard-error, even for 2027.

As for @workingjubilee's context about fixing soundness, there is some latitude to make fixes regarding soundness, but in this case it was decided to fix it in a non-breaking manner. (Unless I'm misunderstanding the point being drawn?)

[ehuss]

Posted a draft of the docs at rust-lang/edition-guide#304 if anyone wants to help review that.