Merge pull request #78 from TheBlueMatt/2018-07-43-rebased

Add DisconnectPeer event

Author: TheBlueMatt

fuzz/Cargo.toml

@@ -114,3 +114,7 @@ path = "fuzz_targets/msg_targets/msg_update_fail_htlc_target.rs"
 [[bin]]
 name = "msg_channel_reestablish_target"
 path = "fuzz_targets/msg_targets/msg_channel_reestablish_target.rs"
+
+[[bin]]
+name = "msg_error_message_target"
+path = "fuzz_targets/msg_targets/msg_error_message_target.rs"

fuzz/fuzz_targets/channel_target.rs

@@ -118,6 +118,7 @@ pub fn do_test(data: &[u8]) {
 					msgs::DecodeError::UnknownRealmByte => return,
 					msgs::DecodeError::BadPublicKey => return,
 					msgs::DecodeError::BadSignature => return,
+					msgs::DecodeError::BadText => return,
 					msgs::DecodeError::ExtraAddressesPerType => return,
 					msgs::DecodeError::WrongLength => panic!("We picked the length..."),
 				}
@@ -138,6 +139,7 @@ pub fn do_test(data: &[u8]) {
 						msgs::DecodeError::UnknownRealmByte => return,
 						msgs::DecodeError::BadPublicKey => return,
 						msgs::DecodeError::BadSignature => return,
+						msgs::DecodeError::BadText => return,
 						msgs::DecodeError::ExtraAddressesPerType => return,
 						msgs::DecodeError::WrongLength => panic!("We picked the length..."),
 					}

fuzz/fuzz_targets/full_stack_target.rs

@@ -25,7 +25,9 @@ use lightning::util::reset_rng_state;
 use secp256k1::key::{PublicKey,SecretKey};
 use secp256k1::Secp256k1;
 
+use std::cell::RefCell;
 use std::collections::HashMap;
+use std::hash::Hash;
 use std::sync::Arc;
 use std::sync::atomic::{AtomicUsize,Ordering};
 
@@ -104,15 +106,31 @@ impl BroadcasterInterface for TestBroadcaster {
 	fn broadcast_transaction(&self, _tx: &Transaction) {}
 }
 
-#[derive(Clone, PartialEq, Eq, Hash)]
-struct Peer {
+#[derive(Clone)]
+struct Peer<'a> {
 	id: u8,
+	peers_connected: &'a RefCell<[bool; 256]>,
 }
