From 9578fb4035d14690bd2ae8d9788cbb05486dbeaa Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 19 Sep 2019 16:14:24 +0930 Subject: [PATCH] lightningd: fix crash in cancel_channel This path is not atomic, so we can't assume channel stays around. It could be the peer closes, it could be we get a parallel fund_channel_cancel. test_funding_cancel_race caused this crash: FATAL SIGNAL 6 (version 5f0a18e) backtrace: common/daemon.c:45 (send_backtrace) 0x55c7c373a429 backtrace: common/daemon.c:53 (crashdump) 0x55c7c373a479 backtrace: (null):0 ((null)) 0x7f88ee6ddf5f backtrace: (null):0 ((null)) 0x7f88ee6dded7 backtrace: (null):0 ((null)) 0x7f88ee6bf534 backtrace: ccan/ccan/tal/tal.c:93 (call_error) 0x55c7c379427c backtrace: ccan/ccan/tal/tal.c:165 (check_bounds) 0x55c7c3794444 backtrace: ccan/ccan/tal/tal.c:174 (to_tal_hdr) 0x55c7c3794483 backtrace: ccan/ccan/tal/tal.c:186 (to_tal_hdr_or_null) 0x55c7c3794504 backtrace: ccan/ccan/tal/tal.c:421 (tal_alloc_) 0x55c7c3794c10 backtrace: ccan/ccan/tal/tal.c:466 (tal_alloc_arr_) 0x55c7c3794ded backtrace: ccan/ccan/tal/str/str.c:91 (tal_vfmt_) 0x55c7c3793560 backtrace: common/wire_error.c:22 (towire_errorfmtv) 0x55c7c3747f7b backtrace: common/wire_error.c:39 (towire_errorfmt) 0x55c7c37480a1 backtrace: lightningd/channel_control.c:635 (process_check_funding_broadcast) 0x55c7c37015bb backtrace: lightningd/bitcoind.c:558 (process_gettxout) 0x55c7c36f8e75 backtrace: lightningd/bitcoind.c:227 (bcli_finished) 0x55c7c36f8090 backtrace: ccan/ccan/io/poll.c:244 (destroy_conn) 0x55c7c37869fe backtrace: ccan/ccan/io/poll.c:250 (destroy_conn_close_fd) 0x55c7c3786a1e backtrace: ccan/ccan/tal/tal.c:235 (notify) 0x55c7c3794629 backtrace: ccan/ccan/tal/tal.c:397 (del_tree) 0x55c7c3794b18 backtrace: ccan/ccan/tal/tal.c:481 (tal_free) 0x55c7c3794ea4 backtrace: ccan/ccan/io/io.c:450 (io_close) 0x55c7c378521d backtrace: ccan/ccan/io/poll.c:449 (io_loop) 0x55c7c3787139 backtrace: lightningd/io_loop_with_timers.c:24 (io_loop_with_timers) 0x55c7c370b26d backtrace: lightningd/lightningd.c:837 (main) 0x55c7c3711661 backtrace: (null):0 ((null)) 0x7f88ee6c0b6a backtrace: (null):0 ((null)) 0x55c7c36f70b9 backtrace: (null):0 ((null)) 0xffffffffffffffff Signed-off-by: Rusty Russell --- lightningd/channel_control.c | 36 ++++++++++++++++++++++++++++++------ 1 file changed, 30 insertions(+), 6 deletions(-) diff --git a/lightningd/channel_control.c b/lightningd/channel_control.c index 87f4e7068..a1d57aeca 100644 --- a/lightningd/channel_control.c +++ b/lightningd/channel_control.c @@ -630,11 +630,31 @@ static struct channel *find_channel_by_id(const struct peer *peer, return NULL; } -static void process_check_funding_broadcast(struct bitcoind *bitcoind UNUSED, +/* Since this could vanish while we're checking with bitcoind, we need to save + * the details and re-lookup. + * + * channel_id *should* be unique, but it can be set by the counterparty, so + * we cannot rely on that! */ +struct channel_to_cancel { + struct node_id peer; + struct channel_id cid; +}; + +static void process_check_funding_broadcast(struct bitcoind *bitcoind, const struct bitcoin_tx_output *txout, void *arg) { - struct channel *cancel = arg; + struct channel_to_cancel *cc = arg; + struct peer *peer; + struct channel *cancel; + + /* Peer could have errored out while we were waiting */ + peer = peer_by_id(bitcoind->ld, &cc->peer); + if (!peer) + return; + cancel = find_channel_by_id(peer, &cc->cid); + if (!cancel) + return; if (txout != NULL) { for (size_t i = 0; i < tal_count(cancel->forgets); i++) @@ -662,7 +682,9 @@ struct command_result *cancel_channel_before_broadcast(struct command *cmd, const jsmntok_t *cidtok) { struct channel *cancel_channel; + struct channel_to_cancel *cc = tal(cmd, struct channel_to_cancel); + cc->peer = peer->id; if (!cidtok) { struct channel *channel; @@ -678,13 +700,15 @@ struct command_result *cancel_channel_before_broadcast(struct command *cmd, if (!cancel_channel) return command_fail(cmd, LIGHTNINGD, "No channels matching that peer_id"); + derive_channel_id(&cc->cid, + &cancel_channel->funding_txid, + cancel_channel->funding_outnum); } else { - struct channel_id cid; - if (!json_tok_channel_id(buffer, cidtok, &cid)) + if (!json_tok_channel_id(buffer, cidtok, &cc->cid)) return command_fail(cmd, JSONRPC2_INVALID_PARAMS, "Invalid channel_id parameter."); - cancel_channel = find_channel_by_id(peer, &cid); + cancel_channel = find_channel_by_id(peer, &cc->cid); if (!cancel_channel) return command_fail(cmd, LIGHTNINGD, "Channel ID not found: '%.*s'", @@ -721,6 +745,6 @@ struct command_result *cancel_channel_before_broadcast(struct command *cmd, &cancel_channel->funding_txid, cancel_channel->funding_outnum, process_check_funding_broadcast, - cancel_channel); + notleak(cc)); return command_still_pending(cmd); }