ENV is not thread safe with glibc

[c42f]

For some reason I was reading our ENV code, and I noticed we don't have a lock around our calls to getenv and setenv on linux, or for iterating the environment via the environ variable.

This makes ENV unsafe for use with multithreaded julia, as getenv and setenv are not mutually threadsafe in glibc. (See https://github.com/bminor/glibc/blob/master/stdlib/getenv.c and https://github.com/bminor/glibc/blob/master/stdlib/setenv.c. Unsurprisingly getenv is safe by itself, but mysteriously setenv is protected by a lock which getenv ignores! So you can mutually setenv safely, but nobody can presume to getenv elsewhere in the same multithreaded code.)

We can easily add some more locks on our side as mitigation, but unfortunately we can't really fix setenv without fixing glibc; a random C library we link against may decide to call getenv at any point. This definitely happens in practice and I've experienced it personally: OpenMathLib/OpenBLAS#716 (comment). There's a nice discussion on the rust issue tracker including this gem: rust-lang/rust#24741 (comment).

Some options:

1. Mitigation: Put our own lock around getenv/setenv/environ access, and just hope that no C library we link against calls these functions itself. Add a big warning to the docs. This is the current rust approach though it's fragile.
2. Avoidance: Ban using libc setenv entirely; create a shadow environment which is a copy of the system environment. This is the C# approach but with Julia's tradition of calling into C libraries I doubt that will work out (it creates surprises even in C#; see https://yizhang82.dev/set-environment-variable)
3. Fix glibc??: Every portable language runtime has this exact problem, so people have tried fixing this in the past. In older times they were met with surprising hostility. The seemingly-current bug is marked suspended https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c2 whatever that means.

For now the only easy / possible thing to do is to clearly option (1): add some locking and a big warning. If glibc could be fixed it could morph into a long term plan. More recent bugs suggest the glibc maintainers may possibly accept a patch.

As a side note, I feel we should consider removing withenv in the future because the shape of that API misrepresents reality. On the surface withenv appears to offer dynamic scoping, but ENV is global state. So withenv can never work reliably for concurrent code. That is, unless we took option (2) and avoid the C environment completely.

[ViralBShah]

Does musl also have the same issue?

[c42f]

Does musl also have the same issue?

It certainly seems so. In fact there's even less locking:
https://git.musl-libc.org/cgit/musl/tree/src/env/getenv.c
https://git.musl-libc.org/cgit/musl/tree/src/env/setenv.c

I should point out that independently of any implementation this is a POSIX issue which doesn't require these functions to be threadsafe. See notes at http://man7.org/linux/man-pages/man3/setenv.3.html for example.

[c42f]

this is a POSIX issue

Having said that, I feel it's reasonable for libc implementations to mitigate this, because regardless of the posix standard, the way that C libraries use these functions in the wild has proven to be unreliable. But fixing this is not at all easy even inside glibc. As described here https://sourceware.org/bugzilla/show_bug.cgi?id=15607#c4 it may be necessary for glibc to leak some memory every time setenv is called because some other part of the process may be holding onto the result of a previous getenv.

Amusingly windows gets all this right and GetEnvironmentVariableW and friends just do what you'd expect and want with no fuss.

[ecsx1]

There's been some discussion on musl mailing list back in 2015 about getenv and thread-safety.
https://www.openwall.com/lists/musl/2015/03/04/11 cc @richfelker

[richfelker]

Putting locks around them does not make thread-unsafe functions thread-safe. A fairly large number of libc functions are specified to use the environment, and if you modify the environment, via setenv or direct manipulation of environ, any concurrent call to one of them produces a data race and thereby undefined behavior. musl is pretty strict about not introducing environment access into functions not specified to depend on the environment, but glibc is not, so it's hard to even make a viable approach of adding locks all the functions that might use the environment.

The right answer is simply that you don't modify the environment; it's effectively immutable input state of the program. To execute another program with changes to the environment, you use one of the exec/posix_spawn family functions that takes a complete new environment as an argument rather than editing your own environment first. If you have a language interpreter/runtime that allows logical modification to the environment (like the shell does), you don't actually modify your own environment, but keep a separate logical copy that's accessed under appropriate locks that you expose back to the hosted program and export to child processes when execing them.

The only problem this does not solve is a need to set environment variables that change behavior of library functions within the current process, like TZ.

[Keno]

Our process spawning already operates on copies of the environment, so that's not the issue (though as mentioned we do still have withenv and of course the ENV["FOO"] = "BAR" accessors that do call the underlying libc setenv). The big problem is the last issue mentioned:

The only problem this does not solve is a need to set environment variables that change behavior of library functions within the current process, like TZ.

C libraries (not just the libc) unfortunately often like to use the environment for configuration options that we dynamically need to change. Where we have influence over the C library in question, I think we should make it a policy for these libraries to have an API other than the environment for setting configuration options.

For other libraries that we don't control and that are unwilling to take such patches, I'm not sure what to do though. Perhaps the only thing we can do is put a big scary warning in the docs like Rust did.

There's also the question what to do with withenv. In theory, we could just have that be a task-local immutable overlay over the actual process environment. That would achieve the dynamically-scoped environment manipulation that I do think is probably useful for people using Julia for shell-like tasks. Of course at that point, we'd probably still need a way to modify the global environment (unsafe_setenv!?) for aforementioned miscellaneous C libraries, but perhaps that's a better situation in that it signals to users to avoid this interface if possible.

[c42f]

Thanks @richfelker for your perspective! You're right; the existence of direct access to environ and the fact that getenv returns a pointer to memory owned by libc (or the system) leaves no reasonable solution in sight for *libc. Either

1. Using the result of getenv in the presence of setenv leads to a data race or
2. setenv would have to permanently leak the memory from the previous environment.

We can somewhat-mitigate this from within julia by using a lock and copying the result of getenv into our own buffer before releasing the lock, but in no way does that fix the problem globally.

Also thanks for pointing out something which I hadn't considered; that setenv has an implicit data race with several libc functions. (The list for musl looks very short: https://wiki.musl-libc.org/environment-variables.html)

The only problem this does not solve is a need to set environment variables that change behavior of library functions within the current process, like TZ.

Exactly. I imagine this is why Julia's ENV currently uses setenv the way it does. We're now in a sticky situation because changing to an immutable environment model would leave no way to control libraries which rely on the environment. Perhaps we could backpedal a bit in future and expose an unsafe_setenv to change the C environment, but make ENV modify a copy of it.

If we'd prefer the leaky approach in julia, another nasty idea is to take matters into our own hands on linux and point environ to some memory we own. Of course then it's not clear what to do if some C library calls setenv.

[c42f]

There's also the question what to do with withenv. In theory, we could just have that be a task-local immutable overlay over the actual process environment. That would achieve the dynamically-scoped environment manipulation that I do think is probably useful for people using Julia for shell-like tasks. Of course at that point, we'd probably still need a way to modify the global environment (unsafe_setenv!?) for aforementioned miscellaneous C libraries, but perhaps that's a better situation in that it signals to users to avoid this interface if possible.

I'm feeling like this is the way to go. In addition, make ENV lookup also access that task-local immutable overlay of the process environment. At the moment, I feel like I see various julia libraries recommending ENV for configuration, and having syntax as simple and enticing as ENV["FOO"] = "BAR" as a surprising alias for please_create_a_data_race_with_some_random_c_library() doesn't seem great :-)

