From 28c3706f873cf66ee669c88efa55b97b052a48f7 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Mon, 9 Jul 2018 20:47:59 +0930 Subject: [PATCH] hsmd: fix missing status messages. I crashed the HSMD, and it gave no output at all. That's because we were only reading the status fd when we were waiting for a reply. Fix this by using a separate request fd and status fd, which also means that hsm_sync_read() is no longer required. Signed-off-by: Rusty Russell --- hsmd/hsm.c | 7 +++--- lightningd/gossip_control.c | 2 +- lightningd/hsm_control.c | 32 +++++++++++---------------- lightningd/hsm_control.h | 1 - lightningd/invoice.c | 5 ++--- lightningd/lightningd.c | 1 + lightningd/lightningd.h | 2 +- lightningd/opening_control.c | 2 +- lightningd/subd.c | 42 +++++++----------------------------- lightningd/subd.h | 6 ------ wallet/walletrpc.c | 2 +- 11 files changed, 32 insertions(+), 70 deletions(-) diff --git a/hsmd/hsm.c b/hsmd/hsm.c index de19fe7f2..e42f0fd89 100644 --- a/hsmd/hsm.c +++ b/hsmd/hsm.c @@ -41,6 +41,8 @@ #include #include +#define REQ_FD 3 + /* Nobody will ever find it here! */ static struct { struct secret hsm_secret; @@ -856,15 +858,14 @@ int main(int argc, char *argv[]) struct client *client; subdaemon_setup(argc, argv); + status_setup_sync(STDIN_FILENO); - client = new_client(NULL, NULL, 0, HSM_CAP_MASTER | HSM_CAP_SIGN_GOSSIP, handle_client, STDIN_FILENO); + client = new_client(NULL, NULL, 0, HSM_CAP_MASTER | HSM_CAP_SIGN_GOSSIP, handle_client, REQ_FD); /* We're our own master! */ client->master = &client->dc; io_set_finish(client->dc.conn, master_gone, &client->dc); - status_setup_async(&client->dc); - /* When conn closes, everything is freed. */ tal_steal(client->dc.conn, client); io_loop(NULL, NULL); diff --git a/lightningd/gossip_control.c b/lightningd/gossip_control.c index d6b4d15a4..8f2f91f80 100644 --- a/lightningd/gossip_control.c +++ b/lightningd/gossip_control.c @@ -215,7 +215,7 @@ void gossip_init(struct lightningd *ld) if (!wire_sync_write(ld->hsm_fd, msg)) fatal("Could not write to HSM: %s", strerror(errno)); - msg = hsm_sync_read(tmpctx, ld); + msg = wire_sync_read(tmpctx, ld->hsm_fd); if (!fromwire_hsm_client_hsmfd_reply(msg)) fatal("Malformed hsmfd response: %s", tal_hex(msg, msg)); diff --git a/lightningd/hsm_control.c b/lightningd/hsm_control.c index 160278a36..8e28eb714 100644 --- a/lightningd/hsm_control.c +++ b/lightningd/hsm_control.c @@ -14,23 +14,11 @@ #include #include #include +#include +#include #include #include -u8 *hsm_sync_read(const tal_t *ctx, struct lightningd *ld) -{ - for (;;) { - u8 *msg = wire_sync_read(ctx, ld->hsm_fd); - - if (!msg) - fatal("Could not read from HSM: %s", strerror(errno)); - if (log_status_msg(ld->log, msg)) - tal_free(msg); - else - return msg; - } -} - int hsm_get_client_fd(struct lightningd *ld, const struct pubkey *id, u64 dbid, @@ -44,7 +32,7 @@ int hsm_get_client_fd(struct lightningd *ld, if (!wire_sync_write(ld->hsm_fd, take(msg))) fatal("Could not write to HSM: %s", strerror(errno)); - msg = hsm_sync_read(tmpctx, ld); + msg = wire_sync_read(tmpctx, ld->hsm_fd); if (!fromwire_hsm_client_hsmfd_reply(msg)) fatal("Bad reply from HSM: %s", tal_hex(tmpctx, msg)); @@ -57,17 +45,23 @@ int hsm_get_client_fd(struct lightningd *ld, void hsm_init(struct lightningd *ld) { u8 *msg; + int fds[2]; + + /* We actually send requests synchronously: only status is async. */ + if (socketpair(AF_LOCAL, SOCK_STREAM, 0, fds) != 0) + err(1, "Could not create hsm socketpair"); - ld->hsm_fd = subd_raw(ld, "lightning_hsmd"); - if (ld->hsm_fd < 0) + ld->hsm = new_global_subd(ld, "lightning_hsmd", NULL, NULL, + take(&fds[1]), NULL); + if (!ld->hsm) err(1, "Could not subd hsm"); - ld->hsm_log = new_log(ld, ld->log_book, "hsmd:"); + ld->hsm_fd = fds[0]; if (!wire_sync_write(ld->hsm_fd, towire_hsm_init(tmpctx))) err(1, "Writing init msg to hsm"); ld->wallet->bip32_base = tal(ld->wallet, struct ext_key); - msg = hsm_sync_read(tmpctx, ld); + msg = wire_sync_read(tmpctx, ld->hsm_fd); if (!fromwire_hsm_init_reply(msg, &ld->id, &ld->peer_seed, diff --git a/lightningd/hsm_control.h b/lightningd/hsm_control.h index c820309ad..2a5ddd9f8 100644 --- a/lightningd/hsm_control.h +++ b/lightningd/hsm_control.h @@ -15,6 +15,5 @@ int hsm_get_client_fd(struct lightningd *ld, u64 dbid, int capabilities); -u8 *hsm_sync_read(const tal_t *ctx, struct lightningd *ld); void hsm_init(struct lightningd *ld); #endif /* LIGHTNING_LIGHTNINGD_HSM_CONTROL_H */ diff --git a/lightningd/invoice.c b/lightningd/invoice.c index b0969324c..30f63aa5e 100644 --- a/lightningd/invoice.c +++ b/lightningd/invoice.c @@ -93,17 +93,16 @@ static bool hsm_sign_b11(const u5 *u5bytes, secp256k1_ecdsa_recoverable_signature *rsig, struct lightningd *ld) { - u8 *msg = towire_hsm_sign_invoice(ld, u5bytes, hrpu8); + u8 *msg = towire_hsm_sign_invoice(NULL, u5bytes, hrpu8); if (!wire_sync_write(ld->hsm_fd, take(msg))) fatal("Could not write to HSM: %s", strerror(errno)); - msg = hsm_sync_read(ld, ld); + msg = wire_sync_read(tmpctx, ld->hsm_fd); if (!fromwire_hsm_sign_invoice_reply(msg, rsig)) fatal("HSM gave bad sign_invoice_reply %s", tal_hex(msg, msg)); - tal_free(msg); return true; } diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 99995c1b9..efecc4004 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -207,6 +207,7 @@ static void shutdown_subdaemons(struct lightningd *ld) /* Let everyone shutdown cleanly. */ close(ld->hsm_fd); subd_shutdown(ld->gossip, 10); + subd_shutdown(ld->hsm, 10); free_htlcs(ld, NULL); diff --git a/lightningd/lightningd.h b/lightningd/lightningd.h index f085a881d..1775e37da 100644 --- a/lightningd/lightningd.h +++ b/lightningd/lightningd.h @@ -124,7 +124,7 @@ struct lightningd { /* Bearer of all my secrets. */ int hsm_fd; - struct log *hsm_log; + struct subd *hsm; /* Daemon looking after peers during init / before channel. */ struct subd *gossip; diff --git a/lightningd/opening_control.c b/lightningd/opening_control.c index 4381dac93..587df7779 100644 --- a/lightningd/opening_control.c +++ b/lightningd/opening_control.c @@ -398,7 +398,7 @@ static void opening_funder_finished(struct subd *openingd, const u8 *resp, if (!wire_sync_write(ld->hsm_fd, take(msg))) fatal("Could not write to HSM: %s", strerror(errno)); - msg = hsm_sync_read(fc, ld); + msg = wire_sync_read(fc, ld->hsm_fd); if (!fromwire_hsm_sign_funding_reply(tmpctx, msg, &fundingtx)) fatal("HSM gave bad sign_funding_reply %s", tal_hex(msg, resp)); diff --git a/lightningd/subd.c b/lightningd/subd.c index 1a521dcb8..82727beb4 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -180,16 +180,14 @@ static int subd(const char *dir, const char *name, } /* Dup any extra fds up first. */ - if (ap) { - while ((fd = va_arg(*ap, int *)) != NULL) { - int actual_fd = *fd; - /* If this were stdin, we moved it above! */ - if (actual_fd == STDIN_FILENO) - actual_fd = stdin_is_now; - if (!move_fd(actual_fd, fdnum)) - goto child_errno_fail; - fdnum++; - } + while ((fd = va_arg(*ap, int *)) != NULL) { + int actual_fd = *fd; + /* If this were stdin, we moved it above! */ + if (actual_fd == STDIN_FILENO) + actual_fd = stdin_is_now; + if (!move_fd(actual_fd, fdnum)) + goto child_errno_fail; + fdnum++; } /* Make (fairly!) sure all other fds are closed. */ @@ -246,30 +244,6 @@ fail: return -1; } -int subd_raw(struct lightningd *ld, const char *name) -{ - pid_t pid; - int msg_fd; - const char *debug_subd = NULL; - int disconnect_fd = -1; - -#if DEVELOPER - debug_subd = ld->dev_debug_subdaemon; - disconnect_fd = ld->dev_disconnect_fd; -#endif /* DEVELOPER */ - - pid = subd(ld->daemon_dir, name, debug_subd, - &msg_fd, disconnect_fd, - NULL); - if (pid == (pid_t)-1) { - log_unusual(ld->log, "subd %s failed: %s", - name, strerror(errno)); - return -1; - } - - return msg_fd; -} - static struct io_plan *sd_msg_read(struct io_conn *conn, struct subd *sd); static void mark_freed(struct subd *unused UNUSED, bool *freed) diff --git a/lightningd/subd.h b/lightningd/subd.h index 037e9c7c2..4dacd59a3 100644 --- a/lightningd/subd.h +++ b/lightningd/subd.h @@ -139,12 +139,6 @@ struct subd *new_channel_subd_(struct lightningd *ld, (channel), bool, \ const char *), \ __VA_ARGS__) -/** - * subd_raw - raw interface to get a subdaemon on an fd (for HSM) - * @ld: global state - * @name: basename of daemon - */ -int subd_raw(struct lightningd *ld, const char *name); /** * subd_send_msg - queue a message to the subdaemon. diff --git a/wallet/walletrpc.c b/wallet/walletrpc.c index b5dd4157e..28ce72f3c 100644 --- a/wallet/walletrpc.c +++ b/wallet/walletrpc.c @@ -138,7 +138,7 @@ static void json_withdraw(struct command *cmd, fatal("Could not write sign_withdrawal to HSM: %s", strerror(errno)); - msg = hsm_sync_read(cmd, cmd->ld); + msg = wire_sync_read(cmd, cmd->ld->hsm_fd); if (!fromwire_hsm_sign_withdrawal_reply(msg, msg, &tx)) fatal("HSM gave bad sign_withdrawal_reply %s",