The call to `localtime_r` may be unsound

[quininer]

because getenv and setenv are not thread-safe, localtime_r in time may data race with std::env::set_var in libstd.

I described this problem in chrono issue, and time is also affected.

---

Edit from @jhpratt:

Chrono users please see this comment.

[jhpratt]

First off: thanks! It's a nice find.

What would be an appropriate fix for this?

Mainly for myself: this is the sole location this function is called.

[quininer]

I don't think there is any quick solution, we may have to reimplement localtime_r in Rust.

[jhpratt]

Ping @ymtvx, who created the original implementation. I'm not terribly familiar with these C APIs.

What output are you expecting from the snippet provided?

[quininer]

@jhpratt Oh, I realized that I paste wrong code and it has been updated. POC code will crash.

[quininer]

[quininer]

In addition, I found that when using time crate, this crash is easy to reproduce in glibc, but it is difficult to use chrono.

[jhpratt]

So what is actually causing the segfault? Just trying to determine the true root cause so that I can figure out what action to take. I've never used lldb before, so all I see is the basic segfault message when running.

[quininer]

setenv and getenv are not thread-safe, and localtime_r will call getenv, which cause to data races if setenv and localtime_r occur at same time.

To be more specific, getenv will return a pointer, and after setenv the pointer may point to an invalid address.

Rust's libstd avoids this problem by locking std::env::set_var and std::env::var, but call localtime_r directly will bypass the lock.

[dekellum]

Rust libstd has a similar, long open issue, for its own calls to getaddrinfo: rust-lang/rust#27970

Like localtime_r(3), getaddrinfo(3) is similarly marked as vulnerable to concurrent ENV mutation.

In the linked issue, it is contemplated using the libstd internal ENV lock around calls to getaddrinfo, but with concern over performance implications.

[hlavaatch]

So instead of worrying about concurrent env modification, you could read the env variable on startup and cache it, and avoid any calls to real getenv. If you want to be fancy you could intercept setenv and update the cached value in a thread safe way.