libstd: Add thread unsafety warnings around setenv() and unsetenv()

[cgwalters]

See:
https://sourceware.org/bugzilla/show_bug.cgi?id=4887#c9
https://bugs.freedesktop.org/show_bug.cgi?id=65681

I just noticed this while talking to someone who was using
os.environ['FOO'] = 'BAR' in Python and since I'm learning Rust, I
was curious if it did anything special here (and the answer appears to
be no).

Java got this right by disallowing setenv() from the start.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[cgwalters]

I read too quickly - Rust has its own mutex, but that doesn't help if there are other non-rust threads in the process. I'll update this PR.

[cgwalters]

Reworked to note that libstd does have an internal mutex.

[alexcrichton]

Thanks for the PR! I think this isn't quite the message we want to be sending, however, as "possibly unsafe" code is still unsafe and needs to be marked as such with unsafe, which these APIs cannot be. I think the wording may want to be toned down a bit in severity and simply mention that external users of getenv and setenv on Unix platforms are recommended to synchronize with Rust code to ensure that C doesn't corrupt the state that Rust sees.

It's possible that one day we will expose the environment variable lock to C to allow it to acquire it, but for now it's unlikely we'll commit to that so soon.

[cgwalters]

Hi @alexcrichton , thanks for the review! I'm excited to make a small contribution to Rust =) On to the topic:

It doesn't actually work to just wrap a mutex around getenv() because it returns a const char * - but that returned pointer can be invalidated by a subsequent setenv().

See https://sourceware.org/bugzilla/show_bug.cgi?id=15607 (which is linked from https://bugs.freedesktop.org/show_bug.cgi?id=65681 )

