Skip to content

Commit

Permalink
Auto merge of #116402 - joboet:global_alloc_tls_unsoundness, r=thomcc…
Browse files Browse the repository at this point in the history
…,workingjubilee

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.
  • Loading branch information
bors committed Oct 19, 2023
2 parents 0039d73 + 88efb1b commit 89432aa
Show file tree
Hide file tree
Showing 5 changed files with 51 additions and 29 deletions.
14 changes: 8 additions & 6 deletions library/std/src/sys/hermit/thread_local_dtor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
15 changes: 8 additions & 7 deletions library/std/src/sys/solid/thread_local_dtor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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();
}
}

Expand Down
15 changes: 8 additions & 7 deletions library/std/src/sys/unix/thread_local_dtor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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", target_os = "tvos"))]
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());
Expand All @@ -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();
}
}
}
Expand Down
19 changes: 15 additions & 4 deletions library/std/src/sys/windows/thread_local_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -37,11 +42,17 @@ 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() {
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;
};
(dtor)(ptr);
}
// We're done so free the memory.
DESTRUCTORS = Vec::new();
DESTRUCTORS.replace(Vec::new());
}

type Key = c::DWORD;
Expand Down
17 changes: 12 additions & 5 deletions library/std/src/sys_common/thread_local_dtor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);
}
Expand Down

0 comments on commit 89432aa

Please sign in to comment.