time-rs / time

The most used Rust library for date and time handling.
https://time-rs.github.io
Apache License 2.0
1.13k stars 281 forks source link

Add macOS to `local_offset_at`'s `OS_HAS_THREAD_SAFE_ENVIRONMENT` #718

Open PaulDance opened 1 week ago

PaulDance commented 1 week ago

Dear maintainers,

I know this may seem like the 1298th duplicate issue about the underlying problem at first glance, but I have seen the previous issues regarding macOS vs. local offsets and still think something may have been missed. In any case, I'm open to discussion, hence my opening this.

Indeed, the local_offset_at has a few soundness checks, one of them being OS_HAS_THREAD_SAFE_ENVIRONMENT. When true, which is rarely the case, further soundness checks are disabled.

However, macOS is not part of the list, while what I'm seeing seems to indicate it should: although FreeBSD's implementation is most probably not thread-safe and the macOS man pages do not mention it either, the actual Apple libc implementations of getenv and setenv really do seem to be thread-safe, which was added in this commit, itself included as part of macOS 10.12.

Therefore, is there still a good reason not to include macOS in the thread-safe list? I've seen segmentation faults mentioned around here and specifically for this platform, but I would be surprised it would come from the environment manipulation itself.

Thanks in advance, Paul.

jhpratt commented 6 days ago

No worries! It's immediately clear that this is not the same thing brought up repeatedly :)

Without digging into this again, you're 100% correct. I previously verified it, though after someone brought up that the behavior was relatively recent I removed the opt-out. While I don't recall the timeframe when this happened, it has almost certainly been long enough now.

With that said, there is no reason to open a pull request to add an opt-out here. On one of my SSDs I have a few commits that I haven't pushed yet, one of which removes the soundness checks entirely in favor of a better solution. These commits should be pushed in the coming days when I get a new laptop set up (hence the temporary storage).

PaulDance commented 6 days ago

Good, thanks for the quick answer, glad to hear!