Skip to content

Commit 34cdca9

Browse files
authored
Merge pull request #1238 from TheBlueMatt/2022-01-lockorder-checks
Fix and test lockorder
2 parents 89cbb6d + 8f007c7 commit 34cdca9

File tree

8 files changed

+244
-17
lines changed

8 files changed

+244
-17
lines changed

.github/workflows/build.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,10 @@ jobs:
101101
RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client
102102
RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client,rest-client
103103
RUSTFLAGS="-C link-dead-code" cargo build --verbose --color always --features rpc-client,rest-client,tokio
104+
- name: Test backtrace-debug builds on Rust ${{ matrix.toolchain }}
105+
if: "matrix.build-no-std"
106+
run: |
107+
cd lightning && cargo test --verbose --color always --features backtrace
104108
- name: Test on Rust ${{ matrix.toolchain }} with net-tokio
105109
if: "matrix.build-net-tokio && !matrix.coverage"
106110
run: cargo test --verbose --color always

lightning/Cargo.toml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ secp256k1 = { version = "0.20.2", default-features = false, features = ["alloc"]
3939
hashbrown = { version = "0.11", optional = true }
4040
hex = { version = "0.3", optional = true }
4141
regex = { version = "0.1.80", optional = true }
42+
backtrace = { version = "0.3", optional = true }
4243

4344
core2 = { version = "0.3.0", optional = true, default-features = false }
4445

lightning/src/chain/chainmonitor.rs

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -920,9 +920,10 @@ mod tests {
920920
merkle_root: Default::default() };
921921
nodes[0].chain_monitor.chain_monitor.best_block_updated(&latest_header, nodes[0].best_block_info().1 + LATENCY_GRACE_PERIOD_BLOCKS);
922922
} else {
923-
for (funding_outpoint, update_ids) in chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().iter() {
923+
let persistences = chanmon_cfgs[0].persister.chain_sync_monitor_persistences.lock().unwrap().clone();
924+
for (funding_outpoint, update_ids) in persistences {
924925
for update_id in update_ids {
925-
nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(*funding_outpoint, *update_id).unwrap();
926+
nodes[0].chain_monitor.chain_monitor.channel_monitor_updated(funding_outpoint, update_id).unwrap();
926927
}
927928
}
928929
}

lightning/src/debug_sync.rs

Lines changed: 213 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,213 @@
1+
pub use ::alloc::sync::Arc;
2+
use core::ops::{Deref, DerefMut};
3+
use core::time::Duration;
4+
5+
use std::collections::HashSet;
6+
use std::cell::RefCell;
7+
8+
use std::sync::atomic::{AtomicUsize, Ordering};
9+
10+
use std::sync::Mutex as StdMutex;
11+
use std::sync::MutexGuard as StdMutexGuard;
12+
use std::sync::RwLock as StdRwLock;
13+
use std::sync::RwLockReadGuard as StdRwLockReadGuard;
14+
use std::sync::RwLockWriteGuard as StdRwLockWriteGuard;
15+
use std::sync::Condvar as StdCondvar;
16+
17+
#[cfg(feature = "backtrace")]
18+
use backtrace::Backtrace;
19+
20+
pub type LockResult<Guard> = Result<Guard, ()>;
21+
22+
pub struct Condvar {
23+
inner: StdCondvar,
24+
}
25+
26+
impl Condvar {
27+
pub fn new() -> Condvar {
28+
Condvar { inner: StdCondvar::new() }
29+
}
30+
31+
pub fn wait<'a, T>(&'a self, guard: MutexGuard<'a, T>) -> LockResult<MutexGuard<'a, T>> {
32+
let mutex: &'a Mutex<T> = guard.mutex;
33+
self.inner.wait(guard.into_inner()).map(|lock| MutexGuard { mutex, lock }).map_err(|_| ())
34+
}
35+
36+
#[allow(unused)]
37+
pub fn wait_timeout<'a, T>(&'a self, guard: MutexGuard<'a, T>, dur: Duration) -> LockResult<(MutexGuard<'a, T>, ())> {
38+
let mutex = guard.mutex;
39+
self.inner.wait_timeout(guard.into_inner(), dur).map(|(lock, _)| (MutexGuard { mutex, lock }, ())).map_err(|_| ())
40+
}
41+
42+
pub fn notify_all(&self) { self.inner.notify_all(); }
43+
}
44+
45+
thread_local! {
46+
/// We track the set of locks currently held by a reference to their `MutexMetadata`
47+
static MUTEXES_HELD: RefCell<HashSet<Arc<MutexMetadata>>> = RefCell::new(HashSet::new());
48+
}
49+
static MUTEX_IDX: AtomicUsize = AtomicUsize::new(0);
50+
51+
/// Metadata about a single mutex, by id, the set of things locked-before it, and the backtrace of
52+
/// when the Mutex itself was constructed.
53+
struct MutexMetadata {
54+
mutex_idx: u64,
55+
locked_before: StdMutex<HashSet<Arc<MutexMetadata>>>,
56+
#[cfg(feature = "backtrace")]
57+
mutex_construction_bt: Backtrace,
58+
}
59+
impl PartialEq for MutexMetadata {
60+
fn eq(&self, o: &MutexMetadata) -> bool { self.mutex_idx == o.mutex_idx }
61+
}
62+
impl Eq for MutexMetadata {}
63+
impl std::hash::Hash for MutexMetadata {
64+
fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) { hasher.write_u64(self.mutex_idx); }
65+
}
66+
67+
pub struct Mutex<T: Sized> {
68+
inner: StdMutex<T>,
69+
deps: Arc<MutexMetadata>,
70+
}
71+
72+
#[must_use = "if unused the Mutex will immediately unlock"]
73+
pub struct MutexGuard<'a, T: Sized + 'a> {
74+
mutex: &'a Mutex<T>,
75+
lock: StdMutexGuard<'a, T>,
76+
}
77+
78+
impl<'a, T: Sized> MutexGuard<'a, T> {
79+
fn into_inner(self) -> StdMutexGuard<'a, T> {
80+
// Somewhat unclear why we cannot move out of self.lock, but doing so gets E0509.
81+
unsafe {
82+
let v: StdMutexGuard<'a, T> = std::ptr::read(&self.lock);
83+
std::mem::forget(self);
84+
v
85+
}
86+
}
87+
}
88+
89+
impl<T: Sized> Drop for MutexGuard<'_, T> {
90+
fn drop(&mut self) {
91+
MUTEXES_HELD.with(|held| {
92+
held.borrow_mut().remove(&self.mutex.deps);
93+
});
94+
}
95+
}
96+
97+
impl<T: Sized> Deref for MutexGuard<'_, T> {
98+
type Target = T;
99+
100+
fn deref(&self) -> &T {
101+
&self.lock.deref()
102+
}
103+
}
104+
105+
impl<T: Sized> DerefMut for MutexGuard<'_, T> {
106+
fn deref_mut(&mut self) -> &mut T {
107+
self.lock.deref_mut()
108+
}
109+
}
110+
111+
impl<T> Mutex<T> {
112+
pub fn new(inner: T) -> Mutex<T> {
113+
Mutex {
114+
inner: StdMutex::new(inner),
115+
deps: Arc::new(MutexMetadata {
116+
locked_before: StdMutex::new(HashSet::new()),
117+
mutex_idx: MUTEX_IDX.fetch_add(1, Ordering::Relaxed) as u64,
118+
#[cfg(feature = "backtrace")]
119+
mutex_construction_bt: Backtrace::new(),
120+
}),
121+
}
122+
}
123+
124+
pub fn lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
125+
MUTEXES_HELD.with(|held| {
126+
// For each mutex which is currently locked, check that no mutex's locked-before
127+
// set includes the mutex we're about to lock, which would imply a lockorder
128+
// inversion.
129+
for locked in held.borrow().iter() {
130+
for locked_dep in locked.locked_before.lock().unwrap().iter() {
131+
if *locked_dep == self.deps {
132+
#[cfg(feature = "backtrace")]
133+
panic!("Tried to violate existing lockorder.\nMutex that should be locked after the current lock was created at the following backtrace.\nNote that to get a backtrace for the lockorder violation, you should set RUST_BACKTRACE=1\n{:?}", locked.mutex_construction_bt);
134+
#[cfg(not(feature = "backtrace"))]
135+
panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info.");
136+
}
137+
}
138+
// Insert any already-held mutexes in our locked-before set.
139+
self.deps.locked_before.lock().unwrap().insert(Arc::clone(locked));
140+
}
141+
held.borrow_mut().insert(Arc::clone(&self.deps));
142+
});
143+
self.inner.lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ())
144+
}
145+
146+
pub fn try_lock<'a>(&'a self) -> LockResult<MutexGuard<'a, T>> {
147+
let res = self.inner.try_lock().map(|lock| MutexGuard { mutex: self, lock }).map_err(|_| ());
148+
if res.is_ok() {
149+
MUTEXES_HELD.with(|held| {
150+
// Since a try-lock will simply fail if the lock is held already, we do not
151+
// consider try-locks to ever generate lockorder inversions. However, if a try-lock
152+
// succeeds, we do consider it to have created lockorder dependencies.
153+
for locked in held.borrow().iter() {
154+
self.deps.locked_before.lock().unwrap().insert(Arc::clone(locked));
155+
}
156+
held.borrow_mut().insert(Arc::clone(&self.deps));
157+
});
158+
}
159+
res
160+
}
161+
}
162+
163+
pub struct RwLock<T: ?Sized> {
164+
inner: StdRwLock<T>
165+
}
166+
167+
pub struct RwLockReadGuard<'a, T: ?Sized + 'a> {
168+
lock: StdRwLockReadGuard<'a, T>,
169+
}
170+
171+
pub struct RwLockWriteGuard<'a, T: ?Sized + 'a> {
172+
lock: StdRwLockWriteGuard<'a, T>,
173+
}
174+
175+
impl<T: ?Sized> Deref for RwLockReadGuard<'_, T> {
176+
type Target = T;
177+
178+
fn deref(&self) -> &T {
179+
&self.lock.deref()
180+
}
181+
}
182+
183+
impl<T: ?Sized> Deref for RwLockWriteGuard<'_, T> {
184+
type Target = T;
185+
186+
fn deref(&self) -> &T {
187+
&self.lock.deref()
188+
}
189+
}
190+
191+
impl<T: ?Sized> DerefMut for RwLockWriteGuard<'_, T> {
192+
fn deref_mut(&mut self) -> &mut T {
193+
self.lock.deref_mut()
194+
}
195+
}
196+
197+
impl<T> RwLock<T> {
198+
pub fn new(inner: T) -> RwLock<T> {
199+
RwLock { inner: StdRwLock::new(inner) }
200+
}
201+
202+
pub fn read<'a>(&'a self) -> LockResult<RwLockReadGuard<'a, T>> {
203+
self.inner.read().map(|lock| RwLockReadGuard { lock }).map_err(|_| ())
204+
}
205+
206+
pub fn write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
207+
self.inner.write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
208+
}
209+
210+
pub fn try_write<'a>(&'a self) -> LockResult<RwLockWriteGuard<'a, T>> {
211+
self.inner.try_write().map(|lock| RwLockWriteGuard { lock }).map_err(|_| ())
212+
}
213+
}

