`std::env::{set_var, remove_var}` is called in tests without safety documentation

[tbu-]

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

	assert_eq!(get_dbpath_for_term(""), None);
	unsafe {
	env::set_var("TERMINFO_DIRS", ":");
	}
	assert_eq!(x("screen"), PathBuf::from("/usr/share/terminfo/s/screen"));
	unsafe {
	env::remove_var("TERMINFO_DIRS");
	}

rust/library/std/src/process/tests.rs

Lines 326 to 332 in 8c32e31

	unsafe {
	env::set_var("RUN_TEST_NEW_ENV2", "456");
	}
	let result = cmd.output().unwrap();
	unsafe {
	env::remove_var("RUN_TEST_NEW_ENV2");
	}

rust/library/std/src/env.rs

Line 571 in ef20a1b

/// unsafe { env::set_var("PATH", &new_path); }

The last example is even in user-facing documentation.

[ais523]

@rustbot label +A-testsuite

To me, these look a) unsound but b) difficult to fix:

• The first example is a visibility problem: the function that's being tested uses environment variables from the current process in order to find information, whereas the test wants to set them to known values for testing purposes. Doing this is unsound if a library that the test runner depends on starts a background thread that reads environment variables, and is written in a language other than Rust – and there's no obvious way to verify that that isn't the case.
• What the second example is trying to do can, in most cases, be cleanly and safely written using Command::env instead. The problem is that this is one of the tests of Command::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.
• The third example appears to be a program fragment, rather than a complete program: it sets an environment variable but doesn't do anything with it (the program just exits immediately afterwards). Programs using the technique shown in that fragment should probably be using Command::env instead, but not enough of the code is shown to make the replacement possible (and a complete example using Command::env would 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-]

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]

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]

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.