From: Matt Corallo <git@bluematt.me>
Date: Thu, 10 Jun 2021 16:36:16 +0000 (+0000)
Subject: Fix bogus reentrancy from read_event -> do_attempt_write_data
X-Git-Url: http://git.bitcoin.ninja/?a=commitdiff_plain;h=62d4007c19b090c18fb1183dc3af19eb1b4b31b1;p=rust-lightning

Fix bogus reentrancy from read_event -> do_attempt_write_data

`Julian Knutsen <julianknutsen@users.noreply.github.com>` pointed
out in a previous discussion that `read_event` can reenter user
code despite the documentation stating explicitly that it will not.

This was addressed in #456 by simply codifying the reentrancy, but
its somewhat simpler to just drop the `do_attempt_write_data` call.

Ideally we could land most of Julian's work, but its still in need
of substantial git history cleanup to get it in a reviewable state
and this solves the immediate issue.
---

diff --git a/lightning/src/ln/peer_handler.rs b/lightning/src/ln/peer_handler.rs
index 3cc52789a..8d2d85d42 100644
--- a/lightning/src/ln/peer_handler.rs
+++ b/lightning/src/ln/peer_handler.rs
@@ -794,7 +794,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 											}
 										};
 
-										if let Err(handling_error) = self.handle_message(&mut peers.peers_needing_send, peer, peer_descriptor.clone(), message){
+										if let Err(handling_error) = self.handle_message(&mut peers.peers_needing_send, peer, peer_descriptor.clone(), message) {
 											match handling_error {
 												MessageHandlingError::PeerHandleError(e) => { return Err(e) },
 												MessageHandlingError::LightningError(e) => {
@@ -808,8 +808,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref, RM: Deref, L: Deref> PeerManager<D
 						}
 					}
 
-					self.do_attempt_write_data(peer_descriptor, peer);
-
 					peer.pending_outbound_buffer.len() > 10 // pause_read
 				}
 			};
@@ -1470,7 +1468,9 @@ mod tests {
 		let initial_data = peer_b.new_outbound_connection(a_id, fd_b.clone()).unwrap();
 		peer_a.new_inbound_connection(fd_a.clone()).unwrap();
 		assert_eq!(peer_a.read_event(&mut fd_a, &initial_data).unwrap(), false);
+		peer_a.process_events();
 		assert_eq!(peer_b.read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
+		peer_b.process_events();
 		assert_eq!(peer_a.read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap(), false);
 		(fd_a.clone(), fd_b.clone())
 	}
@@ -1509,10 +1509,12 @@ mod tests {
 
 		// peers[0] awaiting_pong is set to true, but the Peer is still connected
 		peers[0].timer_tick_occurred();
+		peers[0].process_events();
 		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 1);
 
 		// Since timer_tick_occurred() is called again when awaiting_pong is true, all Peers are disconnected
 		peers[0].timer_tick_occurred();
+		peers[0].process_events();
 		assert_eq!(peers[0].peers.lock().unwrap().peers.len(), 0);
 	}
 
@@ -1533,7 +1535,9 @@ mod tests {
 		let (mut fd_a, mut fd_b) = establish_connection(&peers[0], &peers[1]);
 
 		// Make each peer to read the messages that the other peer just wrote to them.
+		peers[0].process_events();
 		peers[1].read_event(&mut fd_b, &fd_a.outbound_data.lock().unwrap().split_off(0)).unwrap();
+		peers[1].process_events();
 		peers[0].read_event(&mut fd_a, &fd_b.outbound_data.lock().unwrap().split_off(0)).unwrap();
 
 		// Check that each peer has received the expected number of channel updates and channel