Check lockorders in tests with a trivial lockorder tracker

Author: TheBlueMatt

.github/workflows/build.yml

@@ -101,6 +101,10 @@ jobs:
           RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client
           RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client,rest-client
           RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client,rest-client,tokio
+      - name: Test backtrace-debug builds on Rust ${{ matrix.toolchain }}
+        if: "matrix.build-no-std"
+        run: |
+          cd lightning && cargo test --verbose --color always --features backtrace
       - name: Test on Rust ${{ matrix.toolchain }} with net-tokio
         if: "matrix.build-net-tokio && !matrix.coverage"
         run: cargo test --verbose --color always

lightning/Cargo.toml

@@ -39,6 +39,7 @@ secp256k1 = { version = "0.20.2", default-features = false, features = ["alloc"]
 hashbrown = { version = "0.11", optional = true }
 hex = { version = "0.3", optional = true }
 regex = { version = "0.1.80", optional = true }
+backtrace = { version = "0.3", optional = true }
 
 core2 = { version = "0.3.0", optional = true, default-features = false }
 

lightning/src/chain/channelmonitor.rs

@@ -3321,7 +3321,7 @@ mod tests {
 	use util::events::{ClosureReason, MessageSendEventsProvider};
 	use util::test_utils::{TestLogger, TestBroadcaster, TestFeeEstimator};
 	use util::ser::{ReadableArgs, Writeable};
-	use sync::{Arc, Mutex};
+	use std::sync::{Arc, Mutex};
 	use io;
 	use prelude::*;
 

lightning/src/debug_sync.rs

@@ -0,0 +1,216 @@
+pub use ::alloc::sync::Arc;
+use core::ops::{Deref, DerefMut};
+use core::time::Duration;
+
+use std::collections::HashSet;
+use std::cell::RefCell;
+
+use std::sync::atomic::{AtomicUsize, Ordering};
+
+use std::sync::Mutex as StdMutex;
+use std::sync::MutexGuard as StdMutexGuard;
+use std::sync::RwLock as StdRwLock;
+use std::sync::RwLockReadGuard as StdRwLockReadGuard;
+use std::sync::RwLockWriteGuard as StdRwLockWriteGuard;
+use std::sync::Condvar as StdCondvar;
+
+#[cfg(feautre = "backtrace")]
+use backtrace::Backtrace;
+
+pub type LockResult<Guard> = Result<Guard, ()>;
+
+pub struct Condvar {
+	inner: StdCondvar,
+}
+
+impl Condvar {
+	pub fn new() -> Condvar {
+		Condvar { inner: StdCondvar::new() }
+	}
+
+	pub fn wait<'a, T>(&'a self, guard: MutexGuard<'a, T>) -> LockResult<MutexGuard<'a, T>> {
+		let mutex: &'a Mutex<T> = guard.mutex;
+		self.inner.wait(guard.into_inner()).map(|lock| MutexGuard { mutex, lock }).map_err(|_| ())
+	}
+
+	#[allow(unused)]
+	pub fn wait_timeout<'a, T>(&'a self, guard: MutexGuard<'a, T>, dur: Duration) -> LockResult<(MutexGuard<'a, T>, ())> {
+		let mutex = guard.mutex;
+		self.inner.wait_timeout(guard.into_inner(), dur).map(|(lock, _)| (MutexGuard { mutex, lock }, ())).map_err(|_| ())
+	}
+
+	pub fn notify_all(&self) { self.inner.notify_all(); }
+}
+
+thread_local! {
+	/// We track the set of locks currently held by a reference to their `MutexMetadata`
+	static MUTEXES_HELD: RefCell<HashSet<Arc<MutexMetadata>>> = RefCell::new(HashSet::new());
+}
+static MUTEX_IDX: AtomicUsize = AtomicUsize::new(0);
+
+/// Metadata about a single mutex, by id, the set of things locked-before it, and the backtrace of
+/// when the Mutex itself was constructed.
+struct MutexMetadata {
+	mutex_idx: u64,
+	locked_before: StdMutex<HashSet<Arc<MutexMetadata>>>,
+	#[cfg(feautre = "backtrace")]
+	mutex_construction_bt: Backtrace,
+}
+impl PartialEq for MutexMetadata {
+	fn eq(&self, o: &MutexMetadata) -> bool { self.mutex_idx == o.mutex_idx }
+}
+impl Eq for MutexMetadata {}
+impl std::hash::Hash for MutexMetadata {
+	fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) { hasher.write_u64(self.mutex_idx); }
+}
+
+pub struct Mutex<T: Sized> {
+	inner: StdMutex<T>,
+	deps: Arc<MutexMetadata>,
+	skip_checks: bool
+}
+
+#[must_use = "if unused the Mutex will immediately unlock"]
+pub struct MutexGuard<'a, T: Sized + 'a> {
+	mutex: &'a Mutex<T>,
+	lock: StdMutexGuard<'a, T>,
+}
+
+impl<'a, T: Sized> MutexGuard<'a, T> {
+	fn into_inner(self) -> StdMutexGuard<'a, T> {
+		// Somewhat unclear why this is not possible without unsafe, but oh well.
+		unsafe {
+			let v: StdMutexGuard<'a, T> = std::ptr::read(&self.lock);
+			std::mem::forget(self);
+			v
+		}
+	}
+}
+
+impl<T: Sized> Drop for MutexGuard<'_, T> {
+	fn drop(&mut self) {
+		MUTEXES_HELD.with(|held| {
+			held.borrow_mut().remove(&self.mutex.deps);
+		});
+	}
+}
+
+impl<T: Sized> Deref for MutexGuard<'_, T> {
+	type Target = T;
+
+	fn deref(&self) -> &T {
+		&self.lock.deref()
+	}
+}
+
+impl<T: Sized> DerefMut for MutexGuard<'_, T> {
+	fn deref_mut(&mut self) -> &mut T {
+		self.lock.deref_mut()
+	}
+}
+
+impl<T> Mutex<T> {
+	pub fn new(inner: T) -> Mutex<T> {
+		Mutex {
+			inner: StdMutex::new(inner),
+			deps: Arc::new(MutexMetadata {
+				locked_before: StdMutex::new(HashSet::new()),
+				mutex_idx: MUTEX_IDX.fetch_add(1, Ordering::Relaxed) as u64,
+				#[cfg(feautre = "backtrace")]
+				mutex_construction_bt: Backtrace::new(),
+			}),
+			skip_checks: false,
+		}
+	}
+
+	pub fn lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
+		if !self.skip_checks {
+			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() {
+					for locked_dep in locked.locked_before.lock().unwrap().iter() {
+						if *locked_dep == self.deps {
+							#[cfg(feautre = "backtrace")]
+							panic!("Tried to violate existing lockorder. Mutex which should be locked after the current lock was created at: {:?}", locked.mutex_construction_bt);
+							#[cfg(not(feautre = "backtrace"))]
+							panic!("Tried to violate existing lockorder. Build with backtrace feature for more info.");
+						}
+					}
+					// Insert any already-held mutexes in our locked-before set.
+					self.deps.locked_before.lock().unwrap().insert(Arc::clone(locked));
+				}
+				held.borrow_mut().insert(Arc::clone(&self.deps));
+			});
+		}
+		self.inner.lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ())
+	}
+
+	pub fn try_lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
+		let res = self.inner.try_lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ());
+		if !self.skip_checks && res.is_ok() {
+			MUTEXES_HELD.with(|held| {
+				// In a try-lock, only insert already-held mutexes in our locked-before set if we
+				// succeed in locking, and never check for lockinversions directly with this mutex.
+				for locked in held.borrow().iter() {
+					self.deps.locked_before.lock().unwrap().insert(Arc::clone(locked));
+				}
+				held.borrow_mut().insert(Arc::clone(&self.deps));
+			});
+		}
+		res
+	}
+}
+
+pub struct RwLock<T: ?Sized> {
+	inner: StdRwLock<T>
+}
+
+pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
+	lock: StdRwLockReadGuard<'a, T>,
+}
+
+pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
+	lock: StdRwLockWriteGuard<'a, 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> {
+	type Target = T;
+
+	fn deref(&self) -> &T {
+		&self.lock.deref()
+	}
+}
+
+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) }
+	}
+
+	pub fn read<'a>(&'a self) -> LockResult<RwLockReadGuard<'a, T>> {
+		self.inner.read().map(|lock| RwLockReadGuard { lock }).map_err(|_| ())
+	}
+
+	pub fn write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
+		self.inner.write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
+	}
+
+	pub fn try_write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
+		self.inner.try_write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
+	}
+}

