channeld: fix dev_disconnect doublefree crash.

We shouldn't unconditionally free msg in enqueue_peer_msg: DEBUG: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: dev_disconnect: @WIRE_REVOKE_AND_ACK BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: FATAL SIGNAL 6 (version 8aae6a8) ... BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: backtrace: ccan/ccan/tal/tal.c:98 (call_error) 0x80855d1 BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: backtrace: ccan/ccan/tal/tal.c:170 (check_bounds) 0x8085730 BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: backtrace: ccan/ccan/tal/tal.c:181 (to_tal_hdr) 0x8085791 BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: backtrace: ccan/ccan/tal/tal.c:504 (tal_free) 0x8085fe6 BROKEN: lightning_channeld-0266e4598d1d3c415f572a8488830b60f7e744ed9235eb0b1ba93283b315c03518 chan #1: backtrace: channeld/channel.c:2651 (main) 0x8050639 For additional safety, handle each msg allocation separately, rather than freeing at bottom of large branch. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell · Jul 26, 2018 · 64fd1a4 · 64fd1a4
1 parent e8edecf
commit 64fd1a4
Showing 1 changed file with 9 additions and 8 deletions.
diff --git a/channeld/channel.c b/channeld/channel.c
@@ -277,7 +277,8 @@ static void enqueue_peer_msg(struct peer *peer, const u8 *msg TAKES)
 		/* Should not return */
 		abort();
 	case DEV_DISCONNECT_DROPPKT:
-		tal_free(msg);
+		if (taken(msg))
+			tal_free(msg);
 		/* Fail next time we try to do something. */
 		dev_sabotage_fd(PEER_FD);
 		return;
@@ -1834,7 +1835,7 @@ static void resend_commitment(struct peer *peer, const struct changed_htlc *last
 
 static bool channeld_send_reply(struct crypto_state *cs UNUSED,
 			    int peer_fd UNUSED,
-			    const u8 *msg UNUSED,
+			    const u8 *msg,
 			    struct peer *peer)
 {
 	enqueue_peer_msg(peer, msg);
@@ -2625,15 +2626,15 @@ int main(int argc, char *argv[])
 		}
 
 		if (FD_ISSET(MASTER_FD, &rfds)) {
-			msg = wire_sync_read(peer, MASTER_FD);
+			msg = wire_sync_read(tmpctx, MASTER_FD);
 
 			if (!msg)
 				status_failed(STATUS_FAIL_MASTER_IO,
 					      "Can't read command: %s",
 					      strerror(errno));
 			req_in(peer, msg);
 		} else if (FD_ISSET(GOSSIP_FD, &rfds)) {
-			msg = wire_sync_read(peer, GOSSIP_FD);
+			msg = wire_sync_read(tmpctx, GOSSIP_FD);
 			/* Gossipd hangs up on us to kill us when a new
 			 * connection comes in. */
 			if (!msg)
@@ -2644,11 +2645,11 @@ int main(int argc, char *argv[])
 		} else if (FD_ISSET(PEER_FD, &rfds)) {
 			/* This could take forever, but who cares? */
 			msg = channeld_read_peer_msg(peer);
-			if (msg)
+			if (msg) {
 				peer_in(peer, msg);
-		} else
-			msg = NULL;
-		tal_free(msg);
+				tal_free(msg);
+			}
+		}
 	}
 
 	/* We only exit when shutdown is complete. */