From 378d73cd9673eada76c640f3ac351a7cdb54f31c Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 26 Jul 2018 14:51:40 +0930 Subject: [PATCH] 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 --- channeld/channel.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/channeld/channel.c b/channeld/channel.c index 5d14358b7..e40bc5420 100644 --- 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,7 +2626,7 @@ 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, @@ -2633,7 +2634,7 @@ int main(int argc, char *argv[]) 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. */