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>
This commit is contained in:
Rusty Russell 2018-07-26 14:51:40 +09:30 committed by Christian Decker
parent e8edecfbf7
commit 378d73cd96

View File

@ -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. */