std: stop using TLS in signal handler #140628
Conversation
joboet
added
A-thread-locals
|
rustbot has assigned @Mark-Simulacrum. Use |
rustbot
added
O-unix
This comment has been minimized.
This comment has been minimized.
well, that doesn't seem good |
|
The Miri subtree was changed cc @rust-lang/miri |
That's no cause for concern, these miri tests just assumed that there is no synchronisation between threads created independently from each other. That's no longer true however as the threads all acquire the thread info lock. I've fixed this now by moving the operations around a bit and by using |
| //! Unfortunately, because thread local storage isn't async-signal-safe, we | ||
| //! cannot soundly use it in our stack overflow handler. While this works | ||
| //! without problems on most platforms, it can lead to undefined behaviour | ||
| //! on others (such as GNU/Linux). Luckily, the POSIX specification documents |
There was a problem hiding this comment.
I guess we expect that all cfg(unix) platforms satisfy POSIX? Is that a safe assumption to make?
It seems a little odd to me that errno is somehow special (vs arbitrary thread-locals)...
There was a problem hiding this comment.
It seems a little odd to me that
errnois somehow special (vs arbitrary thread-locals)...
The C runtime can rely on the fact that it's always loaded and use e.g. the initial-exec TLS model – which is async-signal-safe – for errno (that's what glibc does). Or they embed errno into the thread control block, which is accessible through the thread pointer (that's what musl does).
I guess we expect that all cfg(unix) platforms satisfy POSIX? Is that a safe assumption to make?
The expectation must only hold for platforms that enable the stack overflow handler.
There was a problem hiding this comment.
It is not a safe assumption to make on various niche cases, however if they implement any threading, have signals, yet do not implement POSIX correctly-enough for this, they will have a lot of problems that we will immediately notice. In particular, this is a very mild demand: "implement errno in a POSIX-conforming way". If you have threads and signals AND want to run C code, you pretty much have to do this. Your only choice with being non-conformant in that case is to deliberately pick a non-conforming implementation that still somehow doesn't get stomped by C code running concurrently.
There was a problem hiding this comment.
In practice the only other implementation of errno I would expect to see is the global version that gets stomped by running concurrently and thus cannot support threading.
| // This uses a `BTreeMap` instead of a hashmap since it supports constant | ||
| // initialization and automatically reduces the amount of memory used when | ||
| // items are removed. | ||
| static mut THREAD_INFO: BTreeMap<usize, ThreadInfo> = BTreeMap::new(); |
There was a problem hiding this comment.
Maybe s/static mut/ with a SyncUnsafeCell or something like that?
There was a problem hiding this comment.
I don't feel strongly about this, but I didn't think it any clearer...
| // items are removed. | ||
| static mut THREAD_INFO: BTreeMap<usize, ThreadInfo> = BTreeMap::new(); | ||
|
|
||
| struct UnlockOnDrop; |
There was a problem hiding this comment.
Could we make this be SpinGuard<'a, BTreeMap<usize, ThreadInfo>> or so?
There was a problem hiding this comment.
That would necessitate Deref and DerefMut implementations that aren't really adding any clarity.
| } | ||
| } | ||
|
|
||
| pub fn set_current_info(guard_page_range: Range<usize>, thread_name: Option<Box<str>>) { |
There was a problem hiding this comment.
nit: why not take the ThreadInfo struct as the argument?
There was a problem hiding this comment.
I thought the struct creation to be unnecessary clutter at the call site.
Are we OK with that guarantee propagating out to "userspace"? I guess it's sort of unavoidable if we want some kind of lock at thread creation/death but it feels a bit like that should be a more intentional decision? I guess it's undocumented but might still be a bit hard to claw back if we found a way to eliminate the lock later. |
I've added a helper function that abstracts all of the |
This comment has been minimized.
This comment has been minimized.
|
Hm, I guess that is the other option. It'd also let us remove some other hacks from Miri probably, since we don't really support the functions that code uses. OTOH it does leave some code without our usual UB test coverage. |
TLS is not async-signal-safe, making its use in the signal handler used to detect stack overflows unsound (c.f. rust-lang#133698). POSIX however lists two thread-specific identifiers that can be obtained in a signal handler: the current `pthread_t` and the address of `errno`. Since `pthread_equal` is not AS-safe, `pthread_t` should be considered opaque, so for our purposes, `&errno` is the only option. This however works nicely: we can use the address as a key into a map that stores information for each thread. This PR uses a `BTreeMap` protected by a spin lock to hold the guard page address and thread name and thus fixes rust-lang#133698.
| mod thread_info; | ||
|
|
||
| // miri doesn't model signals, and this code has some synchronization properties | ||
| // that we don't want to expose to user code. |
There was a problem hiding this comment.
| // that we don't want to expose to user code. | |
| // that we don't want to expose to user code. Also, Miri fundamentally cannot even have a stack overflow. |
TLS is not async-signal-safe, making its use in the signal handler used to detect stack overflows unsound (c.f. #133698). POSIX however lists two thread-specific identifiers that can be obtained in a signal handler: the current
pthread_tand the address oferrno. Sincepthread_equalis not AS-safe,pthread_tshould be considered opaque, so for our purposes,&errnois the only option. This however works nicely: we can use the address as a key into a map that stores information for each thread. This PR uses aBTreeMapprotected by a spin lock to hold the guard page address and thread name and thus fixes #133698.