Skip to content

Commit

Permalink
clarify partially initialized Mutex issues
Browse files Browse the repository at this point in the history
  • Loading branch information
RalfJung committed Aug 6, 2018
1 parent 7c98d2e commit d3d3110
Show file tree
Hide file tree
Showing 8 changed files with 33 additions and 2 deletions.
7 changes: 7 additions & 0 deletions src/libstd/io/lazy.rs
Expand Up @@ -27,6 +27,9 @@ unsafe impl<T> Sync for Lazy<T> {}

impl<T: Send + Sync + 'static> Lazy<T> {
pub const fn new(init: fn() -> Arc<T>) -> Lazy<T> {
// `lock` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
Lazy {
lock: Mutex::new(),
ptr: Cell::new(ptr::null_mut()),
Expand All @@ -48,6 +51,7 @@ impl<T: Send + Sync + 'static> Lazy<T> {
}
}

// Must only be called with `lock` held
unsafe fn init(&'static self) -> Arc<T> {
// If we successfully register an at exit handler, then we cache the
// `Arc` allocation in our own internal box (it will get deallocated by
Expand All @@ -60,6 +64,9 @@ impl<T: Send + Sync + 'static> Lazy<T> {
};
drop(Box::from_raw(ptr))
});
// This could reentrantly call `init` again, which is a problem
// because our `lock` allows reentrancy!
// FIXME: Add argument why this is okay.
let ret = (self.init)();
if registered.is_ok() {
self.ptr.set(Box::into_raw(Box::new(ret.clone())));
Expand Down
3 changes: 3 additions & 0 deletions src/libstd/sys/unix/args.rs
Expand Up @@ -80,6 +80,9 @@ mod imp {

static mut ARGC: isize = 0;
static mut ARGV: *const *const u8 = ptr::null();
// `ENV_LOCK` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
static LOCK: Mutex = Mutex::new();

pub unsafe fn init(argc: isize, argv: *const *const u8) {
Expand Down
6 changes: 4 additions & 2 deletions src/libstd/sys/unix/mutex.rs
Expand Up @@ -25,8 +25,10 @@ unsafe impl Sync for Mutex {}
#[allow(dead_code)] // sys isn't exported yet
impl Mutex {
pub const fn new() -> Mutex {
// Might be moved and address is changing it is better to avoid
// initialization of potentially opaque OS data before it landed
// Might be moved to a different address, so it is better to avoid
// initialization of potentially opaque OS data before it landed.
// Be very careful using this newly constructed `Mutex`, it should
// be initialized by calling `init()` first!
Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
}
#[inline]
Expand Down
3 changes: 3 additions & 0 deletions src/libstd/sys/unix/os.rs
Expand Up @@ -33,6 +33,9 @@ use sys::fd;
use vec;

const TMPBUF_SZ: usize = 128;
// `ENV_LOCK` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
static ENV_LOCK: Mutex = Mutex::new();


Expand Down
6 changes: 6 additions & 0 deletions src/libstd/sys_common/at_exit_imp.rs
Expand Up @@ -23,6 +23,9 @@ type Queue = Vec<Box<dyn FnBox()>>;
// on poisoning and this module needs to operate at a lower level than requiring
// the thread infrastructure to be in place (useful on the borders of
// initialization/destruction).
// `LOCK` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
static LOCK: Mutex = Mutex::new();
static mut QUEUE: *mut Queue = ptr::null_mut();

Expand Down Expand Up @@ -72,6 +75,9 @@ pub fn push(f: Box<dyn FnBox()>) -> bool {
unsafe {
let _guard = LOCK.lock();
if init() {
// This could reentrantly call `push` again, which is a problem because
// `LOCK` allows reentrancy!
// FIXME: Add argument why this is okay.
(*QUEUE).push(f);
true
} else {
Expand Down
4 changes: 4 additions & 0 deletions src/libstd/sys_common/mutex.rs
Expand Up @@ -24,11 +24,15 @@ impl Mutex {
///
/// Behavior is undefined if the mutex is moved after it is
/// first used with any of the functions below.
/// Also, the mutex might not be fully functional without calling
/// `init`! For example, on unix, the mutex is reentrant
/// until `init` reconfigures it appropriately.
pub const fn new() -> Mutex { Mutex(imp::Mutex::new()) }

/// Prepare the mutex for use.
///
/// This should be called once the mutex is at a stable memory address.
/// It must not be called concurrently with any other operation.
#[inline]
pub unsafe fn init(&mut self) { self.0.init() }

Expand Down
3 changes: 3 additions & 0 deletions src/libstd/sys_common/thread_local.rs
Expand Up @@ -161,6 +161,9 @@ impl StaticKey {
// Additionally a 0-index of a tls key hasn't been seen on windows, so
// we just simplify the whole branch.
if imp::requires_synchronized_create() {
// `INIT_LOCK` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
static INIT_LOCK: Mutex = Mutex::new();
let _guard = INIT_LOCK.lock();
let mut key = self.key.load(Ordering::SeqCst);
Expand Down
3 changes: 3 additions & 0 deletions src/libstd/thread/mod.rs
Expand Up @@ -940,6 +940,9 @@ pub struct ThreadId(u64);
impl ThreadId {
// Generate a new unique thread ID.
fn new() -> ThreadId {
// `GUARD` is never initialized fully, so this mutex is reentrant!
// Do not use it in a way that might be reentrant, that could lead to
// aliasing `&mut`.
static GUARD: mutex::Mutex = mutex::Mutex::new();
static mut COUNTER: u64 = 0;

Expand Down

0 comments on commit d3d3110

Please sign in to comment.