utc_tm_to_time isn't memory-safe on nacl/solaris/illumos targets because it uses `std::env::set_var`

[briansmith]

As discussed in #499, std::env::set_var isn't memory-safe, and should not be used.

The code in question is https://github.com/chronotope/chrono/blob/3467172c31188006147585f6ed3727629d642fed/src/sys/unix.rs#L55-L75

I don't know of a good solution but we should have a separate issue to track specifically set_var being problematic, to clarify the discussion in #499, which is about reading the environment.

[briansmith]

    let current_tz = var_os("TZ"); 
    set_var("TZ", "UTC");

Also, even if var_os and set_var were memory-safe, this would still be racy because another thread could be reading or writing TZ concurrently. In particular, current_tz may no longer be the "right" timezome to restore TZ to later, if another thread (permanently) changed it to another value. Similarly, if another thread is calling a function that reads TZ (including ones that implicitly or explicitly calls tzset()) then that other thread will get wrong results during the time we've temporarily set TZ to "UTC".

In other words, it isn't enough to find a way to make set_var memory-safe(r) because there would still be races even if set_var/get_var were memory-safe.

[Kixunil]

Yes, I noticed this when trying to understand what chrono does. Interestingly, there's nothing that says you have to use system-implemented localtime functions! Both the inputs and outputs are well-defined, so there's no problem with writing our own - in Rust, or exactly as I did, by just patching an existing implementation. Maybe it'll need some more tweaks to work on solaris but should be doable.

[briansmith]

this would still be racy because another thread could be reading or writing TZ concurrently. In particular, current_tz may no longer be the "right" timezome to restore TZ to later

In particular, calling these threads concurrently in different threads over and over again will likely result in TZ getting fixed to the value "UTC" regardless of what it was originally set to be, as eventually we'll have:

Thread A: current_tz = get_var("TZ"); // value X Thread A: set_var("TZ", "UTC"); Thread B: current_tz = get_var("TZ"); // "UTC" Thread A: set_var("TZ", current_tz); // correctly restored to X Thread B: set_var("TZ", "UTC"); Thread B: set_var("TZ", current_tz); // "UTC", but it should be X.

[esheppa]

This should be fixed by releases >= 0.4.20

Interestingly, there's nothing that says you have to use system-implemented localtime functions! Both the inputs and outputs are well-defined, so there's no problem with writing our own

We are now directly parsing the tzinfo files and calculating the offsets where relevant, and avoiding mktime on all cfg(unix) platforms