-
Notifications
You must be signed in to change notification settings - Fork 12.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use atomics instead of mutex in exit guard #127863
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,8 @@ | ||
cfg_if::cfg_if! { | ||
if #[cfg(target_os = "linux")] { | ||
/// pthread_t is a pointer on some platforms, | ||
/// so we wrap it in this to impl Send + Sync. | ||
#[derive(Clone, Copy)] | ||
#[repr(transparent)] | ||
struct PThread(libc::pthread_t); | ||
// Safety: pthread_t is safe to send between threads | ||
unsafe impl Send for PThread {} | ||
// Safety: pthread_t is safe to share between threads | ||
unsafe impl Sync for PThread {} | ||
use crate::mem; | ||
use crate::sync::atomic::{AtomicUsize, Ordering}; | ||
|
||
/// Mitigation for <https://github.com/rust-lang/rust/issues/126600> | ||
/// | ||
/// On glibc, `libc::exit` has been observed to not always be thread-safe. | ||
|
@@ -31,27 +25,29 @@ cfg_if::cfg_if! { | |
#[cfg_attr(any(test, doctest), allow(dead_code))] | ||
pub(crate) fn unique_thread_exit() { | ||
let this_thread_id = unsafe { libc::pthread_self() }; | ||
use crate::sync::{Mutex, PoisonError}; | ||
static EXITING_THREAD_ID: Mutex<Option<PThread>> = Mutex::new(None); | ||
let mut exiting_thread_id = | ||
EXITING_THREAD_ID.lock().unwrap_or_else(PoisonError::into_inner); | ||
match *exiting_thread_id { | ||
None => { | ||
|
||
const _: () = assert!(mem::size_of::<libc::pthread_t>() <= mem::size_of::<usize>()); | ||
|
||
const NONE: usize = 0; | ||
static EXITING_THREAD_ID: AtomicUsize = AtomicUsize::new(NONE); | ||
match EXITING_THREAD_ID.compare_exchange(NONE, this_thread_id as usize, Ordering::Relaxed, Ordering::Relaxed).unwrap_or_else(|id| id) { | ||
NONE => { | ||
// This is the first thread to call `unique_thread_exit`, | ||
// and this is the first time it is called. | ||
// Set EXITING_THREAD_ID to this thread's ID and return. | ||
*exiting_thread_id = Some(PThread(this_thread_id)); | ||
}, | ||
Some(exiting_thread_id) if exiting_thread_id.0 == this_thread_id => { | ||
// | ||
// We set `EXITING_THREAD_ID` to this thread's ID already | ||
// and will return. | ||
} | ||
id if id == this_thread_id as usize => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a fuzzy-provenance cast, at least on musl. Please use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not using the value as a pointer again. Is it still required in this case? Do you have a link where I can read up on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. Casting a pointer to an integer is a side-effecting operation. Also, miri will warn against this once it supports There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is casting an arbitrary integer to an (invalid) pointer value a problem under this model? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could use the strict provenance APIs. I.e. |
||
// This is the first thread to call `unique_thread_exit`, | ||
// but this is the second time it is called. | ||
// | ||
// Abort the process. | ||
core::panicking::panic_nounwind("std::process::exit called re-entrantly") | ||
} | ||
Some(_) => { | ||
_ => { | ||
// This is not the first thread to call `unique_thread_exit`. | ||
// Pause until the process exits. | ||
drop(exiting_thread_id); | ||
loop { | ||
// Safety: libc::pause is safe to call. | ||
unsafe { libc::pause(); } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you confirmed that zero is definitely not an allowed
pthread_t
value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://man7.org/linux/man-pages/man3/pthread_equal.3.html:
I don't think it's a good idea to assume
pthread_t
can be trivially compared as integers. I can't immediately tell if the original implementation uses integer equality, but probablypthread_equal
(or a PartialEq implementation based on that) should be used instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in retrospect I should have insisted on that in #126606. We only use this on Linux, so this currently works. In the long-term, I think
thread::current().id()
is the best solution, but that's currently not available in TLS destructors (#124881 will improve that situation and I have an idea for solving this completely).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK the passage you quoted was added because C can't use
==
forstruct
s. We can see that none of the targets supported bylibc
use astruct
forpthread_t
. I think we should disregard this section of the man page.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used
==
in the original PR mostly just becauselibc
does not (yet) exposepthread_equal
. glibc and musl at least both appear to implementpthread_equal
as just==
from what I found.This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exit
isn't signal-safe, so it mustn't be called afterfork
anyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated #127912 to include a change that makes
thread::current_id
always succeed and always return the same ID. Once that gets merged, we can use it here.EDIT: nevermind,
current_id
always returns the same ID on Linux anyway because it supports#[thread_local]
.