Repository navigation
std::env::{set_var, remove_var} is called in tests without safety documentation #148432
Description
Introduced by @ehuss in #136983 (36733f3, ef20a1b), apparently without double-checking whether these functions were actually safe to call.
Found when trying to audit whether this repository's calls to these unsafe environment variable functions were safe. It doesn't appear to be the case to me, e.g.
rust/library/test/src/term/terminfo/searcher/tests.rs
Lines 13 to 20 in f2bae99
rust/library/std/src/process/tests.rs
Lines 326 to 332 in 8c32e31
Line 571 in ef20a1b
The last example is even in user-facing documentation.
Activity
ais523 commentedon Nov 3, 2025
@rustbot label +A-testsuite
To me, these look a) unsound but b) difficult to fix:
Command::envinstead. The problem is that this is one of the tests ofCommand::env, and going via the method it's testing would cause it to not properly test the case it's trying to test. This is unsound in the same circumstances where the first example would be.Command::envinstead, but not enough of the code is shown to make the replacement possible (and a complete example usingCommand::envwould be somewhat longer).In the first two examples, the technically correct fix looks like it would be to request the test runner to set particular environment variables when running the test, but that would be a somewhat major new feature. Alternatively, the tests could be rewritten to spawn a separate test process themselves, with the required environment variable already set, and test there. (I guess a way to summarise this is "these tests need a specific environment to work, but we can't soundly change the environment that the test runner starts the program with, so we are going to need to either mock the environment somehow or eles teach the the test runner how to set it".)
Perhaps the second example could be fixed by performing the test using one of the environment variables that Cargo's test runner sets unconditionally? It would be a little hacky, but would avoid an unsound environment change as the variable would already be present at the time of testing.
tbu- commentedon Nov 3, 2025
If parts are known to be unsound but accepted, I'd expect a comment explaining that. Would you be interested in a PR documenting this unsoundness?
ais523 commentedon Nov 3, 2025
I'm the wrong person to ask – I haven't interacted with the code in question at all and am not responsible for it, I was just trying to add additional details to the bug report. (In particular I don't know whether this is an intentional unsoundness or not.)
ChrisDenton commentedon Nov 3, 2025
The first two could just be standalone integration tests. We already have a few of those for tests that must be run in their own process (e.g.: https://github.com/rust-lang/rust/blob/master/library/std/tests/create_dir_all_bare.rs).
The example either needs to explain why it's safe (as started) or else be rewritten to not set the environment. I don't think the example particularly needs to do that so I think explaining it is a bit of a distraction.