6 ms·

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

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. More reading: https://docs.rs/chrono/latest/chrono/#security-advisories https://docs.rs/chrono/latest/chrono/#security-advisories

rewmie 3y ago
> Rust declares it safe to modify the environment, while POSIX declares it unsafe. There's your problem right there, and it ain't the behavior specified in the standard.
- pitdicker 3y ago
  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. But libraries and users are caught in the middle.
  - eptcyka 3y ago
    It is safe to use the Rust standard library interface.
    - pitdicker 3y ago
      Unless the environment is also touched by a part of the program written in Go, Julia, I don't know... The lock is not shared across languages.
      - the_mitsuhiko 3y ago
        > The lock is not shared across languages. Which just to be clear: it cannot without changing the standard. There is really nothing anyone can do without a change in the standard.
      - bbatha 3y ago
        However, to access any of those languages from rust you need to use unsafe.
        eptcyka 3y ago
        There is no safe way to access the environment, even if you mark this API unsafe, what are you going to do?
        bbatha 3y ago
        You can safely access the environment so long as you use the rust apis and don't have unsafe code that calls `setenv` without synchronization.
- Sytten 3y ago
  There is an issue in the std to name setenv unsafe but that is a breaking change so it's complicated.
  - kibwen 3y ago
    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.
- kibwen 3y ago
  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." https://doc.rust-lang.org/std/env/fn.set_var.html 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/ https://blog.sunfishcode.online/eyra-does-the-impossible/
SkiFire13 3y ago
> 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.
- asveikau 3y ago
  > 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.
- manwe150 3y ago
  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. The Apple Libc appears to just unconditionally drops the environ lock in the child (https://github.com/apple-oss-distributions/Libc/blob/c5a3293354e22262702a3add5b2dfc9bb0b93b85/sys/_libc_atfork.c#L65 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/6ae7b5f43d4b13f24606d7108d822e469a96af3d/stdlib/setenv.c#L57-L59 https://github.com/bminor/glibc/blob/6ae7b5f43d4b13f24606d71...)
  - connicpu 3y ago
    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.
tsukikage 3y ago
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.
- formerly_proven 3y ago
  > 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.
  - Sprocklem 3y ago
    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.
    - comex 3y ago
      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.)
lifthrasiir 3y ago
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. [1] https://news.ycombinator.com/item?id=9953898 https://news.ycombinator.com/item?id=9953898
- pitdicker 3y ago
  My respects for your work on Chrono! 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.
  - lifthrasiir 3y ago
    > My respects for your work on Chrono! 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.
- wavesquid 3y ago
  I believe systemd has a way to subscribe to timezone changes.
- account42 3y ago
Sytten 3y ago
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.

rewmie 3y ago

> Rust declares it safe to modify the environment, while POSIX declares it unsafe. There's your problem right there, and it ain't the behavior specified in the standard.

pitdicker 3y ago
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. But libraries and users are caught in the middle.
- eptcyka 3y ago
  It is safe to use the Rust standard library interface.
  - pitdicker 3y ago
    Unless the environment is also touched by a part of the program written in Go, Julia, I don't know... The lock is not shared across languages.
    - the_mitsuhiko 3y ago
      > The lock is not shared across languages. Which just to be clear: it cannot without changing the standard. There is really nothing anyone can do without a change in the standard.
    - bbatha 3y ago
      However, to access any of those languages from rust you need to use unsafe.
      - eptcyka 3y ago
        There is no safe way to access the environment, even if you mark this API unsafe, what are you going to do?
        bbatha 3y ago
        You can safely access the environment so long as you use the rust apis and don't have unsafe code that calls `setenv` without synchronization.
Sytten 3y ago
There is an issue in the std to name setenv unsafe but that is a breaking change so it's complicated.
- kibwen 3y ago
  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.
kibwen 3y ago
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." https://doc.rust-lang.org/std/env/fn.set_var.html 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/ https://blog.sunfishcode.online/eyra-does-the-impossible/

SkiFire13 3y ago

> 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.

asveikau 3y ago
> 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.
manwe150 3y ago
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. The Apple Libc appears to just unconditionally drops the environ lock in the child (https://github.com/apple-oss-distributions/Libc/blob/c5a3293354e22262702a3add5b2dfc9bb0b93b85/sys/_libc_atfork.c#L65 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/6ae7b5f43d4b13f24606d7108d822e469a96af3d/stdlib/setenv.c#L57-L59 https://github.com/bminor/glibc/blob/6ae7b5f43d4b13f24606d71...)
- connicpu 3y ago
  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.

tsukikage 3y ago

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.

formerly_proven 3y ago
> 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.
- Sprocklem 3y ago
  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.
  - comex 3y ago
    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.)

lifthrasiir 3y ago

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. [1] https://news.ycombinator.com/item?id=9953898 https://news.ycombinator.com/item?id=9953898

pitdicker 3y ago
My respects for your work on Chrono! 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.
- lifthrasiir 3y ago
  > My respects for your work on Chrono! 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.
wavesquid 3y ago
I believe systemd has a way to subscribe to timezone changes.
account42 3y ago

Sytten 3y ago

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.