-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Changes from 5 commits
d3d3110
819645b
22457de
ab3e4a2
6453885
31bec78
25db842
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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! | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
Mutex { inner: UnsafeCell::new(libc::PTHREAD_MUTEX_INITIALIZER) } | ||
} | ||
#[inline] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
|
@@ -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(); | ||
} | ||
} | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() } | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
There was a problem hiding this comment.
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 aninit
'd mutex. Could this perhaps be reworded as: