Audit usage of NativeMutex

Once a native mutex has been used once, it is never allowed to be moved again.
This is because some pthreads implementations take pointers inside the mutex
itself.

This commit adds stern wording around the methods on native mutexes, and fixes
one use case in the codebase. The Mutex type in libsync was susceptible to
movement, so the inner static mutex is now boxed to ensure that the address of
the native mutex is constant.

diff --git a/src/librustrt/mutex.rs b/src/librustrt/mutex.rs
index eec19e9d5db86bf9cb5ad55587516b9a2b668d40..3fece698a1de491726f07a29255eea5935d6a985 100644 (file)
--- a/src/librustrt/mutex.rs
+++ b/src/librustrt/mutex.rs
@@ -115,6 +115,18 @@ pub unsafe fn new() -> StaticNativeMutex {
     ///     // critical section...
     /// } // automatically unlocked in `_guard`'s destructor
     /// ```
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe because it will not function correctly if this
+    /// mutex has been *moved* since it was last used. The mutex can move an
+    /// arbitrary number of times before its first usage, but once a mutex has
+    /// been used once it is no longer allowed to move (or otherwise it invokes
+    /// undefined behavior).
+    ///
+    /// Additionally, this type does not take into account any form of
+    /// scheduling model. This will unconditionally block the *os thread* which
+    /// is not always desired.
     pub unsafe fn lock<'a>(&'a self) -> LockGuard<'a> {
         self.inner.lock();
 
@@ -123,6 +135,10 @@ pub unsafe fn lock<'a>(&'a self) -> LockGuard<'a> {
 
     /// Attempts to acquire the lock. The value returned is `Some` if
     /// the attempt succeeded.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock`.
     pub unsafe fn trylock<'a>(&'a self) -> Option<LockGuard<'a>> {
         if self.inner.trylock() {
             Some(LockGuard { lock: self })
@@ -135,6 +151,12 @@ pub unsafe fn trylock<'a>(&'a self) -> Option<LockGuard<'a>> {
     ///
     /// These needs to be paired with a call to `.unlock_noguard`. Prefer using
     /// `.lock`.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock`. Additionally, this
+    /// does not guarantee that the mutex will ever be unlocked, and it is
+    /// undefined to drop an already-locked mutex.
     pub unsafe fn lock_noguard(&self) { self.inner.lock() }
 
     /// Attempts to acquire the lock without creating a
@@ -143,12 +165,22 @@ pub unsafe fn lock_noguard(&self) { self.inner.lock() }
     ///
     /// If `true` is returned, this needs to be paired with a call to
     /// `.unlock_noguard`. Prefer using `.trylock`.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock_noguard`.
     pub unsafe fn trylock_noguard(&self) -> bool {
         self.inner.trylock()
     }
 
     /// Unlocks the lock. This assumes that the current thread already holds the
     /// lock.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock`. Additionally, it
+    /// is not guaranteed that this is unlocking a previously locked mutex. It
+    /// is undefined to unlock an unlocked mutex.
     pub unsafe fn unlock_noguard(&self) { self.inner.unlock() }
 
     /// Block on the internal condition variable.
@@ -156,9 +188,19 @@ pub unsafe fn unlock_noguard(&self) { self.inner.unlock() }
     /// This function assumes that the lock is already held. Prefer
     /// using `LockGuard.wait` since that guarantees that the lock is
     /// held.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock`. Additionally, this
+    /// is unsafe because the mutex may not be currently locked.
     pub unsafe fn wait_noguard(&self) { self.inner.wait() }
 
     /// Signals a thread in `wait` to wake up
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe for the same reasons as `lock`. Additionally, this
+    /// is unsafe because the mutex may not be currently locked.
     pub unsafe fn signal_noguard(&self) { self.inner.signal() }
 
     /// This function is especially unsafe because there are no guarantees made
@@ -181,6 +223,7 @@ pub unsafe fn new() -> NativeMutex {
     /// already hold the lock.
     ///
     /// # Example
+    ///
     /// ```rust
     /// use std::rt::mutex::NativeMutex;
     /// unsafe {
@@ -192,12 +235,22 @@ pub unsafe fn new() -> NativeMutex {
     ///     } // automatically unlocked in `_guard`'s destructor
     /// }
     /// ```
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::lock`.
     pub unsafe fn lock<'a>(&'a self) -> LockGuard<'a> {
         self.inner.lock()
     }
 
     /// Attempts to acquire the lock. The value returned is `Some` if
     /// the attempt succeeded.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::trylock`.
     pub unsafe fn trylock<'a>(&'a self) -> Option<LockGuard<'a>> {
         self.inner.trylock()
     }
