From 559e83a3e4ebfa342819a677aa0a78a8d6b3f1a3 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 29 Feb 2024 10:12:35 +0100 Subject: [PATCH 1/3] Fix deadlock on deserialization failure Previously, we introduced disconnecting a counterparty who sends us bogus messages that we're unable to parse. However, before we disconnect we also send out a last `LSPSMessage::Invalid` in the hopes that the counterparty understands this. However, when we added this logic we unfortunately overlooked that we lock the `request_id_to_method_map` `Mutex` for parsing the message, but also try to lock when the `PeerHandler` calls `get_and_clear_pending_msgs`. Here, we avoid the resulting deadlock by dropping the lock as soon as it's not required anymore after parsing. --- src/manager.rs | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/manager.rs b/src/manager.rs index aa2576a..1a170ae 100644 --- a/src/manager.rs +++ b/src/manager.rs @@ -481,22 +481,23 @@ where &self, msg: Self::CustomMessage, sender_node_id: &PublicKey, ) -> Result<(), lightning::ln::msgs::LightningError> { let message = { - let mut request_id_to_method_map = self.request_id_to_method_map.lock().unwrap(); - LSPSMessage::from_str_with_id_map(&msg.payload, &mut request_id_to_method_map).map_err( - |_| { - let error = ResponseError { - code: JSONRPC_INVALID_MESSAGE_ERROR_CODE, - message: JSONRPC_INVALID_MESSAGE_ERROR_MESSAGE.to_string(), - data: None, - }; - - self.pending_messages.enqueue(sender_node_id, LSPSMessage::Invalid(error)); - let err = format!("Failed to deserialize invalid LSPS message."); - let err_msg = - Some(ErrorMessage { channel_id: ChannelId([0; 32]), data: err.clone() }); - LightningError { err, action: ErrorAction::DisconnectPeer { msg: err_msg } } - }, - )? + { + let mut request_id_to_method_map = self.request_id_to_method_map.lock().unwrap(); + LSPSMessage::from_str_with_id_map(&msg.payload, &mut request_id_to_method_map) + } + .map_err(|_| { + let error = ResponseError { + code: JSONRPC_INVALID_MESSAGE_ERROR_CODE, + message: JSONRPC_INVALID_MESSAGE_ERROR_MESSAGE.to_string(), + data: None, + }; + + self.pending_messages.enqueue(sender_node_id, LSPSMessage::Invalid(error)); + let err = format!("Failed to deserialize invalid LSPS message."); + let err_msg = + Some(ErrorMessage { channel_id: ChannelId([0; 32]), data: err.clone() }); + LightningError { err, action: ErrorAction::DisconnectPeer { msg: err_msg } } + })? }; self.handle_lsps_message(message, sender_node_id) From 0bc72033e7cb51ebfafe4d44c0c5961a5000ae03 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 29 Feb 2024 10:54:22 +0100 Subject: [PATCH 2/3] Avoid unnecessary locking `request_ids_and_methods_map` .. in `get_and_clear_pending_msg`. We also remove a potentially dangerous (if we ever were to fail serialization for some reason) `unwrap`. --- src/manager.rs | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/manager.rs b/src/manager.rs index 1a170ae..643242f 100644 --- a/src/manager.rs +++ b/src/manager.rs @@ -504,18 +504,26 @@ where } fn get_and_clear_pending_msg(&self) -> Vec<(PublicKey, Self::CustomMessage)> { - let mut request_id_to_method_map = self.request_id_to_method_map.lock().unwrap(); - self.pending_messages - .get_and_clear_pending_msgs() + let pending_messages = self.pending_messages.get_and_clear_pending_msgs(); + + let mut request_ids_and_methods = pending_messages .iter() - .map(|(public_key, lsps_message)| { - if let Some((request_id, method)) = lsps_message.get_request_id_and_method() { - request_id_to_method_map.insert(request_id, method); - } - ( - *public_key, - RawLSPSMessage { payload: serde_json::to_string(&lsps_message).unwrap() }, - ) + .filter_map(|(_, msg)| msg.get_request_id_and_method()) + .peekable(); + + if request_ids_and_methods.peek().is_some() { + let mut request_id_to_method_map_lock = self.request_id_to_method_map.lock().unwrap(); + for (request_id, method) in request_ids_and_methods { + request_id_to_method_map_lock.insert(request_id, method); + } + } + + pending_messages + .into_iter() + .filter_map(|(public_key, msg)| { + serde_json::to_string(&msg) + .ok() + .and_then(|payload| Some((public_key, RawLSPSMessage { payload }))) }) .collect() } From 8e06b0db5991a6fbe5cd32535886256ce540b671 Mon Sep 17 00:00:00 2001 From: Elias Rohrer Date: Thu, 29 Feb 2024 15:12:27 +0100 Subject: [PATCH 3/3] Ignore rather than disconnect peers who send us bogus data Disconnecting might be unfortunate if we have open channels with the counterparty. If they send us bogus data, we now just add them to a (currently unpersisted) ignore list. We should in the future figure out when to drop them from this list, so that peers have the chance to recover. --- src/manager.rs | 29 ++++++++++++++++++++++------- 1 file changed, 22 insertions(+), 7 deletions(-) diff --git a/src/manager.rs b/src/manager.rs index 643242f..8fbaa07 100644 --- a/src/manager.rs +++ b/src/manager.rs @@ -19,16 +19,15 @@ use crate::lsps1::service::{LSPS1ServiceConfig, LSPS1ServiceHandler}; use crate::lsps2::client::{LSPS2ClientConfig, LSPS2ClientHandler}; use crate::lsps2::msgs::LSPS2Message; use crate::lsps2::service::{LSPS2ServiceConfig, LSPS2ServiceHandler}; -use crate::prelude::{HashMap, ToString, Vec}; +use crate::prelude::{HashMap, HashSet, ToString, Vec}; use crate::sync::{Arc, Mutex, RwLock}; use lightning::chain::{self, BestBlock, Confirm, Filter, Listen}; use lightning::ln::channelmanager::{AChannelManager, ChainParameters}; use lightning::ln::features::{InitFeatures, NodeFeatures}; -use lightning::ln::msgs::{ErrorAction, ErrorMessage, LightningError}; +use lightning::ln::msgs::{ErrorAction, LightningError}; use lightning::ln::peer_handler::CustomMessageHandler; use lightning::ln::wire::CustomMessageReader; -use lightning::ln::ChannelId; use lightning::sign::EntropySource; use lightning::util::logger::Level; use lightning::util::ser::Readable; @@ -94,6 +93,8 @@ where pending_messages: Arc, pending_events: Arc, request_id_to_method_map: Mutex>, + // We ignore peers if they send us bogus data. + ignored_peers: RwLock>, lsps0_client_handler: LSPS0ClientHandler, lsps0_service_handler: Option, #[cfg(lsps1)] @@ -126,6 +127,7 @@ where where { let pending_messages = Arc::new(MessageQueue::new()); let pending_events = Arc::new(EventQueue::new()); + let ignored_peers = RwLock::new(HashSet::new()); let lsps0_client_handler = LSPS0ClientHandler::new( entropy_source.clone(), @@ -192,6 +194,7 @@ where { pending_messages, pending_events, request_id_to_method_map: Mutex::new(HashMap::new()), + ignored_peers, lsps0_client_handler, lsps0_service_handler, #[cfg(lsps1)] @@ -480,6 +483,16 @@ where fn handle_custom_message( &self, msg: Self::CustomMessage, sender_node_id: &PublicKey, ) -> Result<(), lightning::ln::msgs::LightningError> { + { + if self.ignored_peers.read().unwrap().contains(&sender_node_id) { + let err = format!("Ignoring message from peer {}.", sender_node_id); + return Err(LightningError { + err, + action: ErrorAction::IgnoreAndLog(Level::Trace), + }); + } + } + let message = { { let mut request_id_to_method_map = self.request_id_to_method_map.lock().unwrap(); @@ -493,10 +506,12 @@ where }; self.pending_messages.enqueue(sender_node_id, LSPSMessage::Invalid(error)); - let err = format!("Failed to deserialize invalid LSPS message."); - let err_msg = - Some(ErrorMessage { channel_id: ChannelId([0; 32]), data: err.clone() }); - LightningError { err, action: ErrorAction::DisconnectPeer { msg: err_msg } } + self.ignored_peers.write().unwrap().insert(*sender_node_id); + let err = format!( + "Failed to deserialize invalid LSPS message. Ignoring peer {} from now on.", + sender_node_id + ); + LightningError { err, action: ErrorAction::IgnoreAndLog(Level::Info) } })? };