Actually remove forward<>intercepted htlcs lock dep
What the developer wrote
Authored by Valentine Wallace
Actually remove forward<>intercepted htlcs lock dep
8513341b1f7c39da3e6ad4521d4161362b0f4562 purported to remove a lock dependency
between ChannelManager::pending_intercepted_htlcs and ::forward_htlcs. However,
in practice the dependency remained because the locks of the two maps were
still held at the same time in some cases. Here we fix this.
What changed, and why it matters
This commit fixes an incomplete earlier change that was meant to prevent two internal data structures in LDK's channel manager from being locked at the same time. Holding both locks together can create a lock-order dependency, which in the worst case can lead to deadlock (the program freezing up). The fix narrows the use of one lock so the two locks are no longer held together in the affected code path. There is no direct evidence in the commit of an exploitable security vulnerability such as theft of funds.
Treat as a liveness/deadlock hardening patch. Apply in routine maintenance. Monitor for any related deadlock reports or follow-up commits that address remaining lock-order issues. No emergency response is indicated by the diff alone.
Security signals we found
Lock-order dependency between two internal maps
Potential deadlock if lock acquisition order is inconsistent elsewhere
Follow-up to an incomplete prior fix (8513341b1f7c39da3e6ad4521d4161362b0f4562)
No input validation, cryptographic, or memory-safety issue visible in diff
Evidence from the diff
Commit b9f5fd6 removes a remaining instance where ChannelManager::forward_htlcs was locked while pending_intercepted_htlcs was also held. The prior commit 8513341 claimed to break this lock dependency, but forward_htlcs was still locked for the entire process_pending_intercepted_htlcs iteration. The patch moves the forward_htlcs.lock() call from before the per-HTLC processing into the else branch where it is actually needed, so the two mutexes are no longer held simultaneously. This is a deadlock-prevention / liveness fix rather than a memory-safety or cryptographic bug.
Changed components
lightning/src/ln/channelmanager.rsChannelManager::process_pending_intercepted_htlcsforward_htlcs map lockingInspect captured patch +1 / −2
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 4d3a3ea..53f45f3 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -10776,7 +10776,6 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
// Pull this now to avoid introducing a lock order with `forward_htlcs`.
let is_our_scid = self.short_to_chan_info.read().unwrap().contains_key(&scid);
- let mut forward_htlcs = self.forward_htlcs.lock().unwrap();
let payment_hash = forward_info.payment_hash;
let logger = WithContext::from(
&self.logger,
@@ -10878,7 +10877,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
},
}
} else {
- match forward_htlcs.entry(scid) {
+ match self.forward_htlcs.lock().unwrap().entry(scid) {
hash_map::Entry::Occupied(mut entry) => {
entry.get_mut().push(HTLCForwardInfo::AddHTLC(pending_add));
},
Why this scored 40/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.