set_var/remove_var are unsound in combination with C code that reads the environment

[bluss]

Like the documentation for our set_var (setenv) says, care must be taken with mutating environment variables in a multithreaded program. See this glibc bug #13271 that says getaddrinfo may call getenv.

It looks like we have an unsynchronized call to getaddrinfo and this may cause trouble with glibc/Linux.

Seeing glibc's attitude to setenv in multithreaded programs, set_var seems like a big hazard in general(?).

Discovered as an issue tangential to #27966

cc @alexcrichton

[bluss]

cc @luqmana

[alexcrichton]

If we did take this route I'd want to convert the environment lock to a rwlock to ensure that we could at least have parallel dns queries. Also cc #27705.

[bluss]

Yeah, either way the situation is tricky. We get tangled up in glibc internals.

[alexcrichton]

I found the wording on this page also particularly interesting:

       env    Functions marked with env as an MT-Safety issue access the
              environment with getenv(3) or similar, without any guards to
              ensure safety in the presence of concurrent modifications.

              We do not mark these functions as MT-Unsafe, however, because
              functions that modify the environment are all marked with
              const:env and regarded as unsafe.  Being unsafe, the latter
              are not to be called when multiple threads are running or
              asynchronous signals are enabled, and so the environment can
              be considered effectively constant in these contexts, which
              makes the former safe.

The getaddrinfo function is indeed marked with this tag

[fweimer]

std::env::set_var is just not safe to use in a multi-threaded program, as I commented on issue #24741:

Even without low-level races, std::env::set_var is not quite safe because you can change a variable (such as TZ), do something, and change it back to the original value, without impacting something else in the process which runs at the same time.

This problem would not go away if we provided thread-safe environment access at the libc layer. Therefore, I'm surprised the function isn't marked unsafe.

[steveklabnik]

Triage: as far as I know, this hasn't changed at all. The docs do describe this possible footgun.

[smmalis37]

This issue can also apply to any external crate that calls into something that calls get_env, for example time-rs/time#293 and chronotope/chrono#499. Maybe some way for these crates to observe the lock is called for?

[RalfJung]

Seems to me like this is a soundness issue, and should be marked I-unsound?