Merge pull request #241 from TheBlueMatt/2018-10-peer-free-panic

Fix pre-noise outbound peer disconnect panic found by fuzzer

Author: TheBlueMatt

fuzz/fuzz_targets/full_stack_target.rs

@@ -209,15 +209,17 @@ impl<'a> MoneyLossDetector<'a> {
 
 impl<'a> Drop for MoneyLossDetector<'a> {
 	fn drop(&mut self) {
-		// Disconnect all peers
-		for (idx, peer) in self.peers.borrow().iter().enumerate() {
-			if *peer {
-				self.handler.disconnect_event(&Peer{id: idx as u8, peers_connected: &self.peers});
+		if !::std::thread::panicking() {
+			// Disconnect all peers
+			for (idx, peer) in self.peers.borrow().iter().enumerate() {
+				if *peer {
+					self.handler.disconnect_event(&Peer{id: idx as u8, peers_connected: &self.peers});
+				}
 			}
-		}
 
-		// Force all channels onto the chain (and time out claim txn)
-		self.manager.force_close_all_channels();
+			// Force all channels onto the chain (and time out claim txn)
+			self.manager.force_close_all_channels();
+		}
 	}
 }
 

src/ln/channelmanager.rs

@@ -2733,6 +2733,7 @@ impl ChannelMessageHandler for ChannelManager {
 			let short_to_id = channel_state.short_to_id;
 			let pending_msg_events = channel_state.pending_msg_events;
 			if no_connection_possible {
+				log_debug!(self, "Failing all channels with {} due to no_connection_possible", log_pubkey!(their_node_id));
 				channel_state.by_id.retain(|_, chan| {
 					if chan.get_their_node_id() == *their_node_id {
 						if let Some(short_id) = chan.get_short_channel_id() {
@@ -2750,6 +2751,7 @@ impl ChannelMessageHandler for ChannelManager {
 					}
 				});
 			} else {
+				log_debug!(self, "Marking channels with {} disconnected and generating channel_updates", log_pubkey!(their_node_id));
 				channel_state.by_id.retain(|_, chan| {
 					if chan.get_their_node_id() == *their_node_id {
 						//TODO: mark channel disabled (and maybe announce such after a timeout).
@@ -2780,6 +2782,8 @@ impl ChannelMessageHandler for ChannelManager {
 	}
 
 	fn peer_connected(&self, their_node_id: &PublicKey) {
+		log_debug!(self, "Generating channel_reestablish events for {}", log_pubkey!(their_node_id));
+
 		let _ = self.total_consistency_lock.read().unwrap();
 		let mut channel_state_lock = self.channel_state.lock().unwrap();
 		let channel_state = channel_state_lock.borrow_parts();

src/ln/peer_handler.rs

@@ -359,6 +359,9 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 
 							macro_rules! try_potential_handleerror {
 								($thing: expr) => {
+									try_potential_handleerror!($thing, false);
+								};
+								($thing: expr, $pre_noise: expr) => {
 									match $thing {
 										Ok(x) => x,
 										Err(e) => {
@@ -367,6 +370,9 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 													msgs::ErrorAction::DisconnectPeer { msg: _ } => {
 														//TODO: Try to push msg
 														log_trace!(self, "Got Err handling message, disconnecting peer because {}", e.err);
+														if $pre_noise {
+															peer.their_node_id = None; // Unset so that we don't generate a peer_disconnected event
+														}
 														return Err(PeerHandleError{ no_connection_possible: false });
 													},
 													msgs::ErrorAction::IgnoreError => {
@@ -417,23 +423,27 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 								() => {
 									match peers.node_id_to_descriptor.entry(peer.their_node_id.unwrap()) {
 										hash_map::Entry::Occupied(_) => {
+											log_trace!(self, "Got second connection with {}, closing", log_pubkey!(peer.their_node_id.unwrap()));
 											peer.their_node_id = None; // Unset so that we don't generate a peer_disconnected event
 											return Err(PeerHandleError{ no_connection_possible: false })
 										},
-										hash_map::Entry::Vacant(entry) => entry.insert(peer_descriptor.clone()),
+										hash_map::Entry::Vacant(entry) => {
+											log_trace!(self, "Finished noise handshake for connection with {}", log_pubkey!(peer.their_node_id.unwrap()));
+											entry.insert(peer_descriptor.clone())
+										},
 									};
 								}
 							}
 
 							let next_step = peer.channel_encryptor.get_noise_step();
 							match next_step {
 								NextNoiseStep::ActOne => {
-									let act_two = try_potential_handleerror!(peer.channel_encryptor.process_act_one_with_key(&peer.pending_read_buffer[..], &self.our_node_secret)).to_vec();
+									let act_two = try_potential_handleerror!(peer.channel_encryptor.process_act_one_with_key(&peer.pending_read_buffer[..], &self.our_node_secret), true).to_vec();
 									peer.pending_outbound_buffer.push_back(act_two);
 									peer.pending_read_buffer = [0; 66].to_vec(); // act three is 66 bytes long
 								},
 								NextNoiseStep::ActTwo => {
-									let act_three = try_potential_handleerror!(peer.channel_encryptor.process_act_two(&peer.pending_read_buffer[..], &self.our_node_secret)).to_vec();
+									let act_three = try_potential_handleerror!(peer.channel_encryptor.process_act_two(&peer.pending_read_buffer[..], &self.our_node_secret), true).to_vec();
 									peer.pending_outbound_buffer.push_back(act_three);
 									peer.pending_read_buffer = [0; 18].to_vec(); // Message length header is 18 bytes
 									peer.pending_read_is_header = true;
@@ -450,7 +460,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 									}, 16);
 								},
 								NextNoiseStep::ActThree => {
-									let their_node_id = try_potential_handleerror!(peer.channel_encryptor.process_act_three(&peer.pending_read_buffer[..]));
+									let their_node_id = try_potential_handleerror!(peer.channel_encryptor.process_act_three(&peer.pending_read_buffer[..]), true);
 									peer.pending_read_buffer = [0; 18].to_vec(); // Message length header is 18 bytes
 									peer.pending_read_is_header = true;
 									peer.their_node_id = Some(their_node_id);
@@ -477,6 +487,7 @@ impl<Descriptor: SocketDescriptor> PeerManager<Descriptor> {
 										log_trace!(self, "Received message of type {} from {}", msg_type, log_pubkey!(peer.their_node_id.unwrap()));
 										if msg_type != 16 && peer.their_global_features.is_none() {
 											// Need an init message as first message
+											log_trace!(self, "Peer {} sent non-Init first message", log_pubkey!(peer.their_node_id.unwrap()));
 											return Err(PeerHandleError{ no_connection_possible: false });
 										}
 										let mut reader = ::std::io::Cursor::new(&msg_data[2..]);