From 1ac08e3b112060c94fa3a883ea9f0fef9daf616f Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Tue, 29 Sep 2015 09:47:56 +0930 Subject: [PATCH] test_state_coverage: test all accept_pkt failure paths. Reveals a number of places where we don't handle errors correctly. Note: this takes about 14.5 GB to test on my x86-64 box. Signed-off-by: Rusty Russell --- state.c | 62 +++++++++++++++++++++++++------------- test/test_state_coverage.c | 51 ++++++++++++++++++++++++++++++- 2 files changed, 91 insertions(+), 22 deletions(-) diff --git a/state.c b/state.c index 95bcedcee..615996337 100644 --- a/state.c +++ b/state.c @@ -450,6 +450,7 @@ enum state state(const enum state state, const struct state_data *sdata, idata->pkt, &sig); if (err) { fail_cmd(effect, CMD_SEND_UPDATE_ANY, NULL); + set_effect(effect, htlc_abandon, true); goto err_start_unilateral_close; } set_effect(effect, update_theirsig, sig); @@ -485,8 +486,10 @@ enum state state(const enum state state, const struct state_data *sdata, if (input_is(input, PKT_UPDATE_COMPLETE)) { err = accept_pkt_update_complete(effect, sdata, idata->pkt); - if (err) + if (err) { + fail_cmd(effect, CMD_SEND_UPDATE_ANY, NULL); goto err_start_unilateral_close; + } complete_cmd(effect, CMD_SEND_UPDATE_ANY); return toggle_prio(state, STATE_NORMAL); } else if (input_is(input, BITCOIN_ANCHOR_UNSPENT)) { @@ -515,8 +518,10 @@ enum state state(const enum state state, const struct state_data *sdata, struct signature *sig; err = accept_pkt_update_signature(effect, sdata, idata->pkt, &sig); - if (err) + if (err) { + set_effect(effect, htlc_abandon, true); goto err_start_unilateral_close; + } set_effect(effect, update_theirsig, sig); set_effect(effect, send, pkt_update_complete(effect, sdata)); @@ -552,7 +557,7 @@ enum state state(const enum state state, const struct state_data *sdata, err = accept_pkt_close_complete(effect, sdata, idata->pkt); if (err) - goto err_start_unilateral_close; + goto err_start_unilateral_close_already_closing; set_effect(effect, close_status, CMD_STATUS_SUCCESS); set_effect(effect, send, pkt_close_ack(effect, sdata)); set_effect(effect, broadcast, @@ -565,7 +570,7 @@ enum state state(const enum state state, const struct state_data *sdata, err = accept_pkt_simultaneous_close(effect, sdata, idata->pkt); if (err) - goto err_start_unilateral_close; + goto err_start_unilateral_close_already_closing; set_effect(effect, close_status, CMD_STATUS_SUCCESS); set_effect(effect, send, pkt_close_ack(effect, sdata)); set_effect(effect, broadcast, @@ -573,28 +578,17 @@ enum state state(const enum state state, const struct state_data *sdata, set_effect(effect, stop_commands, true); set_effect(effect, stop_packets, true); return STATE_CLOSE_WAIT_CLOSE; + } else if (input_is(input, PKT_ERROR)) { + set_effect(effect, in_error, + set_errpkt(effect, idata->pkt)); + goto start_unilateral_close_already_closing; } else if (input_is_pkt(input)) { /* We ignore all other packets while closing. */ return STATE_WAIT_FOR_CLOSE_COMPLETE; } else if (input_is(input, INPUT_CLOSE_COMPLETE_TIMEOUT)) { /* They didn't respond in time. Unilateral close. */ - set_effect(effect, send, - pkt_err(effect, "Close timed out")); - set_effect(effect, close_status, CMD_STATUS_FAILED); - set_effect(effect, stop_commands, true); - set_effect(effect, stop_packets, true); - set_effect(effect, broadcast, - bitcoin_commit(effect, sdata)); - set_effect(effect, watch, - bitcoin_watch_delayed(effect, - effect->broadcast, - BITCOIN_ANCHOR_OURCOMMIT_DELAYPASSED)); - - /* We agreed to close: shouldn't have any HTLCs */ - if (committed_to_htlcs(sdata)) - return STATE_ERR_INTERNAL; - - return STATE_CLOSE_WAIT_CLOSE_OURCOMMIT; + err = pkt_err(effect, "Close timed out"); + goto err_start_unilateral_close_already_closing; } set_effect(effect, close_status, CMD_STATUS_FAILED); set_effect(effect, stop_commands, true); @@ -943,6 +937,32 @@ start_unilateral_close: } return STATE_CLOSE_WAIT_OURCOMMIT; +err_start_unilateral_close_already_closing: + /* + * They timed out, or were broken; we are going to close unilaterally. + */ + set_effect(effect, send, err); + +start_unilateral_close_already_closing: + set_effect(effect, close_status, CMD_STATUS_FAILED); + + /* + * Close unilaterally. + */ + /* No more inputs, no more commands. */ + set_effect(effect, stop_packets, true); + set_effect(effect, stop_commands, true); + set_effect(effect, broadcast, bitcoin_commit(effect, sdata)); + set_effect(effect, watch, + bitcoin_watch_delayed(effect, effect->broadcast, + BITCOIN_ANCHOR_OURCOMMIT_DELAYPASSED)); + + /* We agreed to close: shouldn't have any HTLCs */ + if (committed_to_htlcs(sdata)) + return STATE_ERR_INTERNAL; + + return STATE_CLOSE_WAIT_CLOSE_OURCOMMIT; + them_unilateral: assert(input == BITCOIN_ANCHOR_THEIRSPEND); diff --git a/test/test_state_coverage.c b/test/test_state_coverage.c index 514b5d687..49d405df6 100644 --- a/test/test_state_coverage.c +++ b/test/test_state_coverage.c @@ -30,6 +30,21 @@ static enum state_input *mapping_inputs; enum failure { FAIL_NONE, FAIL_DECLINE_HTLC, + FAIL_STEAL, + FAIL_ACCEPT_OPEN, + FAIL_ACCEPT_ANCHOR, + FAIL_ACCEPT_OPEN_COMMIT_SIG, + FAIL_ACCEPT_HTLC_UPDATE, + FAIL_ACCEPT_HTLC_ROUTEFAIL, + FAIL_ACCEPT_HTLC_TIMEDOUT, + FAIL_ACCEPT_HTLC_FULFILL, + FAIL_ACCEPT_UPDATE_ACCEPT, + FAIL_ACCEPT_UPDATE_COMPLETE, + FAIL_ACCEPT_UPDATE_SIGNATURE, + FAIL_ACCEPT_CLOSE, + FAIL_ACCEPT_CLOSE_COMPLETE, + FAIL_ACCEPT_CLOSE_ACK, + FAIL_ACCEPT_SIMULTANEOUS_CLOSE }; struct htlc { @@ -635,16 +650,22 @@ Pkt *unexpected_pkt(const tal_t *ctx, enum state_input input) Pkt *accept_pkt_open(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_OPEN)) + return pkt_err(effect, "Error inject"); return NULL; } Pkt *accept_pkt_anchor(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_ANCHOR)) + return pkt_err(effect, "Error inject"); return NULL; } Pkt *accept_pkt_open_commit_sig(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_OPEN_COMMIT_SIG)) + return pkt_err(effect, "Error inject"); return NULL; } @@ -653,6 +674,9 @@ Pkt *accept_pkt_htlc_update(struct state_effect *effect, Pkt **decline, struct htlc_progress **htlcprog) { + if (fail(sdata, FAIL_ACCEPT_HTLC_UPDATE)) + return pkt_err(effect, "Error inject"); + if (fail(sdata, FAIL_DECLINE_HTLC)) *decline = new_pkt(effect, PKT_UPDATE_DECLINE_HTLC); else { @@ -673,6 +697,9 @@ Pkt *accept_pkt_htlc_routefail(struct state_effect *effect, unsigned int id = htlc_id_from_pkt(pkt); const struct htlc *h = find_htlc(sdata, id); + if (fail(sdata, FAIL_ACCEPT_HTLC_ROUTEFAIL)) + return pkt_err(effect, "Error inject"); + /* The shouldn't fail unless it's to them */ assert(h->to_them); @@ -689,6 +716,9 @@ Pkt *accept_pkt_htlc_timedout(struct state_effect *effect, unsigned int id = htlc_id_from_pkt(pkt); const struct htlc *h = find_htlc(sdata, id); + if (fail(sdata, FAIL_ACCEPT_HTLC_TIMEDOUT)) + return pkt_err(effect, "Error inject"); + /* The shouldn't timeout unless it's to us */ assert(!h->to_them); @@ -705,6 +735,9 @@ Pkt *accept_pkt_htlc_fulfill(struct state_effect *effect, unsigned int id = htlc_id_from_pkt(pkt); const struct htlc *h = find_htlc(sdata, id); + if (fail(sdata, FAIL_ACCEPT_HTLC_FULFILL)) + return pkt_err(effect, "Error inject"); + /* The shouldn't complete unless it's to them */ assert(h->to_them); @@ -716,12 +749,17 @@ Pkt *accept_pkt_htlc_fulfill(struct state_effect *effect, Pkt *accept_pkt_update_accept(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt, struct signature **sig) { + if (fail(sdata, FAIL_ACCEPT_UPDATE_ACCEPT)) + return pkt_err(effect, "Error inject"); + *sig = (struct signature *)tal_strdup(effect, "from PKT_UPDATE_ACCEPT"); return NULL; } Pkt *accept_pkt_update_complete(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_UPDATE_COMPLETE)) + return pkt_err(effect, "Error inject"); return NULL; } @@ -729,27 +767,37 @@ Pkt *accept_pkt_update_signature(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt, struct signature **sig) { + if (fail(sdata, FAIL_ACCEPT_UPDATE_SIGNATURE)) + return pkt_err(effect, "Error inject"); *sig = (struct signature *)tal_strdup(effect, "from PKT_UPDATE_SIGNATURE"); return NULL; } Pkt *accept_pkt_close(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_CLOSE)) + return pkt_err(effect, "Error inject"); return NULL; } Pkt *accept_pkt_close_complete(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_CLOSE_COMPLETE)) + return pkt_err(effect, "Error inject"); return NULL; } Pkt *accept_pkt_simultaneous_close(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_SIMULTANEOUS_CLOSE)) + return pkt_err(effect, "Error inject"); return NULL; } Pkt *accept_pkt_close_ack(struct state_effect *effect, const struct state_data *sdata, const Pkt *pkt) { + if (fail(sdata, FAIL_ACCEPT_CLOSE_ACK)) + return pkt_err(effect, "Error inject"); return NULL; } @@ -920,7 +968,8 @@ struct bitcoin_tx *bitcoin_steal(const tal_t *ctx, const struct state_data *sdata, struct bitcoin_event *btc) { - /* FIXME: Test this failing! */ + if (fail(sdata, FAIL_STEAL)) + return NULL; return bitcoin_tx("steal"); }