From 065dc6e689cecce8de8123b412949eca55808c5a Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 26 Feb 2023 20:22:28 +0000 Subject: [PATCH] Make sure individual mutexes are constructed on different lines Our lockdep logic (on Windows) identifies a mutex based on which line it was constructed on. Thus, if we have two mutexes constructed on the same line it will generate false positives. --- lightning/src/chain/channelmonitor.rs | 5 ++++- lightning/src/ln/channelmanager.rs | 5 ++++- lightning/src/sync/debug_sync.rs | 28 ++++++++++++++------------- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs index aaf78fdf..a664c7c7 100644 --- a/lightning/src/chain/channelmonitor.rs +++ b/lightning/src/chain/channelmonitor.rs @@ -4070,7 +4070,10 @@ mod tests { fn test_prune_preimages() { let secp_ctx = Secp256k1::new(); let logger = Arc::new(TestLogger::new()); - let broadcaster = Arc::new(TestBroadcaster{txn_broadcasted: Mutex::new(Vec::new()), blocks: Arc::new(Mutex::new(Vec::new()))}); + let broadcaster = Arc::new(TestBroadcaster { + txn_broadcasted: Mutex::new(Vec::new()), + blocks: Arc::new(Mutex::new(Vec::new())) + }); let fee_estimator = TestFeeEstimator { sat_per_kw: Mutex::new(253) }; let dummy_key = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&[42; 32]).unwrap()); diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index e7e3acdd..1045e77a 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -7532,7 +7532,10 @@ where } } - let pending_outbounds = OutboundPayments { pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()), retry_lock: Mutex::new(()) }; + let pending_outbounds = OutboundPayments { + pending_outbound_payments: Mutex::new(pending_outbound_payments.unwrap()), + retry_lock: Mutex::new(()) + }; if !forward_htlcs.is_empty() || pending_outbounds.needs_abandon() { // If we have pending HTLCs to forward, assume we either dropped a // `PendingHTLCsForwardable` or the user received it but never processed it as they diff --git a/lightning/src/sync/debug_sync.rs b/lightning/src/sync/debug_sync.rs index 72124581..5b6acbca 100644 --- a/lightning/src/sync/debug_sync.rs +++ b/lightning/src/sync/debug_sync.rs @@ -75,7 +75,7 @@ struct LockDep { } #[cfg(feature = "backtrace")] -fn get_construction_location(backtrace: &Backtrace) -> String { +fn get_construction_location(backtrace: &Backtrace) -> (String, Option) { // Find the first frame that is after `debug_sync` (or that is in our tests) and use // that as the mutex construction site. Note that the first few frames may be in // the `backtrace` crate, so we have to ignore those. @@ -86,13 +86,7 @@ fn get_construction_location(backtrace: &Backtrace) -> String { let symbol_name = symbol.name().unwrap().as_str().unwrap(); if !sync_mutex_constr_regex.is_match(symbol_name) { if found_debug_sync { - if let Some(col) = symbol.colno() { - return format!("{}:{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap(), col); - } else { - // Windows debug symbols don't support column numbers, so fall back to - // line numbers only if no `colno` is available - return format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()); - } + return (format!("{}:{}", symbol.filename().unwrap().display(), symbol.lineno().unwrap()), symbol.colno()); } } else { found_debug_sync = true; } } @@ -113,11 +107,17 @@ impl LockMetadata { #[cfg(feature = "backtrace")] { - let lock_constr_location = get_construction_location(&res._lock_construction_bt); + let (lock_constr_location, lock_constr_colno) = + get_construction_location(&res._lock_construction_bt); LOCKS_INIT.call_once(|| { unsafe { LOCKS = Some(StdMutex::new(HashMap::new())); } }); let mut locks = unsafe { LOCKS.as_ref() }.unwrap().lock().unwrap(); match locks.entry(lock_constr_location) { - hash_map::Entry::Occupied(e) => return Arc::clone(e.get()), + hash_map::Entry::Occupied(e) => { + assert_eq!(lock_constr_colno, + get_construction_location(&e.get()._lock_construction_bt).1, + "Because Windows doesn't support column number results in backtraces, we cannot construct two mutexes on the same line or we risk lockorder detection false positives."); + return Arc::clone(e.get()) + }, hash_map::Entry::Vacant(e) => { e.insert(Arc::clone(&res)); }, } } @@ -138,7 +138,7 @@ impl LockMetadata { #[cfg(feature = "backtrace")] debug_assert!(_double_lock_self_allowed, "Tried to acquire a lock while it was held!\nLock constructed at {}", - get_construction_location(&this._lock_construction_bt)); + get_construction_location(&this._lock_construction_bt).0); #[cfg(not(feature = "backtrace"))] panic!("Tried to acquire a lock while it was held!"); } @@ -148,8 +148,10 @@ impl LockMetadata { if *locked_dep_idx == this.lock_idx && *locked_dep_idx != locked.lock_idx { #[cfg(feature = "backtrace")] 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\nLock being taken constructed at: {} ({}):\n{:?}\nLock constructed at: {} ({})\n{:?}\n\nLock dep created at:\n{:?}\n\n", - get_construction_location(&this._lock_construction_bt), this.lock_idx, this._lock_construction_bt, - get_construction_location(&locked._lock_construction_bt), locked.lock_idx, locked._lock_construction_bt, + get_construction_location(&this._lock_construction_bt).0, + this.lock_idx, this._lock_construction_bt, + get_construction_location(&locked._lock_construction_bt).0, + locked.lock_idx, locked._lock_construction_bt, _locked_dep._lockdep_trace); #[cfg(not(feature = "backtrace"))] panic!("Tried to violate existing lockorder. Build with the backtrace feature for more info."); -- 2.30.2