rustc's munging of PATH environment is racy when run in parallel

[brson]

Mentioned in #17327, there are two places where rustc sets and resets PATH. Doing this means that running multiple compilations in parallel will stomp on each others' environment variables. This can happen, e.g. in rustdoc which runs tests by driving rustc directly. The proper way to set the environment for subprocesses is through the process Command type.

cc @vadimcn

[vadimcn]

Yes, we could do that for subprocesses, but what about syntax extension dylibs? I guess this depends on why we are munging PATH/DYLD_LIBRARY_PATH/LD_LIBRARY_PATH:

• Is this because we want the OS to find the extension dylib itself? If so, we could just search along the augmented path ourselves, then load by absolute path.
• Or, is this because we need the OS to be able to find other dylibs that the extension dylib depends on? In this case, I don't see another way but to munge the PATH envvar. I don't know about Linux, but Windows does not seem to have a concept of a per-thread dll search path...

@pnkfelix, looks like you wrote the relevant bit of code in dynamic_lib.rs. Do you remember which of the two it was?

[vadimcn]

For the case 2, I suppose we could add DynamicLibrary::open_with_search_path(String, [Path]) with an internal mutex, that would temporarily alter PATH before delegating to open(). Of course, this would still race with direct calls to os::setenv("PATH", ...), but better than nothing.
In fact, because os::setenv essentially allows to mutate global variables, perhaps it should be marked as unsafe?

[pnkfelix]

I thought it was case 1.

Interesting idea, marking setenv as unsafe... though note that the rules about when code needs to be annotated with unsafe is really meant to ensure data race freedom in safe code, not arbitrary race freedom

[steveklabnik]

Triage: In #17327, @alexcrichton mentions this block of code:

rust/src/librustc_driver/driver.rs

Lines 516 to 522 in 1661947

	if cfg!(windows) {
	_old_path = env::var_os("PATH").unwrap_or(_old_path);
	let mut new_path = sess.host_filesearch(PathKind::All)
	.get_dylib_search_paths();
	new_path.extend(env::split_paths(&_old_path));
	env::set_var("PATH", &env::join_paths(new_path).unwrap());
	}

It looks like this code still doesn't prevent adding to the path over and over again.

[alexcrichton]

I believe we've been seeing this a lot recently as #33844, which is spurious test failures from something being too big, and it's probably environment variables (#17327 coming back to haunt us)