Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify partially initialized Mutex issues #53108

Merged
merged 7 commits into from
Aug 9, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion src/libstd/io/lazy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,11 @@ const fn done<T>() -> *mut Arc<T> { 1_usize as *mut _ }
unsafe impl<T> Sync for Lazy<T> {}

impl<T: Send + Sync + 'static> Lazy<T> {
pub const fn new(init: fn() -> Arc<T>) -> Lazy<T> {
/// Safety: `init` must not call `get` on the variable that is being
/// initialized.
pub const unsafe fn new(init: fn() -> Arc<T>) -> Lazy<T> {
// `lock` is never initialized fully, so it is UB to attempt to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This threw me for a spin when I first read it, could it perhaps be reworded? In the Rust sense we fully initialize the lock (aka put valid bytes in place) but what this comment is alluding to is the POSIX-specific behavior tweak in the mutex implementation where Mutex::new isn't as safe as an init'd mutex. Could this perhaps be reworded as:

lock is not valid to use in a reentrant fashion, causing UB if that happens;
for more information see comments in

// acquire this mutex reentrantly!
Lazy {
lock: Mutex::new(),
ptr: Cell::new(ptr::null_mut()),
Expand All @@ -48,6 +52,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 +65,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!
// 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())));
Expand Down
10 changes: 7 additions & 3 deletions src/libstd/io/stdio.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,13 @@ pub struct StdinLock<'a> {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn stdin() -> Stdin {
static INSTANCE: Lazy<Mutex<BufReader<Maybe<StdinRaw>>>> = Lazy::new(stdin_init);
static INSTANCE: Lazy<Mutex<BufReader<Maybe<StdinRaw>>>> = unsafe { Lazy::new(stdin_init) };
return Stdin {
inner: INSTANCE.get().expect("cannot access stdin during shutdown"),
};

fn stdin_init() -> Arc<Mutex<BufReader<Maybe<StdinRaw>>>> {
// This must not reentrantly access `INSTANCE`
let stdin = match stdin_raw() {
Ok(stdin) => Maybe::Real(stdin),
_ => Maybe::Fake
Expand Down Expand Up @@ -396,12 +397,13 @@ pub struct StdoutLock<'a> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn stdout() -> Stdout {
static INSTANCE: Lazy<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>>
= Lazy::new(stdout_init);
= unsafe { Lazy::new(stdout_init) };
return Stdout {
inner: INSTANCE.get().expect("cannot access stdout during shutdown"),
};

fn stdout_init() -> Arc<ReentrantMutex<RefCell<LineWriter<Maybe<StdoutRaw>>>>> {
// This must not reentrantly access `INSTANCE`
let stdout = match stdout_raw() {
Ok(stdout) => Maybe::Real(stdout),
_ => Maybe::Fake,
Expand Down Expand Up @@ -531,12 +533,14 @@ pub struct StderrLock<'a> {
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn stderr() -> Stderr {
static INSTANCE: Lazy<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> = Lazy::new(stderr_init);
static INSTANCE: Lazy<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> =
unsafe { Lazy::new(stderr_init) };
return Stderr {
inner: INSTANCE.get().expect("cannot access stderr during shutdown"),
};

fn stderr_init() -> Arc<ReentrantMutex<RefCell<Maybe<StderrRaw>>>> {
// This must not reentrantly access `INSTANCE`
let stderr = match stderr_raw() {
Ok(stderr) => Maybe::Real(stderr),
_ => Maybe::Fake,
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/sys/unix/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as bove to the wording "initialized fully" here

// acquire this mutex reentrantly!
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
Original file line number Diff line number Diff line change
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!
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be tweaked as well to not use the word "initialized" here? I think it makes sense but it just threw me personally for a spin, I had to go read all the comments in init to understand this.

Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) }
}
#[inline]
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/sys/unix/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ use sys::fd;
use vec;

const TMPBUF_SZ: usize = 128;
// `ENV_LOCK` is never initialized fully, so it is UB to attempt to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as bove to the wording "initialized fully" here

// acquire this mutex reentrantly!
static ENV_LOCK: Mutex = Mutex::new();


Expand Down
5 changes: 5 additions & 0 deletions src/libstd/sys_common/at_exit_imp.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ 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 it is UB to attempt to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as bove to the wording "initialized fully" here

// acquire this mutex reentrantly!
static LOCK: Mutex = Mutex::new();
static mut QUEUE: *mut Queue = ptr::null_mut();

Expand Down Expand Up @@ -61,6 +63,7 @@ pub fn cleanup() {
if !queue.is_null() {
let queue: Box<Queue> = Box::from_raw(queue);
for to_run in *queue {
// We are not holding any lock, so reentrancy is fine.
to_run();
}
}
Expand All @@ -72,6 +75,8 @@ pub fn push(f: Box<dyn FnBox()>) -> 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 {
Expand Down
4 changes: 4 additions & 0 deletions src/libstd/sys_common/mutex.rs
Original file line number Diff line number Diff line change
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, 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.
/// Behavior is undefined unless this is called before any other operation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be clarified to say something like "If this function is called, behavior is undefined ..." to be clear that it's not undefined behavior to not call this method, only to call the method after you've already done other operations.

#[inline]
pub unsafe fn init(&mut self) { self.0.init() }

Expand Down
2 changes: 2 additions & 0 deletions src/libstd/sys_common/thread_local.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
// `INIT_LOCK` is never initialized fully, so it is UB to attempt to
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as bove to the wording "initialized fully" here

// acquire this mutex reentrantly!
static INIT_LOCK: Mutex = Mutex::new();
let _guard = INIT_LOCK.lock();
let mut key = self.key.load(Ordering::SeqCst);
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/thread/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,8 @@ 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as bove to the wording "initialized fully" here

// acquire this mutex reentrantly!
static GUARD: mutex::Mutex = mutex::Mutex::new();
static mut COUNTER: u64 = 0;

Expand Down