From fd23276ca87b4b56918e4a87737859cfd77d657c Mon Sep 17 00:00:00 2001 From: joboet Date: Tue, 3 Oct 2023 23:13:55 +0200 Subject: [PATCH 1/4] std: panic when the global allocator tries to register a TLS destructor --- library/std/src/sys/hermit/thread_local_dtor.rs | 11 +++++------ library/std/src/sys/solid/thread_local_dtor.rs | 12 +++++------- library/std/src/sys/unix/thread_local_dtor.rs | 12 +++++------- library/std/src/sys/windows/thread_local_key.rs | 9 +++++---- library/std/src/sys_common/thread_local_dtor.rs | 14 +++++++++----- 5 files changed, 29 insertions(+), 29 deletions(-) diff --git a/library/std/src/sys/hermit/thread_local_dtor.rs b/library/std/src/sys/hermit/thread_local_dtor.rs index 613266b9530a8..89db78adffc6a 100644 --- a/library/std/src/sys/hermit/thread_local_dtor.rs +++ b/library/std/src/sys/hermit/thread_local_dtor.rs @@ -5,23 +5,22 @@ // The this solution works like the implementation of macOS and // doesn't additional OS support -use crate::mem; +use crate::cell::RefCell; #[thread_local] -static mut DTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new(); +static DTORS: RefCell> = RefCell::new(Vec::new()); pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { - let list = &mut DTORS; - list.push((t, dtor)); + DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); } // every thread call this function to run through all possible destructors pub unsafe fn run_dtors() { - let mut list = mem::take(&mut DTORS); + let mut list = DTORS.take(); while !list.is_empty() { for (ptr, dtor) in list { dtor(ptr); } - list = mem::take(&mut DTORS); + list = DTORS.take(); } } diff --git a/library/std/src/sys/solid/thread_local_dtor.rs b/library/std/src/sys/solid/thread_local_dtor.rs index bad14bb37f720..d0fecede9551f 100644 --- a/library/std/src/sys/solid/thread_local_dtor.rs +++ b/library/std/src/sys/solid/thread_local_dtor.rs @@ -4,14 +4,13 @@ // Simplify dtor registration by using a list of destructors. use super::{abi, itron::task}; -use crate::cell::Cell; -use crate::mem; +use crate::cell::{Cell, RefCell}; #[thread_local] static REGISTERED: Cell = Cell::new(false); #[thread_local] -static mut DTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new(); +static DTORS: RefCell> = RefCell::new(Vec::new()); pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { if !REGISTERED.get() { @@ -22,18 +21,17 @@ pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { REGISTERED.set(true); } - let list = unsafe { &mut DTORS }; - list.push((t, dtor)); + DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); } pub unsafe fn run_dtors() { - let mut list = mem::take(unsafe { &mut DTORS }); + let mut list = DTORS.take(); while !list.is_empty() { for (ptr, dtor) in list { unsafe { dtor(ptr) }; } - list = mem::take(unsafe { &mut DTORS }); + list = DTORS.take(); } } diff --git a/library/std/src/sys/unix/thread_local_dtor.rs b/library/std/src/sys/unix/thread_local_dtor.rs index fba2a676f280f..38d275de4c8d9 100644 --- a/library/std/src/sys/unix/thread_local_dtor.rs +++ b/library/std/src/sys/unix/thread_local_dtor.rs @@ -50,15 +50,14 @@ pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { // _tlv_atexit. #[cfg(any(target_os = "macos", target_os = "ios", target_os = "watchos"))] pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { - use crate::cell::Cell; - use crate::mem; + use crate::cell::{Cell, RefCell}; use crate::ptr; #[thread_local] static REGISTERED: Cell = Cell::new(false); #[thread_local] - static mut DTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new(); + static DTORS: RefCell> = RefCell::new(Vec::new()); if !REGISTERED.get() { _tlv_atexit(run_dtors, ptr::null_mut()); @@ -69,16 +68,15 @@ pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { fn _tlv_atexit(dtor: unsafe extern "C" fn(*mut u8), arg: *mut u8); } - let list = &mut DTORS; - list.push((t, dtor)); + DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); unsafe extern "C" fn run_dtors(_: *mut u8) { - let mut list = mem::take(&mut DTORS); + let mut list = DTORS.take(); while !list.is_empty() { for (ptr, dtor) in list { dtor(ptr); } - list = mem::take(&mut DTORS); + list = DTORS.take(); } } } diff --git a/library/std/src/sys/windows/thread_local_key.rs b/library/std/src/sys/windows/thread_local_key.rs index 036d96596e9c0..dd2c46fdfbc38 100644 --- a/library/std/src/sys/windows/thread_local_key.rs +++ b/library/std/src/sys/windows/thread_local_key.rs @@ -16,14 +16,15 @@ static HAS_DTORS: AtomicBool = AtomicBool::new(false); // Using a per-thread list avoids the problems in synchronizing global state. #[thread_local] #[cfg(target_thread_local)] -static mut DESTRUCTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new(); +static DESTRUCTORS: crate::cell::RefCell> = + crate::cell::RefCell::new(Vec::new()); // Ensure this can never be inlined because otherwise this may break in dylibs. // See #44391. #[inline(never)] #[cfg(target_thread_local)] pub unsafe fn register_keyless_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { - DESTRUCTORS.push((t, dtor)); + DESTRUCTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); HAS_DTORS.store(true, Relaxed); } @@ -37,11 +38,11 @@ unsafe fn run_keyless_dtors() { // the case that this loop always terminates because we provide the // guarantee that a TLS key cannot be set after it is flagged for // destruction. - while let Some((ptr, dtor)) = DESTRUCTORS.pop() { + while let Some((ptr, dtor)) = DESTRUCTORS.borrow_mut().pop() { (dtor)(ptr); } // We're done so free the memory. - DESTRUCTORS = Vec::new(); + DESTRUCTORS.replace(Vec::new()); } type Key = c::DWORD; diff --git a/library/std/src/sys_common/thread_local_dtor.rs b/library/std/src/sys_common/thread_local_dtor.rs index 844946eda031f..5d15140a9da84 100644 --- a/library/std/src/sys_common/thread_local_dtor.rs +++ b/library/std/src/sys_common/thread_local_dtor.rs @@ -13,6 +13,7 @@ #![unstable(feature = "thread_local_internals", issue = "none")] #![allow(dead_code)] +use crate::cell::RefCell; use crate::ptr; use crate::sys_common::thread_local_key::StaticKey; @@ -28,17 +29,20 @@ pub unsafe fn register_dtor_fallback(t: *mut u8, dtor: unsafe extern "C" fn(*mut // flagged for destruction. static DTORS: StaticKey = StaticKey::new(Some(run_dtors)); - type List = Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>; + // FIXME(joboet): integrate RefCell into pointer to avoid infinite recursion + // when the global allocator tries to register a destructor and just panic + // instead. + type List = RefCell>; if DTORS.get().is_null() { - let v: Box = Box::new(Vec::new()); + let v: Box = Box::new(RefCell::new(Vec::new())); DTORS.set(Box::into_raw(v) as *mut u8); } - let list: &mut List = &mut *(DTORS.get() as *mut List); - list.push((t, dtor)); + let list = &*(DTORS.get() as *const List); + list.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); unsafe extern "C" fn run_dtors(mut ptr: *mut u8) { while !ptr.is_null() { - let list: Box = Box::from_raw(ptr as *mut List); + let list = Box::from_raw(ptr as *mut List).into_inner(); for (ptr, dtor) in list.into_iter() { dtor(ptr); } From b18990b1e973a1fd0a0318a88655dc1907509b17 Mon Sep 17 00:00:00 2001 From: joboet Date: Wed, 4 Oct 2023 11:49:48 +0200 Subject: [PATCH 2/4] std: abort instead of panicking if the global allocator uses TLS --- library/std/src/sys/hermit/thread_local_dtor.rs | 5 ++++- library/std/src/sys/solid/thread_local_dtor.rs | 5 ++++- library/std/src/sys/unix/thread_local_dtor.rs | 5 ++++- library/std/src/sys/windows/thread_local_key.rs | 6 +++++- library/std/src/sys_common/thread_local_dtor.rs | 5 ++++- 5 files changed, 21 insertions(+), 5 deletions(-) diff --git a/library/std/src/sys/hermit/thread_local_dtor.rs b/library/std/src/sys/hermit/thread_local_dtor.rs index 89db78adffc6a..98adaf4bff1aa 100644 --- a/library/std/src/sys/hermit/thread_local_dtor.rs +++ b/library/std/src/sys/hermit/thread_local_dtor.rs @@ -11,7 +11,10 @@ use crate::cell::RefCell; static DTORS: RefCell> = RefCell::new(Vec::new()); pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { - DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); + match DTORS.try_borrow_mut() { + Ok(mut dtors) => dtors.push((t, dtor)), + Err(_) => rtabort!("global allocator may not use TLS"), + } } // every thread call this function to run through all possible destructors diff --git a/library/std/src/sys/solid/thread_local_dtor.rs b/library/std/src/sys/solid/thread_local_dtor.rs index d0fecede9551f..26918a4fcb012 100644 --- a/library/std/src/sys/solid/thread_local_dtor.rs +++ b/library/std/src/sys/solid/thread_local_dtor.rs @@ -21,7 +21,10 @@ pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { REGISTERED.set(true); } - DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); + match DTORS.try_borrow_mut() { + Ok(mut dtors) => dtors.push((t, dtor)), + Err(_) => rtabort!("global allocator may not use TLS"), + } } pub unsafe fn run_dtors() { diff --git a/library/std/src/sys/unix/thread_local_dtor.rs b/library/std/src/sys/unix/thread_local_dtor.rs index 38d275de4c8d9..58763e9fce262 100644 --- a/library/std/src/sys/unix/thread_local_dtor.rs +++ b/library/std/src/sys/unix/thread_local_dtor.rs @@ -68,7 +68,10 @@ pub unsafe fn register_dtor(t: *mut u8, dtor: unsafe extern "C" fn(*mut u8)) { fn _tlv_atexit(dtor: unsafe extern "C" fn(*mut u8), arg: *mut u8); } - DTORS.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); + match DTORS.try_borrow_mut() { + Ok(mut dtors) => dtors.push((t, dtor)), + Err(_) => rtabort!("global allocator may not use TLS"), + } unsafe extern "C" fn run_dtors(_: *mut u8) { let mut list = DTORS.take(); diff --git a/library/std/src/sys/windows/thread_local_key.rs b/library/std/src/sys/windows/thread_local_key.rs index dd2c46fdfbc38..3c4e1abbba936 100644 --- a/library/std/src/sys/windows/thread_local_key.rs +++ b/library/std/src/sys/windows/thread_local_key.rs @@ -24,7 +24,11 @@ static DESTRUCTORS: crate::cell::RefCell dtors.push((t, dtor)), + Err(_) => rtabort!("global allocator may not use TLS"), + } + HAS_DTORS.store(true, Relaxed); } diff --git a/library/std/src/sys_common/thread_local_dtor.rs b/library/std/src/sys_common/thread_local_dtor.rs index 5d15140a9da84..98382fc6acc23 100644 --- a/library/std/src/sys_common/thread_local_dtor.rs +++ b/library/std/src/sys_common/thread_local_dtor.rs @@ -38,7 +38,10 @@ pub unsafe fn register_dtor_fallback(t: *mut u8, dtor: unsafe extern "C" fn(*mut DTORS.set(Box::into_raw(v) as *mut u8); } let list = &*(DTORS.get() as *const List); - list.try_borrow_mut().expect("global allocator may not use TLS").push((t, dtor)); + match list.try_borrow_mut() { + Ok(mut dtors) => dtors.push((t, dtor)), + Err(_) => rtabort!("global allocator may not use TLS"), + } unsafe extern "C" fn run_dtors(mut ptr: *mut u8) { while !ptr.is_null() { From 65c66a15bf99efc75ea855733b94881953a6b9e8 Mon Sep 17 00:00:00 2001 From: joboet Date: Thu, 5 Oct 2023 10:58:39 +0200 Subject: [PATCH 3/4] std: fix registering of Windows TLS destructors --- library/std/src/sys/windows/thread_local_key.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/library/std/src/sys/windows/thread_local_key.rs b/library/std/src/sys/windows/thread_local_key.rs index 3c4e1abbba936..831f39817f4d2 100644 --- a/library/std/src/sys/windows/thread_local_key.rs +++ b/library/std/src/sys/windows/thread_local_key.rs @@ -42,7 +42,10 @@ unsafe fn run_keyless_dtors() { // the case that this loop always terminates because we provide the // guarantee that a TLS key cannot be set after it is flagged for // destruction. - while let Some((ptr, dtor)) = DESTRUCTORS.borrow_mut().pop() { + loop { + let Some((ptr, dtor)) = DESTRUCTORS.borrow_mut().pop() else { + break; + }; (dtor)(ptr); } // We're done so free the memory. From 88efb1bdefc61289ab55e0483e6af36eebf8e8b8 Mon Sep 17 00:00:00 2001 From: joboet Date: Mon, 9 Oct 2023 15:08:49 +0200 Subject: [PATCH 4/4] std: explain unconventional choice of let-else binding over while-let loop --- library/std/src/sys/windows/thread_local_key.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/library/std/src/sys/windows/thread_local_key.rs b/library/std/src/sys/windows/thread_local_key.rs index 831f39817f4d2..5eee4a9667ba4 100644 --- a/library/std/src/sys/windows/thread_local_key.rs +++ b/library/std/src/sys/windows/thread_local_key.rs @@ -43,6 +43,9 @@ unsafe fn run_keyless_dtors() { // guarantee that a TLS key cannot be set after it is flagged for // destruction. loop { + // Use a let-else binding to ensure the `RefCell` guard is dropped + // immediately. Otherwise, a panic would occur if a TLS destructor + // tries to access the list. let Some((ptr, dtor)) = DESTRUCTORS.borrow_mut().pop() else { break; };