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?

[spastorino]

Assigning P-medium as discussed as part of the Prioritization Working Group procedure and removing I-prioritize.

[jhpratt]

Just repeating what I've said on Zulip: To resolve the soundness bug in time & chrono, one option would be to provide a std::env::lock method, which holds a lock on the environment until dropped. It's my understanding this is basically what is happening internally on some functions already in stdlib. Exposing this would allow much safer usage of FFI.

[RalfJung]

The env lock has to be exposed somehow, I agree.

An alternative API would be a function that takes a closure that will be called with the lock held. This is less powerful (which might be a good thing: provide the least powerful API that is good enough).

[RalfJung]

However, talking of the env lock -- also see #64718 for how the current use of the env lock is wrong, so exposing it in that state might be a problem.

[jhpratt]

Sure, that would work as well. End result is the same, if done properly.

[DemiMarie]

Honestly, I don’t believe std::env::set_var can be made safe.

In a pure-Rust program, it is already safe, but in C, setenv is considered thread-unsafe. Therefore, C libraries assume that they can call getenv at any time. Furthermore, adding a call to getenv is probably not considered worthy of a major version bump in the C library, so what was previously a safe call into that library can become unsafe without warning!

Even without the thread safety problems, there are other reasons that std::env::set_var is unsafe. Libraries such as gtk will load plugins from a path specified in an environment variable, and loading a shared library is inherently unsafe. If the shared library is in the system shared library directory, it can be assumed safe, since the OS is considered trusted. But it isn’t safe to (say) load a library from the Downloads folder (which could contain anything).

What might be safe is a function that fails if multiple threads are running, and which only allows setting a limited number of env vars, such as the timezone.

[fweimer-rh]

I've posted a patch that should fix this for glibc:

• [PATCH] stdlib: Make getenv thread-safe in more cases

Reviews would be appreciated. It does not solve the conceptual problem that the environment is a process-global resource. Other threads still observe temporary environment changes intended for the current thread only. Just the crashes and missed variable lookups are gone.

[briansmith]

I've posted a patch that should fix this for glibc:

Just to clarify, your patch doesn't fix it "enough" for Rust to consider them to set_var/remove_var to be safe, because C code is still allowed to read directly from environ and do other things that aren't protected by your improvements. That doesn't mean your improvement isn't welcome though.

[richfelker]

I don't see how this provides any significant thread-safety. The core unsafety of modifying the environment is that the lifetime of a string returned by getenv can end asynchronously if another thread changes the environment. The only way to avoid this is to leak everything ever added.

[tbu-]

Another way is for getenv to put the result into a thread-local variable. This is compatible with POSIX's guarantees, but probably not in line with glibc's backward compatibility guarantees.

[RalfJung]

From the patch:

I think this achieves full thread safety and async-signal-safety for
getenv if the environment is only updated using setenv, unsetenv,
clearenv. If putenv is used, it depends on how the passed string is
updated afterwards. Likewise for direct environ writes. This is fairly
easy to accomplish because the current implementation never frees any
variables set using setenv.

So, seems to me like setenv is already leaking, and this patch builds on that.

Very nice to see progress on this!

[VorpalBlade]

Even if that patch is accepted in glibc, POSIX doesn't guarantee thread safety, so this cannot change the decision for Rust (unless you get a change into musl, all the *BSDs, OpenSolaris, Darwin/Mac OS X etc, as well as the POSIX standard itself). And I don't foresee that happening due to backward compatibility around environ as well as the issue mentioned with putenv.

Still, it is a step in the right direction.

[tmccombs]

Well, if it weren't for direct access to environ and the putenv issue, there could possibly be a safe function that is only available on platforms that use a new enough version of glibc.

[tbu-]

Even if that patch is accepted in glibc, POSIX doesn't guarantee thread safety, so this cannot change the decision for Rust (unless you get a change into musl, all the *BSDs, OpenSolaris, Darwin/Mac OS X etc, as well as the POSIX standard itself).

Note that Rust doesn't strictly adhere to POSIX, Rust is fine with stuff that all the platforms implement. E.g. pthread_t is assumed to be an integer even though POSIX explicitly says that it might not be.

I haven't looked into whether musl, *BSDs or Darwin implement synchronization for environment variables. I just wanted to say that POSIX itself might not be a requirement.

[tmccombs]

What if another thread reads environ directly? Would that still be undefined behavior unless setenv uses an atomic operation to swap the pointer, and the reader uses an atomic load to read environ?

[fweimer-rh]

What if another thread reads environ directly? Would that still be undefined behavior unless setenv uses an atomic operation to swap the pointer, and the reader uses an atomic load to read environ?

@tmccombs Is this a question about the proposed new glibc implementation? It's not possible to ensure a proper snapshot with just atomic access to environ. Even if individual accesses never fault, it's impossible to know whether you have actually observed all currently active environment variables. Concurrent unsetenv calls may have shuffled around the array while you were reading it. I assume that this question is related to the implementation of std::env:vars(). Realistically, this will a new interface. For glibc's internal use in execve, posix_spawn etc., we can add something that treats environ differently so that we can obtain a snapshot similar to getenv, but exposing that externally as-is would constraint the implementation too much.

[tmccombs]

My point is that even if a c library reads from environ directly, then no amount of synchronization around setenv in rust would be safe in a multi-threaded program.