Auto merge of #116402 - joboet:global_alloc_tls_unsoundness, r=thomcc

Panic when the global allocator tries to register a TLS destructor Using a `RefCell` avoids the undefined behaviour encountered in #116390 and reduces the amount of `unsafe` code in the codebase.
rust-lang · Oct 5, 2023 · a64a576 · a64a576
2 parents cae0791 + 7953b56
commit a64a576
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 29 deletions.
diff --git a/library/std/src/sys/hermit/thread_local_dtor.rs b/library/std/src/sys/hermit/thread_local_dtor.rs
@@ -5,23 +5,25 @@
 // 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<Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>> = 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));
+    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
 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
@@ -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<bool> = Cell::new(false);
 
 #[thread_local]
-static mut DTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new();
+static DTORS: RefCell<Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>> = 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,20 @@ 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));
+    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() {
-    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
@@ -69,15 +69,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<bool> = Cell::new(false);
 
     #[thread_local]
-    static mut DTORS: Vec<(*mut u8, unsafe extern "C" fn(*mut u8))> = Vec::new();
+    static DTORS: RefCell<Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>> = RefCell::new(Vec::new());
 
     if !REGISTERED.get() {
         _tlv_atexit(run_dtors, ptr::null_mut());
@@ -88,16 +87,18 @@ 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));
+    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 = 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
@@ -16,14 +16,19 @@ 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<Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>> =
+    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));
+    match DESTRUCTORS.try_borrow_mut() {
+        Ok(mut dtors) => dtors.push((t, dtor)),
+        Err(_) => rtabort!("global allocator may not use TLS"),
+    }
+
     HAS_DTORS.store(true, Relaxed);
 }
 
@@ -37,11 +42,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
@@ -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,23 @@ 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<Vec<(*mut u8, unsafe extern "C" fn(*mut u8))>>;
     if DTORS.get().is_null() {
-        let v: Box<List> = Box::new(Vec::new());
+        let v: Box<List> = 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);
+    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() {
-            let list: Box<List> = 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);
             }