-impl SocketDescriptor for Peer {
+impl<'a> SocketDescriptor for Peer<'a> {
 	fn send_data(&mut self, data: &Vec<u8>, write_offset: usize, _resume_read: bool) -> usize {
 		assert!(write_offset < data.len());
 		data.len() - write_offset
 	}
+	fn disconnect_socket(&mut self) {
+		assert!(self.peers_connected.borrow()[self.id as usize]);
+		self.peers_connected.borrow_mut()[self.id as usize] = false;
+	}
+}
+impl<'a> PartialEq for Peer<'a> {
+	fn eq(&self, other: &Self) -> bool {
+		self.id == other.id
+	}
+}
+impl<'a> Eq for Peer<'a> {}
+impl<'a> Hash for Peer<'a> {
+	fn hash<H : std::hash::Hasher>(&self, h: &mut H) {
+		self.id.hash(h)
+	}
 }
 
 #[inline]
@@ -158,12 +176,12 @@ pub fn do_test(data: &[u8]) {
 	let channelmanager = ChannelManager::new(our_network_key, slice_to_be32(get_slice!(4)), get_slice!(1)[0] != 0, Network::Bitcoin, fee_est.clone(), monitor.clone(), watch.clone(), broadcast.clone()).unwrap();
 	let router = Arc::new(Router::new(PublicKey::from_secret_key(&secp_ctx, &our_network_key).unwrap()));
 
+	let peers = RefCell::new([false; 256]);
 	let handler = PeerManager::new(MessageHandler {
 		chan_handler: channelmanager.clone(),
 		route_handler: router.clone(),
 	}, our_network_key);
 
-	let mut peers = [false; 256];
 	let mut should_forward = false;
 	let mut payments_received = Vec::new();
 	let mut payments_sent = 0;
@@ -176,39 +194,39 @@ pub fn do_test(data: &[u8]) {
 			0 => {
 				let mut new_id = 0;
 				for i in 1..256 {
-					if !peers[i-1] {
+					if !peers.borrow()[i-1] {
 						new_id = i;
 						break;
 					}
 				}
 				if new_id == 0 { return; }
-				peers[new_id - 1] = true;
-				handler.new_outbound_connection(get_pubkey!(), Peer{id: (new_id - 1) as u8}).unwrap();
+				peers.borrow_mut()[new_id - 1] = true;
+				handler.new_outbound_connection(get_pubkey!(), Peer{id: (new_id - 1) as u8, peers_connected: &peers}).unwrap();
 			},
 			1 => {
 				let mut new_id = 0;
 				for i in 1..256 {
-					if !peers[i-1] {
+					if !peers.borrow()[i-1] {
 						new_id = i;
 						break;
 					}
 				}
 				if new_id == 0 { return; }
-				peers[new_id - 1] = true;
-				handler.new_inbound_connection(Peer{id: (new_id - 1) as u8}).unwrap();
+				peers.borrow_mut()[new_id - 1] = true;
+				handler.new_inbound_connection(Peer{id: (new_id - 1) as u8, peers_connected: &peers}).unwrap();
 			},
 			2 => {
 				let peer_id = get_slice!(1)[0];
-				if !peers[peer_id as usize] { return; }
-				peers[peer_id as usize] = false;
-				handler.disconnect_event(&Peer{id: peer_id});
+				if !peers.borrow()[peer_id as usize] { return; }
+				peers.borrow_mut()[peer_id as usize] = false;
+				handler.disconnect_event(&Peer{id: peer_id, peers_connected: &peers});
 			},
 			3 => {
 				let peer_id = get_slice!(1)[0];
-				if !peers[peer_id as usize] { return; }
-				match handler.read_event(&mut Peer{id: peer_id}, get_slice!(get_slice!(1)[0]).to_vec()) {
+				if !peers.borrow()[peer_id as usize] { return; }
+				match handler.read_event(&mut Peer{id: peer_id, peers_connected: &peers}, get_slice!(get_slice!(1)[0]).to_vec()) {
 					Ok(res) => assert!(!res),
-					Err(_) => { peers[peer_id as usize] = false; }
+					Err(_) => { peers.borrow_mut()[peer_id as usize] = false; }
 				}
 			},
 			4 => {
@@ -231,7 +249,7 @@ pub fn do_test(data: &[u8]) {
 			},
 			5 => {
 				let peer_id = get_slice!(1)[0];
-				if !peers[peer_id as usize] { return; }
+				if !peers.borrow()[peer_id as usize] { return; }
 				let their_key = get_pubkey!();
 				let chan_value = slice_to_be24(get_slice!(3)) as u64;
 				if channelmanager.create_channel(their_key, chan_value, 0).is_err() { return; }

fuzz/fuzz_targets/msg_targets/gen_target.sh

@@ -1,4 +1,4 @@
-for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish; do
+for target in CommitmentSigned FundingCreated FundingLocked FundingSigned OpenChannel RevokeAndACK Shutdown UpdateAddHTLC UpdateFailHTLC UpdateFailMalformedHTLC UpdateFee UpdateFulfillHTLC AcceptChannel ClosingSigned ChannelReestablish ErrorMessage; do
 	tn=$(echo $target | sed 's/\([a-z0-9]\)\([A-Z]\)/\1_\2/g')
 	fn=msg_$(echo $tn | tr '[:upper:]' '[:lower:]')_target.rs
 	cat msg_target_template.txt | sed s/MSG_TARGET/$target/ > $fn

fuzz/fuzz_targets/msg_targets/msg_error_message_target.rs

@@ -0,0 +1,48 @@
+// This file is auto-generated by gen_target.sh based on msg_target_template.txt
+// To modify it, modify msg_target_template.txt and run gen_target.sh instead.
+
+extern crate lightning;
+
+use lightning::ln::msgs;
+use lightning::util::reset_rng_state;
+
+use lightning::ln::msgs::{MsgEncodable, MsgDecodable};
+
+mod utils;
+
+#[inline]
+pub fn do_test(data: &[u8]) {
+	reset_rng_state();
+	test_msg!(msgs::ErrorMessage, data);
+}
+
+#[cfg(feature = "afl")]
+extern crate afl;
+#[cfg(feature = "afl")]
+fn main() {
+	afl::read_stdio_bytes(|data| {
+		do_test(&data);
+	});
+}
+
+#[cfg(feature = "honggfuzz")]
+#[macro_use] extern crate honggfuzz;
+#[cfg(feature = "honggfuzz")]
+fn main() {
+	loop {
+		fuzz!(|data| {
+			do_test(data);
+		});
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use utils::extend_vec_from_hex;
+	#[test]
+	fn duplicate_crash() {
+		let mut a = Vec::new();
+		extend_vec_from_hex("00", &mut a);
+		super::do_test(&a);
+	}
+}

src/ln/channel.rs

@@ -328,7 +328,7 @@ macro_rules! secp_call {
 		match $res {
 			Ok(key) => key,
 			//TODO: make the error a parameter
-			Err(_) => return Err(HandleError{err: $err, action: Some(msgs::ErrorAction::DisconnectPeer{})})
+			Err(_) => return Err(HandleError{err: $err, action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })})
 		}
 	};
 }
@@ -433,10 +433,10 @@ impl Channel {
 
 	fn check_remote_fee(fee_estimator: &FeeEstimator, feerate_per_kw: u32) -> Result<(), HandleError> {
 		if (feerate_per_kw as u64) < fee_estimator.get_est_sat_per_vbyte(ConfirmationTarget::Background) * 250 {
-			return Err(HandleError{err: "Peer's feerate much too low", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Peer's feerate much too low", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if (feerate_per_kw as u64) > fee_estimator.get_est_sat_per_vbyte(ConfirmationTarget::HighPriority) * 375 { // 375 = 250 * 1.5x
-			return Err(HandleError{err: "Peer's feerate much too high", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Peer's feerate much too high", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		Ok(())
 	}
@@ -448,29 +448,29 @@ impl Channel {
 	pub fn new_from_req(fee_estimator: &FeeEstimator, chan_keys: ChannelKeys, their_node_id: PublicKey, msg: &msgs::OpenChannel, user_id: u64, announce_publicly: bool) -> Result<Channel, HandleError> {
 		// Check sanity of message fields:
 		if msg.funding_satoshis >= MAX_FUNDING_SATOSHIS {
-			return Err(HandleError{err: "funding value > 2^24", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "funding value > 2^24", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if msg.channel_reserve_satoshis > msg.funding_satoshis {
-			return Err(HandleError{err: "Bogus channel_reserve_satoshis", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Bogus channel_reserve_satoshis", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if msg.push_msat > (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
-			return Err(HandleError{err: "push_msat more than highest possible value", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "push_msat more than highest possible value", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if msg.dust_limit_satoshis > msg.funding_satoshis {
-			return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Peer never wants payout outputs?", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if msg.htlc_minimum_msat >= (msg.funding_satoshis - msg.channel_reserve_satoshis) * 1000 {
-			return Err(HandleError{err: "Minimum htlc value is full channel value", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "Minimum htlc value is full channel value", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		Channel::check_remote_fee(fee_estimator, msg.feerate_per_kw)?;
 		if msg.to_self_delay > MAX_LOCAL_BREAKDOWN_TIMEOUT {
-			return Err(HandleError{err: "They wanted our payments to be delayed by a needlessly long period", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "They wanted our payments to be delayed by a needlessly long period", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if msg.max_accepted_htlcs < 1 {
-			return Err(HandleError{err: "0 max_accpted_htlcs makes for a useless channel", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "0 max_accpted_htlcs makes for a useless channel", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 		if (msg.channel_flags & 254) != 0 {
-			return Err(HandleError{err: "unknown channel_flags", action: Some(msgs::ErrorAction::DisconnectPeer{})});
+			return Err(HandleError{err: "unknown channel_flags", action: Some(msgs::ErrorAction::DisconnectPeer{ msg: None })});
 		}
 
 		// Convert things into internal flags and prep our state:

src/ln/channelmanager.rs

@@ -705,6 +705,16 @@ impl ChannelManager {
 	/// Call this upon creation of a funding transaction for the given channel.
 	/// Panics if a funding transaction has already been provided for this channel.
 	pub fn funding_transaction_generated(&self, temporary_channel_id: &[u8; 32], funding_txo: OutPoint) {
+
+		macro_rules! add_pending_event {
+			($event: expr) => {
+				{
+					let mut pending_events = self.pending_events.lock().unwrap();
+					pending_events.push($event);
+				}
+			}
+		}
+
 		let (chan, msg, chan_monitor) = {
 			let mut channel_state = self.channel_state.lock().unwrap();
 			match channel_state.by_id.remove(temporary_channel_id) {
@@ -713,10 +723,15 @@ impl ChannelManager {
 						Ok(funding_msg) => {
 							(chan, funding_msg.0, funding_msg.1)
 						},
-						Err(_e) => {
-							//TODO: Push e to pendingevents
+						Err(e) => {
+							mem::drop(channel_state);
+							add_pending_event!(events::Event::DisconnectPeer {
+								node_id: chan.get_their_node_id(),
+								msg: if let Some(msgs::ErrorAction::DisconnectPeer { msg } ) = e.action { msg } else { None },
+							});
+
 							return;
-						}
+						},
 					}
 				},
 				None => return
@@ -725,13 +740,10 @@ impl ChannelManager {
 		if let Err(_e) = self.monitor.add_update_monitor(chan_monitor.get_funding_txo().unwrap(), chan_monitor) {
 			unimplemented!(); // maybe remove from claimable_htlcs?
 		}
-		{
-			let mut pending_events = self.pending_events.lock().unwrap();
-			pending_events.push(events::Event::SendFundingCreated {
-				node_id: chan.get_their_node_id(),
-				msg: msg,
-			});
-		}
+		add_pending_event!(events::Event::SendFundingCreated {
+			node_id: chan.get_their_node_id(),
+			msg: msg,
+		});
 
 		let mut channel_state = self.channel_state.lock().unwrap();
 		channel_state.by_id.insert(chan.channel_id(), chan);

src/ln/msgs.rs

@@ -31,6 +31,8 @@ pub enum DecodeError {
 	BadPublicKey,
 	/// Failed to decode a signature (ie it's invalid)
 	BadSignature,
+	/// Value expected to be text wasn't decodable as text
+	BadText,
 	/// Buffer not of right length (either too short or too long)
 	WrongLength,
 	/// node_announcement included more than one address of a given type!
@@ -138,6 +140,11 @@ pub struct Init {
 	pub local_features: LocalFeatures,
 }
 
+pub struct ErrorMessage {
+	pub channel_id: [u8; 32],
+	pub data: String,
+}
+
 pub struct Ping {
 	pub ponglen: u16,
 	pub byteslen: u16,
@@ -372,9 +379,15 @@ pub enum ErrorAction {
 		msg: UpdateFailHTLC
 	},
 	/// The peer took some action which made us think they were useless. Disconnect them.
-	DisconnectPeer,
+	DisconnectPeer {
+		msg: Option<ErrorMessage>
+	},
 	/// The peer did something harmless that we weren't able to process, just log and ignore
 	IgnoreError,
+	/// The peer did something incorrect. Tell them.
+	SendErrorMessage {
+		msg: ErrorMessage
+	},
 }
 
 pub struct HandleError { //TODO: rename me
@@ -486,6 +499,7 @@ impl Error for DecodeError {
 			DecodeError::UnknownRealmByte => "Unknown realm byte in Onion packet",
 			DecodeError::BadPublicKey => "Invalid public key in packet",
 			DecodeError::BadSignature => "Invalid signature in packet",
+			DecodeError::BadText => "Invalid text in packet",
 			DecodeError::WrongLength => "Data was wrong length for packet",
 			DecodeError::ExtraAddressesPerType => "More than one address of a single type",
 		}
@@ -1600,6 +1614,37 @@ impl MsgEncodable for OnionErrorPacket {
 	}
 }
 
+impl MsgEncodable for ErrorMessage {
+	fn encode(&self) -> Vec<u8> {
+		let mut res = Vec::with_capacity(34 + self.data.len());
+		res.extend_from_slice(&self.channel_id);
+		res.extend_from_slice(&byte_utils::be16_to_array(self.data.len() as u16));
+		res.extend_from_slice(&self.data.as_bytes());
+		res
+	}
+}
+impl MsgDecodable for ErrorMessage {
+	fn decode(v: &[u8]) -> Result<Self,DecodeError> {
+		if v.len() < 34 {
+			return Err(DecodeError::WrongLength);
+		}
+		let len = byte_utils::slice_to_be16(&v[32..34]);
+		if v.len() < 34 + len as usize {
+			return Err(DecodeError::WrongLength);
+		}
+		let data = match String::from_utf8(v[34..34 + len as usize].to_vec()) {
+			Ok(s) => s,
+			Err(_) => return Err(DecodeError::BadText),
+		};
+		let mut channel_id = [0; 32];
+		channel_id[..].copy_from_slice(&v[0..32]);
+		Ok(Self {
+			channel_id,
+			data,
+		})
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use bitcoin::util::misc::hex_bytes;