Implement lockorder checking on RwLocks in debug_sync

Author: TheBlueMatt

lightning/src/debug_sync.rs

@@ -74,12 +74,21 @@ impl MutexMetadata {
 		}
 	}
 
-	fn pre_lock(this: &Arc<MutexMetadata>) {
+	fn pre_lock(this: &Arc<MutexMetadata>, read: bool) {
 		MUTEXES_HELD.with(|held| {
 			// For each mutex which is currently locked, check that no mutex's locked-before
 			// set includes the mutex we're about to lock, which would imply a lockorder
 			// inversion.
 			for locked in held.borrow().iter() {
+				if read && *locked == *this {
+					// Recursive read locks are explicitly allowed
+					return;
+				}
+			}
+			for locked in held.borrow().iter() {
+				if !read && *locked == *this {
+					panic!("Tried to lock a mutex while it was held!");
+				}
 				for locked_dep in locked.locked_before.lock().unwrap().iter() {
 					if *locked_dep == *this {
 						#[cfg(feature = "backtrace")]
@@ -158,7 +167,7 @@ impl<T> Mutex<T> {
 	}
 
 	pub fn lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
-		MutexMetadata::pre_lock(&self.deps);
+		MutexMetadata::pre_lock(&self.deps, false);
 		self.inner.lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ())
 	}
 
@@ -171,54 +180,109 @@ impl<T> Mutex<T> {
 	}
 }
 
-pub struct RwLock<T: ?Sized> {
-	inner: StdRwLock<T>
+pub struct RwLock<T: Sized> {
+	inner: StdRwLock<T>,
+	deps: Arc<MutexMetadata>,
 }
 
-pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
+pub struct RwLockReadGuard<'a, T: Sized + 'a> {
+	mutex: &'a RwLock<T>,
 	lock: StdRwLockReadGuard<'a, T>,
 }
 
-pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
+pub struct RwLockWriteGuard<'a, T: Sized + 'a> {
+	mutex: &'a RwLock<T>,
 	lock: StdRwLockWriteGuard<'a, T>,
 }
 
-impl<T: ?Sized> Deref for RwLockReadGuard<'_, T> {
+impl<T: Sized> Deref for RwLockReadGuard<'_, T> {
 	type Target = T;
 
 	fn deref(&self) -> &T {
 		&self.lock.deref()
 	}
 }
 
-impl<T: ?Sized> Deref for RwLockWriteGuard<'_, T> {
+impl<T: Sized> Drop for RwLockReadGuard<'_, T> {
+	fn drop(&mut self) {
+		MUTEXES_HELD.with(|held| {
+			held.borrow_mut().remove(&self.mutex.deps);
+		});
+	}
+}
+
+impl<T: Sized> Deref for RwLockWriteGuard<'_, T> {
 	type Target = T;
 
 	fn deref(&self) -> &T {
 		&self.lock.deref()
 	}
 }
 
-impl<T: ?Sized> DerefMut for RwLockWriteGuard<'_, T> {
+impl<T: Sized> Drop for RwLockWriteGuard<'_, T> {
+	fn drop(&mut self) {
+		MUTEXES_HELD.with(|held| {
+			held.borrow_mut().remove(&self.mutex.deps);
+		});
+	}
+}
+
+impl<T: Sized> DerefMut for RwLockWriteGuard<'_, T> {
 	fn deref_mut(&mut self) -> &mut T {
 		self.lock.deref_mut()
 	}
 }
 
 impl<T> RwLock<T> {
 	pub fn new(inner: T) -> RwLock<T> {
-		RwLock { inner: StdRwLock::new(inner) }
+		RwLock { inner: StdRwLock::new(inner), deps: Arc::new(MutexMetadata::new()) }
 	}
 
 	pub fn read<'a>(&'a self) -> LockResult<RwLockReadGuard<'a, T>> {
-		self.inner.read().map(|lock| RwLockReadGuard { lock }).map_err(|_| ())
+		MutexMetadata::pre_lock(&self.deps, true);
+		self.inner.read().map(|lock| RwLockReadGuard { mutex: self, lock }).map_err(|_| ())
 	}
 
 	pub fn write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
-		self.inner.write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
+		MutexMetadata::pre_lock(&self.deps, false);
+		self.inner.write().map(|lock| RwLockWriteGuard { mutex: self, lock }).map_err(|_| ())
 	}
 
 	pub fn try_write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
-		self.inner.try_write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
+		let res = self.inner.try_write().map(|lock| RwLockWriteGuard { mutex: self, lock }).map_err(|_| ());
+		if res.is_ok() {
+			MutexMetadata::try_locked(&self.deps);
+		}
+		res
+	}
+}
+
+#[test]
+#[should_panic]
+fn recursive_lock_fail() {
+	let mutex = Mutex::new(());
+	let _a = mutex.lock().unwrap();
+	let _b = mutex.lock().unwrap();
+}
+
+#[test]
+fn recursive_read() {
+	let mutex = RwLock::new(());
+	let _a = mutex.read().unwrap();
+	let _b = mutex.read().unwrap();
+}
+
+#[test]
+#[should_panic]
+fn lockorder_fail() {
+	let a = Mutex::new(());
+	let b = Mutex::new(());
+	{
+		let _a = a.lock().unwrap();
+		let _b = b.lock().unwrap();
+	}
+	{
+		let _b = b.lock().unwrap();
+		let _a = a.lock().unwrap();
 	}
 }