@@ -206,6 +259,11 @@ pub unsafe fn trylock<'a>(&'a self) -> Option<LockGuard<'a>> {
     ///
     /// These needs to be paired with a call to `.unlock_noguard`. Prefer using
     /// `.lock`.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::lock_noguard`.
     pub unsafe fn lock_noguard(&self) { self.inner.lock_noguard() }
 
     /// Attempts to acquire the lock without creating a
@@ -214,12 +272,22 @@ pub unsafe fn lock_noguard(&self) { self.inner.lock_noguard() }
     ///
     /// If `true` is returned, this needs to be paired with a call to
     /// `.unlock_noguard`. Prefer using `.trylock`.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::trylock_noguard`.
     pub unsafe fn trylock_noguard(&self) -> bool {
         self.inner.trylock_noguard()
     }
 
     /// Unlocks the lock. This assumes that the current thread already holds the
     /// lock.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::unlock_noguard`.
     pub unsafe fn unlock_noguard(&self) { self.inner.unlock_noguard() }
 
     /// Block on the internal condition variable.
@@ -227,9 +295,19 @@ pub unsafe fn unlock_noguard(&self) { self.inner.unlock_noguard() }
     /// This function assumes that the lock is already held. Prefer
     /// using `LockGuard.wait` since that guarantees that the lock is
     /// held.
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::wait_noguard`.
     pub unsafe fn wait_noguard(&self) { self.inner.wait_noguard() }
 
     /// Signals a thread in `wait` to wake up
+    ///
+    /// # Unsafety
+    ///
+    /// This method is unsafe due to the same reasons as
+    /// `StaticNativeMutex::signal_noguard`.
     pub unsafe fn signal_noguard(&self) { self.inner.signal_noguard() }
 }
 

diff --git a/src/librustrt/unwind.rs b/src/librustrt/unwind.rs
index f7475db1552f7181acd5bd414aca32e0d7ecd290..aebed5a8829c5290c3acc2965ee623934c7086c7 100644 (file)
--- a/src/librustrt/unwind.rs
+++ b/src/librustrt/unwind.rs
@@ -73,7 +73,6 @@
 
 use local::Local;
 use task::{Task, Result};
-use exclusive::Exclusive;
 
 use uw = libunwind;
 
@@ -88,7 +87,6 @@ struct Exception {
 }
 
 pub type Callback = fn(msg: &Any:Send, file: &'static str, line: uint);
-type Queue = Exclusive<Vec<Callback>>;
 
 // Variables used for invoking callbacks when a task starts to unwind.
 //

diff --git a/src/libsync/mutex.rs b/src/libsync/mutex.rs
index 6b9ff3cf0527cf4602424f6abcde74fb64d7616c..ef558d3f9241bb0d04f6259cd0aa425fea457c00 100644 (file)
--- a/src/libsync/mutex.rs
+++ b/src/libsync/mutex.rs
@@ -97,7 +97,14 @@
 /// drop(guard); // unlock the lock
 /// ```
 pub struct Mutex {
-    lock: StaticMutex,
+    // Note that this static mutex is in a *box*, not inlined into the struct
+    // itself. This is done for memory safety reasons with the usage of a
+    // StaticNativeMutex inside the static mutex above. Once a native mutex has
+    // been used once, its address can never change (it can't be moved). This
+    // mutex type can be safely moved at any time, so to ensure that the native
+    // mutex is used correctly we box the inner lock to give it a constant
+    // address.
+    lock: Box<StaticMutex>,
 }
 
 #[deriving(PartialEq, Show)]
@@ -458,7 +465,7 @@ impl Mutex {
     /// Creates a new mutex in an unlocked state ready for use.
     pub fn new() -> Mutex {
         Mutex {
-            lock: StaticMutex {
+            lock: box StaticMutex {
                 state: atomics::AtomicUint::new(0),
                 flavor: Unsafe::new(Unlocked),
                 green_blocker: Unsafe::new(0),