Repository navigation
Consider deprecating and/or modifying behavior of std::env::set_var #90308
Description
Activity
Shall it be made
unsafe fn, just likestd::os::unix::process::CommandExt::pre_exec?Indeed. Notably they retroactively changed
pre_exectounsafebased on a justification that there weren't real-world uses of it.That's not the case with
set_var: https://github.com/search?l=Rust&type=Code&q=set_varI think to accomplish a similar effect,
set_varwould have to be deprecated, and a newunsafe fnintroduced in its place (which is more or less combining options 1 & 4 proposed in the OP)I think to accomplish a similar effect, set_var would have to be deprecated, and a new unsafe fn introduced in its place (which is more or less combining options 1 & 4 proposed in the OP)
That's what happened with
pre_exec.before_execis the old name.Reacted by Tony Arcieri, Vitaly Shukela, Gabriel Araújo, Sunny Huang and Harry WuHi, there's a bunch of discussion on this issue, but I think https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475 has the most information right now.
Reacted by Petros Angelatos and Harry WuHi, there's a bunch of discussion on this issue, but I think https://internals.rust-lang.org/t/synchronized-ffi-access-to-posix-environment-variable-functions/15475 has the most information right now.
There is a lot of discussion in #27970 that's really about this issue. This is largely my fault because I didn't know this issue existed.
I looked at an application that is using
set_varin multiple places:To provide a default value forRUST_LOG=debugwhen it wasn't already set. We'd prefer a way to do this without setting the environment variable.- To force
RUST_BACKTRACE=fullregardless of what the environment says. Again, we'd prefer a way to do this without setting an environment variable. - To test functions that read from the environment. At some point we should have a new solution for supporting this.
To do shell scripting, but in Rust. Not a big deal for us to use an[Edit: We can useunsafefunction for this, though not ideal.Command::{env,envs}.]
- To provide a default value for
RUST_LOG=debugwhen it wasn't already set. We'd prefer a way to do this without setting the environment variable.
https://docs.rs/env_logger/latest/env_logger/struct.Env.html#method.filter_or
- To force
RUST_BACKTRACE=fullregardless of what the environment says. Again, we'd prefer a way to do this without setting an environment variable.
EDIT: Actually, this only affects the library backtrace, the panic runtime probably does also need a in-code setter too.
Reacted by Brian Smith and Jon Gjengset- To provide a default value for
- To test functions that read from the environment. At some point we shouldhave a new solution for supporting this. I don't think there's a reasonable way to support this other than running that test in its own process. That's already prone to logic race conditions since tests are multithreaded. At the very least, in an integration test it should be pretty much obviously safe to put the relevant `set_env` calls at the top of `main`.…On Tue, Jan 4, 2022, 4:55 PM Brian Smith ***@***.***> wrote: I looked at an application that is using set_var in multiple places: - To provide a default value for RUST_LOG=debug when it wasn't already set. We'd prefer a way to do this without setting the environment variable. - To force RUST_BACKTRACE=full regardless of what the environment says. Again, we'd prefer a way to do this without setting an environment variable. - To test functions that read from the environment. At some point we should have a new solution for supporting this. - To do shell scripting, but in Rust. Not a big deal for us to use an unsafe function for this, though not ideal. — Reply to this email directly, view it on GitHub <#90308 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB7X32OITRPXKVTYYIVWD7LUUNUGHANCNFSM5GYFKLHA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>
First, it turns out that our use of
RUST_LOGwas a bit different than I described. I agree current APIs are sufficient, at least for the logging crates we use.As far as testing goes, if the
serial_testcrate truly ensures no other threads are doing anything during the test, then that might work for our test uses ofset_varandremove_var.In order to use
serial_testto makeset_varandremove_varin a test safe, the tests would have to be integration tests and where there are no other tests in the same file (really, resultant binary), so that all the tests in the binary are marked#[serial], because#[serial]from theserial_testcrate only serializes#[serial]tests; it doesn't prevent non-#[serial]tests from executing concurrently with#[serial]tests. Further, all of those#[serial]tests would need to avoid spawning threads (that outlive the test). So it'd be pretty error-prone.To provide a default value for RUST_LOG=debug when it wasn't already set. We'd prefer a way to do this without setting the environment variable.
Miri needs something similar here, and I think we somehow have two loggers here in this process so I don't know if we can use the env_logger API as a replacement.
Further, all of those #[serial] tests would need to avoid spawning threads (that outlive the test).
And even inside such a serial test, any use of e.g. tokio or rayon could spawn threads in a not immediately obvious way. So I wouldn't bet on this approach as a good mitigation.
35 remaining items
- added a commit that references this issue
on Nov 16, 2023 - added a commit that references this issue
on Apr 7, 2024 - added a commit that references this issue
on Apr 27, 2024 - added 2 commits that reference this issue
on May 2, 2024 - added a commit that references this issue
on May 4, 2024 - added 2 commits that reference this issue
on May 13, 2024
Observing an environment variable concurrently with a call to
setenvconstitutes a data race. Rust currently handles this by using a mutex, but that only protects against functions instd::env. My interpretation of POSIX (which appears to be the same as the glibc developers') is that any libc function is allowed to callgetenv. This has already caused problems withgetaddrinfoin #27970.Additionally, a large amount of C code calls
getenvbut is otherwise thread-safe. While this isn't necessarily the standard library's issue, it's impossible for third-party libraries to soundly use this code without requiring the user to certify that this isn't an issue via anunsafeblock. Some examples of this happening in practice are in time-rs/time#293 and rust-lang/flate2-rs#272.rustsec/advisory-db#926 had several proposals on how this could be handled brought up.
Make
std::env::set_varunsafeThis is arguably the cleanest solution to the issue. This could be considered a soundness fix, which would make it an option, but the ecosystem impact feels like it'd be too big.
Don't actually call
setenvstdcould keep track of the environment itself, and makeset_varchanges only visible tovarand subprocesses. This is probably the way to solve the issue with the least impact on existing code, but the behavior is somewhat unexpected and not zero-cost.Only call
setenvin single-threaded codeThis would reduce the impact further, but seems even less expected for negligible benefit.
Deprecate
std::env::set_varThis would make it clear that setting an environment variable in the current process is discouraged. It could also be combined with not actually calling
setenv, which would be my preferred solution to this issue.