lightning/src/lib.rs

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,8 +143,16 @@ mod prelude {
143143
pub use alloc::string::ToString;
144144
}
145145

146+
#[cfg(all(feature = "std", test))]
147+
mod debug_sync;
148+
#[cfg(all(feature = "backtrace", feature = "std", test))]
149+
extern crate backtrace;
150+
146151
#[cfg(feature = "std")]
147152
mod sync {
153+
#[cfg(test)]
154+
pub use debug_sync::*;
155+
#[cfg(not(test))]
148156
pub use ::std::sync::{Arc, Mutex, Condvar, MutexGuard, RwLock, RwLockReadGuard};
149157
}
150158

lightning/src/ln/channelmanager.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -3931,12 +3931,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
39313931
}
39323932

39333933
fn finalize_claims(&self, mut sources: Vec<HTLCSource>) {
3934+
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
39343935
let mut pending_events = self.pending_events.lock().unwrap();
39353936
for source in sources.drain(..) {
39363937
if let HTLCSource::OutboundRoute { session_priv, payment_id, path, .. } = source {
39373938
let mut session_priv_bytes = [0; 32];
39383939
session_priv_bytes.copy_from_slice(&session_priv[..]);
3939-
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
39403940
if let hash_map::Entry::Occupied(mut payment) = outbounds.entry(payment_id) {
39413941
assert!(payment.get().is_fulfilled());
39423942
if payment.get_mut().remove(&session_priv_bytes, None) {
@@ -5321,8 +5321,8 @@ where
53215321
inbound_payment.expiry_time > header.time as u64
53225322
});
53235323

5324-
let mut pending_events = self.pending_events.lock().unwrap();
53255324
let mut outbounds = self.pending_outbound_payments.lock().unwrap();
5325+
let mut pending_events = self.pending_events.lock().unwrap();
53265326
outbounds.retain(|payment_id, payment| {
53275327
if payment.remaining_parts() != 0 { return true }
53285328
if let PendingOutboundPayment::Retryable { starting_block_height, payment_hash, .. } = payment {
@@ -6151,6 +6151,8 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
61516151
peer_state.latest_features.write(writer)?;
61526152
}
61536153

6154+
let pending_inbound_payments = self.pending_inbound_payments.lock().unwrap();
6155+
let pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
61546156
let events = self.pending_events.lock().unwrap();
61556157
(events.len() as u64).write(writer)?;
61566158
for event in events.iter() {
@@ -6172,14 +6174,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
61726174
(self.last_node_announcement_serial.load(Ordering::Acquire) as u32).write(writer)?;
61736175
(self.highest_seen_timestamp.load(Ordering::Acquire) as u32).write(writer)?;
61746176

6175-
let pending_inbound_payments = self.pending_inbound_payments.lock().unwrap();
61766177
(pending_inbound_payments.len() as u64).write(writer)?;
61776178
for (hash, pending_payment) in pending_inbound_payments.iter() {
61786179
hash.write(writer)?;
61796180
pending_payment.write(writer)?;
61806181
}
61816182

6182-
let pending_outbound_payments = self.pending_outbound_payments.lock().unwrap();
61836183
// For backwards compat, write the session privs and their total length.
61846184
let mut num_pending_outbounds_compat: u64 = 0;
61856185
for (_, outbound) in pending_outbound_payments.iter() {

0 commit comments

Comments
 (0)