Recently there have been a number of vulnerabilities reported due to unsynchronized access to environment variable APIs such as getenv(3) or setenv(3), the latter of which is marked as MT-Unsafe const:env by glibc.
Notably these vulnerabilities were in some cases discovered via segfaults in real-world Rust programs.
The safe environmental accessor functions in std::env are synchronized through OS-specific backends. In cfg(unix) environments synchronization is provided by std::sys::unix::os::ENV_LOCK, and the std::sys::unix::os::env_read_lock wrapper function.
Unfortunately, these are all private APIs, which means that there is no way for FFI users who need to directly invoke getenv/setenv or other e.g. libc functions that vicariously invoke them, such as localtime_r.
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 external environment accesses are properly synchronized with accesses in Rust.
Missing from this description is a solution for soundly invoking any APIs which might need to acquire something like ENV_LOCK first in order to ensure synchronized access to the environment.
As far as I can tell there are only two options here:
Add APIs that FFI users can use to synchronize access to the environment
Rewrite functions like localtime_r in safe Rust, using std::env's safe, already-synchronized API for environment access
Personally I really like option 2, but so far various people have looked at porting localtime_r to safe Rust and souring on it due to the complexity. I really hope this still happens and it would be a huge benefit to the ecosystem.
But really I opened this issue to talk about option 1: is there a safe API that makes sense to expose from std? Could there be something like a std::os::unix::env module which provides something like read_lock and write_lock functions which an FFI caller can obtain before calling into APIs that invoke getenv(3)/setenv(3) respectively?
I think there's a possible solution between 1 and 2 though.
Make set_var only change things visible from std::env functions (as proposed in 2 above).
Add a new unsafe function for modifying the real honest-to-god global env, visible to everyone.
As there's no sound way to have the changes visible to FFI at the moment since the lock is not exposed, the first is not really a breaking change. And the second recovers the lost ability, but makes it actually explicit that there's a real thread safety issue.
I'm not a huge fan of the approach being exposing the lock — there are a lot of cases where doing so would not be viable for one reason or another — use from no_std (which is something some FFI libs try to support), performance, ...
It also doubles down on what I see as an existing design mistake ("add locks" is only a solution to a thread unsafe API if you can control all accesses — clearly not the case for the environment)
Yes, you can argue that it should have been unsafe, and maybe we should deprecate and replace it, like we did with before_exec to pre_exec. But I still don't think we should break programs that are using set_var now, as they may already be respecting those safety conditions.
So really it seems set_var and perhaps the current locking scheme existing at all were a mistake. Though set_var is not flagged as unsafe, it seems unsound to use if there is more than one running thread in a given program, regardless of locking.
Has anyone ever opened a PR which attempts to deprecate it entirely? It sounds like that's the best way forward.
If it is deprecated, suitable replacements need to be provided that existing code can migrate to.
My own uses of set_var could (I think) be replaced by something that only affects Rust code (including new processes spawned via Command), but it looks like we would probably also need some unsafe function that actually mutates the global environment shared with other languages.
As I said many times, I don't think there is a problem at all. If you want to do something, outlaw setenv and putenv, especially in multi-threaded code. Environments should be constructed for a process before the start and then left alone.
I'm firmly in the "Don't call setenv() after you have multiple threads" camp. It's tempting to allow binaries to somehow opt-in to an abort() if the C library detects that happens - maybe an ELF note or something.
That's the state of affairs today. However, the purpose of exposing the locks would be so things like FFI wrapper crates can use them synchronize with std.
However, now I properly understand that glibc's notion of MT-Unsafe means that any usage of setenv in a multithreaded program, even if synchronized, is unsafe. And to me, that's the real reason not to expose them: because they're unsound to begin with, even just using set_var through std's safe API.
Oh, that's an interesting idea. It could be implemented as "hardened" versions of setenv and putenv, with distinct names, enabled for C programs via the existing -D_FORTIFY_SOURCE=n mechanism, and Rust's runtime could call __setenv_chk (or whatever the name ends up being) directly.
Would you mind posting about this on libc-alpha@sourceware.org? I don't have time myself, but there are at least two glibc devs I can think of who might jump on it.
If we're in the territory of adding symbols to glibc, personally I think it would be easier, cleaner, and more backwards-compatible to 'just' make a getenv_r that's safe for multithreading – a.k.a. what Drepper was ambivalent about in 2009:
The only alternative would be a new interface which, unlike getenv, does provide a buffer for the content of the envvar to be copied in. Then the lifetime of the string is in the hands of the application. That's a new interface of course. I don't think I would even agree to that. I really think the environment should be immutable after startup.
After all, implementation-wise, glibc's setenv is actually safe to call from multiple threads at once, documentation notwithstanding. It takes a lock. The problem is getenv, which
(a) does not take the same lock, and
(b) returns a pointer to memory that could be freed or altered by the next setenv.
But (a) could be easily fixed; if there's any performance concern, make it into a read-write lock. And a new interface would solve (b).
A big problem with the getenv_r approach is that even if it was added to glibc tomorrow, it'd take quite a while to be available in enough places where it could be an assumed default. And then if it's added to glibc but not musl it becomes a huge portability hazard - i.e. we've only successfully pushed the problem space to "you may only get random memory corruption in your OS installer using libparted if you're using musl". And even beyond Linux of course, this issue exists for MacOS (AFAIK) and the BSDs etc.
(Random side note: I just want to say it's interesting to me from a cultural perspective how Java banned setenv for this reason long, long ago. Java also doesn't expose chdir(). Yet for Rust, the tension between "avoid global mutable state" and "systems programming language" won out in favor of exposing those APIs. Invoking chdir() long after process startup, while it can't cause memory corruption, is just a really bad idea for very similar reasons. No one would want to use a library crate that called chdir() behind your back, any more than one should use one that called setenv().)
The trouble with this idea is that it wouldn't truly be safe as long as environ and the third argument to C main still exist. And there are legitimate use cases for both. Also it would reopen the argument, and despite Drepper no longer being involved, I doubt anyone has the appetite for that.
The hypothetical "crash if there are multiple threads active" versions of setenv and putenv do not entirely solve the problem either, for the same reason -- nothing can prevent someone from scribbling on the environment block directly, via environ -- but they would only enforce a rule that already has consensus within both glibc and the POSIX committee, so they're an easier sell.
I have never learned Java in detail, but I have the impression that it was designed around the assumption that any nontrivial program will have multiple threads. Myself, I think this was a design error, and there are many reasons to want to write resolutely single-threaded programs even today. And, if you know there will only ever be one thread, there is no problem with using these old-fashioned APIs as they were expected to be used, back in the 1970s.