UNIX thread safety check exemptions may not be sound

[BlackHoleFox]

Hiyo, I was digging around for popular crates and seeing if they had any specific macOS version dependencies. time is big enough I figured it was worth a look. What I found is in 9d3cbef, where some OSes got added to an allow-list that lets them skip the soundness and/or thread safety checks other UNIXy things need.

macOS is currently in the list:

time/time/src/sys/local_offset_at/unix.rs

Line 20 in a98fe2d

| b"macos"

But, Apple's libc hasn't supported env thread safety forever and pretty much is guaranteed not to in all versions of macOS Rust supports today (officially back to 10.7 even if its harder to actually do). The API they're using internally is os_unfair_lock, which at least in the public API came into existence in macOS 10.12. Though even with that, the headers say the locking functions used in getenv and setenv were added in 12.3, which is very recent in the scale of things and far past what people are shipping apps with compatibility for.

With all that, I think macOS should just be removed from the list unless time wants to dynamically check the OS version at runtime (which Rust is yet to provide a built-in for). However...

With all of the "maybe it would be fine if..." done, the general pattern that all three of these UNIX OSes have employed appears fraught anyway. All of them lock, get the value pointer, unlock, and then return it to the user. I think that macOS and the BSDs will just leak the old string if you setenv with a new larger value, but if its the same length then it updates the existing slot in place and changes the contents of that pointer returned to userspace. So, a data race, even if it isn't an actual problem in practice. I'll leave it up to you to determine how bad that actually is.

In short, the current macOS exemption is unsound due to older versions without it existing and dubious on newer macOS + other currently exempted UNIXes.

[jhpratt]

Agreed. macOS 12 looks like it was released just two years ago and is still maintained, so it's unquestionably a recent enough version to care about. Let's remove the exception.

[thomcc]

It's probably not sound on other unix either unless the hold this lock for the duration of the use of the pointers returned by getenv. E.g.

// inside getenv call
acquire_lock();
char *p = real_getenv();
release_lock();
// outside
use(p);

So just taking a lock around the duration of getenv/setenv is insufficient. It would need to be around the duration of whatever calls you're making into libc (or at least more of it than just getenv/setenv).

[jhpratt]

I'm not sure I understand what you're referring to — time doesn't use a lock for any of this. The only lock present is that in std, and the fact that it can't be relied upon is the underlying issue.

Specifically, there doesn't need to be a lock if

• the OS has thread-safe environment variables (which is this issue and at least one other)
• or the process is single-threaded.

The reason for the second is that it's inherently guaranteed that no one else can mutate the environment if there's no other code running.

[thomcc]

My point is that the internals of getenv/setenv taking a lock is insufficient for an OS to have a thread-safe environment.

[jhpratt]

Ah, I see what you're saying. For illumos, thread safety is ensured by the fact that setting env vars leaks memory. I believe the same is true for NetBSD, though I don't have time to check at the moment.

[thomcc]

Yeah, technically even that's not enough, assuming they only only leak on growth. In non-growth cases you'd still have a data race (as mentioned above by @BlackHoleFox), but it's probably fine in practice.

[jhpratt]

For clarity, it's the combination of leaking and locks.

it's probably fine in practice

I would have said that about the initial unsoundness. Even having the calls in a tight loop, it takes a couple seconds (tens of thousands of iterations) before anything happens. Even without the checks in place, it's very unlikely that unsoundness occurs. But apparently the original report was in fact found in the wild, not someone fuzzing. With that said, I have not the slightest clue what they were doing that would get anywhere near this.

Overall, as far as I am aware the current exemptions are sound. I only have access to my personal Linux machine (plus a Linux SSH machine from the Rust Foundation), so I am unable to verify any other OS. If someone wants to run the reproducer, they are free to do so and file an issue if it fails.

[thomcc]

A data race like that likely wouldn't show up in a a reproducer, unless you compiled libc and the rust code with a data race detector (like thread sanitizer).

[jhpratt]

That is true. Regardless, anyone is free to file an issue if they find that an exemption is wrong, regardless of how that's discovered.