From 9b8fe618f60f5d90ec97464dec6634e59e1236a0 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Fri, 2 Feb 2018 11:25:06 +1030 Subject: [PATCH] pay: remove cmd pointer from htlc_out. Maintaining it was always fraught, since the command could go away if the JSON RPC died. Most recently, it was broken again on shutdown (see below). In future we may allow pay commands to block on previous payments, so it won't even be a 1:1 mapping. Generalize it: keep commands in a simple list and do a lookup when a payment fails/succeeds. Valgrind error file: valgrind-errors.5732 ==5732== Invalid read of size 8 ==5732== at 0x4149FD: remove_cmd_from_hout (pay.c:292) ==5732== by 0x468BAB: notify (tal.c:237) ==5732== by 0x469077: del_tree (tal.c:400) ==5732== by 0x4690C7: del_tree (tal.c:410) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x40F1EA: main (lightningd.c:362) ==5732== Address 0x69df148 is 1,512 bytes inside a block of size 1,544 free'd ==5732== at 0x4C2EDEB: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x469150: del_tree (tal.c:421) ==5732== by 0x46948A: tal_free (tal.c:509) ==5732== by 0x4198F2: free_htlcs (peer_control.c:1281) ==5732== by 0x40EBA9: shutdown_subdaemons (lightningd.c:209) ==5732== by 0x40F1DE: main (lightningd.c:360) ==5732== Block was alloc'd at ==5732== at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) ==5732== by 0x468C30: allocate (tal.c:250) ==5732== by 0x4691F7: tal_alloc_ (tal.c:448) ==5732== by 0x40A279: new_htlc_out (htlc_end.c:143) ==5732== by 0x41FD64: send_htlc_out (peer_htlcs.c:397) ==5732== by 0x41511C: send_payment (pay.c:388) ==5732== by 0x41589E: json_sendpay (pay.c:513) ==5732== by 0x40D9B1: parse_request (jsonrpc.c:600) ==5732== by 0x40DCAC: read_json (jsonrpc.c:667) ==5732== by 0x45C706: next_plan (io.c:59) ==5732== by 0x45D1DD: do_plan (io.c:387) ==5732== by 0x45D21B: io_ready (io.c:397) Signed-off-by: Rusty Russell --- lightningd/htlc_end.c | 6 +-- lightningd/htlc_end.h | 6 +-- lightningd/lightningd.c | 1 + lightningd/lightningd.h | 3 ++ lightningd/pay.c | 90 ++++++++++++++++++++++++++++------------- lightningd/peer_htlcs.c | 6 +-- lightningd/peer_htlcs.h | 1 - wallet/wallet.c | 1 - 8 files changed, 70 insertions(+), 44 deletions(-) diff --git a/lightningd/htlc_end.c b/lightningd/htlc_end.c index 6de06e33f..ca6ecc72a 100644 --- a/lightningd/htlc_end.c +++ b/lightningd/htlc_end.c @@ -125,8 +125,6 @@ struct htlc_out *htlc_out_check(const struct htlc_out *hout, htlc_state_name(hout->hstate)); else if (hout->failuremsg && hout->preimage) return corrupt(hout, abortstr, "Both failed and succeeded"); - else if (hout->cmd && hout->in) - return corrupt(hout, abortstr, "Both local and forwarded"); return cast_const(struct htlc_out *, hout); } @@ -137,8 +135,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, u64 msatoshi, u32 cltv_expiry, const struct sha256 *payment_hash, const u8 *onion_routing_packet, - struct htlc_in *in, - struct command *cmd) + struct htlc_in *in) { struct htlc_out *hout = tal(ctx, struct htlc_out); @@ -150,7 +147,6 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, hout->msatoshi = msatoshi; hout->cltv_expiry = cltv_expiry; hout->payment_hash = *payment_hash; - hout->cmd = cmd; memcpy(hout->onion_routing_packet, onion_routing_packet, sizeof(hout->onion_routing_packet)); diff --git a/lightningd/htlc_end.h b/lightningd/htlc_end.h index c6901c998..1d2a8c9c7 100644 --- a/lightningd/htlc_end.h +++ b/lightningd/htlc_end.h @@ -75,9 +75,6 @@ struct htlc_out { /* Where it's from, if not going to us. */ struct htlc_in *in; - - /* Otherwise, this MAY be non-null if there's a pay command waiting */ - struct command *cmd; }; static inline const struct htlc_key *keyof_htlc_in(const struct htlc_in *in) @@ -132,8 +129,7 @@ struct htlc_out *new_htlc_out(const tal_t *ctx, u64 msatoshi, u32 cltv_expiry, const struct sha256 *payment_hash, const u8 *onion_routing_packet, - struct htlc_in *in, - struct command *cmd); + struct htlc_in *in); void connect_htlc_in(struct htlc_in_map *map, struct htlc_in *hin); void connect_htlc_out(struct htlc_out_map *map, struct htlc_out *hout); diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 6de9f185c..25a3fca51 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -62,6 +62,7 @@ static struct lightningd *new_lightningd(const tal_t *ctx, ld->alias = NULL; ld->rgb = NULL; list_head_init(&ld->connects); + list_head_init(&ld->pay_commands); ld->wireaddrs = tal_arr(ld, struct wireaddr, 0); ld->portnum = DEFAULT_PORT; timers_init(&ld->timers, time_mono()); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index eebfdb794..86d5a065e 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -128,6 +128,9 @@ struct lightningd { struct wallet *wallet; + /* Outstanding sendpay/pay commands. */ + struct list_head pay_commands; + /* Maintained by invoices.c */ struct invoices *invoices; diff --git a/lightningd/pay.c b/lightningd/pay.c index b9645910f..d52221ee6 100644 --- a/lightningd/pay.c +++ b/lightningd/pay.c @@ -18,28 +18,74 @@ #include #include -static void json_pay_success(struct command *cmd, const struct preimage *rval) +/* pay/sendpay command */ +struct pay_command { + struct list_node list; + + struct sha256 payment_hash; + struct command *cmd; +}; + +static void destroy_pay_command(struct pay_command *pc) { - struct json_result *response; + list_del(&pc->list); +} - /* Can be NULL if JSON RPC goes away. */ - if (!cmd) - return; +/* Owned by cmd */ +static struct pay_command *new_pay_command(struct command *cmd, + const struct sha256 *payment_hash, + struct lightningd *ld) +{ + struct pay_command *pc = tal(cmd, struct pay_command); + + pc->payment_hash = *payment_hash; + pc->cmd = cmd; + list_add(&ld->pay_commands, &pc->list); + tal_add_destructor(pc, destroy_pay_command); + return pc; +} + +static void json_pay_command_success(struct command *cmd, + const struct preimage *payment_preimage) +{ + struct json_result *response; response = new_json_result(cmd); json_object_start(response, NULL); - json_add_hex(response, "preimage", rval, sizeof(*rval)); + json_add_hex(response, "preimage", + payment_preimage, sizeof(*payment_preimage)); json_object_end(response); command_success(cmd, response); } -static void json_pay_failed(struct command *cmd, +static void json_pay_success(struct lightningd *ld, + const struct sha256 *payment_hash, + const struct preimage *payment_preimage) +{ + struct pay_command *pc, *next; + + list_for_each_safe(&ld->pay_commands, pc, next, list) { + if (!structeq(payment_hash, &pc->payment_hash)) + continue; + + /* Deletes itself. */ + json_pay_command_success(pc->cmd, payment_preimage); + } +} + +static void json_pay_failed(struct lightningd *ld, + const struct sha256 *payment_hash, enum onion_type failure_code, const char *details) { - /* Can be NULL if JSON RPC goes away. */ - if (cmd) { - command_fail(cmd, "Failed: %s (%s)", + struct pay_command *pc, *next; + + list_for_each_safe(&ld->pay_commands, pc, next, list) { + if (!structeq(payment_hash, &pc->payment_hash)) + continue; + + /* Deletes itself. */ + command_fail(pc->cmd, "failed: %s (%s)", onion_type_name(failure_code), details); } } @@ -49,10 +95,7 @@ void payment_succeeded(struct lightningd *ld, struct htlc_out *hout, { wallet_payment_set_status(ld->wallet, &hout->payment_hash, PAYMENT_COMPLETE, rval); - /* Can be NULL if JSON RPC goes away. */ - if (hout->cmd) - json_pay_success(hout->cmd, rval); - hout->cmd = NULL; + json_pay_success(ld, &hout->payment_hash, rval); } /* Return NULL if the wrapped onion error message has no @@ -282,17 +325,10 @@ void payment_failed(struct lightningd *ld, const struct htlc_out *hout, * and payment again. */ (void) retry_plausible; - json_pay_failed(hout->cmd, failcode, failmsg); + json_pay_failed(ld, &hout->payment_hash, failcode, failmsg); tal_free(tmpctx); } -/* When JSON RPC goes away, cmd is freed: detach from the hout */ -static void remove_cmd_from_hout(struct command *cmd, struct htlc_out *hout) -{ - assert(hout->cmd == cmd); - hout->cmd = NULL; -} - /* Returns true if it's still pending. */ static bool send_payment(struct command *cmd, const struct sha256 *rhash, @@ -362,7 +398,8 @@ static bool send_payment(struct command *cmd, &payment->destination)); return false; } - json_pay_success(cmd, payment->payment_preimage); + json_pay_command_success(cmd, + payment->payment_preimage); return false; } wallet_payment_delete(cmd->ld->wallet, rhash); @@ -387,8 +424,7 @@ static bool send_payment(struct command *cmd, failcode = send_htlc_out(peer, route[0].amount, base_expiry + route[0].delay, - rhash, onion, NULL, cmd, - &hout); + rhash, onion, NULL, &hout); if (failcode) { command_fail(cmd, "First peer not ready: %s", onion_type_name(failcode)); @@ -416,9 +452,7 @@ static bool send_payment(struct command *cmd, /* We write this into db when HTLC is actually sent. */ wallet_payment_setup(cmd->ld->wallet, payment); - /* If we fail, remove cmd ptr from htlc_out. */ - tal_add_destructor2(cmd, remove_cmd_from_hout, hout); - + new_pay_command(cmd, rhash, cmd->ld); tal_free(tmpctx); return true; } diff --git a/lightningd/peer_htlcs.c b/lightningd/peer_htlcs.c index f295937bc..b1fb0bfd0 100644 --- a/lightningd/peer_htlcs.c +++ b/lightningd/peer_htlcs.c @@ -375,7 +375,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct command *cmd, struct htlc_out **houtp) { struct htlc_out *hout; @@ -395,8 +394,7 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, /* Make peer's daemon own it, catch if it dies. */ hout = new_htlc_out(out->owner, out, amount, cltv, - payment_hash, onion_routing_packet, in, - cmd); + payment_hash, onion_routing_packet, in); tal_add_destructor(hout, hout_subd_died); msg = towire_channel_offer_htlc(out, amount, cltv, payment_hash, @@ -488,7 +486,7 @@ static void forward_htlc(struct htlc_in *hin, failcode = send_htlc_out(next, amt_to_forward, outgoing_cltv_value, &hin->payment_hash, - next_onion, hin, NULL, NULL); + next_onion, hin, NULL); if (!failcode) return; diff --git a/lightningd/peer_htlcs.h b/lightningd/peer_htlcs.h index 5f46297c5..585d16667 100644 --- a/lightningd/peer_htlcs.h +++ b/lightningd/peer_htlcs.h @@ -39,7 +39,6 @@ enum onion_type send_htlc_out(struct peer *out, u64 amount, u32 cltv, const struct sha256 *payment_hash, const u8 *onion_routing_packet, struct htlc_in *in, - struct command *cmd, struct htlc_out **houtp); struct htlc_out *find_htlc_out_by_ripemd(const struct peer *peer, diff --git a/wallet/wallet.c b/wallet/wallet.c index c881728d8..b9e983b32 100644 --- a/wallet/wallet.c +++ b/wallet/wallet.c @@ -1059,7 +1059,6 @@ static bool wallet_stmt2htlc_out(const struct wallet_channel *channel, out->failuremsg = NULL; out->failcode = 0; - out->cmd = NULL; /* Need to defer wiring until we can look up all incoming * htlcs, will wire using origin_htlc_id */