lightning/src/lib.rs

@@ -143,8 +143,16 @@ mod prelude {
 	pub use alloc::string::ToString;
 }
 
+#[cfg(all(feature = "std", test))]
+mod debug_sync;
+#[cfg(all(feature = "backtrace", feature = "std", test))]
+extern crate backtrace;
+
 #[cfg(feature = "std")]
 mod sync {
+	#[cfg(test)]
+	pub use debug_sync::*;
+	#[cfg(not(test))]
 	pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard};
 }
 

lightning/src/ln/chanmon_update_fail_tests.rs

@@ -36,7 +36,7 @@ use util::test_utils;
 
 use io;
 use prelude::*;
-use sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex};
 
 #[test]
 fn test_simple_monitor_permanent_update_fail() {

lightning/src/ln/channelmanager.rs

@@ -7146,7 +7146,7 @@ pub mod bench {
 	use bitcoin::hashes::sha256::Hash as Sha256;
 	use bitcoin::{Block, BlockHeader, Transaction, TxOut};
 
-	use sync::{Arc, Mutex};
+	use std::sync::{Arc, Mutex};
 
 	use test::Bencher;
 

lightning/src/ln/functional_test_utils.rs

@@ -41,7 +41,7 @@ use io;
 use prelude::*;
 use core::cell::RefCell;
 use alloc::rc::Rc;
-use sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex};
 use core::mem;
 
 pub const CHAN_CONFIRM_DEPTH: u32 = 10;

lightning/src/ln/functional_tests.rs

@@ -51,7 +51,7 @@ use io;
 use prelude::*;
 use alloc::collections::BTreeSet;
 use core::default::Default;
-use sync::{Arc, Mutex};
+use std::sync::{Arc, Mutex};
 
 use ln::functional_test_utils::*;
 use ln::chan_utils::CommitmentTransaction;

lightning/src/util/enforcing_trait_impls.rs

@@ -13,8 +13,8 @@ use chain::keysinterface::{Sign, InMemorySigner, BaseSign};
 
 use prelude::*;
 use core::cmp;
-use sync::{Mutex, Arc};
-#[cfg(test)] use sync::MutexGuard;
+use std::sync::{Mutex, Arc};
+#[cfg(test)] use std::sync::MutexGuard;
 
 use bitcoin::blockdata::transaction::{Transaction, SigHashType};
 use bitcoin::util::bip143;

lightning/src/util/test_utils.rs

@@ -44,7 +44,7 @@ use regex;
 use io;
 use prelude::*;
 use core::time::Duration;
-use sync::{Mutex, Arc};
+use std::sync::{Mutex, Arc};
 use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
 use core::{cmp, mem};
 use chain::keysinterface::{InMemorySigner, KeyMaterial};