Browse Source

Fix reentrant locking behavior in thread keys

The standard library RwLock checks if it's panicking while making the
guard. This check needs to use a thread local key, which it lazily
creates. This causes reentrant/recursive locking, which leads to a
deadlock.

This commit replaces the std RwLock with one from spin, which does not
need to check thread local storage to lock. It's a spin-lock, but it is
configured to yield the thread on every unsuccessful spin. Also, the
lock can be created statically, which means we don't need once_cell.

An alternative to spin is parking_lot, but that has some compile errors
(it wants condvars to support monotonic clocks) and brings in more
dependencies.
pull/6/head
AzureMarker 3 years ago
parent
commit
fdd1b58452
No known key found for this signature in database
GPG Key ID: 47A133F3BF9D03D3
  1. 2
      Cargo.toml
  2. 20
      src/lib.rs

2
Cargo.toml

@ -7,4 +7,4 @@ edition = "2021" @@ -7,4 +7,4 @@ edition = "2021"
[dependencies]
libc = "0.2.116"
once_cell = "1.0"
spin = { version = "0.9", default-features = false, features = ["rwlock", "std"] }

20
src/lib.rs

@ -399,26 +399,24 @@ pub unsafe extern "C" fn pthread_rwlockattr_destroy( @@ -399,26 +399,24 @@ pub unsafe extern "C" fn pthread_rwlockattr_destroy(
// THREAD KEYS IMPLEMENTATION FOR RUST STD
use once_cell::sync::Lazy;
use spin::rwlock::RwLock;
use std::collections::BTreeMap;
use std::ptr;
use std::sync::atomic::{AtomicUsize, Ordering};
use std::sync::{PoisonError, RwLock};
type Key = usize;
type Destructor = unsafe extern "C" fn(*mut libc::c_void);
static NEXT_KEY: AtomicUsize = AtomicUsize::new(1);
static KEYS: Lazy<RwLock<BTreeMap<Key, Option<Destructor>>>> = Lazy::new(RwLock::default);
// This is a spin-lock RwLock which yields the thread every loop
static KEYS: RwLock<BTreeMap<Key, Option<Destructor>>, spin::Yield> = RwLock::new(BTreeMap::new());
#[thread_local]
static mut LOCALS: BTreeMap<Key, *mut libc::c_void> = BTreeMap::new();
fn is_valid_key(key: Key) -> bool {
KEYS.read()
.unwrap_or_else(PoisonError::into_inner)
.contains_key(&(key as Key))
KEYS.read().contains_key(&(key as Key))
}
#[no_mangle]
@ -427,9 +425,7 @@ pub unsafe extern "C" fn pthread_key_create( @@ -427,9 +425,7 @@ pub unsafe extern "C" fn pthread_key_create(
destructor: Option<Destructor>,
) -> libc::c_int {
let new_key = NEXT_KEY.fetch_add(1, Ordering::SeqCst);
KEYS.write()
.unwrap_or_else(PoisonError::into_inner)
.insert(new_key, destructor);
KEYS.write().insert(new_key, destructor);
*key = new_key as libc::pthread_key_t;
@ -438,11 +434,7 @@ pub unsafe extern "C" fn pthread_key_create( @@ -438,11 +434,7 @@ pub unsafe extern "C" fn pthread_key_create(
#[no_mangle]
pub unsafe extern "C" fn pthread_key_delete(key: libc::pthread_key_t) -> libc::c_int {
match KEYS
.write()
.unwrap_or_else(PoisonError::into_inner)
.remove(&(key as Key))
{
match KEYS.write().remove(&(key as Key)) {
// We had a entry, so it was a valid key.
// It's officially undefined behavior if they use the key after this,
// so don't worry about cleaning up LOCALS, especially since we can't

Loading…
Cancel
Save