[c42f]

What do people think about compatibility here? I'm happy to do the work to fix this but I'm fairly sure we can't fix it completely without breaking compatibility. So we'll have to decide which trade off to take.

Here's several possible plans:

Plan 1

• Make withenv add a Dict or possibly ImmutableDict{String,String} to task_local_storage (using :ENV_overlay as key?)
• Make all ENV access first look that up before falling back to the global ccall to getenv. Including getindex and iteration.
• Add locks around all the internal access to environ via getenv etc
• Soft-deprecate setindex!(ENV) by adding a big warning discouraging it.
• Add unsafe_setenv! and document that it's inherently unsafe, but is for configuring C libraries which insist on using environment variables for config.

This will avoid julia-only crashes but will do nothing for getenv data races in C libraries. It should keep most things working though there may be some edge cases with withenv and C libraries (which while they may work have no hope of ever working concurrently).

Plan 2

As in Plan 1, but essentially make ENV a normal Dict which is a copy of the environ at julia initialization. Add locks around it so that modifying it is threadsafe.

This will fix the data races and preserve normal within-julia use of ENV as it's done currently. However, it will break any code which is relying on C libraries seeing the updated ENV and those would have to transition to unsafe_setenv!. However doing that via Compat would be simple.

Plan 3

Make ENV an immutable copy of the environment. Encourage use of withenv (implemented as above) for convenience in launching processes. Provide unsafe_setenv! for desperate situations and discourage its use.

This may be conceptually clean but it will break so many scripts and startup files that I think it's Julia 2.0 material.

It has the benefit of preventing confusion for those who expect that ENV can be "truly" modified. On the other hand it would be less natural for simple imperative-style scripting which wants to do a lot of process spawning.

[StefanKarpinski]

musl is pretty strict about not introducing environment access into functions not specified to depend on the environment, but glibc is not, so it's hard to even make a viable approach of adding locks all the functions that might use the environment.

Perhaps I'm being naive, but this doesn't seem so bad to me. We don't use a whole lot of libc functions and we can use even fewer pretty easily. Auditing which glibc calls we use that depend on the environment doesn't seem too bad. I guess the bad/intractable thing is that others may well write Julia code that uses libc directly via ccall or indirectly via calls to C that use libc, none of which would do the appropriate locking.

[richfelker]

I guess the bad/intractable thing is that others may well write Julia code that uses libc directly via ccall or indirectly via calls to C that use libc, none of which would do the appropriate locking.

Exactly. If it's intended that programs be able to call C library functions, then the problem is not tractable.

[richfelker]

Regarding #34726 (comment), I'm not a julia user, but I would think plan 2 is the best solution.

[StefanKarpinski]

Here's an alternative. Since calling setenv is rare and slow anyway, halt all threads but the currently executing one while doing it. I.e. not just holding a lock which other threads have to know about: shut them down entirely and only continue them after the environment is modified.

[vtjnash]

Yeah, we can't entirely avoid them: we ship with e.g. BLAS which uses them, so working around this is problematic. It seems like it'd have to be mitigated at the libc level, but only Win32 exposes a thread-safe API. As c42f said above, it seems that libc maintainers could have chosen to mitigate this, though not required by posix to do so, but have apparently generally opted not to.

I'd suggest a plan 4:
Eliminate withenv, rename setenv! to unsafe_setenv!, use (and improve) setenv for launching processes.

[yuyichao]

Stopping the thread won’t work. You either cannot guarantee that the code will ever finish or you can’t know you didn’t interrupt in the middle of a critical region.