diff --git a/src/libstd/io/lazy.rs b/src/libstd/io/lazy.rs index d357966be9227..4fb367fb6ba52 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, @@ -26,7 +27,9 @@ const fn done() -> *mut Arc { 1_usize as *mut _ } unsafe impl Sync for Lazy {} impl Lazy { - pub const fn new(init: fn() -> Arc) -> Lazy { + /// Safety: `init` must not call `get` on the variable that is being + /// initialized. + pub const unsafe fn new(init: fn() -> Arc) -> Lazy { 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! + // That's why `new` is unsafe and requires the caller to ensure no reentrancy happens. let ret = (self.init)(); if registered.is_ok() { self.ptr.set(Box::into_raw(Box::new(ret.clone()))); diff --git a/src/libstd/io/stdio.rs b/src/libstd/io/stdio.rs index fffe8fc559b81..1f256f518c7ce 100644 --- a/src/libstd/io/stdio.rs +++ b/src/libstd/io/stdio.rs @@ -197,12 +197,13 @@ pub struct StdinLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stdin() -> Stdin { - static INSTANCE: Lazy>>> = Lazy::new(stdin_init); + static INSTANCE: Lazy>>> = unsafe { Lazy::new(stdin_init) }; return Stdin { inner: INSTANCE.get().expect("cannot access stdin during shutdown"), }; fn stdin_init() -> Arc>>> { + // This must not reentrantly access `INSTANCE` let stdin = match stdin_raw() { Ok(stdin) => Maybe::Real(stdin), _ => Maybe::Fake @@ -396,12 +397,13 @@ pub struct StdoutLock<'a> { #[stable(feature = "rust1", since = "1.0.0")] pub fn stdout() -> Stdout { static INSTANCE: Lazy>>>> - = Lazy::new(stdout_init); + = unsafe { Lazy::new(stdout_init) }; return Stdout { inner: INSTANCE.get().expect("cannot access stdout during shutdown"), }; fn stdout_init() -> Arc>>>> { + // This must not reentrantly access `INSTANCE` let stdout = match stdout_raw() { Ok(stdout) => Maybe::Real(stdout), _ => Maybe::Fake, @@ -531,12 +533,14 @@ pub struct StderrLock<'a> { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn stderr() -> Stderr { - static INSTANCE: Lazy>>> = Lazy::new(stderr_init); + static INSTANCE: Lazy>>> = + unsafe { Lazy::new(stderr_init) }; return Stderr { inner: INSTANCE.get().expect("cannot access stderr during shutdown"), }; fn stderr_init() -> Arc>>> { + // This must not reentrantly access `INSTANCE` let stderr = match stderr_raw() { Ok(stderr) => Maybe::Real(stderr), _ => Maybe::Fake, diff --git a/src/libstd/sys/unix/args.rs b/src/libstd/sys/unix/args.rs index 7e32ec1347e9e..c3c033dfbc739 100644 --- a/src/libstd/sys/unix/args.rs +++ b/src/libstd/sys/unix/args.rs @@ -80,6 +80,8 @@ mod imp { static mut ARGC: isize = 0; static mut ARGV: *const *const u8 = ptr::null(); + // We never call `ENV_LOCK.init()`, 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/mutex.rs b/src/libstd/sys/unix/mutex.rs index 60b03cdbeb0e9..1d447de11343c 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`, reentrant + // locking is undefined behavior until `init` is called! 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..08c3e15497843 100644 --- a/src/libstd/sys/unix/os.rs +++ b/src/libstd/sys/unix/os.rs @@ -33,6 +33,8 @@ use sys::fd; use vec; const TMPBUF_SZ: usize = 128; +// 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 b28a4d2f8be01..76e5df2c8654a 100644 --- a/src/libstd/sys_common/at_exit_imp.rs +++ b/src/libstd/sys_common/at_exit_imp.rs @@ -23,6 +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). +// 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(); @@ -61,6 +63,7 @@ pub fn cleanup() { if !queue.is_null() { let queue: Box = Box::from_raw(queue); for to_run in *queue { + // We are not holding any lock, so reentrancy is fine. to_run(); } } @@ -72,6 +75,8 @@ pub fn push(f: Box) -> bool { unsafe { let _guard = LOCK.lock(); if init() { + // We are just moving `f` around, not calling it. + // There is no possibility of reentrancy here. (*QUEUE).push(f); true } else { diff --git a/src/libstd/sys_common/mutex.rs b/src/libstd/sys_common/mutex.rs index 608355b7d70f8..c6d531c7a1ac5 100644 --- a/src/libstd/sys_common/mutex.rs +++ b/src/libstd/sys_common/mutex.rs @@ -24,11 +24,17 @@ impl Mutex { /// /// Behavior is undefined if the mutex is moved after it is /// first used with any of the functions below. + /// 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. /// /// This should be called once the mutex is at a stable memory address. + /// 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 75f6b9ac7fd8c..bb72cb0930a7f 100644 --- a/src/libstd/sys_common/thread_local.rs +++ b/src/libstd/sys_common/thread_local.rs @@ -161,6 +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() { + // 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(); let mut key = self.key.load(Ordering::SeqCst); diff --git a/src/libstd/thread/mod.rs b/src/libstd/thread/mod.rs index bbe80df7e8bdb..61c6084a25023 100644 --- a/src/libstd/thread/mod.rs +++ b/src/libstd/thread/mod.rs @@ -940,6 +940,8 @@ pub struct ThreadId(u64); impl ThreadId { // Generate a new unique thread ID. fn new() -> ThreadId { + // 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;