Avoid re-locking same UTXO future
What the developer wrote
Authored by Elias Rohrer
Avoid re-locking same UTXO future
UtxoLookup implementations may cache and return the same async future
for repeated requests for a short channel id. When a replacement channel
announcement arrives while that future is in-flight, the pending-entry
comparison may point back to the future state already held by the async
path.
Detect that case with Arc::ptr_eq inside check_replace_previous_entry
and compare against the held messages instead of taking the mutex again.
This keeps duplicate-announcement filtering intact while letting
replacement announcements update the pending entry without re-entering
the lock.
Co-Authored-By: HAL 9000
This finding was discovered by Project Loupe
What changed, and why it matters
This commit fixes a potential deadlock in how Lightning Dev Kit handles repeated lookups of the same short channel ID. When a custom UTXO lookup service returns the same in-flight future object for repeated requests, and a replacement channel announcement arrives during that lookup, the code could try to lock the same data twice. The patch detects when the pending entry refers to the already-held future state and compares messages without re-acquiring the lock, preventing deadlock while still filtering duplicate announcements.
Review and merge the patch, then ensure any downstream users running custom UtxoLookup implementations with async futures update to this version. No immediate incident response is indicated, but denial-of-service via deadlock should be considered in threat models for nodes using custom UTXO sources.
Security signals we found
Potential self-deadlock in async UTXO lookup path
Re-entrant mutex lock on same Arc<Mutex<UtxoMessages>>
Custom UtxoLookup returning cached future could trigger lock ordering violation
Replacement channel announcement handling interacts with pending async state
Regression test added for deadlock avoidance
Evidence from the diff
In lightning/src/routing/utxo.rs, check_replace_previous_entry previously accepted an Option<Weak<Mutex<UtxoMessages>>> for replacement entries. When called from the async path, it could attempt to lock a Mutex<UtxoMessages> whose guard was already held by the caller, creating a self-deadlock risk if the same Arc<Mutex<...>> was referenced. The patch changes the replacement parameter to Option<(&Arc<Mutex<UtxoMessages>>, &UtxoMessages)> so the function can use Arc::ptr_eq to detect when the pending entry points to the same mutex the caller already holds, and compare against the held UtxoMessages directly instead of calling unsafe_well_ordered_double_lock_self. A regression test test_no_deadlock_same_future_different_announcement is added to verify replacement announcements update the pending entry without re-locking.
Changed components
lightning/src/routing/utxo.rsPendingChecks::check_replace_previous_entryUtxoLookup async future handlingNetwork graph channel announcement processingInspect captured patch +105 / −28
diff --git a/lightning/src/routing/utxo.rs b/lightning/src/routing/utxo.rs
index 6b2f296..1027036 100644
--- a/lightning/src/routing/utxo.rs
+++ b/lightning/src/routing/utxo.rs
@@ -293,11 +293,34 @@ impl PendingChecks {
Ok(())
}
+ fn pending_channel_announcement_matches(
+ msg: &msgs::UnsignedChannelAnnouncement, full_msg: Option<&msgs::ChannelAnnouncement>,
+ pending_state: &UtxoMessages,
+ ) -> bool {
+ match &pending_state.channel_announce {
+ Some(ChannelAnnouncement::Full(pending_msg)) => Some(pending_msg) == full_msg,
+ Some(ChannelAnnouncement::Unsigned(pending_msg)) => pending_msg == msg,
+ None => {
+ // This can be reached if `resolve_single_future` has already consumed
+ // `channel_announce` via `.take()` while the `Arc<Mutex<UtxoMessages>>` is still
+ // alive (e.g. held on the stack of `check_resolved_futures`). In that case,
+ // `complete` should also have been taken. Treat it as non-matching and let the
+ // new request fly.
+ debug_assert!(
+ pending_state.complete.is_none(),
+ "channel_announce is None but complete is still pending"
+ );
+ false
+ },
+ }
+ }
+
fn check_replace_previous_entry(
msg: &msgs::UnsignedChannelAnnouncement, full_msg: Option<&msgs::ChannelAnnouncement>,
- replacement: Option<Weak<Mutex<UtxoMessages>>>,
+ replacement: Option<(&Arc<Mutex<UtxoMessages>>, &UtxoMessages)>,
pending_channels: &mut HashMap<u64, Weak<Mutex<UtxoMessages>>>,
) -> Result<(), msgs::LightningError> {
+ let replacement_state = replacement.map(|(state, _)| state);
match pending_channels.entry(msg.short_channel_id) {
hash_map::Entry::Occupied(mut e) => {
// There's already a pending lookup for the given SCID. Check if the messages
@@ -305,30 +328,34 @@ impl PendingChecks {
// lookup if we haven't gotten that far yet).
match Weak::upgrade(&e.get()) {
Some(pending_msgs) => {
- // This may be called with the mutex held on a different UtxoMessages
- // struct, however in that case we have a global lockorder of new messages
- // -> old messages, which makes this safe.
- let pending_state = pending_msgs.unsafe_well_ordered_double_lock_self();
- let pending_matches = match &pending_state.channel_announce {
- Some(ChannelAnnouncement::Full(pending_msg)) => {
- Some(pending_msg) == full_msg
+ let pending_matches = match replacement {
+ Some((replacement, replacement_messages))
+ if Arc::ptr_eq(&pending_msgs, replacement) =>
+ {
+ // The pending entry points to the state whose mutex the caller
+ // already holds. Compare through the held guard instead of locking
+ // it again.
+ Self::pending_channel_announcement_matches(
+ msg,
+ full_msg,
+ replacement_messages,
+ )
},
- Some(ChannelAnnouncement::Unsigned(pending_msg)) => pending_msg == msg,
- None => {
- // This can be reached if `resolve_single_future` has already
- // consumed `channel_announce` via `.take()` while the
- // `Arc<Mutex<UtxoMessages>>` is still alive (e.g. held on
- // the stack of `check_resolved_futures`). In that case,
- // `complete` should also have been taken. Treat it as
- // non-matching and let the new request fly.
- debug_assert!(
- pending_state.complete.is_none(),
- "channel_announce is None but complete is still pending"
+ _ => {
+ // This may be called with the mutex held on a different
+ // UtxoMessages struct, however in that case we have a global
+ // lockorder of new messages -> old messages, which makes this safe.
+ let pending_state =
+ pending_msgs.unsafe_well_ordered_double_lock_self();
+ let matches = Self::pending_channel_announcement_matches(
+ msg,
+ full_msg,
+ &pending_state,
);
- false
+ drop(pending_state);
+ matches
},
};
- drop(pending_state);
if pending_matches {
return Err(LightningError {
err: "Channel announcement is already being checked".to_owned(),
@@ -340,16 +367,16 @@ impl PendingChecks {
// Note that in the replace case whether to replace is somewhat
// arbitrary - both results will be handled, we're just updating the
// value that will be compared to future lookups with the same SCID.
- if let Some(item) = replacement {
- *e.get_mut() = item;
+ if let Some(item) = replacement_state {
+ *e.get_mut() = Arc::downgrade(item);
}
}
},
None => {
// The earlier lookup already resolved. We can't be sure its the same
// so just remove/replace it and move on.
- if let Some(item) = replacement {
- *e.get_mut() = item;
+ if let Some(item) = replacement_state {
+ *e.get_mut() = Arc::downgrade(item);
} else {
e.remove();
}
@@ -357,8 +384,8 @@ impl PendingChecks {
}
},
hash_map::Entry::Vacant(v) => {
- if let Some(item) = replacement {
- v.insert(item);
+ if let Some(item) = replacement_state {
+ v.insert(Arc::downgrade(item));
}
},
}
@@ -442,7 +469,7 @@ impl PendingChecks {
Self::check_replace_previous_entry(
msg,
full_msg,
- Some(Arc::downgrade(&future.state)),
+ Some((&future.state, &async_messages)),
&mut pending_checks.channels,
)?;
async_messages.channel_announce = Some(if let Some(msg) = full_msg {
@@ -1028,6 +1055,56 @@ mod tests {
assert!(!is_test_feature_set);
}
+ #[test]
+ fn test_no_deadlock_same_future_different_announcement() {
+ // A user's UtxoLookup may return the same UtxoFuture for repeated lookups for a
+ // given SCID. A different channel_announcement with that SCID should replace the
+ // pending message without re-locking the already-held future state.
+ let (valid_announcement, chain_source, network_graph, good_script, ..) = get_test_objects();
+ let scid = valid_announcement.contents.short_channel_id;
+
+ let notifier = Arc::new(Notifier::new());
+ let future = UtxoFuture::new(Arc::clone(¬ifier));
+ *chain_source.utxo_ret.lock().unwrap() = UtxoResult::Async(future.clone());
+
+ assert_eq!(
+ network_graph
+ .update_channel_from_announcement(&valid_announcement, &Some(&chain_source))
+ .unwrap_err()
+ .err,
+ "Channel being checked async"
+ );
+ assert_eq!(chain_source.get_utxo_call_count.load(Ordering::Relaxed), 1);
+
+ let secp_ctx = Secp256k1::new();
+ let replacement_pk_1 = &SecretKey::from_slice(&[99; 32]).unwrap();
+ let replacement_pk_2 = &SecretKey::from_slice(&[98; 32]).unwrap();
+ let replacement_announcement = get_signed_channel_announcement(
+ |msg| msg.features.set_unknown_feature_optional(),
+ replacement_pk_1,
+ replacement_pk_2,
+ &secp_ctx,
+ );
+ assert_eq!(
+ network_graph
+ .update_channel_from_announcement(&replacement_announcement, &Some(&chain_source))
+ .unwrap_err()
+ .err,
+ "Channel being checked async"
+ );
+ assert_eq!(chain_source.get_utxo_call_count.load(Ordering::Relaxed), 2);
+
+ future
+ .resolve(Ok(TxOut { value: Amount::from_sat(1_000_000), script_pubkey: good_script }));
+ assert!(notifier.notify_pending());
+ network_graph.pending_checks.check_resolved_futures(&network_graph);
+ #[rustfmt::skip]
+ let is_replacement_feature_set =
+ network_graph.read_only().channels().get(&scid).unwrap().announcement_message
+ .as_ref().unwrap().contents.features.supports_unknown_test_feature();
+ assert!(is_replacement_feature_set);
+ }
+
#[test]
fn test_checks_backpressure() {
// Test that too_many_checks_pending returns true when there are many checks pending, and
Why this scored 56/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.