mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #112292 - thomcc:tls-ohno, r=m-ou-se

Browse Source 
Avoid unwind across `extern "C"` in `thread_local::fast_local`

This is a minimal fix for #112285, in case we want a simple patch that can be easily to backported if that's desirable.

*(Note: I have another broader cleanup which I've mostly omitted from here to avoid clutter, except for the `Cell` change, which isn't needed to fix UB, but simplifies safety comments).*

The only tier-1 target that this occurs on in a way that seems likely to cause problems in practice linux-gnu, although I believe some folks care about that platform somewhat 😉. I'm unsure how big of an issue this is. I've seen stuff like this behave quite badly, but there's a number of reasons to think this might actually be "fine in practice".

I've hedged my bets and assumed we'll backport this at least to beta but my feeling is that there's not enough evidence this is a problem worth backporting further than that.

### More details

This issue seems to have existed since `thread_local!`'s `const` init functionality was added. It occurs if you have a `const`-initialized thread local for a type that `needs_drop`, the drop panics, and you're on a target with support for static thread locals. In this case, we will end up defining an `extern "C"` function in the user crate rather than in libstd, and because the user crate will not have `#![feature(c_unwind)]` enabled, their panic will not be caught by an auto-inserted abort guard.

In practice, the actual situation where problems are likely[^ub] is somewhat narrower.

On most targets with static thread locals, we manage the TLS dtor list by hand (for reentrancy reasons among others). In these cases, while the users code may panic, we're calling it inside our own `extern "C"` (or `extern "system"`) function, which seems to (at least in practice) catch the panic and convert it to an abort.

However, on a few targets, most notably linux-gnu with recent glibc (but also fuchsia and redox), a tls dtor registration mechanism exists which we can actually use directly, [`__cxa_thread_atexit_impl`](https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/thread_local_dtor.rs#L26-L36).

This is the case that seems most likely to be a cause for concern, as now we're passing a function to the system library and panicking out of it in a case where there are may not be Rust frames above it on the call stack (since it's running thread shutdown), and even if there were, it may not be prepared to handle such unwinding. If that's the case, it'd be bad.

Is it? Dunno. The fact that it's a `__cxa_*` function makes me think they probably have considered that the callback could throw but I have no evidence here and it doesn't seem to be written down anywhere, so it's just a guess. (I would not be surprised if someone comes into this thread to tell me how definitely-bad-news it is).

That said, as I said, all this is actually UB! If this isn't a "technically UB but fine in practice", but all bets are off if this is the kind of thing we are telling LLVM about.

[^ub]: This is UB so take that with a grain of salt -- I'm absolutely making assumptions about how the UB will behave "in practice" here, which is almost certainly a mistake.
This commit is contained in:
bors 2023-06-08 04:44:08 +00:00
commit 80917360d3
3 changed files with 33 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

22
library/std/src/sys/common/thread_local/fast_local.rs
View File

@ -33,20 +33,21 @@ unsafe fn __getit(
// 1 == dtor registered, dtor not run
// 2 == dtor registered and is running or has run
#[thread_local]
static mut STATE: $crate::primitive::u8 = 0;
static STATE: $crate::cell::Cell<$crate::primitive::u8> = $crate::cell::Cell::new(0);
// Safety: Performs `drop_in_place(ptr as *mut $t)`, and requires
// all that comes with it.
unsafe extern "C" fn destroy(ptr: *mut $crate::primitive::u8) {
let ptr = ptr as *mut $t;
unsafe {
$crate::debug_assert_eq!(STATE, 1);
STATE = 2;
$crate::ptr::drop_in_place(ptr);
}
$crate::thread::local_impl::abort_on_dtor_unwind(|| {
let old_state = STATE.replace(2);
$crate::debug_assert_eq!(old_state, 1);
// Safety: safety requirement is passed on to caller.
unsafe { $crate::ptr::drop_in_place(ptr.cast::<$t>()); }
});
}
unsafe {
match STATE {
match STATE.get() {
// 0 == we haven't registered a destructor, so do
// so now.
0 => {
@ -54,7 +55,7 @@ unsafe fn __getit(
$crate::ptr::addr_of_mut!(VAL) as *mut $crate::primitive::u8,
destroy,
);
STATE = 1;
STATE.set(1);
$crate::option::Option::Some(&VAL)
}
// 1 == the destructor is registered and the value
@ -148,7 +149,6 @@ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
f.debug_struct("Key").finish_non_exhaustive()
}
}
impl<T> Key<T> {
pub const fn new() -> Key<T> {
Key { inner: LazyKeyInner::new(), dtor_state: Cell::new(DtorState::Unregistered) }

21
library/std/src/sys/common/thread_local/mod.rs
View File

@ -101,3 +101,24 @@ pub unsafe fn take(&mut self) -> Option<T> {
}
}
}
/// Run a callback in a scenario which must not unwind (such as a `extern "C"
/// fn` declared in a user crate). If the callback unwinds anyway, then
/// `rtabort` with a message about thread local panicking on drop.
#[inline]
pub fn abort_on_dtor_unwind(f: impl FnOnce()) {
// Using a guard like this is lower cost.
let guard = DtorUnwindGuard;
f();
core::mem::forget(guard);
struct DtorUnwindGuard;
impl Drop for DtorUnwindGuard {
#[inline]
fn drop(&mut self) {
// This is not terribly descriptive, but it doesn't need to be as we'll
// already have printed a panic message at this point.
rtabort!("thread local panicked on drop");
}
}
}

2
library/std/src/thread/mod.rs
View File

@ -206,7 +206,7 @@
#[doc(hidden)]
#[unstable(feature = "thread_local_internals", issue = "none")]
pub mod local_impl {
pub use crate::sys::common::thread_local::{thread_local_inner, Key};
pub use crate::sys::common::thread_local::{thread_local_inner, Key, abort_on_dtor_unwind};
}
}
}