std: Fix unsoundness of std::thread::ScopedKey #25952
Conversation
Currently the compiler has no knowledge of `#[thread_local]` which forces users to take on two burdens of unsafety: * The lifetime of the borrow of a `#[thread_local]` static is **not** `'static` * Types in `static`s are required to be `Sync` The thread-local modules mostly curb these facets of unsafety by only allowing very limited scopes of borrows as well as allowing all types to be stored in a thread-local key (regardless of whether they are `Sync`) through an `unsafe impl`. Unfortunately these measures have the consequence of being able to take the address of the key itself and send it to another thread, allowing the same key to be accessed from two different threads. This is clearly unsafe, and this commit fixes this problem with the same trick used by `LocalKey`, which is to have an indirect function call to find the address of the *current thread's* thread local. This way the address of thread local keys can safely be sent among threads as their lifetime truly is `'static`. This commit will reduce the performance of cross-crate scoped thread locals as it now requires an indirect function call, but this can likely be overcome in a future commit. Closes rust-lang#25894
|
r? @aturon |
|
r? @brson (rust_highfive has picked a reviewer for you, use r? to override) |
|
Not a fan of decreasing the performance of thread locals. At all. "this can likely be overcome in a future commit" does not sound promising. Hm, I suppose as long as this only affects the tls module and not the low level implementation I'm okay with it. |
To be clear, this only affects the performance of cross-crate scoped thread locals. This is an
By this I mean implementing a form of inlining for statics where the function accessor is inlined into external crates as, likely using |
alexcrichton
added
the
T-libs-api
|
Sorry for the delay... The future of scoped thread locals is a little unclear; to the best of my knowledge they are barely used. So I think it's fine to take a perf hit for now, gaining soundness, and then continue to iterate on both the API and implementation over time. It's all unstable anyhow. @bors: r+ |
|
📌 Commit 4d3cffa has been approved by |
|
⌛ Testing commit 4d3cffa with merge 3fdfef6... |
|
⛄ The build was interrupted to prioritize another pull request. |
Currently the compiler has no knowledge of `#[thread_local]` which forces users to take on two burdens of unsafety: * The lifetime of the borrow of a `#[thread_local]` static is **not** `'static` * Types in `static`s are required to be `Sync` The thread-local modules mostly curb these facets of unsafety by only allowing very limited scopes of borrows as well as allowing all types to be stored in a thread-local key (regardless of whether they are `Sync`) through an `unsafe impl`. Unfortunately these measures have the consequence of being able to take the address of the key itself and send it to another thread, allowing the same key to be accessed from two different threads. This is clearly unsafe, and this commit fixes this problem with the same trick used by `LocalKey`, which is to have an indirect function call to find the address of the *current thread's* thread local. This way the address of thread local keys can safely be sent among threads as their lifetime truly is `'static`. This commit will reduce the performance of cross-crate scoped thread locals as it now requires an indirect function call, but this can likely be overcome in a future commit. Closes #25894
Currently the compiler has no knowledge of
#[thread_local]which forces usersto take on two burdens of unsafety:
#[thread_local]static is not'staticstatics are required to beSyncThe thread-local modules mostly curb these facets of unsafety by only allowing
very limited scopes of borrows as well as allowing all types to be stored in a
thread-local key (regardless of whether they are
Sync) through anunsafe impl.Unfortunately these measures have the consequence of being able to take the
address of the key itself and send it to another thread, allowing the same key
to be accessed from two different threads. This is clearly unsafe, and this
commit fixes this problem with the same trick used by
LocalKey, which is tohave an indirect function call to find the address of the current thread's
thread local. This way the address of thread local keys can safely be sent among
threads as their lifetime truly is
'static.This commit will reduce the performance of cross-crate scoped thread locals as
it now requires an indirect function call, but this can likely be overcome in a
future commit.
Closes #25894