From 86143fd69ddf88a03fc19244aca33ec598ee1720 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 13 Jan 2020 16:10:30 -0500 Subject: [PATCH] Fix deadlock in handle_error!() when we have HTLCs to fail-back. This partially reverts 933ae3470309f21ef7537ffbcdc42070d60e1e74, though note that 933ae3470309f21ef7537ffbcdc42070d60e1e74 fixed a similar deadlock while introducing this one. If we have HTLCs to fail backwards, handle_error!() will call finish_force_close_channel() which will attempt to lock channel_state while it is locked at the original caller. Instead, hold the lock for shorter scopes such that it is not held upon entering handle_error!(). Co-authored-by: Matt Corallo Co-authored-by: Jeffrey Czyz --- lightning/src/ln/channelmanager.rs | 161 ++++++++++------------------- 1 file changed, 54 insertions(+), 107 deletions(-) diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs index cde40ee32..e9dfe5a56 100644 --- a/lightning/src/ln/channelmanager.rs +++ b/lightning/src/ln/channelmanager.rs @@ -467,21 +467,41 @@ pub struct ChannelDetails { } macro_rules! handle_error { - ($self: ident, $internal: expr, $their_node_id: expr, $locked_channel_state: expr) => { + ($self: ident, $internal: expr, $their_node_id: expr) => { match $internal { Ok(msg) => Ok(msg), Err(MsgHandleErrInternal { err, shutdown_finish }) => { + #[cfg(debug_assertions)] + { + // In testing, ensure there are no deadlocks where the lock is already held upon + // entering the macro. + assert!($self.channel_state.try_lock().is_ok()); + } + + let mut msg_events = Vec::with_capacity(2); + if let Some((shutdown_res, update_option)) = shutdown_finish { $self.finish_force_close_channel(shutdown_res); if let Some(update) = update_option { - $locked_channel_state.pending_msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { + msg_events.push(events::MessageSendEvent::BroadcastChannelUpdate { msg: update }); } } + log_error!($self, "{}", err.err); if let msgs::ErrorAction::IgnoreError = err.action { - } else { $locked_channel_state.pending_msg_events.push(events::MessageSendEvent::HandleError { node_id: $their_node_id, action: err.action.clone() }); } + } else { + msg_events.push(events::MessageSendEvent::HandleError { + node_id: $their_node_id, + action: err.action.clone() + }); + } + + if !msg_events.is_empty() { + $self.channel_state.lock().unwrap().pending_msg_events.append(&mut msg_events); + } + // Return error in case higher-API need one Err(err) }, @@ -1191,9 +1211,8 @@ impl ChannelMan let _ = self.total_consistency_lock.read().unwrap(); - let mut channel_lock = self.channel_state.lock().unwrap(); let err: Result<(), _> = loop { - + let mut channel_lock = self.channel_state.lock().unwrap(); let id = match channel_lock.short_to_id.get(&route.hops.first().unwrap().short_channel_id) { None => return Err(APIError::ChannelUnavailable{err: "No channel available with first hop!"}), Some(id) => id.clone(), @@ -1242,7 +1261,7 @@ impl ChannelMan return Ok(()); }; - match handle_error!(self, err, route.hops.first().unwrap().pubkey, channel_lock) { + match handle_error!(self, err, route.hops.first().unwrap().pubkey) { Ok(_) => unreachable!(), Err(e) => { Err(APIError::ChannelUnavailable { err: e.err }) } } @@ -1261,8 +1280,7 @@ impl ChannelMan let _ = self.total_consistency_lock.read().unwrap(); let (mut chan, msg, chan_monitor) = { - let mut channel_state = self.channel_state.lock().unwrap(); - let (res, chan) = match channel_state.by_id.remove(temporary_channel_id) { + let (res, chan) = match self.channel_state.lock().unwrap().by_id.remove(temporary_channel_id) { Some(mut chan) => { (chan.get_outbound_funding_created(funding_txo) .map_err(|e| if let ChannelError::Close(msg) = e { @@ -1272,7 +1290,7 @@ impl ChannelMan }, None => return }; - match handle_error!(self, res, chan.get_their_node_id(), channel_state) { + match handle_error!(self, res, chan.get_their_node_id()) { Ok(funding_msg) => { (chan, funding_msg.0, funding_msg.1) }, @@ -1284,12 +1302,9 @@ impl ChannelMan if let Err(e) = self.monitor.add_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) { match e { ChannelMonitorUpdateErr::PermanentFailure => { - { - let mut channel_state = self.channel_state.lock().unwrap(); - match handle_error!(self, Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", *temporary_channel_id, chan.force_shutdown(true), None)), chan.get_their_node_id(), channel_state) { - Err(_) => { return; }, - Ok(()) => unreachable!(), - } + match handle_error!(self, Err(MsgHandleErrInternal::from_finish_shutdown("ChannelMonitor storage failure", *temporary_channel_id, chan.force_shutdown(true), None)), chan.get_their_node_id()) { + Err(_) => { return; }, + Ok(()) => unreachable!(), } }, ChannelMonitorUpdateErr::TemporaryFailure => { @@ -1520,10 +1535,8 @@ impl ChannelMan }, ChannelError::CloseDelayBroadcast { .. } => { panic!("Wait is only generated on receipt of channel_reestablish, which is handled by try_chan_entry, we don't bother to support it here"); } }; - match handle_error!(self, err, their_node_id, channel_state) { - Ok(_) => unreachable!(), - Err(_) => { continue; }, - } + handle_errors.push((their_node_id, err)); + continue; } }; if let Err(e) = self.monitor.update_monitor(chan.get().get_funding_txo().unwrap(), monitor_update) { @@ -1579,11 +1592,8 @@ impl ChannelMan }; } - if handle_errors.len() > 0 { - let mut channel_state_lock = self.channel_state.lock().unwrap(); - for (their_node_id, err) in handle_errors.drain(..) { - let _ = handle_error!(self, err, their_node_id, channel_state_lock); - } + for (their_node_id, err) in handle_errors.drain(..) { + let _ = handle_error!(self, err, their_node_id); } if new_events.is_empty() { return } @@ -1835,7 +1845,8 @@ impl ChannelMan return; }; - let _ = handle_error!(self, err, their_node_id, channel_state_lock); + mem::drop(channel_state_lock); + let _ = handle_error!(self, err, their_node_id); } /// Gets the node_id held by this ChannelManager @@ -2579,9 +2590,9 @@ impl ChannelMan #[doc(hidden)] pub fn update_fee(&self, channel_id: [u8;32], feerate_per_kw: u64) -> Result<(), APIError> { let _ = self.total_consistency_lock.read().unwrap(); - let mut channel_state_lock = self.channel_state.lock().unwrap(); let their_node_id; let err: Result<(), _> = loop { + let mut channel_state_lock = self.channel_state.lock().unwrap(); let channel_state = &mut *channel_state_lock; match channel_state.by_id.entry(channel_id) { @@ -2620,7 +2631,7 @@ impl ChannelMan return Ok(()) }; - match handle_error!(self, err, their_node_id, channel_state_lock) { + match handle_error!(self, err, their_node_id) { Ok(_) => unreachable!(), Err(e) => { Err(APIError::APIMisuseError { err: e.err })} } @@ -2830,146 +2841,82 @@ impl