Skip to content

Fix pre-noise outbound peer disconnect panic found by fuzzer #241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 3, 2018
Merged

Fix pre-noise outbound peer disconnect panic found by fuzzer #241

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b030e84
Add logging for (dis)connect in peer_handler/channelmanager
TheBlueMatt Nov 2, 2018
366e796
Fix pre-noise outbound peer disconnect panic found by fuzzer
TheBlueMatt Nov 2, 2018
38584e3
Avoid double-panic in full_stack_target to assist debugging
TheBlueMatt Nov 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions fuzz/fuzz_targets/full_stack_target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}

Expand Down
4 changes: 4 additions & 0 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2703,6 +2703,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() {
Expand All @@ -2720,6 +2721,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).
Expand Down Expand Up @@ -2750,6 +2752,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();
Expand Down
19 changes: 15 additions & 4 deletions src/ln/peer_handler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand All @@ -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 => {
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand All @@ -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..]);
Expand Down