Skip to content
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

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 17 additions & 21 deletions library/std/src/sys/exit_guard.rs
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.
Expand All @@ -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;
Copy link
Member

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?

Copy link
Member

@jieyouxu jieyouxu Jul 17, 2024

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:

POSIX.1 allows an implementation wide freedom in choosing the
type used to represent a thread ID; for example, representation
using either an arithmetic type or a structure is permitted.
Therefore, variables of type pthread_t can't portably be compared
using the C equality operator (==); use pthread_equal(3) instead.

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 probably pthread_equal (or a PartialEq implementation based on that) should be used instead.

Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 probably pthread_equal (or a PartialEq implementation based on that) should be used instead.

AFAIK the passage you quoted was added because C can't use == for structs. We can see that none of the targets supported by libc use a struct for pthread_t. I think we should disregard this section of the man page.

Copy link
Contributor

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 because libc does not (yet) expose pthread_equal. glibc and musl at least both appear to implement pthread_equal as just == from what I found.

This comment was marked as resolved.

Copy link
Member

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 after fork anyway.

Copy link
Member

@joboet joboet Jul 20, 2024

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].

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 => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fuzzy-provenance cast, at least on musl. Please use AtomicPtr instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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 atexit, since the exposed provenance model isn't finalized yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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? pthread_t is an integer on some platforms and a pointer on others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the strict provenance APIs. I.e. addr

// 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(); }
Expand Down
Loading