From d3d31105e99f5265880d0f010436ed5c6baab034 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 6 Aug 2018 12:45:27 +0200 Subject: clarify partially initialized Mutex issues --- src/libstd/sys/unix/args.rs | 3 +++ src/libstd/sys/unix/mutex.rs | 6 ++++-- src/libstd/sys/unix/os.rs | 3 +++ 3 files changed, 10 insertions(+), 2 deletions(-) (limited to 'src/libstd/sys') diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs index 7e32ec1347e..c91bd5b22af 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 52cf3f97c5c..f0711b60320 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 1d92e8fc97c..6ef9502ba62 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(); -- cgit 1.4.1-3-g733a5 From 645388583ca47357a6a2e5878a9cde84e2e579d3 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Mon, 6 Aug 2018 14:39:55 +0200 Subject: actually, reentrant uninitialized mutex acquisition is outright UB --- src/libstd/io/lazy.rs | 5 ++--- src/libstd/sys/unix/args.rs | 5 ++--- src/libstd/sys/unix/os.rs | 5 ++--- src/libstd/sys_common/at_exit_imp.rs | 5 ++--- src/libstd/sys_common/mutex.rs | 6 +++--- src/libstd/sys_common/thread_local.rs | 5 ++--- src/libstd/thread/mod.rs | 5 ++--- 7 files changed, 15 insertions(+), 21 deletions(-) (limited to 'src/libstd/sys') diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs index 9513cc7fb2d..5743ea51af3 100644 --- a/src/libstd/io/lazy.rs +++ b/src/libstd/io/lazy.rs @@ -29,9 +29,8 @@ impl Lazy { /// Safety: `init` must not call `get` on the variable that is being /// initialized. pub const unsafe 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`. + // `lock` is never initialized fully, so it is UB to attempt to + // acquire this mutex reentrantly! Lazy { lock: Mutex::new(), ptr: Cell::new(ptr::null_mut()), diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs index c91bd5b22af..220bd11b1f1 100644 --- a/src/libstd/sys/unix/args.rs +++ b/src/libstd/sys/unix/args.rs @@ -80,9 +80,8 @@ 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`. + // `ENV_LOCK` is never initialized fully, so it is UB to attempt to + // acquire this mutex reentrantly! static LOCK: Mutex = Mutex::new(); pub unsafe fn init(argc: isize, argv: *const *const u8) { diff --git a/src/libstd/sys/unix/os.rs b/src/libstd/sys/unix/os.rs index 6ef9502ba62..3d98b2efdf1 100644 --- a/src/libstd/sys/unix/os.rs +++ b/src/libstd/sys/unix/os.rs @@ -33,9 +33,8 @@ 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`. +// `ENV_LOCK` is never initialized fully, so it is UB to attempt to +// acquire this mutex reentrantly! 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 30019088eb6..85679837312 100644 --- a/src/libstd/sys_common/at_exit_imp.rs +++ b/src/libstd/sys_common/at_exit_imp.rs @@ -23,9 +23,8 @@ 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`. +// `LOCK` is never initialized fully, so it is UB to attempt to +// acquire this mutex reentrantly! static LOCK: Mutex = Mutex::new(); static mut QUEUE: *mut Queue = ptr::null_mut(); diff --git a/src/libstd/sys_common/mutex.rs b/src/libstd/sys_common/mutex.rs index a4efe4d128e..74e1defd9f4 100644 --- a/src/libstd/sys_common/mutex.rs +++ b/src/libstd/sys_common/mutex.rs @@ -24,9 +24,9 @@ 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. + /// Also, until `init` is called, behavior is undefined if this + /// mutex is ever used reentrantly, i.e., `raw_lock` or `try_lock` + /// are called by the thread currently holding the lock. pub const fn new() -> Mutex { Mutex(imp::Mutex::new()) } /// Prepare the mutex for use. diff --git a/src/libstd/sys_common/thread_local.rs b/src/libstd/sys_common/thread_local.rs index 2cc5372e268..9db7d732698 100644 --- a/src/libstd/sys_common/thread_local.rs +++ b/src/libstd/sys_common/thread_local.rs @@ -161,9 +161,8 @@ 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`. + // `INIT_LOCK` is never initialized fully, so it is UB to attempt to + // acquire this mutex reentrantly! 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 98b4ca36c26..0078a05e597 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -940,9 +940,8 @@ 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`. + // `GUARD` is never initialized fully, so it is UB to attempt to + // acquire this mutex reentrantly! static GUARD: mutex::Mutex = mutex::Mutex::new(); static mut COUNTER: u64 = 0; -- cgit 1.4.1-3-g733a5 From 31bec788f46c73ab14c72868dc6141141320a058 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 8 Aug 2018 18:12:33 +0200 Subject: avoid using the word 'initialized' to talk about that non-reentrant-capable state of the mutex --- src/libstd/io/lazy.rs | 3 +-- src/libstd/sys/unix/args.rs | 2 +- src/libstd/sys/unix/os.rs | 2 +- src/libstd/sys_common/at_exit_imp.rs | 2 +- src/libstd/sys_common/mutex.rs | 4 +++- src/libstd/sys_common/thread_local.rs | 2 +- src/libstd/thread/mod.rs | 2 +- 7 files changed, 9 insertions(+), 8 deletions(-) (limited to 'src/libstd/sys') diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs index 5743ea51af3..4fb367fb6ba 100644 --- a/src/libstd/io/lazy.rs +++ b/src/libstd/io/lazy.rs @@ -15,6 +15,7 @@ use sys_common; use sys_common::mutex::Mutex; pub struct Lazy { + // We never call `lock.init()`, so it is UB to attempt to acquire this mutex reentrantly! lock: Mutex, ptr: Cell<*mut Arc>, init: fn() -> Arc, @@ -29,8 +30,6 @@ impl Lazy { /// Safety: `init` must not call `get` on the variable that is being /// initialized. pub const unsafe fn new(init: fn() -> Arc) -> Lazy { - // `lock` is never initialized fully, so it is UB to attempt to - // acquire this mutex reentrantly! Lazy { lock: Mutex::new(), ptr: Cell::new(ptr::null_mut()), diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs index 220bd11b1f1..c3c033dfbc7 100644 --- a/src/libstd/sys/unix/args.rs +++ b/src/libstd/sys/unix/args.rs @@ -80,7 +80,7 @@ mod imp { static mut ARGC: isize = 0; static mut ARGV: *const *const u8 = ptr::null(); - // `ENV_LOCK` is never initialized fully, so it is UB to attempt to + // We never call `ENV_LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! static LOCK: Mutex = Mutex::new(); diff --git a/src/libstd/sys/unix/os.rs b/src/libstd/sys/unix/os.rs index 3d98b2efdf1..08c3e154978 100644 --- a/src/libstd/sys/unix/os.rs +++ b/src/libstd/sys/unix/os.rs @@ -33,7 +33,7 @@ use sys::fd; use vec; const TMPBUF_SZ: usize = 128; -// `ENV_LOCK` is never initialized fully, so it is UB to attempt to +// We never call `ENV_LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! 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 85679837312..76e5df2c865 100644 --- a/src/libstd/sys_common/at_exit_imp.rs +++ b/src/libstd/sys_common/at_exit_imp.rs @@ -23,7 +23,7 @@ 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 it is UB to attempt to +// We never call `LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! static LOCK: Mutex = Mutex::new(); static mut QUEUE: *mut Queue = ptr::null_mut(); diff --git a/src/libstd/sys_common/mutex.rs b/src/libstd/sys_common/mutex.rs index 74e1defd9f4..c6d531c7a1a 100644 --- a/src/libstd/sys_common/mutex.rs +++ b/src/libstd/sys_common/mutex.rs @@ -32,7 +32,9 @@ impl Mutex { /// Prepare the mutex for use. /// /// This should be called once the mutex is at a stable memory address. - /// Behavior is undefined unless this is called before any other operation. + /// If called, this must be the very first thing that happens to the mutex. + /// Calling it in parallel with or after any operation (including another + /// `init()`) is undefined behavior. #[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 9db7d732698..bb72cb0930a 100644 --- a/src/libstd/sys_common/thread_local.rs +++ b/src/libstd/sys_common/thread_local.rs @@ -161,7 +161,7 @@ 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 it is UB to attempt to + // We never call `INIT_LOCK.init()`, so it is UB to attempt to // acquire this mutex reentrantly! static INIT_LOCK: Mutex = Mutex::new(); let _guard = INIT_LOCK.lock(); diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index 0078a05e597..61c6084a250 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -940,7 +940,7 @@ pub struct ThreadId(u64); impl ThreadId { // Generate a new unique thread ID. fn new() -> ThreadId { - // `GUARD` is never initialized fully, so it is UB to attempt to + // We never call `GUARD.init()`, so it is UB to attempt to // acquire this mutex reentrantly! static GUARD: mutex::Mutex = mutex::Mutex::new(); static mut COUNTER: u64 = 0; -- cgit 1.4.1-3-g733a5 From 25db84206b681731960d88558bc53640fe117b09 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Wed, 8 Aug 2018 18:14:06 +0200 Subject: missed one --- src/libstd/sys/unix/mutex.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/libstd/sys') diff --git a/src/libstd/sys/unix/mutex.rs b/src/libstd/sys/unix/mutex.rs index f0711b60320..efa6e8ec036 100644 --- a/src/libstd/sys/unix/mutex.rs +++ b/src/libstd/sys/unix/mutex.rs @@ -27,8 +27,8 @@ impl Mutex { pub const fn new() -> Mutex { // 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! + // Be very careful using this newly constructed `Mutex`, reentrant + // locking is undefined behavior until `init` is called! Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) } } #[inline] -- cgit 1.4.1-3-g733a5