From d3d31105e99f5265880d0f010436ed5c6baab034 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 6 Aug 2018 12:45:27 +0200 Subject: [PATCH] clarify partially initialized Mutex issues --- src/libstd/io/lazy.rs | 7 +++++++ src/libstd/sys/unix/args.rs | 3 +++ src/libstd/sys/unix/mutex.rs | 6 ++++-- src/libstd/sys/unix/os.rs | 3 +++ src/libstd/sys_common/at_exit_imp.rs | 6 ++++++ src/libstd/sys_common/mutex.rs | 4 ++++ src/libstd/sys_common/thread_local.rs | 3 +++ src/libstd/thread/mod.rs | 3 +++ 8 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs index d357966be9227..09b2ddcbcac66 100644 --- a/src/libstd/io/lazy.rs +++ b/src/libstd/io/lazy.rs @@ -27,6 +27,9 @@ unsafe impl Sync for Lazy {} impl Lazy { pub const fn new(init: fn() -> Arc) -> Lazy { + // `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()), @@ -48,6 +51,7 @@ impl Lazy { } } + // Must only be called with `lock` held unsafe fn init(&'static self) -> Arc { // If we successfully register an at exit handler, then we cache the // `Arc` allocation in our own internal box (it will get deallocated by @@ -60,6 +64,9 @@ impl Lazy { }; 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()))); diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs index 7e32ec1347e9e..c91bd5b22afcb 100644 --- a/src/libstd/sys/unix/args.rs +++ b/src/libstd/sys/unix/args.rs @@ -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) { diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs index 52cf3f97c5c83..f0711b603204e 100644 --- a/src/libstd/sys/unix/mutex.rs +++ b/src/libstd/sys/unix/mutex.rs @@ -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] diff --git a/src/libstd/sys/unix/os.rs b/src/libstd/sys/unix/os.rs index 1d92e8fc97c7a..6ef9502ba624e 100644 --- a/src/libstd/sys/unix/os.rs +++ b/src/libstd/sys/unix/os.rs @@ -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(); diff --git a/src/libstd/sys_common/at_exit_imp.rs b/src/libstd/sys_common/at_exit_imp.rs index b28a4d2f8be01..633605039afdd 100644 --- a/src/libstd/sys_common/at_exit_imp.rs +++ b/src/libstd/sys_common/at_exit_imp.rs @@ -23,6 +23,9 @@ type Queue = Vec>; // 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(); @@ -72,6 +75,9 @@ pub fn push(f: Box) -> 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 { diff --git a/src/libstd/sys_common/mutex.rs b/src/libstd/sys_common/mutex.rs index 608355b7d70f8..77a33e4c6be86 100644 --- a/src/libstd/sys_common/mutex.rs +++ b/src/libstd/sys_common/mutex.rs @@ -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() } diff --git a/src/libstd/sys_common/thread_local.rs b/src/libstd/sys_common/thread_local.rs index 75f6b9ac7fd8c..2cc5372e26818 100644 --- a/src/libstd/sys_common/thread_local.rs +++ b/src/libstd/sys_common/thread_local.rs @@ -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); diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index bbe80df7e8bdb..98b4ca36c2633 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -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;