So every C library user including glibc itself (for things like gettext() would have to be changed to either use an allocating getenv() or hold an external mutex for the duration of their use of the pointers.

It's just not going to happen. Or even if it did happen today, we'd still have many years of people running old glibc/rust/other C libraries (such as glib, which is a project I contribute to and where we saw this bug in the real world).

I'm happy to reword, but I don't think we can recommend external synchronization for the above reasons.

[cgwalters]

Another option is to drop set_var and remove_var from libstd - anyone who wants to do this can drop to the OS-specific wrapper in e.g. libc::setenv. It'd obviously be a breaking change. But I suspect most uses of environment variable sets for the current process are OS specific anyways (e.g. things like LD_LIBRARY_PATH), the major exception probably being PATH.

[cgwalters]

BTW, it looks like rustc itself is a nontrivial Rust application calling env::set_var which links to a large external C/C++ codebase named LLVM that calls the C getenv(). Now from a quick look most of LLVM's invocations of getenv() happen pretty early so it might be OK today...but I didn't spend the time for a thorough analysis. The bits in driver.rs are under cfg!(windows) so might be safe[1].

Also std::dynamic_lib::DynamicLibrary::prepend_search_path() is a global public API that wraps std::set_var(), but we could probably delete that and tell anyone who wants to do this to do it early in process startup via libc::setenv. (Or better, use RPATH for at least Linux/ELF)

[1] I actually don't know offhand all of the details of Windows threadsafety around this, but https://public.kitware.com/Bug/view.php?id=13156#c29334 is a useful comment in a thread.

[alexcrichton]

It doesn't actually work to just wrap a mutex around getenv() because it returns a const char * - but that returned pointer can be invalidated by a subsequent setenv().

True, but this is why the two functions in Rust returned owned buffers, and I think it's still reasonable to require that external users at least synchronize access, even when using external libraries, to have this kind of protection.

Another option is to drop set_var and remove_var from libstd - anyone who wants to do this can drop to the OS-specific wrapper in e.g. libc::setenv

Unfortunately I think this may be a non-starter. Not only is it a large breaking change, but it's not cross-platform as Windows environment variables work fairly differently.

[cgwalters]

True, but this is why the two functions in Rust returned owned
buffers,

Yes, there's no issues if one's program is 100% Rust. This PR is about
programs that aren't.

and I think it's still reasonable to require that external users at
least synchronize access, even when using external libraries, to have
this kind of protection.

I would assert that very few Rust program authors are going to be in a
position where they can (easily) change some of these external C
libraries. Carrying patches for glibc in particular if you want to make
use of gettext in this way would be hugely painful.

Another option is to drop set_var and remove_var from libstd - anyone
who wants to do this can drop to the OS-specific wrapper in e.g.
libc::setenv

Unfortunately I think this may be a non-starter. Not only is it a
large breaking change, but it's not cross-platform as Windows
environment variables work fairly differently.

It'd be painful, yes. Ideally this would have been caught far earlier.

It's possible that there are no Rust programs that are broken today -
although I'm suspicious of the rustc/llvm interaction at least.

But several GNOME components were burned by this, as well as the Fedora
installer Anaconda at least, and had to do some unwinding away from
setenv(). Remember we're talking about use of freed memory, with all
of the potential bad consequences. If things like getenv("HOME") are
corrupted it's a risk.

On the plus side, the Rust std::process::Command already has correct
and ergnomic ways to control the child environment.

[alexcrichton]

I would assert that very few Rust program authors are going to be in a position where they can (easily) change some of these external C libraries.

Right, but it's not required to patch up existing libraries. This is perhaps just part of the contract of calling an extern function defined in C. Libraries typically do not race on getenv/setenv and this is simply another aspect which needs to be considered when calling out to foreign code (or providing a "safe" interface to foreign code). If races are possible then necessary synchronization must be provided between Rust and C to resolve the race.

Put another way this is not an inherent unsafety of this method, but rather a contract which needs to be upheld when crossing the FFI boundary on some platforms.

[cgwalters]

Let me state this plainly: I assert that if one's application has both
multithreading and a substantial body of nontrivial C/C++ (or any
language calling getenv()), it is going to be dangerous for any
thread to invoke setenv().

The only reliable thing to do is to call setenv() early in
process startup only (i.e. near the top of main()), before any
threads are created.

Here's a message from one of the glibc developers implying the above:
https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2

Here's a link I just found to a POSIX spec bug on this:
http://austingroupbugs.net/view.php?id=188

"I.e., any program that wants to use getenv simply must refrain from
using setenv and putenv."

Right, but it's not required to patch up existing libraries. This is
perhaps just part of the contract of calling an extern function
defined in C. Libraries typically do not race on getenv/setenv

What you're saying is that if I wanted my Rust app to use say
gettext(3), I could (if Rust exposed its current private mutex for
std::env) synchronize using it by holding that mutex in a Rust
wrapper for it?

Yes...except what if I'm trying to call a C library function which in
turn uses gettext(3) (very common). Then I need to serialize that
entire call using the environment lock.

In this case where the actual getenv() call is buried deep down in the
internals of a long-running C function, if you wrap a mutex around it,
you lose all of the parallelism you were trying to gain from running it
in a thread in the first place.

Let's take a real world example; say I'm writing Rust bindings for
libparted.

http://git.savannah.gnu.org/cgit/parted.git/tree/libparted/disk.c?id=098bf9ca4c1ea7955ad683694c64f0201760de60#n152

There deep in the bowels of a disk probing function, it happens to call
getenv(). And this is a classic "debug by environment variable" thing
that a C library author could add at any time - even if you audited
version X.Y of the library for getenv(), it's very easy for X.Y+1
to add a new call.

(Ok, in the parted case above, at the most you'd have a read of freed
memory returning TRUE, resulting in a spurious fprintf()...)

http://git.savannah.gnu.org/cgit/parted.git/tree/libparted/arch/linux.c?id=098bf9ca4c1ea7955ad683694c64f0201760de60#n469

Looks like a better example where the library is actually trying to
process the returned string.

And if you're still thinking that it's reasonable for the author of Rust
parted bindings to hold the std::env mutex around every invocation to
a parted function, there's an even harder case from my background, which
is https://git.gnome.org/browse/glib We have a "GDBus worker thread"
created by C code which calls getenv() - so wrapping the entrypoints
with an environment mutex wouldn't help synchronize the worker thread.
To make that work we'd need a C API to inject an external mutex to call
around every invocation of gettext() and getenv().

There really is no sane solution to this other than banning setenv after
threads have been created.

[alexcrichton]

@cgwalters I definitely agree that this is a bug that comes up in practice, and it's definitely something that needs to be warned about, I think the message may just need to be updated slightly. For example, how about this?

Note that while concurrent access to environment variables is safe in Rust,
some platforms only expose inherently unsafe non-threadsafe APIs for
inspecting the environment. As a result extra care needs to be taken when
auditing calls to unsafe external FFI functions to ensure that any
environment accesses are properly synchronized with accesses in Rust.

[cgwalters]

I feel like that doesn't quite leave the reader with enough information to figure out "how do I do this synchronization" or in general find out more information about the issue.

There doesn't seem to be significant precedent for linking to URLs from documentation, but how about something like ⬆️ ? Happy to do other URLs too (could we use this PR?)

[cgwalters]

(That said, I'm fine to just do exactly what you suggest in the interest of moving on from this issue)

[alexcrichton]

Linking to an external site is definitely fine by me!

• Re-reading what I wrote, could you re-word to "As a result, extra care needs to be taken when auditing calls to unsafe external FFI functions to ensure that external environment accesses are..."
• Could you add a bit of text around the link? Something like An example of this unsafety on Unix can be [found here][link].

[brson]

Assigned to @alexcrichton

[cgwalters]

Ok, pushed an update which does both of the above.

[alexcrichton]

Thanks @cgwalters! It looks like there's only one piece remaining, the make tidy check is failing (the line lengths are over 100 chars). Perhaps syntax like this?

Words words words words [link][link].

[link]: http://...

[cgwalters]

Oops, thanks. OK, now I know about make tidy. I ended up reworking it to link to two bugs, as together they link to lots of other information.

[alexcrichton]

@bors: r+ 44a5bf1 rollup

Thanks!

[bors]

⌛️ Testing commit 44a5bf1 with merge 0a1a53d...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt

[fweimer]

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.

[bstrie]

@fweimer Can you use that behavior to demonstrate memory-unsafe behavior in a Rust program that does not use the unsafe keyword? If so, then it will be guaranteed to be addressed. (Though you should probably open a new issue for it.)

[cgwalters]

Just for fun: from my previous comment

Let's take a real world example; say I'm writing Rust bindings for
libparted.

I just happened to notice my theoretical example exists now: https://github.com/pop-os/libparted