Should apps orchestrate a super-global lock of a foreign namespace?
An environment variable's value, for a running process, is just what it is: an initial value from outside.
Adding complexity around it smells like an attempt to control a distributed mutex, like checking an API for real-time value changes in a while loop across several instances of the same app.
I thought there would be alternatives to this, like pubsub, Kafka, or other asynchronous event handling.
Imagine having to test an app for its ability to handle safe read-write of OS-level state. It's definitionally bankrupt: not really a unit, not easy to set up quickly, and not isolated.
> An environment variable's value, for a running process, is just what it is: an initial value from outside.
Then calling setenv() from anywhere except in the time between a fork() and exec() call should be banned, but it's not. Honestly, calling abort() if setenv is called in the presence of threads would seem like a better status-quo than what we have today.
You may have a mutex on getenv/setenv, like the Rust stdlib does, but when libc doesn't look at that mutex, even on the read side, you run into UB.
So the next step is never calling into seemingly innocent libc functions in safe code (which you have to enforce on your dependencies as well), implementing safe alternatives to a good chunk of libc (and making sure your dependencies use those), to cordon off anything that looks at the environment. This makes a good chunk of POSIX functionality useless.
I thought they were talking about inside the libc implementation. Though, if that was done and people call getenv from async contexts, it could deadlock.
WG14 also doesn't want to provide safer string and array manipulation libraries for decades, even Dennis Ritchie failed to get fat pointers into ISO C, why should fixing this be any different?
The person who fixed this would be incredibly sexy in my book. Though, that probably tells you more about my own sexiness than it invalidates your comment.
The right thing is not to modify a global state once you have started all the threads. If you need to do that, use your own data structures. The case of getaddrinfo - would need a copy of the entire env. in a thread safe manner, then return the result. That would pretty much apply to anything that uses env
The essential problem is that there is no thread-safe way to implement this while maintaining backwards compatibility -- applications can alter the environment block by changing the environ global pointer, applications can also alter the environment block by replacing individual pointers in the environ array, applications can also alter the environment block by altering the strings pointed to by the individual members of the environ array, applications can also alter the environment block by using setenv/putenv/etc.
Inserting a mutex into the setenv/getenv/etc. functions is pointless because applications are explicitly allowed to modify the environ pointer and array directly without any locking.
The only argument I see is that an API can be misused. There are ergonomics debates we can have about it, but a user intentionally abusing an API wanting to do something that's completely wrong is hardly indication the API is broken.
Exposing global mutable access to something and providing no thread safe version counts as broken in my book. Your book may differ.
A good API would probably only allow constant access. If mutation is for some reason deemed necessary then it should be through a separate set API and the return results of get should be guaranteed safe.
It seems they are arguing that setenv should not exist in the first place: the fact it exists suggests it can and should be used, and thus not be a footgun.
> the fact it exists suggests it can and should be used
I think most people argue about that. Just because it exists doesn't mean it should be used imho.
I've used it exactly once, and that was a school exercise where I had to write a Posix shell (most of a posix shell actually), including built-ins. I do not see another use case tbh.
> can and should be used, and thus not be a footgun
It can and should be used in the cases where it makes sense, with the restrictions that are documented. It's an API that is fundamentally not thread-safe, you can not use it "safely" (in the modern sense of using it after a lobotomy, in any way that the compiler allows) in a multi-threaded context.
There are other such APIs, and if those APIs were removed it would hurt a lot of old software that is running perfectly fine.
> Please show me from which book you got the idea that env variables are expected to change throughout the lifetime of a process.
POSIX specifies two functions that alter environment variables. It could've specified that env variables are supposed to be mapped into a read-only memory page where available to indicate that they shouldn't be altered, but it didn't, and instead provided an explicit read/write system.
As with most things programming related, “it depends”.
Functional programming has really, truly done untold and massive damage to the industry. Fortunately, you are free to unpoison your mind. I just hope people eventually do.
> Inserting a mutex into the setenv/getenv/etc. functions is pointless because applications are explicitly allowed to modify the environ pointer and array directly without any locking.
by that logic mutex themselves are pointless because nothing ever forces you to use them, even in memory-safe languages you can still access /dev/mem and change bytes? It's stil a useful thing to have.
The difference is that modifying the environ pointer is explicitly supported behaviour in the standard, poking through /dev/mem is not.
Although I guess a middle ground solution wouldn't be too bad either - most programs don't modify environ directly, so POSIX could offer thread safety for the functions and make multithreading through "environ" UB. This is already kind of explained in the standard:
> The difference is that modifying the environ pointer is explicitly supported behaviour in the standard
they just have to fix the standard. e.g. in my country they manage to improve for instance the standard for electrical plugs every three years, there is NO REASON posix cannot do the same
Another problem is that it is hard to reason about security in a C program if all environment variables could change at all times.
If we are talking about the C application deciding when it wants to rescan the environment that is something different, but if your environment can change potentially before and after you check it this opens you up for a heap of new attacks.
I think the memory leak solution (copy over the env variables to a new location in memory every time you call setenv and keep the old pointers alive) will cause the fewest crashes.
I would personally go for the aggressive approach (release a new major version of libc that detects multithreaded environments and intentionally crashes out when calling setenv() so people actually notice and fix their broken programs) but I suspect not many people will agree with me on that.
The API is not necessarily bad (it's just very 80s UNIX), but the lack of enforcement of thread-safety causing all kinds of bugs and crashes.
It solves hard crashes during DNS lookups by wasting a few kilobytes of RAM. Seems like a fine solution to me. The memory leak only occurs in circumstances where the program would've crashed or started messing with random memory anyway.
A proper solution would be to either nuke put/setenv() in the C standard library or redesign the *env() calls entirely, but that would break existing programs.
A proper solution would be to implement your own put/setenv in the program that needs special behavior. That solves everything and does not affect any other programs.
There are circumstances where it's a perfectly valid solution. For example, suppose you're trying to acquire the lock on something to destroy it. It can be the lesser of two evils just to leak that memory instead of just waiting forever/a very long time to acquire that lock. You just need to ensure that you aren't leaking memory too quickly for whatever your constraints are. For example, most programs wouldn't care about a 1kb/day leak of memory because that would take a very long time to actually become noticeable. Furthermore, there's pretty much always some degree of memory growth just due to heap fragmentation (at least if you're using a language like C which can't do memory compaction via GC).
Yeah, but the program may not be broken until you, the glibc maintainer, calls `raise(SIGSEGV)`.
Most programs using setenv call it before starting any threads. That is not broken.
Detecting the linkage of thread support and crashing that program on purpose is, frankly, a pathological way to fix a non-broken program.
Besides which, your proposal won't work anyway, because this remains a potential problem in single threaded programs anyway: a program calling getenv, storing the result, and then calling setenv on the same variable and using the previously stored result will break anyway.
In summary, your proposal is broken in two different ways: 1) it breaks well-defined programs, and 2) it fails to break broken programs.
I wouldn't implement it during linkage, obviously single threaded putenv/setenv calls should still be permitted as part of initialisation routines.
Count the number if children in /proc/self/task for all I care, the detection needs to happen during runtime.
You're right that putenv/setenv are also horribly broken in other ways, and doing multi thread detection doesn't prevent those problems. In a perfect world we would just kill off these two functions all together, replacing them with either crashes or no-ops, but that'd be an even harder sell.
> I wouldn't implement it during linkage, obviously single threaded putenv/setenv calls should still be permitted as part of initialisation routines. Count the number if children in /proc/self/task for all I care, the detection needs to happen during runtime.
That still breaks well-defined, non-broken programs which don't call getenv/setenv in racing ways. There is no way for you do a conditional-upon-threads mechanism without false positives.
> You're right that putenv/setenv are also horribly broken in other ways, and doing multi thread detection doesn't prevent those problems. In a perfect world we would just kill off these two functions all together, replacing them with either crashes or no-ops, but that'd be an even harder sell.
But you don't need to in order to meet your original goal - breaking programs which do call setenv/getenv in the wrong order. Proposing to remove them altogether doesn't fulfill the goal of finding the breakages immediately and introduces breakages in existing programs.
My alternative: use LD_PRELOAD and provide alternative setenv/getenv functions which raise SIGSEGV when setenv is called on a variable more than once, and when getenv is called on a variable that was already setted once. It requires nothing more than a counter for each of setenv/getenv per variable.
That finds programs which actually are broken, with no false positives, and ignores threads altogether because they don't matter under the counter system[1].
Best of all, you can implement this in an afternoon, without needing to modify glibc, and then test it with every single executable on your system to see which ones break.[2]
[1] Since the caller knows they are not thread-safe anyway, we aren't looking for the error where the caller calls setenv concurrently in different threads. That's a different problem.
[2] I would wager good money that few, if any systems, will break under this test.
you're right; in addition to that though, I'd like to highlight that the use of some form of locking "inside" set/getenv would gain you nothing at all. That is not because of setenv, but because of getenv. The latter returns you a _pointer_. Whether you call that a reference leak, an ownership breakage ... it's not "yours" and when you have it, you don't "hold" it even if getenv internally were to lock whatever the underlaying data structure might be.
_That_ is the issue. You can only solve that if you change the interface. Make a new one, getenv_r(), have it _copy_ the env var value into a user-provided, user-owned buffer. In that case, you can then assure the returned value is both point-in-time correct and immutable. You can never achieve that with getenv() because if you copy/make the returned pointer owned, the owner needs to free it. which is a break from the current behaviour and so not backwards-compatible ... and hence out of the question.
Lamenting about how broken the interfaces might be and then insisting that the implementation should be fixed is ... "conveniently shortsighted". Not saying this isn't worth fixing, but fix it the right way in the right place.
Make getenv copy to an OS-provided buffer. Free at program exit, like any other memory leak. There's an obvious drawback, but it's not changing the function signature.
Wrong. As I've pointed out several times in this thread and in other recent threads about getenv(), Solaris/Illumos has an implementation that is lock-less to read (except when you change `_environ`, then it takes a lock at most once until the next time you change `_environ`). It's made safe by "leaking", and by locking in the functions that write. It's only unsafe if you replace the value of `_environ` repeatedly and free the old settings (which I've never seen any code do, and which if you do then you get what you deserve).
All of this is false, including the last statement. Proof by existence: Solaris/Illumos has a thread-safe (leaking, though the leaks are hidden from memory debuggers), lock-less (whenever you don't write to `environ`) `getenv()`, and thread-safe, locking `setenv()`/`putenv()`/`unsetenv()`: https://src.illumos.org/source/xref/illumos-gate/usr/src/lib...
Yes, it can be done and has been done. glibc has no excuse.
Cool, ever since that rachelbythebay article [1] I was wondering how different libcs handle the issue! Nice to see that someone else confirming the behavior of apple'c libc. It's not mentioned in the article, but while apple's libc seems to suffer from the use-after-free issue, if I'm reading it right it does seem to have locking for setenv/getenv [3]
The described problem (thread safety) for a global configuration seems mostly a misunderstanding by the author.
The usual case for modifying a global state is: modify once, then proceed (e.g. start new threads). Even if all the calls become thread safe, the behavior would be inconsistent, still.
It is perfectly reasonable and consistent for one thread to set an environment variable while other threads are reading different environment variables.
I wonder if you could work around this by using LD_PRELOAD to load in a shim around get_env and set_env. You'd still have the problem of environ potentially getting mutated, but it very well may solve the problem if it's limited to those two functions.
You would still need to design a fix. You'll probably either break programs that modify the pointer returned by getenv() while doing so.
However, this only makes sense for other people's software crashing on getenv() related memory bugs. If you control the software, you can simply prevent the setenv() call yourself. No need to LD_PRELOAD anything, just load a library or write your own hooking code to work around the POSIX madness.
> This is a list of some uses of environment variables from fairly widely used libraries and services. This shows that environment variables are pretty widely used.
Widely used, yes. Used as in read. Why do any of these need to change at runtime? And if they do - why are they environment variables?
Changing the env during runtime is actually quite handy for debugging and forcing the program into specific states.
Other than that, it can also be handy in k8s with a VPA. You get more/less memory and then update the env to reflect that. Your service picks up the env change and updates the runtime.
IIRC, there is/was some way to listen to those changes in C#, and automatically update runtime settings.
> Other than that, it can also be handy in k8s with a VPA. You get more/less memory and then update the env to reflect that. Your service picks up the env change and updates the runtime.
You… can't change the env from outside the process…
are you saying this is used by disjoint components within a single process? Or is this just a misunderstanding?
But /proc/pid/mem is, if you like living dangerously! You'd just have to parse the dynamic-linker metadata to find where libc's environ is hiding. (Though statically-linked programs would be tougher.)
> You… can't change the env from outside the process…
Not with that attitude you can't.
(OK, without the joke: you can do this with an interactive debugger. But I think OP just meant "change it in the container and then restart the child process")
> Changing the env during runtime is actually quite handy for debugging and forcing the program into specific states.
Most debuggers nowadays support altering variables at runtime after hitting breakpoints. In the meantime this was the very first time I ever heard anyone even considering changing env vars at runtime, let alone use it to debug stuff. Sounds like an ass-backwards way of going about debugging.
> Changing the env during runtime is actually quite handy for debugging and forcing the program into specific states.
Wait, why would this need to happen at runtime? I have used env cars a lot to trigger specific cases but why would you want to do this while the process is running from within the process itself?
If you control the process you can start it with the right env to begin with, no?
Have you ever exported anything in a shell script? Sure you can keep the necessary changes in local state and pass those to execve(2)/execvpe(3)/posix_spawn(3), and that would be safe AFAIK, but setenv(3) is there and more convenient if you're unaware of the hidden dangers. Also that doesn't work for PATH in execvp/execvpe, which is read from the current process; how do you change search paths for execvp without setenv (short of doing the search yourself)?
Edit: I just realized macOS/FreeBSD has execvP() that allows passing a custom search path, so PATH is now safe, but without a -e variant, everything else is again unsafe.
Shell scripts aren’t different from “real” programs using exec or posix_spawn in this regard, it’s just that fewer people have done the latter than the former, so the former is a more relatable example. “Real” programs spawn other processes too you know, sometimes with modified environ.
Yes, I’m not saying shell scripts are affected, merely using them as an example to answer the question “Why do any of these [env vars] need to change at runtime?”
The discussion is only relevant for a shared unguarded resource (the env) modified and read by multiple threads. Single threaded operations are just fine.
“The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3).”
“Successive calls to setenv() that assign a larger-sized value than any previous value to the same name will result in a memory leak. The FreeBSD semantics for this function (namely, that the contents of value are copied and that old values remain accessible indefinitely) make this bug unavoidable”
>Have you ever exported anything in a shell script
So, shells use a single thread that can safely modify the environment - then start new child processes by the same thread. The child processes get a =copy= of the said environment. That's a textbook example how to use env.
Starting multiple threads on your own, then modifying env should be considered a textbook example how not to do things - env is not intended for interprocess communication.
Yeah, fork()+immediately exec() should be safe, but those use cases are almost always better with posix_spawn(), due to issues with fork(), like memory copying. And if you want to use the p-variant of posix_spawn you’re back to setting PATH beforehand. These APIs designed back in Stone Age just aren’t very well thought-out wrt concurrency and high performance.
Why would you change the path just to call posix_spawnp()? If you want that control, that is an indication that you want to specify the path to the executable, not use PATH.
Shells don't generally use the libc environment; this would be too limited to implement even standard POSIX shell functions with local variables, or non-exported variables. It's much easier to set up purpose-built data structures to track variables, and construct an argument for execve().
(Edit: removed unneeded pointing out execve)
Also shells generally have their own program search anyway since they need to support built-in commands. It's not particularly hard to implement PATH search.
Once again, the OP asked why setenv is even needed, which implies they likely don’t have much experience with spawning processes in low level languages, so I used the more familiar shell script setting as an illustrative example, as setenv is analogous to export in POSIX sh. I never said export is implemented with setenv, or shell script exports aren’t thread safe. Unfortunately, replies hung up on shell scripts.
As for I’m not aware of execve etc… You need to re-read my comment which clearly mentions execve, execvep, posix_spawn, as well as implementing PATH search on your own.
> Once again, the OP asked why setenv is even needed, which implies they likely don’t have much experience with spawning processes in low level languages
I am the OP and your assumption is incorrect. You may consider why the post ends with:
"export" in shells has to change the environ before they start the new process. It may not be "at runtime" for the new process, but it would be for the shell.
Wrong, export does not have to change the shell's environment at all. There are plenty of exec variants that accept a different environment pointer, same for posix_spawn.
Mostly integration, for example some library can only be configured via env variables, but a developer might want to configure it from with-in the app it's integrated into and used from.
Also, few weeks ago I found a use for them when trying to pass configuration from Java/Kotlin to C++ library to be used during static constructors (invoked during dlopen) on Android, because at that phase native code cannot call back to JVM.
I agree that would be a poor implementation, but the library could be loaded at runtime using dlopen or equivalent.
This issue with that “interface” is the environment is process global. If the library is being loaded dynamically (specifically for some task) it would seem that the parameters are local to that task and should be taken by some reentrent init method. Alternatively, the process could be forked and environment set in the child without concern for thread safety or polluting the environment (think of the children!).
Indeed -- it's an extremely unconvincing list, because any sensible library which may require a library user to set env variables (which includes all the ones I checked on the list) can also be configured without setting env variables. Most of the time the env variables set fallback defaults for parameters not specified by the caller. In these cases, the sane thing to do, regardless of the thread-safety of setenv(), is simply to supply the parameter in code.
The only exception is things like debug logging, which is unlikely even to work dynamically.
On the other hand, setenv() is clearly broken in modern code, particularly in a library context, and the man page (at least on my Linux machine) does not make that particularly obvious -- "Thread safety: MT-Unsafe" is the only note, with a reference to attributes(7) for more information. It could definitely be made more obvious.
Just asking: If you pass security tokens via environment variables to the process, doesn't it make sense to delete them from within the process after they have been used?
Yes it would make sense, but no there is no way to actually ensure they have been deleted. A trivial but nonetheless very common case would be if your process is started with a wrapper shell script. But even just within your process, there is no guarantee at all against some random library (or the kernel) making a copy of the entire environment.
If you want to pass secrets into a process at startup, I would strongly recommend passing a pipe as an additional open file descriptor (e.g. fd #4, but this FD number you can then put in an env variable) and writing it onto the pipe. It can only be read once, and you can control where the value propagates.
Now if only someone would explain this to the authors of mongosh (which REFUSES to accept credentials in an environment variable and will ONLY read them from stdin or from argv...)
I mean YES you can factor your code (tests, whatever) to make this a non-issue but supposing some person wrote some code 10 years ago in an OSS project or on your team and you start banging into this issue.
It’s not going to be trivial to unwind let alone find the root issue.
Let’s start fixing things like this for our future selves, right?
Digging heels in and saying “eh, you just got to learn this one weird quirk.. oh yeah this other one too..” is kind of a fun glass bead game until it’s not; as is not a winnning way to endear hearts and minds.
This also caused a lot of trouble for time libraries in Rust. The two foundational libraries, chrono and time, rely on localtime_r to get the local time instead of the clock value in UTC. localtime_r reads the TZ environment variable (and optionally others like TZ_DIR). Rust declares it safe to modify the environment, while POSIX declares it unsafe.
CVE-2020-26235, RUSTSEC-2020-0071 and RUSTSEC-2020-0159 where opened against the crates. That left the Rust ecosystem with a pretty much unsolvable issue for many months. Chrono went with the solution to parse the timezone database of the OS natively and read the environment using the Rust locks. Time tries to detect if the libc version has thread-safety guarantees to access the environment, and otherwise panics if there are multiple threads.
You are right. POSIX specifies one thing, the standard library in Rust and some other libraries specifies something different. 'Safe to use unless there are other threads' is not really something you can or want to encode in a type system.
One problem is that marking that function as unsafe would unfairly penalize platforms like Windows that don't have this issue. Even if it turns out to be the least-bad compromise solution, it sure would be nice if we could have nice things.
But Rust doesn't declare it safe to modify the environment in general. It declares it safe to modify the environment using std::env::set_var, which uses locking internally. The docs explicitly note that there's potential unsafety if non-Rust code modifies the environment:
"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."
> Rust declares it safe to modify the environment, while POSIX declares it unsafe.
Arguably, Rust declares it is safe to modify the environment through its stdlib methods. The tricky detail is that this means it is unsafe to read/modify the environment through other means, but sometimes this is really hard to avoid.
> The tricky detail is that this means it is unsafe to read/modify the environment through other means, but sometimes this is really hard to avoid.
If you have C and Rust in the same process and C code calls setenv(3), for one ...
Edit: why downvotes? It's very typical to link to C libraries which may call the libc environment stuff ... My point is you can't control library code as easily, if it's some dependency of a dependency eventually calling libc.
Does rust also add an pthread_atfork handler? Otherwise, it seems likely still unsafe for rust to claim to support calling fork (for execv) or posix_spawn, as most libc call realloc on the `environ` contents, but do not appear to take any care to ensure that (v)fork/posix_spawn doesn't happen concurrently with that. Worse yet, the `posix_spawnp` API takes an `envp` parameter and expects you to pass it the global pointer `environ`, which is completely unsynchronized across that fork call. It is not obvious to me that this is a security gap, but certainly it seems to me that this would violate rust's safety claim, if it is not taking added precautions there.
I don't think Rust's stdlib provides any kind of safe way to call just fork(), it only has methods for creating child processes because that's the only interface that works on every supported Tier 1 platform. Calling fork is always going to necessarily be an unsafe{} libc call or syscall, and the caller will have to take care to ensure nothing funny is going on.
`std::os::unix does` adds some additional methods in that vein like exec(), but no fork(). `std::os::linux` only adds the ability to get `pidfd`s for child processes you create. There's simply no safe way for the stdlib to provide safe fork() without knowing a lot of things about how you're going to set up your process and what other libraries you might pull in that may not be fork-safe. If you're willing to ensure you only call it in a safe way, you can still call fork, the language just cannot guarantee it will be safe, same as when you're doing it in C.
If you are modifying TZ while another thread is relying on it to calculate time, those threads are racing, and hiding the crash won't solve the race: the reading thread will now randomly return values in the wrong timezone instead, subsequent code will use it in whatever operation it is it wanted the time for, the end result will be garbage, and this will be super hard to debug because there won't be a loud obvious crash pointing to the root cause and also depending on the winner of the race the symptoms will be random/intermittent.
Fix the high level race, and suddenly you no longer need the low level mutex.
> If you are modifying TZ while another thread is relying on it to calculate time
environ is a single contiguous null-terminated segment of null-terminated key-value pairs; any change of any environment variable might reallocate it, changing the address and invalidating the old address.
Also why it's a bad idea to store the pointer returned by getenv, it might be invalidated by any environment modification.
The strings in environ is only contiguous at program start. In every libc I'm aware of, both putenv and setenv replace only the specified key-value pair (and possibly environ itself, if it needs to be larger) and should not affect the address of any other environment variables. It is still thread-unsafe, but far more limited in its unsafety.
In current glibc master, it's unsafe for any putenv/setenv to race with any getenv, even if the variable names are different, for two reasons. (Note that multiple calls to putenv/setenv are serialized by a lock, but getenv does not take the lock.)
(1) setenv resizes environ using realloc, which frees the old buffer, so getenv can end up reading from a freed array.
(2) The code does not use atomics or memory barriers, so on weakly ordered architectures, getenv could observe another thread's write to one of the pointers in the environ array, or to the environ pointer itself, while observing stale values for the memory behind it.
In both cases, getenv could end up returning a bogus pointer or just crashing.
However, those issues can be fixed without changing the API, and at least Apple's libc seems to do the right thing here. On the other hand, other libcs such as musl, FreeBSD libc, and even OpenBSD libc (!) do worse than glibc and have no locking at all.
If someone could convince the maintainers of all those libcs to add a lock and make getenv/setenv 'thread safe as long as you're not racing on the same variable name', then that would be a good starting point. But in my opinion it would still be a half-measure. We need a fully thread-safe environment.
And honestly, it might be easier to convince the maintainers to add a full solution than a half-measure, even if it involved API changes. (But it may be hard either way. Rich Felker showed up in a Rust thread a while back and was highly negative on the idea of making any changes to musl.)
IMHO - I am sympathetic to the BSDs, Apple (presumably forked BSD), and musl approach.
In what sane world would someone reasonable treat (initial shell) Environment Variables as a proper ACID complaint database? About the 'best' solution I can see for preventing segmentation faults related to resizing the env array during runtime is to defer reclaiming freed memory chunks until after all in-process threads have been given another uninterrupted timeslice to process. Even that wouldn't be 100% but probably would cover any not pathological case.
atomic ordering is very easy if you don't care about performance. So on the other hand we could ask why get/put/setenv have such a terrible need for performance that we can't afford to put a simple lock around them.
> the reading thread will now randomly return values in the wrong timezone instead, subsequent code will use it in whatever operation it is it wanted the time for, the end result will be garbage,
I really strongly disagree with how bad you seem to think this is. If you are designing your application to use the timezone and modify it at the same time, it is a totally natural consequence that you may see the previously set time zone in a timing dependent fashion. That's the nature of the beast. To "solve this" is seemingly to make that other thread capable of time travel or something. It read something before it was written, and acted on it. Reasonable!
The harmful data races are when you read intermediate results. If setting the timezone is a multi-step process, or involves manipulation on complex data structures with pointers that might be deallocated, then you are in grave danger. Seeing a previously valid result is ... I honestly don't know how you'd expect to solve it without threads being able to see the future, or some other unreasonable expectation.
To be exact, it was Chrono and time-rs 0.1, while time-rs 0.2 and later was rewritten from the scratch and didn't have that issue... because the new time-rs didn't yet support general time zones other than fixed offsets. The accepted solution for Chrono surprised me a lot, because as far as I reckon it was the hardest solution. (Disclaimer: I'm the original author of Chrono.)
But a bad API design doesn't end at environment variables. Many POSIX systems rely on `/etc/localtime` to define the system-wide time zone, and every `localtime` call has to check if the file has been changed or not because there is no way to subscribe to the system-wide time zone change event. Of course there is a cache, but many libcs call at least `stat` per each `localtime` call AFAIK. I had even experienced a possible glibc bug due to the lack of guard against I/O error during this process [1]. Windows got this right, I can't see why POSIX couldn't do the same when it does have an asynchronous signal delivery mechanism anyway.
And you are right about time-rs (or I think you are). Version 0.1 was never fixed, and version 0.3 does the OS and thread count checks.
It does have some advantage for chrono to do everything in Rust: it can now return two results for ambiguous local time during DST transition fold, and properly return None during a transition gap.
Thank you. To be frank as a first-time maintainer I did a mediocre job---my biggest regret for Chrono is that I did know most forthcoming issues beforehand and yet didn't take enough time to make them public and explicit so that someone else could prepare for the future.
> Many POSIX systems rely on `/etc/localtime` to define the system-wide time zone, and every `localtime` call has to check if the file has been changed or not because there is no way to subscribe to the system-wide time zone change event.
But you can subscribe to file change events so why not do that?
I did seriously consider inotify back in time, but in order to take advantage of inotify I had to parse all binary TZif files (because otherwise I still had to call `localtime` that would `stat` every time anyway). It was so cumbersome, that was only halfway finished when I stepped down as a maintainer. Hence my surprise when I learned that someone actually did implement all of them.
Unix was "designed" (if you can call it that) a long time before it was possible to move a running system between timezones. So many of these decisions were made in completely different circumstances (I almost said environment) and are laying around like old WWII bombs just waiting for someone to dig one up.
Offhand, and a quick google search; I was unable to find the exact definition / specification for how time-zone data must be obtained rather than how it happens to conventionally be obtained.
It is entirely reasonable that any of the following _might_ be valid behavior.
* Simple but syscall heavy approach which re-reads the env, and possibly /etc/localtime each call and has no stability. (Results may mutate as other processes / threads change things.)
* Same as above, then caches the decision result for some application specific reasonable time; which may be until the application exits.
* The elsewhere mentioned stat / inotify approaches that only track updates to /etc/localtime (and ideally update the cached decision result when notified).
All approaches seem valid. It's sort of like the hostname or any other system level configuration where a reboot may be a reasonable expectation for a complete update.
That was also my thought. To my knowledge `/etc/localtime` is the creation of Arthur David Olson, the founder of the tz database (now maintained by IANA), but his code never read `/etc/localtime` multiple times unless `TZ` environment variable was changed. Tzcode made into glibc but Ulrich Drepper changed it to not cache `/etc/localtime` when `TZ` is unset [1]; I wasn't able to locate the exact rationale, given that the commit was very ancient (1996-12) and no mailing list archive is available for this time period.
The shame of those CVE is that it created a split in the rust community between chrono and time. For a time it looked like people were all moving to time (which handling on TZ is a bit stupid IMO since it just refuses to work if there is more than one thread). But with chrono 0.4 now things are stale and there is no clear winner anymore.
I would argue that those splits are in great part responsible for the feeling that rust is hard to learn. I remember to have had to dig into pretty complex time code to understand why it broke our program that relied on timezone when we switched from chrono to time. It hinders your productivity for sure even if you learn the how.
I can think of using libraries, that get their config through environment variables. So you start your program, modify environment, and then start up the rest.
No, not really. You never know when the library reads the enviroment variables. But i can make sure, that the parts under my control do not modify them after start.
"The returned string pointer might be invalidated or the string content might be overwritten by a subsequent call to getenv()"
You don't even need threads for this to be unsafe; another call to the same function may invalidate earlier-gotten pointers. I don't see how to interpret this as anything but broken.
Now, was memory scarce? The PDP-11 it was designed for could have megabytes of RAM, but I wouldn’t know what the smallest machines this ran on had, and of course, those systems were multi-user.
So, maybe, thisxwas a good choice, but it also could be an early example of using a small system, hacker mindset on machines that didn’t need that anymore.
The text implies that any second call to getenv invalidates the returned value of any prior call to getenv; even without threads, without calling setenv, a second call to the function invalidates the return from an earlier call. I re-affirm my conclusion, without qualification: BROKEN.
Come on! Of all C/Posix thread-safety issues in circulation this can plausibly be considered the most moot one.
Environment variables are not meant to be an inter-thread communication channel and the documentation that points out setenv() is not thread-safe is very much a fair shot.
You rarely, if ever, need to setenv() anything maybe unless you're a shell. For spawning children execve() already takes an envp parameter. For debugging I think I've mostly set in-process environment variables manually from gdb.
Further, because environment variables are an interface between the process and its environment you typically read environment variables at start and cache the parsed values in some internal location. If you need to change that global state on the go you should do it using your own internal variables instead of recycling it through the environment and having the program threads repeatedly getenv() the updated values.
How often do I need to `setenv()` anything? The answer is "Never" in the vast majority of programs, because ENVVRS are usually read rather than set, so this issue is nonexistent for them.
For the vast majority of the small amount of programs that actually need to use `setenv()`, the answer is: "Maybe once or twice during the entire lifetime of the process, and then only at the very start, probably even before running any threads", meaning this issue is nonexistent for them as well.
So, is there a potential issue with thread safetey? Yes. Does it matter given where and under what circumstances it occurs? Not really.
What kind of actual real life production code would continuously set envvars while simutaneously calling a function that tries to read the environment?
Yes, this is a footgun. But even the issues author acknowledges, in the issue thread:
Realistically: this is a pretty rare problem, and documenting
it is probably a fine solution. This is probably going to cost
someone else a couple of days of debugging every couple of
years
> It has wasted thousands of hours of people's time, either debugging the problems, or debating what to do about it.
Unpopular opinion: Neither Go's os.Setenv nor Rust's std::env::set_var() should exist. I was pleased to find that Java only has System.getEnv(), but not a setter.
That is an unpopular opinion for the simple reason that some programs do in fact need to set envvars, particularly programs that will start child processes.
That is still possible using the java.lang.ProcessBuilder API: you can launch a child process and give it a modified environment, but just at launch time. This side-steps the issue.
Nope, execve() and friends ending in 'e' accept a pointer to a completely new set of environment variables, no need to do setenv. Windows has _execve() too.
I think the probably is really that there are 2 times where you should be setting env vars 99% of the time.
1. Right after program startup before any threads are spawned.
2. After a fork before an exec.
In both cases it can be known that no threads are running. (Ok, for 1 it can actually be non-trivial if you have code before main or if you call functions that spawn helper threads, but let's assume that you can know this).
However no languages actually have ways to enforce this. So the APIs can be called at any time and are huge footguns.
I think that the proposed improvement of `getenv_s` is great. It is cheap and easy to use, then software can slowly migrate off of the less safe stuff. You can imagine that if libc stopped using `getenv` internally most of this problem would be solved.
No, in many cases one needs to set them interactively.
Consider for instance something as simple a implementing a shell. Such a program needs to be able to set the environment based on user interaction and this change needs to show up in /proc/$pid/env.
Because the specification of the POSIX shell says that `export` changes the current environment of the running process, not just of any newly started processes.
This is useful to recognize various processes I suppose. I have written code that scans the environment of processes to find particular processes and group them together.
I think there are good reasons for Setenv and set_var to exist, but if they are implemented, they shouldn't be wrappers around POSIX' shitty API and implement their own environment variable system instead (one of which the initial variables are possibly initialised by a call to getenv to make them compatible).
There's no reason why these languages need to restrict themselves the same way C does.
The bug in Golang was because DNS lookups interact with the C library, which looks up environment variables. As long as everything happens in Goland, there is no problem - but this is simply not good enough.
Go makes the assumption that the DNS lookups are thread-safe, but it doesn't have that guarantee (or the C library is spec-incompliant, but I doubt that). It's still something Go can fix.
You can't fix C libraries loaded into Go programs (i.e. and external library calling C's setenv, or I suppose explicit FFI calls by the user), but Go can be responsible for the APIs it calls itself. That may necessitate writing a thread-safe alternative for DNS lookups, or documenting and/or adding compile time warnings that threaded programs doing DNS lookups will just crash sometimes, but the language's standard library can still make it much harder for developers to write buggy code.
My impression is that this was Golang's plan from the start - this is why they didn't want to use the C stdlib at all, issuing the Kernel syscalls directly from the Golang runtime. A good idea, but then they had to backpedal to solve issues such as DNS resolution respecting certain OS settings, and this bug is a symptom of that.
Yes, there are certain things in UNIX which _are_ part of the standard (POSIX / IEEE1003) but _aren't_ usually implemented as system calls.
Name lookups (whether user identities or network resources) are the biggest chunk of these. You have a "choice" as a user/programmer here. Say, the existing name lookup interfaces in most libc implementations don't do DNS-over-HTTP (DoH); you can implement that yourself and just use the addresses returned by your library/package where the system calls ... want addresses.
If you have the go stance, go all the way. Don't say "the C runtime is sh*te but I really really really want that one particular teensy tiny bit of it could someone somewhere somehow please do something to make it a little less sh*te". Legacy baggage is a burden and backwards compatibility shackles you. The C/Unix interfaces are full of this, and with the hindsight of 50 years noone today, not even "C programmers", would implement them all the same way again. But that doesn't mean their behaviour can be arbitrarily changed.
> Go makes the assumption that the DNS lookups are thread-safe
DNS functions are thread-safe.
The thing people aren't understanding here is when you set loose nasal demons (such as by calling `setenv` in a multithreaded program), they can cause problems even in safe code.
If a function is safe only if everyone else you rely on never calls a particular function, it's not that safe. Certainly less safe than other functions guaranteed not to result in crashes if you use them right.
That doesn't fix the problem: these languages has to be able to coexist peacefully with C in the same address space. You can have a dynamically linked library written in Rust in a host program written in C, you can use C libraries in Go, etc.
Even if that wasn't an issue: this is a bug in C as well! You should absolutely be able to use setenv/getenv safely in multi-threaded C, it's insanity that you can't.
Programmers have to deal with a lot of badly written programs all of the time. You'd need this functionality to either debug a program that responds differently to different values of environment variables, or to control it, because, maybe it's the only reasonable way to do so.
It's OK to say that programmers shouldn't rely on this functionality ideally, but, for practical reasons, this functionality is needed. Same happens in "pure" functional languages, for example, when you need to debug programs in such languages interactively, and struggle to create the program state that reproduces the problem, or, in some extreme cases, due to I/O being "impure" even struggle to output diagnostic information.
People don't like APIs that can randomly crash your program while there's no good technical reason for why they should. Why not fix the problem? People like you, who have no issues with the current implementation, won't see any regressions because you're already a good citizen, and myriad other programmers whose programs do occasionally crash because of this will be helped.
> So, is there a potential issue with thread safetey? Yes. Does it matter given where and under what circumstances it occurs? Not really.
"The unpredictable crashes only happen very rarely" doesn't mean the crashes go away.
> What kind of actual real life production code would continuously set envvars while simutaneously calling a function that tries to read the environment?
The reproduction sample calls setenv in a loop so the issue can be reproduced. A single setenv anywhere in the code is enough to trigger the crash, but then you would get one of those "you need to run the program a million times to reproduce it" bug reports that gets pushed down the line.
Because doing so breaks backwards compatibility, simple as that.
The problem isn't even that `setenv` isn't thread save. The problem is that `getenv` returns a `*char` directly into the environment memory space. Many many many programs rely on that being the case.
> People like you
People like me would like every software to be perfect, but that's not the world we live in, so we are forced to be pragmatic. When fixing something causes more problems by breaking backwards compatibility promises, than it prevents, then there is no good argument for a fix, and the correct approach is to say "yes, this sucks, let's document it well so people don't waste too much time on this".
The setenv/getenv problem is such a case. Anyone who disagrees is free to fork glibc, implement whatever fix they think is adequate, and then try to compile the software packages found on a typical Linux server against the result.
> so the issue can be reproduced.
"Can be reproduced" and "is a common issue in production code" are not the same.
Fact is, almost all production programs that set envvars, do so once, very early in the process lifecycle, and then never again, and so are never affected by this.
So why not implement the fix suggested in the article: improve the existing interface to the extent possible, and introduce a new interface which is easier to use correctly.
There is nothing to "improve" on the existing interface, really. From a C point of view ... a _hidden_ global lock is worse than no lock at all. Because in the latter case ... you, as the programmer, have a choice what to do. If you never call setenv(), no locks. If you only ever call setenv() in your startup code, no locks. If you only ever call setenv() after fork&co, no locks. And if you do believe you need to call it at runtime, but are singlethreaded ... still no locks. And if you really really really need to call it from a multithreaded process, concurrently with getenv(), then lock around both and make your getenv() "safe" wrapper create you an owned point-in-time copy - basically a getenv_r().
Note also that "global references" like getenv() returns and point-in-time owned snapshots don't behave the same way. Say, a library initializer code could retrieve a number of env var references by calling getenv(), and then use those at runtime. No more need/use for getenv() again after - and even perf-sensitive code could look at the env var. With a func that copies, the perf-sensitive code would need to do that each time (lock, lookup, copy). Not strongly desirable.
Also ... UNIX is rather flexible ... and if you so wish, you _can_ substitute _your own_ setenv()/getenv() by the magic of dynamic linking. To create a set that locks and returns you leaked copies (changes the semantics of getenv so that the caller must free the pointer to avoid a leak). It's all possible to do this.
I'm getting the impression from this that we see a "go tantrum" here. "I make my own standards but I wanna use that C/Unix standard thing as well but not how it is because it's not nice it should take go into account waaaahwaaah ...".
It is not _nice_ to modify your own env at runtime. Maybe, just maybe ... that's for reasons. Because not everything that can be done is also a great idea.
The real skinny of it is that it’s in the name: “Environment”.
If you’re calling setenv in the middle of your program, you fucked up.
There are those things in programming that should be extremely triggering to your “what the actual fuck?!” senses, and “setenv in the middle of runtime” is one of those things.
True, but for every envvar a program reads, something called setenv on it originally. It’s not like no programs call setenv in the middle of runtime. Examples:
The child process's environment for these purposes is constructed without mutating its parent's environment - a copy is used - and before the child process actually runs the target code it was created to run. So there is no possibility of race between mutations to the environment and reads of the environment. If you are writing such a tool but doing something other than this, you are doing it wrong.
> but for every envvar a program reads, something called setenv on it originally
That's not true, that's just misunderstanding how it works. `execve()` takes an entirely new copy of environment variables to give to the child, that's the "real" way to do it.
No, a process gets its environment variables from the operating system (just like argc, argv) before any code is ever executed and the majority never change them.
It says loud and clear "The setenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."
> "The unpredictable crashes only happen very rarely" doesn't mean the crashes go away.
If you get a crash over setenv() reading the manual page of setenv C call should be your first step. And the only step. The bigger issue is in design of application that has wrongly assumed setenv() is thread-safe. That requires a refactoring and is solely due to developer misunderstanding the API.
Not being re-entrant makes the user-facing API unnecessarily complicated. It creates an avoidable foot gun. It trips people up for no good reason. And unlike stuff like signed integer overflow, there doesn’t even seem to be a (dubious) performance argument to justify this insanity.
The standard should be fixed and that’s the end of it.
I'm a UNIX/C programmer for decades and I don't care about this.
There is no such thing as beautiful API design. Every design is a compromise. If you think non-reentrant calls should be deprecated in POSIX take it to the committee.
There is a myriad of non-reentrant code both in POSIX spec and in libc implemenations. You need to RTFM, I'm sorry.
There is no "coherent API" as far as null termination goes too. Some library functions deal with it, some calls don't. You need to RTFM.
I also want to know OP's reason to even use setenv() in a multithreaded piece of software. It's like an oxymoron. setenv and vars are useful to pass on data from parent process to forked children because they inherit the environment. If you use the threading model you don't need it. If your application is a single process setenv() is useless.
Putting aside whether or not the design is awful, the fact that it's standardized and documented is absolutely a valid argument. Changing it now would break backward compatibility. That should always be a showstopper.
Programmers who are using any library code without reading and understanding the documentation are asking for trouble regardless of language.
The correct solution to your objections is to create new functions that behave as you prefer.
While I am sure that thousands of hours have been spent debugging threaded setenv() attempts (and developing & discarding Annex K), it is clearly not a problem that needs a solution.
Languages that compile to C need be careful not to promise thread-safe implementations of POSIX or C functions that are explicitly documented as not reliably thread-safe, including setenv(). The author seems to want to change C, and POSIX, so that Go can reliably do so.
You are literally putting forth arguments in favour of fixing the thread safety issue, and then conclude it’s not worth the effort.
It’s simple, really: we indeed rarely to `setenv()`. So it’s not a performance problem. So we can make it thread safe, and the performance impact will be negligible. In exchange for this small price, safety will increase.
Sacrificing any amount of safety for a negligible improvement in performance is flat out unprofessional, and should be grounds for immediate termination in most contexts.
How do you propose making it thread-safe? The real problem here is that `getenv()` was designed around it returning a `char *` into some read-only memory. It's a bad API if the backing data can change because the returned pointer is assumed to exist 'forever'.
`setenv()` has no way to knowing where those pointers are floating around so there's no way to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse).
Here's my proposal: Introduce a new threadsafe API (`tgetenv` or whatever) which takes _two_ `char *`s, one of which is a return buffer. This leaves allocation as a responsibility of the caller.
And then you can leave the existing syscalls as they are (thread unsafe) while having a separate thread safe version.
I agree that would be the way to do it, but now we're no longer talking about simply 'fixing' the implementation of the existing API but rather introducing a new function you have to use.
`setenv()` would only be safe if your program never uses `getenv()`, and calls to `getenv()` are so numerous and all over the place that for most non-trivial programs it would be hard to ensure they never happen.
There's also the rub that `setenv()` is not part of the C standard, it's POSIX. I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have, so non-POSIX code would have to continue to call `getenv()` since that's all that is available to them.
> I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have
The C standard has no problem acknowledging that getenv is subject to data races for most of its implementations. As far as I can tell that part was even added at the same time as threading support.
That doesn't really help you determine whether a given library is using `getenv()` or not. That also requires that things are actually recompiled/updated, which for some C libraries is not that often.
There's also the rub that many C programs do not target the latest standard (for a variety of reasons). I didn't realize `getenv_s` was added in C11 (though it's optional), but it doesn't really matter because programs/libraries that target C89 or C99 can't use it anyway.
That’s a good point. I would guess there are ways of doing static analysis to see if a given binary is making getenv() calls though, even if one doesn’t fully grok its source.
Maybe some combo of that with sentenv() in your source or something
Or do “live” analysis under integration and give a low priority warning
Yeah. Most of my code these days works like this, to the point where almost all of my allocation happens roughly in my main entry-point. Or I use arena allocators and pass those into my utilities. But there's basically never a `realloc` or `malloc` call deep in my code anymore.
> to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse).
Arguably worse? My goodness no.
This is a rare edge case that most programs don’t encounter. Option 1 is to crash and explode and die. Option 2 is to leak tens of bytes.
Leaking tens of bytes is for sure NOT worse than crashing.
> Leaking tens of bytes is for sure NOT worse than crashing.
I do really disagree here. The answer is not clear at all.
But then, you are mischaracterizing the problem. The issue is not with crashing, you can get plain bad data too, and this is clearly worse than both leaking memory and crashing.
Also, the GP is mischaracterizing the options. You don't need to leave the old values around, you can just copy them into userspace memory.
My reasoning is simple - the issues here can be avoided if you're careful about how you use `setenv()` and `getenv()`, which many programs already are. The memory leak in contrast would never be avoidable regardless of how you use it.
The problem with “be careful” is that libraries often want to use the very unsafe API and there is no standard mechanism to expose safety. It’s fundamentally a bad API design. It could be good. But it is not.
There’s a reason this problem comes up on HN once or two a year. And don’t even get me started about printf grabbing a mutex for a stupid locale…
You could even put a note in the man page that `setenv()` will leak memory. Then ten or twenty years from now there will be a blog post about how a currently trendy language/runtime can be manipulated into looping over `setenv()` zillions of times and OOM'ing, and comments about how no one can possibly be expected to read the man page for this horrible footgun, and it's wrong to expect developers to have any idea about what they're doing, give a shit, or pay attention at all.
This is not a good argument imo. Its “rarity” still affects a tremendous number of folks in profoundly vexing ways that are difficult to debug on account of this not only affecting C but innumerable other languages’ compilers and interpreters that rely on the stock getenv implementation.
I wouldn’t be surprised if a good chunk of compilers and interpreters in other languages suffer from this gotcha’.
I mean, I wouldn’t even be surprised if some JVM implementations silently expose their users to bugs on account of this implementation.
> You are literally putting forth arguments in favour of fixing the thread safety issue, and then conclude it’s not worth the effort.
Yes. I do. These two concepts don't contradict each other.
> No it’s not a performance problem. So we can make it thread safe, and the performance impact will be negligible
Who said anything about performance being the problem, or a reason not to change it!?
The problem is BACKWARDS COMPATIBILITY. The issue is that `getenv` returns a `*char` into the envvar array. Basically every application that uses this function relies on this fact.
So we have:
a) A potential issue that occurs only in very unusual circumstances, most of which will never occur in production code and on the odd chance that they do, they can easily be avoided. Documenting that well can help prevent time wasted in debugging.
b) A fix that may prevent a) but breaks backwards compatibility promises, and would necessitate reworking god knows how many programs, the vast majority of which were never impacted by the issue in the first place.
Of these 2 options, a) is just the better one. Yes, in an idea world, we could have pure, 100% bug free code, and spend an unlimited amount of time on fixing every last problem. That's not the world we live in however, and so a pragmatic approach is simply a necessity.
Ooh, the old 'unprofessional' epithet! What do you mean by that slur here? Most can't agree on what professional even means. Additionally, why should one be held to artificial, inconsistent, and poorly defined standards of 'professionalism' when they aren't a professional?
> Ooh, the old 'unprofessional' epithet! What do you mean by that slur here?
For an action I generally mean "malpractice". Something bad enough to bar repeat offenders from the profession (if we even were a profession, which we’re not). For a person I mean "unfit to program code other people rely on".
> My care for code robustness scales with income.
Good point: the conditions for us to write code that actually works are too rarely met. The only answer I have for this one is political though, not technical.
Right, why is the problem "changing memory in a shared memory execution model will cause corruption" and not "Why are we using such a fragile shared memory execution model in the first place."
I mean, the environment is just a chunk of memory made available to the process by the OS. It is no more, no less thread safe than any other chunk of memory.
Why would the libc need to protect it more than any other memory location?
Changing any kind of global state is fundamentally not thread safe.
Sure you could use locks, stop the world, etc, but there is no way you can ensure that all the data and information you had derived from the old state is going to be valid.
A better solution is to not rely on global state like this.
What is the use case for a mutable environment past initialization?
It seems like a complicated and error prone thing to be using no matter if it is thread safe or not. You can set up your own environment before you launch threads, and you can launch child processes with a different environment from the current process without modifying your own. If you fork, you can modify the environment in the child without affecting the parent until you exec.
And even setenv() if it was reentrant and couldn't cause crashes, it wouldn't be thread safe, since threads share the environment and could get their environment changed under their feet.
>We should apparently read every function's specification carefully, not use software written by others, and not use threads. These are unrealistic assumptions in modern software.
The first of the three listed items you should certainly do. I hope this author is not writing medical software, or anything important.
I am trying to make sense of the argument of pushing configuration into a library:
* if the library is just a dependency, the Linux loader will set it up. It will have the same environment as the other libraries and as the main program.
* if the library is set up by dlopen(), there is no way to provide an environment pointer
Altering the global environment variable for child processes makes no sense, for
execve()
accepts an
char* envp[]
. So I guess we need to talk about issues with a specific use case of
Maybe the dlopen issue could be hacked around by dlmopen and injecting getenv and setenv symbols that access a different environment variable list than the application's.
Everytime I hear someone lament about something not being thread safe, what I actually hear is - I want this shared invariant global state to be modifiable but it isn't. Which makes me ask the question why would you want that ?
The process environment should not be the mechanism for threads to communicate with each other.
>The argument is that the specification clearly documents that setenv() cannot be used with threads. Therefore, if someone does this, the crashes are their fault.
the env is unix. It's not C's job to fix. Keep studying it till you understand that.
and don't forget, you are using unix because it defeated all the other options, because it was better and they were worse, so also keep studying till you understand why that is too.
then this problem with env will fix itself.
unix gives you tools to handle threads. C gives you tools to handle threads. Learn them, use them.
On Windows you can use GetEnvironmentVariable/SetEnvironmentVariable (on XP and later), which do implement some locking and doesn't run into this issue because GetEnvironmentVariable copies the data out into a caller-supplied buffer. getenv_s was a nice effort, but it failed.
I don't really understand why other languages such as Go and Rust decided to call the weird POSIX API rather than implementing their own API, which matches the semantics they expect. In cross platform C you'll be stuck with the outdated POSIX API design, but there's no reason why other languages should accept those same limitations.
We're not running on PDP-11s anymore. You can afford a thread-safe hash map in your standard library. Ignore the limitations of the old C library. Twenty years ago, Microsoft released a better API, keep the crashy old API with tons of deprecation warnings (hell, add a compiler flag --enable-broken-c-api-designs) and just provide new APIs that are actually usable in modern programming environments.
Go likes to ignore edge cases ("all file names are UTF-8 and if they aren't then we'll just pretend they are") to make it easier to write code, so I'm not very surprised that it got caught in a POSIX related crash here.
It's hard to tell if Microsoft altered the source code since, but the leaked XP source code (https://github.com/tongzx/nt5src/blob/master/Source/XPSP1/NT...) doesn't seem to do any getenv() calls for DNS lookups. The specific bug that started all this nonsense only triggers on (specific) Unix implementations. Unfortunately, Go opts to call the POSIX methods rather than GetEnvironmentVariable/SetEnvironmentVariable on Windows, so I suppose it's still possible that somewhere in the chain this bug gets triggered by Go code.
I'm saying that on UNIX, the C standard library provides the OS API, while on Linux the OS API can be accessed directly via syscall numbers (which are stable). But on Linux `getenv()` is still not an OS API, it's provided by libc (musl or glibc, both provide POSIX APIs as well as the ISO C standard library functions and some other extensions and syscall wrappers).
> On non-UNIX platforms stuff like getenv() belongs to the specific compiler C library, not the OS API, hence why Windows doesn't use it.
Linux isn't a UNIX, and non-POSIX compliant Linux distributions exist. getenv() is still part of the C library, not the OS API, yet Linux distributions still use it. So that's not the only reason why Windows doesn't use it. It's more because Windows design wasn't originally POSIX-compatible (some POSIX wrappers got added with Windows NT) and MS designed their own API.
Once you start to provide C interoperability, it is inevitable because C programs still rely on that broken API and many users would expect that Rust will give the same time zone as C. And with an exception of Windows, that API is often the single existing API throughout the entire system.
Rust's stdlib's API is completely safe here. On Windows[1], it uses the GetEnvironmentVariable/SetEnvironmentVariable API, which as you noted doesn't have this problem. On Unix[2], it maintains its own RwLock to provide synchronisation. Additionally, Rust's API only gives out copies of the data, it never gives you a pointer to the original.
The problem comes when you do FFI on *nix systems, because those foreign functions may start making unsynchronised calls to getenv/setenv.
There are two level of pointers: the environment block points to an array of pointers to C strings, this higher-level pointer can be updated and the previous one freed, which is a problem when it is being iterated on (which getenv does). The C strings themselves aren't freed by glibc, though some applications do modify them in place.
OIC, so because glibc doesn't hold the setenv lock during lookup a concurrent getenv can break, but once getenv has found an entry that entry is guaranteed to be valid (unless the application plays silly bugger) because setenv will not free the individual entries.
Hot take: if a crash can be made to disappear just by adding a mutex inside setenv(), this means code reading the environment is racing with code writing to it, and in this situation adding a mutex inside setenv() will generally make things worse instead of better: it may hide the immediate symptom, but the underlying race between reading and writing to the environment remains, your program will behave differently each run depending on who wins the race (with potentially catastrophic results depending on what the writes are doing), and the cause will be much harder to debug from cold due to the lack of a smoking gun pointing at environment manipulation.
The multithreaded program needs to be restructured so that the parts that communicate via the environment are properly serialised with respect to each other, just as would be needed for any other communication via global state and/or access to a shared resource.
This has to happen at a higher level than the individual getenv/setenv calls: entire blocks of logic containing the calls need to be made atomic (or otherwise refactored; perhaps you could do all the environment writes before spawning any threads) so that no other thread can blow away the environment contents in between the code that sets it up for some purpose and the code that implements that purpose; and once this is properly done, the individual calls themselves do not need further protection.
Sure, some applications might require custom higher-level synchronisation, but it's still important for getenv/setenv to be thread-safe (i.e. not crash):
- The race might be irrelevant (e.g. simultaneous calls that access different variables are fine).
- The application author might not have complete control over all calls to getenv/setenv (e.g. if using a third-party library).
336 comments
[ 3.1 ms ] story [ 245 ms ] threadAn environment variable's value, for a running process, is just what it is: an initial value from outside.
Adding complexity around it smells like an attempt to control a distributed mutex, like checking an API for real-time value changes in a while loop across several instances of the same app.
I thought there would be alternatives to this, like pubsub, Kafka, or other asynchronous event handling.
Imagine having to test an app for its ability to handle safe read-write of OS-level state. It's definitionally bankrupt: not really a unit, not easy to set up quickly, and not isolated.
Also it should be able to handle invariants as modifying multiple variables is not an atomic process, either.
Then calling setenv() from anywhere except in the time between a fork() and exec() call should be banned, but it's not. Honestly, calling abort() if setenv is called in the presence of threads would seem like a better status-quo than what we have today.
This is expected behavior for setting a GLOBAL variable without a lock on the memoryspace..
You may have a mutex on getenv/setenv, like the Rust stdlib does, but when libc doesn't look at that mutex, even on the read side, you run into UB.
So the next step is never calling into seemingly innocent libc functions in safe code (which you have to enforce on your dependencies as well), implementing safe alternatives to a good chunk of libc (and making sure your dependencies use those), to cordon off anything that looks at the environment. This makes a good chunk of POSIX functionality useless.
Inserting a mutex into the setenv/getenv/etc. functions is pointless because applications are explicitly allowed to modify the environ pointer and array directly without any locking.
The world really needs a new C standard library that doesn't suck.
Is it, though?
The only argument I see is that an API can be misused. There are ergonomics debates we can have about it, but a user intentionally abusing an API wanting to do something that's completely wrong is hardly indication the API is broken.
A good API would probably only allow constant access. If mutation is for some reason deemed necessary then it should be through a separate set API and the return results of get should be guaranteed safe.
Please show me from which book you got the idea that env variables are expected to change throughout the lifetime of a process.
I think most people argue about that. Just because it exists doesn't mean it should be used imho.
I've used it exactly once, and that was a school exercise where I had to write a Posix shell (most of a posix shell actually), including built-ins. I do not see another use case tbh.
It can and should be used in the cases where it makes sense, with the restrictions that are documented. It's an API that is fundamentally not thread-safe, you can not use it "safely" (in the modern sense of using it after a lobotomy, in any way that the compiler allows) in a multi-threaded context.
There are other such APIs, and if those APIs were removed it would hurt a lot of old software that is running perfectly fine.
POSIX specifies two functions that alter environment variables. It could've specified that env variables are supposed to be mapped into a read-only memory page where available to indicate that they shouldn't be altered, but it didn't, and instead provided an explicit read/write system.
The same POSIX spec you're citing also states in no ambiguous terms that setenv is not thread-safe.
It's pointless to quote a section of a spec to try to justify failing to comply with the very same section of the very same spec.
The spec is the problem in my opinion, you can't implement it in a way that doesn't introduce footguns.
As with most things programming related, “it depends”.
Functional programming has really, truly done untold and massive damage to the industry. Fortunately, you are free to unpoison your mind. I just hope people eventually do.
by that logic mutex themselves are pointless because nothing ever forces you to use them, even in memory-safe languages you can still access /dev/mem and change bytes? It's stil a useful thing to have.
Although I guess a middle ground solution wouldn't be too bad either - most programs don't modify environ directly, so POSIX could offer thread safety for the functions and make multithreading through "environ" UB. This is already kind of explained in the standard:
https://pubs.opengroup.org/onlinepubs/9699919799.2018edition...
they just have to fix the standard. e.g. in my country they manage to improve for instance the standard for electrical plugs every three years, there is NO REASON posix cannot do the same
If we are talking about the C application deciding when it wants to rescan the environment that is something different, but if your environment can change potentially before and after you check it this opens you up for a heap of new attacks.
I would personally go for the aggressive approach (release a new major version of libc that detects multithreaded environments and intentionally crashes out when calling setenv() so people actually notice and fix their broken programs) but I suspect not many people will agree with me on that.
The API is not necessarily bad (it's just very 80s UNIX), but the lack of enforcement of thread-safety causing all kinds of bugs and crashes.
If you copy, provide a new interface. It's time-honoured and proven in Unix to give *_r() ones in such a case.
A proper solution would be to either nuke put/setenv() in the C standard library or redesign the *env() calls entirely, but that would break existing programs.
Most programs using setenv call it before starting any threads. That is not broken.
Detecting the linkage of thread support and crashing that program on purpose is, frankly, a pathological way to fix a non-broken program.
Besides which, your proposal won't work anyway, because this remains a potential problem in single threaded programs anyway: a program calling getenv, storing the result, and then calling setenv on the same variable and using the previously stored result will break anyway.
In summary, your proposal is broken in two different ways: 1) it breaks well-defined programs, and 2) it fails to break broken programs.
You're right that putenv/setenv are also horribly broken in other ways, and doing multi thread detection doesn't prevent those problems. In a perfect world we would just kill off these two functions all together, replacing them with either crashes or no-ops, but that'd be an even harder sell.
That still breaks well-defined, non-broken programs which don't call getenv/setenv in racing ways. There is no way for you do a conditional-upon-threads mechanism without false positives.
> You're right that putenv/setenv are also horribly broken in other ways, and doing multi thread detection doesn't prevent those problems. In a perfect world we would just kill off these two functions all together, replacing them with either crashes or no-ops, but that'd be an even harder sell.
But you don't need to in order to meet your original goal - breaking programs which do call setenv/getenv in the wrong order. Proposing to remove them altogether doesn't fulfill the goal of finding the breakages immediately and introduces breakages in existing programs.
My alternative: use LD_PRELOAD and provide alternative setenv/getenv functions which raise SIGSEGV when setenv is called on a variable more than once, and when getenv is called on a variable that was already setted once. It requires nothing more than a counter for each of setenv/getenv per variable.
That finds programs which actually are broken, with no false positives, and ignores threads altogether because they don't matter under the counter system[1].
Best of all, you can implement this in an afternoon, without needing to modify glibc, and then test it with every single executable on your system to see which ones break.[2]
[1] Since the caller knows they are not thread-safe anyway, we aren't looking for the error where the caller calls setenv concurrently in different threads. That's a different problem.
[2] I would wager good money that few, if any systems, will break under this test.
_That_ is the issue. You can only solve that if you change the interface. Make a new one, getenv_r(), have it _copy_ the env var value into a user-provided, user-owned buffer. In that case, you can then assure the returned value is both point-in-time correct and immutable. You can never achieve that with getenv() because if you copy/make the returned pointer owned, the owner needs to free it. which is a break from the current behaviour and so not backwards-compatible ... and hence out of the question.
Lamenting about how broken the interfaces might be and then insisting that the implementation should be fixed is ... "conveniently shortsighted". Not saying this isn't worth fixing, but fix it the right way in the right place.
Wrong. As I've pointed out several times in this thread and in other recent threads about getenv(), Solaris/Illumos has an implementation that is lock-less to read (except when you change `_environ`, then it takes a lock at most once until the next time you change `_environ`). It's made safe by "leaking", and by locking in the functions that write. It's only unsafe if you replace the value of `_environ` repeatedly and free the old settings (which I've never seen any code do, and which if you do then you get what you deserve).
Yes, it can be done and has been done. glibc has no excuse.
[1] https://news.ycombinator.com/item?id=37908655 [2] https://news.ycombinator.com/item?id=37952916 [3] https://github.com/apple-open-source-mirror/Libc/blob/master...
The usual case for modifying a global state is: modify once, then proceed (e.g. start new threads). Even if all the calls become thread safe, the behavior would be inconsistent, still.
However, this only makes sense for other people's software crashing on getenv() related memory bugs. If you control the software, you can simply prevent the setenv() call yourself. No need to LD_PRELOAD anything, just load a library or write your own hooking code to work around the POSIX madness.
Widely used, yes. Used as in read. Why do any of these need to change at runtime? And if they do - why are they environment variables?
(NB: starting a new process is not "at runtime")
Other than that, it can also be handy in k8s with a VPA. You get more/less memory and then update the env to reflect that. Your service picks up the env change and updates the runtime.
IIRC, there is/was some way to listen to those changes in C#, and automatically update runtime settings.
You… can't change the env from outside the process…
are you saying this is used by disjoint components within a single process? Or is this just a misunderstanding?
But you only need access to the /proc/pid directory to change another processes env.
/proc/$pid/environ is not writable
(and as a matter of fact, due to how the environment works, it cannot be writable.)
Not with that attitude you can't.
(OK, without the joke: you can do this with an interactive debugger. But I think OP just meant "change it in the container and then restart the child process")
Most debuggers nowadays support altering variables at runtime after hitting breakpoints. In the meantime this was the very first time I ever heard anyone even considering changing env vars at runtime, let alone use it to debug stuff. Sounds like an ass-backwards way of going about debugging.
Wait, why would this need to happen at runtime? I have used env cars a lot to trigger specific cases but why would you want to do this while the process is running from within the process itself?
If you control the process you can start it with the right env to begin with, no?
Edit: I just realized macOS/FreeBSD has execvP() that allows passing a custom search path, so PATH is now safe, but without a -e variant, everything else is again unsafe.
In a program you could have either.
Sort of. https://pubs.opengroup.org/onlinepubs/9699919799/functions/g...:
“The returned string pointer might be invalidated or the string content might be overwritten by a subsequent call to getenv()”
There’s little you can do with a broken API, so Linux has that ‘feature’, too. https://man7.org/linux/man-pages/man3/getenv.3.html:
“The string pointed to by the return value of getenv() may be statically allocated, and can be modified by a subsequent call to getenv(), putenv(3), setenv(3), or unsetenv(3).”
FreeBSD chooses to leak memory, instead. https://man.freebsd.org/cgi/man.cgi?getenv(3):
“Successive calls to setenv() that assign a larger-sized value than any previous value to the same name will result in a memory leak. The FreeBSD semantics for this function (namely, that the contents of value are copied and that old values remain accessible indefinitely) make this bug unavoidable”
So, shells use a single thread that can safely modify the environment - then start new child processes by the same thread. The child processes get a =copy= of the said environment. That's a textbook example how to use env.
Starting multiple threads on your own, then modifying env should be considered a textbook example how not to do things - env is not intended for interprocess communication.
(Edit: removed unneeded pointing out execve)
Also shells generally have their own program search anyway since they need to support built-in commands. It's not particularly hard to implement PATH search.
As for I’m not aware of execve etc… You need to re-read my comment which clearly mentions execve, execvep, posix_spawn, as well as implementing PATH search on your own.
I am the OP and your assumption is incorrect. You may consider why the post ends with:
Also, few weeks ago I found a use for them when trying to pass configuration from Java/Kotlin to C++ library to be used during static constructors (invoked during dlopen) on Android, because at that phase native code cannot call back to JVM.
library has already loaded when you call setenv, so what you're saying doesn't work in most cases.
It seems to be a need to use poorly written libraries. You might consider fixing them instead.
This issue with that “interface” is the environment is process global. If the library is being loaded dynamically (specifically for some task) it would seem that the parameters are local to that task and should be taken by some reentrent init method. Alternatively, the process could be forked and environment set in the child without concern for thread safety or polluting the environment (think of the children!).
The only exception is things like debug logging, which is unlikely even to work dynamically.
On the other hand, setenv() is clearly broken in modern code, particularly in a library context, and the man page (at least on my Linux machine) does not make that particularly obvious -- "Thread safety: MT-Unsafe" is the only note, with a reference to attributes(7) for more information. It could definitely be made more obvious.
If you want to pass secrets into a process at startup, I would strongly recommend passing a pipe as an additional open file descriptor (e.g. fd #4, but this FD number you can then put in an env variable) and writing it onto the pipe. It can only be read once, and you can control where the value propagates.
I mean YES you can factor your code (tests, whatever) to make this a non-issue but supposing some person wrote some code 10 years ago in an OSS project or on your team and you start banging into this issue.
It’s not going to be trivial to unwind let alone find the root issue.
Let’s start fixing things like this for our future selves, right?
Digging heels in and saying “eh, you just got to learn this one weird quirk.. oh yeah this other one too..” is kind of a fun glass bead game until it’s not; as is not a winnning way to endear hearts and minds.
CVE-2020-26235, RUSTSEC-2020-0071 and RUSTSEC-2020-0159 where opened against the crates. That left the Rust ecosystem with a pretty much unsolvable issue for many months. Chrono went with the solution to parse the timezone database of the OS natively and read the environment using the Rust locks. Time tries to detect if the libc version has thread-safety guarantees to access the environment, and otherwise panics if there are multiple threads.
More reading: https://docs.rs/chrono/latest/chrono/#security-advisories
There's your problem right there, and it ain't the behavior specified in the standard.
But libraries and users are caught in the middle.
Which just to be clear: it cannot without changing the standard. There is really nothing anyone can do without a change in the standard.
"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."
https://doc.rust-lang.org/std/env/fn.set_var.html
Ultimately the problem here is with Posix. Rust can only do so much to paper over the pitfalls in the underlying platform.
Although note that if you replace libc with eyra, then the behavior goes from thread-unsafe to "just" a memory leak: https://blog.sunfishcode.online/eyra-does-the-impossible/
Arguably, Rust declares it is safe to modify the environment through its stdlib methods. The tricky detail is that this means it is unsafe to read/modify the environment through other means, but sometimes this is really hard to avoid.
If you have C and Rust in the same process and C code calls setenv(3), for one ...
Edit: why downvotes? It's very typical to link to C libraries which may call the libc environment stuff ... My point is you can't control library code as easily, if it's some dependency of a dependency eventually calling libc.
The Apple Libc appears to just unconditionally drops the environ lock in the child (https://github.com/apple-oss-distributions/Libc/blob/c5a3293...), while glibc doesn't appear to even bother with that (https://github.com/bminor/glibc/blob/6ae7b5f43d4b13f24606d71...)
Fix the high level race, and suddenly you no longer need the low level mutex.
environ is a single contiguous null-terminated segment of null-terminated key-value pairs; any change of any environment variable might reallocate it, changing the address and invalidating the old address.
Also why it's a bad idea to store the pointer returned by getenv, it might be invalidated by any environment modification.
(1) setenv resizes environ using realloc, which frees the old buffer, so getenv can end up reading from a freed array.
(2) The code does not use atomics or memory barriers, so on weakly ordered architectures, getenv could observe another thread's write to one of the pointers in the environ array, or to the environ pointer itself, while observing stale values for the memory behind it.
In both cases, getenv could end up returning a bogus pointer or just crashing.
However, those issues can be fixed without changing the API, and at least Apple's libc seems to do the right thing here. On the other hand, other libcs such as musl, FreeBSD libc, and even OpenBSD libc (!) do worse than glibc and have no locking at all.
If someone could convince the maintainers of all those libcs to add a lock and make getenv/setenv 'thread safe as long as you're not racing on the same variable name', then that would be a good starting point. But in my opinion it would still be a half-measure. We need a fully thread-safe environment.
And honestly, it might be easier to convince the maintainers to add a full solution than a half-measure, even if it involved API changes. (But it may be hard either way. Rich Felker showed up in a Rust thread a while back and was highly negative on the idea of making any changes to musl.)
In what sane world would someone reasonable treat (initial shell) Environment Variables as a proper ACID complaint database? About the 'best' solution I can see for preventing segmentation faults related to resizing the env array during runtime is to defer reclaiming freed memory chunks until after all in-process threads have been given another uninterrupted timeslice to process. Even that wouldn't be 100% but probably would cover any not pathological case.
I really strongly disagree with how bad you seem to think this is. If you are designing your application to use the timezone and modify it at the same time, it is a totally natural consequence that you may see the previously set time zone in a timing dependent fashion. That's the nature of the beast. To "solve this" is seemingly to make that other thread capable of time travel or something. It read something before it was written, and acted on it. Reasonable!
The harmful data races are when you read intermediate results. If setting the timezone is a multi-step process, or involves manipulation on complex data structures with pointers that might be deallocated, then you are in grave danger. Seeing a previously valid result is ... I honestly don't know how you'd expect to solve it without threads being able to see the future, or some other unreasonable expectation.
But a bad API design doesn't end at environment variables. Many POSIX systems rely on `/etc/localtime` to define the system-wide time zone, and every `localtime` call has to check if the file has been changed or not because there is no way to subscribe to the system-wide time zone change event. Of course there is a cache, but many libcs call at least `stat` per each `localtime` call AFAIK. I had even experienced a possible glibc bug due to the lack of guard against I/O error during this process [1]. Windows got this right, I can't see why POSIX couldn't do the same when it does have an asynchronous signal delivery mechanism anyway.
[1] https://news.ycombinator.com/item?id=9953898
And you are right about time-rs (or I think you are). Version 0.1 was never fixed, and version 0.3 does the OS and thread count checks.
It does have some advantage for chrono to do everything in Rust: it can now return two results for ambiguous local time during DST transition fold, and properly return None during a transition gap.
Thank you. To be frank as a first-time maintainer I did a mediocre job---my biggest regret for Chrono is that I did know most forthcoming issues beforehand and yet didn't take enough time to make them public and explicit so that someone else could prepare for the future.
But you can subscribe to file change events so why not do that?
It is entirely reasonable that any of the following _might_ be valid behavior.
* Simple but syscall heavy approach which re-reads the env, and possibly /etc/localtime each call and has no stability. (Results may mutate as other processes / threads change things.)
* Same as above, then caches the decision result for some application specific reasonable time; which may be until the application exits.
* The elsewhere mentioned stat / inotify approaches that only track updates to /etc/localtime (and ideally update the cached decision result when notified).
All approaches seem valid. It's sort of like the hostname or any other system level configuration where a reboot may be a reasonable expectation for a complete update.
[1] https://github.com/bminor/glibc/commit/68dbb3a69e78e24a778c6...
I would argue that those splits are in great part responsible for the feeling that rust is hard to learn. I remember to have had to dig into pretty complex time code to understand why it broke our program that relied on timezone when we switched from chrono to time. It hinders your productivity for sure even if you learn the how.
I can't think of any time I had wanted to do that.
What exactly are the programs that break if this changes?
Look up the mess around LOCALE sometime.
The best the programmer can do is perform all the setenv calls before spawning any threads or making any library calls.
https://pubs.opengroup.org/onlinepubs/9699919799/:
“The getenv() function need not be thread-safe”
I expect most if not all implementations are more robust.
"The returned string pointer might be invalidated or the string content might be overwritten by a subsequent call to getenv()"
You don't even need threads for this to be unsafe; another call to the same function may invalidate earlier-gotten pointers. I don't see how to interpret this as anything but broken.
Reading https://man.freebsd.org/cgi/man.cgi?getenv(3) it was introduced in Version 7 AT&T UNIX (https://en.wikipedia.org/wiki/Version_7_Unix). That didn’t have threads.
Now, was memory scarce? The PDP-11 it was designed for could have megabytes of RAM, but I wouldn’t know what the smallest machines this ran on had, and of course, those systems were multi-user.
So, maybe, thisxwas a good choice, but it also could be an early example of using a small system, hacker mindset on machines that didn’t need that anymore.
Environment variables are not meant to be an inter-thread communication channel and the documentation that points out setenv() is not thread-safe is very much a fair shot.
You rarely, if ever, need to setenv() anything maybe unless you're a shell. For spawning children execve() already takes an envp parameter. For debugging I think I've mostly set in-process environment variables manually from gdb.
Further, because environment variables are an interface between the process and its environment you typically read environment variables at start and cache the parsed values in some internal location. If you need to change that global state on the go you should do it using your own internal variables instead of recycling it through the environment and having the program threads repeatedly getenv() the updated values.
Or: C knows that it doesn't need fixing.
How often do I need to `setenv()` anything? The answer is "Never" in the vast majority of programs, because ENVVRS are usually read rather than set, so this issue is nonexistent for them.
For the vast majority of the small amount of programs that actually need to use `setenv()`, the answer is: "Maybe once or twice during the entire lifetime of the process, and then only at the very start, probably even before running any threads", meaning this issue is nonexistent for them as well.
So, is there a potential issue with thread safetey? Yes. Does it matter given where and under what circumstances it occurs? Not really.
> such as Go's os.Setenv (Go issue)
Here is the link to the "issue":
https://github.com/golang/go/issues/63567
What kind of actual real life production code would continuously set envvars while simutaneously calling a function that tries to read the environment?
Yes, this is a footgun. But even the issues author acknowledges, in the issue thread:
> It has wasted thousands of hours of people's time, either debugging the problems, or debating what to do about it.Source?
looks at SDL
[0] https://wiki.libsdl.org/SDL2/SDL_SetHint
1. Right after program startup before any threads are spawned.
2. After a fork before an exec.
In both cases it can be known that no threads are running. (Ok, for 1 it can actually be non-trivial if you have code before main or if you call functions that spawn helper threads, but let's assume that you can know this).
However no languages actually have ways to enforce this. So the APIs can be called at any time and are huge footguns.
I think that the proposed improvement of `getenv_s` is great. It is cheap and easy to use, then software can slowly migrate off of the less safe stuff. You can imagine that if libc stopped using `getenv` internally most of this problem would be solved.
Consider for instance something as simple a implementing a shell. Such a program needs to be able to set the environment based on user interaction and this change needs to show up in /proc/$pid/env.
This is useful to recognize various processes I suppose. I have written code that scans the environment of processes to find particular processes and group them together.
There's no reason why these languages need to restrict themselves the same way C does.
You can't fix C libraries loaded into Go programs (i.e. and external library calling C's setenv, or I suppose explicit FFI calls by the user), but Go can be responsible for the APIs it calls itself. That may necessitate writing a thread-safe alternative for DNS lookups, or documenting and/or adding compile time warnings that threaded programs doing DNS lookups will just crash sometimes, but the language's standard library can still make it much harder for developers to write buggy code.
Name lookups (whether user identities or network resources) are the biggest chunk of these. You have a "choice" as a user/programmer here. Say, the existing name lookup interfaces in most libc implementations don't do DNS-over-HTTP (DoH); you can implement that yourself and just use the addresses returned by your library/package where the system calls ... want addresses.
If you have the go stance, go all the way. Don't say "the C runtime is sh*te but I really really really want that one particular teensy tiny bit of it could someone somewhere somehow please do something to make it a little less sh*te". Legacy baggage is a burden and backwards compatibility shackles you. The C/Unix interfaces are full of this, and with the hindsight of 50 years noone today, not even "C programmers", would implement them all the same way again. But that doesn't mean their behaviour can be arbitrarily changed.
DNS functions are thread-safe.
The thing people aren't understanding here is when you set loose nasal demons (such as by calling `setenv` in a multithreaded program), they can cause problems even in safe code.
For example, imagine the chaos of `memset(stderr&-4096, 0x42, 4096)`.
Even if that wasn't an issue: this is a bug in C as well! You should absolutely be able to use setenv/getenv safely in multi-threaded C, it's insanity that you can't.
Programmers have to deal with a lot of badly written programs all of the time. You'd need this functionality to either debug a program that responds differently to different values of environment variables, or to control it, because, maybe it's the only reasonable way to do so.
It's OK to say that programmers shouldn't rely on this functionality ideally, but, for practical reasons, this functionality is needed. Same happens in "pure" functional languages, for example, when you need to debug programs in such languages interactively, and struggle to create the program state that reproduces the problem, or, in some extreme cases, due to I/O being "impure" even struggle to output diagnostic information.
People don't like APIs that can randomly crash your program while there's no good technical reason for why they should. Why not fix the problem? People like you, who have no issues with the current implementation, won't see any regressions because you're already a good citizen, and myriad other programmers whose programs do occasionally crash because of this will be helped.
> So, is there a potential issue with thread safetey? Yes. Does it matter given where and under what circumstances it occurs? Not really.
"The unpredictable crashes only happen very rarely" doesn't mean the crashes go away.
> What kind of actual real life production code would continuously set envvars while simutaneously calling a function that tries to read the environment?
The reproduction sample calls setenv in a loop so the issue can be reproduced. A single setenv anywhere in the code is enough to trigger the crash, but then you would get one of those "you need to run the program a million times to reproduce it" bug reports that gets pushed down the line.
Because doing so breaks backwards compatibility, simple as that.
The problem isn't even that `setenv` isn't thread save. The problem is that `getenv` returns a `*char` directly into the environment memory space. Many many many programs rely on that being the case.
> People like you
People like me would like every software to be perfect, but that's not the world we live in, so we are forced to be pragmatic. When fixing something causes more problems by breaking backwards compatibility promises, than it prevents, then there is no good argument for a fix, and the correct approach is to say "yes, this sucks, let's document it well so people don't waste too much time on this".
The setenv/getenv problem is such a case. Anyone who disagrees is free to fork glibc, implement whatever fix they think is adequate, and then try to compile the software packages found on a typical Linux server against the result.
> so the issue can be reproduced.
"Can be reproduced" and "is a common issue in production code" are not the same.
Fact is, almost all production programs that set envvars, do so once, very early in the process lifecycle, and then never again, and so are never affected by this.
Note also that "global references" like getenv() returns and point-in-time owned snapshots don't behave the same way. Say, a library initializer code could retrieve a number of env var references by calling getenv(), and then use those at runtime. No more need/use for getenv() again after - and even perf-sensitive code could look at the env var. With a func that copies, the perf-sensitive code would need to do that each time (lock, lookup, copy). Not strongly desirable.
Also ... UNIX is rather flexible ... and if you so wish, you _can_ substitute _your own_ setenv()/getenv() by the magic of dynamic linking. To create a set that locks and returns you leaked copies (changes the semantics of getenv so that the caller must free the pointer to avoid a leak). It's all possible to do this.
I'm getting the impression from this that we see a "go tantrum" here. "I make my own standards but I wanna use that C/Unix standard thing as well but not how it is because it's not nice it should take go into account waaaahwaaah ...".
It is not _nice_ to modify your own env at runtime. Maybe, just maybe ... that's for reasons. Because not everything that can be done is also a great idea.
If you’re calling setenv in the middle of your program, you fucked up.
There are those things in programming that should be extremely triggering to your “what the actual fuck?!” senses, and “setenv in the middle of runtime” is one of those things.
- Shells
- CI runner
- Container launchers
- IDEs
That's not true, that's just misunderstanding how it works. `execve()` takes an entirely new copy of environment variables to give to the child, that's the "real" way to do it.
I think you're not seeing this from the right POV. People that consume POSIX API need to know POSIX API.
https://pubs.opengroup.org/onlinepubs/009604499/functions/se...
It says loud and clear "The setenv() function need not be reentrant. A function that is not required to be reentrant is not required to be thread-safe."
> "The unpredictable crashes only happen very rarely" doesn't mean the crashes go away.
If you get a crash over setenv() reading the manual page of setenv C call should be your first step. And the only step. The bigger issue is in design of application that has wrongly assumed setenv() is thread-safe. That requires a refactoring and is solely due to developer misunderstanding the API.
Not being re-entrant makes the user-facing API unnecessarily complicated. It creates an avoidable foot gun. It trips people up for no good reason. And unlike stuff like signed integer overflow, there doesn’t even seem to be a (dubious) performance argument to justify this insanity.
The standard should be fixed and that’s the end of it.
I'm a UNIX/C programmer for decades and I don't care about this.
There is no such thing as beautiful API design. Every design is a compromise. If you think non-reentrant calls should be deprecated in POSIX take it to the committee.
There is a myriad of non-reentrant code both in POSIX spec and in libc implemenations. You need to RTFM, I'm sorry.
There is no "coherent API" as far as null termination goes too. Some library functions deal with it, some calls don't. You need to RTFM.
I also want to know OP's reason to even use setenv() in a multithreaded piece of software. It's like an oxymoron. setenv and vars are useful to pass on data from parent process to forked children because they inherit the environment. If you use the threading model you don't need it. If your application is a single process setenv() is useless.
As someone who made and maintains multiple libraries: No. Not gonna happen.
Programmers who are using any library code without reading and understanding the documentation are asking for trouble regardless of language.
The correct solution to your objections is to create new functions that behave as you prefer.
Languages that compile to C need be careful not to promise thread-safe implementations of POSIX or C functions that are explicitly documented as not reliably thread-safe, including setenv(). The author seems to want to change C, and POSIX, so that Go can reliably do so.
It’s simple, really: we indeed rarely to `setenv()`. So it’s not a performance problem. So we can make it thread safe, and the performance impact will be negligible. In exchange for this small price, safety will increase.
Sacrificing any amount of safety for a negligible improvement in performance is flat out unprofessional, and should be grounds for immediate termination in most contexts.
`setenv()` has no way to knowing where those pointers are floating around so there's no way to safely change the environment variables. The best you could do would be to leak memory every time you set new environment variables so that the old pointers don't get invalidated, and that just creates a new problem and reason not to use `setenv()` (that's arguably worse).
And then you can leave the existing syscalls as they are (thread unsafe) while having a separate thread safe version.
`setenv()` would only be safe if your program never uses `getenv()`, and calls to `getenv()` are so numerous and all over the place that for most non-trivial programs it would be hard to ensure they never happen.
There's also the rub that `setenv()` is not part of the C standard, it's POSIX. I don't think the C standard would ever introduce `tgetenv()` to fix a problem it doesn't have, so non-POSIX code would have to continue to call `getenv()` since that's all that is available to them.
The C standard has no problem acknowledging that getenv is subject to data races for most of its implementations. As far as I can tell that part was even added at the same time as threading support.
I mean, why not just deprecate the old one; add a warning if it’s used
There's also the rub that many C programs do not target the latest standard (for a variety of reasons). I didn't realize `getenv_s` was added in C11 (though it's optional), but it doesn't really matter because programs/libraries that target C89 or C99 can't use it anyway.
Maybe some combo of that with sentenv() in your source or something
Or do “live” analysis under integration and give a low priority warning
But yeah it’s hairy, you’re right
I'm in the process of working on a tool in C at the moment, so for once I actually have some context on what's being grumped about here!
Arguably worse? My goodness no.
This is a rare edge case that most programs don’t encounter. Option 1 is to crash and explode and die. Option 2 is to leak tens of bytes.
Leaking tens of bytes is for sure NOT worse than crashing.
I do really disagree here. The answer is not clear at all.
But then, you are mischaracterizing the problem. The issue is not with crashing, you can get plain bad data too, and this is clearly worse than both leaking memory and crashing.
Also, the GP is mischaracterizing the options. You don't need to leave the old values around, you can just copy them into userspace memory.
There’s a reason this problem comes up on HN once or two a year. And don’t even get me started about printf grabbing a mutex for a stupid locale…
I wouldn’t be surprised if a good chunk of compilers and interpreters in other languages suffer from this gotcha’.
I mean, I wouldn’t even be surprised if some JVM implementations silently expose their users to bugs on account of this implementation.
EDIT: … ha, sure looks like it https://github.com/openjdk/jdk/blob/a2c0fa6f9ccefd3d1b088c51...
Yes. I do. These two concepts don't contradict each other.
> No it’s not a performance problem. So we can make it thread safe, and the performance impact will be negligible
Who said anything about performance being the problem, or a reason not to change it!?
The problem is BACKWARDS COMPATIBILITY. The issue is that `getenv` returns a `*char` into the envvar array. Basically every application that uses this function relies on this fact.
So we have:
a) A potential issue that occurs only in very unusual circumstances, most of which will never occur in production code and on the odd chance that they do, they can easily be avoided. Documenting that well can help prevent time wasted in debugging.
b) A fix that may prevent a) but breaks backwards compatibility promises, and would necessitate reworking god knows how many programs, the vast majority of which were never impacted by the issue in the first place.
Of these 2 options, a) is just the better one. Yes, in an idea world, we could have pure, 100% bug free code, and spend an unlimited amount of time on fixing every last problem. That's not the world we live in however, and so a pragmatic approach is simply a necessity.
My care for code robustness scales with income.
For an action I generally mean "malpractice". Something bad enough to bar repeat offenders from the profession (if we even were a profession, which we’re not). For a person I mean "unfit to program code other people rely on".
> My care for code robustness scales with income.
Good point: the conditions for us to write code that actually works are too rarely met. The only answer I have for this one is political though, not technical.
I mean, the environment is just a chunk of memory made available to the process by the OS. It is no more, no less thread safe than any other chunk of memory.
Why would the libc need to protect it more than any other memory location?
Sure you could use locks, stop the world, etc, but there is no way you can ensure that all the data and information you had derived from the old state is going to be valid.
A better solution is to not rely on global state like this.
It seems like a complicated and error prone thing to be using no matter if it is thread safe or not. You can set up your own environment before you launch threads, and you can launch child processes with a different environment from the current process without modifying your own. If you fork, you can modify the environment in the child without affecting the parent until you exec.
And even setenv() if it was reentrant and couldn't cause crashes, it wouldn't be thread safe, since threads share the environment and could get their environment changed under their feet.
The first of the three listed items you should certainly do. I hope this author is not writing medical software, or anything important.
The process environment should not be the mechanism for threads to communicate with each other.
Oh, C is taking the php approach :)
and don't forget, you are using unix because it defeated all the other options, because it was better and they were worse, so also keep studying till you understand why that is too.
then this problem with env will fix itself.
unix gives you tools to handle threads. C gives you tools to handle threads. Learn them, use them.
I don't really understand why other languages such as Go and Rust decided to call the weird POSIX API rather than implementing their own API, which matches the semantics they expect. In cross platform C you'll be stuck with the outdated POSIX API design, but there's no reason why other languages should accept those same limitations.
We're not running on PDP-11s anymore. You can afford a thread-safe hash map in your standard library. Ignore the limitations of the old C library. Twenty years ago, Microsoft released a better API, keep the crashy old API with tons of deprecation warnings (hell, add a compiler flag --enable-broken-c-api-designs) and just provide new APIs that are actually usable in modern programming environments.
Have some fun reading on how Go handles files.
It's hard to tell if Microsoft altered the source code since, but the leaked XP source code (https://github.com/tongzx/nt5src/blob/master/Source/XPSP1/NT...) doesn't seem to do any getenv() calls for DNS lookups. The specific bug that started all this nonsense only triggers on (specific) Unix implementations. Unfortunately, Go opts to call the POSIX methods rather than GetEnvironmentVariable/SetEnvironmentVariable on Windows, so I suppose it's still possible that somewhere in the chain this bug gets triggered by Go code.
I feel you're mixing OS APIs, with the low level mechanism to enter into the kernel space.
> On non-UNIX platforms stuff like getenv() belongs to the specific compiler C library, not the OS API, hence why Windows doesn't use it.
Linux isn't a UNIX, and non-POSIX compliant Linux distributions exist. getenv() is still part of the C library, not the OS API, yet Linux distributions still use it. So that's not the only reason why Windows doesn't use it. It's more because Windows design wasn't originally POSIX-compatible (some POSIX wrappers got added with Windows NT) and MS designed their own API.
The problem comes when you do FFI on *nix systems, because those foreign functions may start making unsynchronised calls to getenv/setenv.
[1] https://github.com/rust-lang/rust/blob/master/library/std/sr...
[2] https://github.com/rust-lang/rust/blob/master/library/std/sr...
It states that glibc “never free[s] environment variables], but then goes on to state
> [in glibc] if a thread calling setenv() needs to resize the array of pointers, it copies the values to a new array and frees the previous one
Since envvars cause crash under glibc, I assume the initial assertion is incorrect.
The multithreaded program needs to be restructured so that the parts that communicate via the environment are properly serialised with respect to each other, just as would be needed for any other communication via global state and/or access to a shared resource.
This has to happen at a higher level than the individual getenv/setenv calls: entire blocks of logic containing the calls need to be made atomic (or otherwise refactored; perhaps you could do all the environment writes before spawning any threads) so that no other thread can blow away the environment contents in between the code that sets it up for some purpose and the code that implements that purpose; and once this is properly done, the individual calls themselves do not need further protection.
- The race might be irrelevant (e.g. simultaneous calls that access different variables are fine).
- The application author might not have complete control over all calls to getenv/setenv (e.g. if using a third-party library).