-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Remove lock guarantee from std::env::{set_var, remove_var}
#125937
Conversation
I didn't want to stall #124636 over this, so I moved it to a new pull request.
I believe this lock guarantee does not give us anything on environments where accessing environment variables is unsafe, such as on Linux with glibc. Adding such a lock would be useful in a Rust-only target where libc is not linked. On Linux with glibc, I think that this comment at most pushes people to think that their multi-threaded use case of I'm interested in hearing about a sound use case of this lock. |
It avoids UB in some cases, like safe Rust code where one So to me the question is -- is this a first step towards eventually actually removing the lock from the implementation?Because that is something I do not think should happen any time soon. With this PR the affected code "just" has library UB but is actually still well-defined in the op.sem, but once we remove the lock that would become language UB. |
I also do not want that to happen anytime soon, but maybe in 10 years or so, when environment variable modifications have largely been removed from Rust.
I'm unsure there's code out there that can somehow guarantee that no C function is called anywhere in the tests, but I can see the comment in the documentation leading people to believe that they can guarantee it. |
I don't think we should make this change. I think even decades from now we're not going to remove this synchronization (except on platforms where environment manipulation is already thread-safe). And given that, I think we should document the synchronization we do, together with the detailed set of caveats we already document. |
Can you explain a sound use case of this lock? I can't think of any Rust application that could use it. If we don't have any sound use case of the lock, why do we document it, even if it makes people think their use case is covered when it is not? Or is our reasoning that even though we cannot imagine anyone using this lock in a sound way, it should still be documented? |
In practice, open source libcs do not update environment variables implicitly and are fairly restrained in checking them. They... also have opinions about the use of While they may be dedicated to maintaining their API and assigning blame for its misuse to the callers of the library, I do not think treating them as pure adversary in this matter is correct. Just because they are, in some cases, averse to adopting nonstandard extensions does not mean they are excited about their use of environment variables and inclined to recklessly do so. I do not view documenting these locks as necessary, but neither would I want to remove them because the protection is "useless". |
The Rust environment lock is a false sense of security, and the documentation should definitely remove any mention of it, although I would be loathe to remove the lock in practice. Rust made a mistake here. It's great that we're fixing the mistakes, but attempting to hide the fact that we made a mistake by documenting the lock doesn't make sense. I consider this similar to men::uninit here, in that while the lang "spec" (consensus, etc) may require locks around these functions (similar to the 0x1 filling), documenting that fact will only exacerbate the pain. |
We discussed this in today's @rust-lang/libs-api meeting. We don't feel that this diff is making either our guarantees or the use of this function any clearer. The current documentation accurately describes the current code, and removing that documentation will not change any assumptions existing code has made. Thus, we don't think this documentation change makes any difference for any future code change we might make. This FCP close isn't making any decisions about any future code changes we might make regarding the lock. @rfcbot close |
Team member @joshtriplett has proposed to close this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot reviewed ... but did you mean this to be a libs-api FCP? |
(This doesn't really answer #125937 (comment), "can we find any way where existing code makes useful assumptions about the lock")
I don't really understand this. AFAIUI, the documentation is usually what guarantees how stuff works, i.e. in my understanding, this documentation does prevent us from changing how the lock works in the future. Anyway, would you accept a pull request that specifies this, saying that the lock isn't guaranteed (since apparently this documentation doesn't change whether you decide to make a code change or not)? |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
set_env: State the conclusion upfront People tend to skim or skip over long explanations so we should be very upfront that `set_var` and `remove_var` are being made unsafe for a very good reason. This is just the conclusion restated almost verbatim but earlier in the docs and separated from the explanation: https://github.com/rust-lang/rust/blob/0c960618b56f662d933e8b864cd9632a99174e87/library/std/src/env.rs#L338-L339 I think this may help with people who may not be entirely comfortable with rust-lang#125937 being rejected.
set_env: State the conclusion upfront People tend to skim or skip over long explanations so we should be very upfront that `set_var` and `remove_var` are being made unsafe for a very good reason. This is just the conclusion restated almost verbatim but earlier in the docs and separated from the explanation: https://github.com/rust-lang/rust/blob/0c960618b56f662d933e8b864cd9632a99174e87/library/std/src/env.rs#L338-L339 I think this may help with people who may not be entirely comfortable with rust-lang#125937 being rejected.
Rollup merge of rust-lang#126281 - ChrisDenton:env, r=jhpratt set_env: State the conclusion upfront People tend to skim or skip over long explanations so we should be very upfront that `set_var` and `remove_var` are being made unsafe for a very good reason. This is just the conclusion restated almost verbatim but earlier in the docs and separated from the explanation: https://github.com/rust-lang/rust/blob/0c960618b56f662d933e8b864cd9632a99174e87/library/std/src/env.rs#L338-L339 I think this may help with people who may not be entirely comfortable with rust-lang#125937 being rejected.
The final comment period, with a disposition to close, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. |
CC #124866.
CC #124636 (comment).