From 09cce4a9c799c1b3f4270bd793fafd90382e27a6 Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 2 Aug 2018 16:19:55 +0930 Subject: [PATCH] common/read_peer_msg: deconstruct into individual helper routines. The One Big API is confusing, and has enough corner cases that we should ditch it rather than add more. See: https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction In particular, when openingd is changed to chat to peers even when it's not actively opening a channel, it wants to handle (most) errors by continuing, not calling peer_failed(). This exposes the constituent parts. Signed-off-by: Rusty Russell --- common/peer_failed.c | 8 +- common/peer_failed.h | 3 +- common/read_peer_msg.c | 186 +++++++++++++++++++++++++++++++---------- common/read_peer_msg.h | 63 ++++++++++++++ 4 files changed, 211 insertions(+), 49 deletions(-) diff --git a/common/peer_failed.c b/common/peer_failed.c index 8dca86f36..bf738b28f 100644 --- a/common/peer_failed.c +++ b/common/peer_failed.c @@ -36,8 +36,12 @@ void peer_failed_received_errmsg(int peer_fd, int gossip_fd, const char *desc, const struct channel_id *channel_id) { - u8 *msg = towire_status_peer_error(NULL, channel_id, - desc, cs, NULL); + static const struct channel_id all_channels; + u8 *msg; + + if (!channel_id) + channel_id = &all_channels; + msg = towire_status_peer_error(NULL, channel_id, desc, cs, NULL); peer_billboard(true, "Received error from peer: %s", desc); status_send_fatal(take(msg), peer_fd, gossip_fd); } diff --git a/common/peer_failed.h b/common/peer_failed.h index 342be57d0..b2ab21618 100644 --- a/common/peer_failed.h +++ b/common/peer_failed.h @@ -21,7 +21,8 @@ void peer_failed_(int peer_fd, int gossip_fd, const char *fmt, ...) PRINTF_FMT(5,6) NORETURN; -/* We're failing because peer sent us an error message */ +/* We're failing because peer sent us an error message: NULL + * channel_id means all channels. */ void peer_failed_received_errmsg(int peer_fd, int gossip_fd, struct crypto_state *cs, const char *desc, diff --git a/common/read_peer_msg.c b/common/read_peer_msg.c index 95ab912b9..f00886924 100644 --- a/common/read_peer_msg.c +++ b/common/read_peer_msg.c @@ -11,6 +11,77 @@ #include #include +u8 *peer_or_gossip_sync_read(const tal_t *ctx, + int peer_fd, int gossip_fd, + struct crypto_state *cs, + bool *from_gossipd) +{ + fd_set readfds; + u8 *msg; + + FD_ZERO(&readfds); + FD_SET(peer_fd, &readfds); + FD_SET(gossip_fd, &readfds); + + select(peer_fd > gossip_fd ? peer_fd + 1 : gossip_fd + 1, + &readfds, NULL, NULL, NULL); + + if (FD_ISSET(gossip_fd, &readfds)) { + msg = wire_sync_read(ctx, gossip_fd); + if (!msg) + status_failed(STATUS_FAIL_GOSSIP_IO, + "Error reading gossip msg: %s", + strerror(errno)); + *from_gossipd = true; + return msg; + } + + msg = sync_crypto_read(ctx, cs, peer_fd); + if (!msg) + peer_failed_connection_lost(); + *from_gossipd = false; + return msg; +} + +bool is_peer_error(const tal_t *ctx, const u8 *msg, + const struct channel_id *channel_id, + char **desc, bool *all_channels) +{ + struct channel_id err_chanid; + + if (fromwire_peektype(msg) != WIRE_ERROR) + return false; + + *desc = sanitize_error(ctx, msg, &err_chanid); + + /* BOLT #1: + * + * The channel is referred to by `channel_id`, unless `channel_id` is + * 0 (i.e. all bytes are 0), in which case it refers to all channels. + * ... + * The receiving node: + * - upon receiving `error`: + * - MUST fail the channel referred to by the error message, if that + * channel is with the sending node. + * - if no existing channel is referred to by the message: + * - MUST ignore the message. + */ + *all_channels = channel_id_is_all(&err_chanid); + if (!*all_channels && !channel_id_eq(&err_chanid, channel_id)) + *desc = tal_free(*desc); + + return true; +} + +bool is_wrong_channel(const u8 *msg, const struct channel_id *expected, + struct channel_id *actual) +{ + if (!extract_channel_id(msg, actual)) + return false; + + return !channel_id_eq(expected, actual); +} + void handle_gossip_msg_(const u8 *msg TAKES, int peer_fd, struct crypto_state *cs, bool (*send_msg)(struct crypto_state *cs, int fd, @@ -42,6 +113,53 @@ void handle_gossip_msg_(const u8 *msg TAKES, int peer_fd, tal_free(msg); } +bool handle_peer_gossip_or_error(int peer_fd, int gossip_fd, + struct crypto_state *cs, + const struct channel_id *channel_id, + const u8 *msg TAKES) +{ + char *err; + bool all_channels; + struct channel_id actual; + + if (is_msg_for_gossipd(msg)) { + wire_sync_write(gossip_fd, msg); + /* wire_sync_write takes, so don't take again. */ + return true; + } + + if (is_peer_error(tmpctx, msg, channel_id, &err, &all_channels)) { + if (err) + peer_failed_received_errmsg(peer_fd, gossip_fd, + cs, err, + all_channels + ? NULL : channel_id); + + /* Ignore unknown channel errors. */ + goto handled; + } + + /* They're talking about a different channel? */ + if (is_wrong_channel(msg, channel_id, &actual)) { + status_trace("Rejecting %s for unknown channel_id %s", + wire_type_name(fromwire_peektype(msg)), + type_to_string(tmpctx, struct channel_id, &actual)); + if (!sync_crypto_write(cs, peer_fd, + take(towire_errorfmt(NULL, &actual, + "Multiple channels" + " unsupported")))) + peer_failed_connection_lost(); + goto handled; + } + + return false; + +handled: + if (taken(msg)) + tal_free(msg); + return true; +} + u8 *read_peer_msg_(const tal_t *ctx, int peer_fd, int gossip_fd, struct crypto_state *cs, @@ -51,73 +169,49 @@ u8 *read_peer_msg_(const tal_t *ctx, void *arg) { u8 *msg; - struct channel_id chanid; - fd_set readfds; + bool from_gossipd, all_channels; + struct channel_id actual; + char *err; - FD_ZERO(&readfds); - FD_SET(peer_fd, &readfds); - if (gossip_fd >= 0) - FD_SET(gossip_fd, &readfds); - - select(peer_fd > gossip_fd ? peer_fd + 1 : gossip_fd + 1, - &readfds, NULL, NULL, NULL); - - if (gossip_fd >= 0 && FD_ISSET(gossip_fd, &readfds)) { - msg = wire_sync_read(NULL, gossip_fd); + if (gossip_fd > 0) { + msg = peer_or_gossip_sync_read(ctx, peer_fd, gossip_fd, + cs, &from_gossipd); + } else { + msg = sync_crypto_read(ctx, cs, peer_fd); if (!msg) - status_failed(STATUS_FAIL_GOSSIP_IO, - "Error reading gossip msg: %s", - strerror(errno)); + peer_failed_connection_lost(); + from_gossipd = false; + } - handle_gossip_msg_(msg, peer_fd, cs, send_reply, arg); + if (from_gossipd) { + handle_gossip_msg_(take(msg), peer_fd, cs, send_reply, arg); return NULL; } - msg = sync_crypto_read(ctx, cs, peer_fd); - if (!msg) - peer_failed_connection_lost(); - if (is_msg_for_gossipd(msg)) { /* Forward to gossip daemon */ wire_sync_write(gossip_fd, take(msg)); return NULL; } - if (fromwire_peektype(msg) == WIRE_ERROR) { - char *err = sanitize_error(msg, msg, &chanid); - - /* BOLT #1: - * - * The channel is referred to by `channel_id`, unless - * `channel_id` is 0 (i.e. all bytes are 0), in which - * case it refers to all channels. - * ... - - * The receiving node: - * - upon receiving `error`: - * - MUST fail the channel referred to by the error - * message, if that channel is with the sending node. - * - if no existing channel is referred to by the - * message: - * - MUST ignore the message. - */ - if (channel_id_eq(&chanid, channel) - || channel_id_is_all(&chanid)) { + if (is_peer_error(tmpctx, msg, channel, &err, &all_channels)) { + if (err) peer_failed_received_errmsg(peer_fd, gossip_fd, - cs, err, &chanid); - } + cs, err, + all_channels + ? NULL : channel); + /* Ignore unknown channel errors. */ return tal_free(msg); } /* They're talking about a different channel? */ - if (extract_channel_id(msg, &chanid) - && !channel_id_eq(&chanid, channel)) { + if (is_wrong_channel(msg, channel, &actual)) { status_trace("Rejecting %s for unknown channel_id %s", wire_type_name(fromwire_peektype(msg)), - type_to_string(tmpctx, struct channel_id, &chanid)); + type_to_string(tmpctx, struct channel_id, &actual)); if (!send_reply(cs, peer_fd, - take(towire_errorfmt(NULL, &chanid, + take(towire_errorfmt(NULL, &actual, "Multiple channels" " unsupported")), arg)) diff --git a/common/read_peer_msg.h b/common/read_peer_msg.h index b0f433af5..ab813129c 100644 --- a/common/read_peer_msg.h +++ b/common/read_peer_msg.h @@ -8,6 +8,69 @@ struct crypto_state; struct channel_id; +/** + * peer_or_gossip_sync_read - read a peer message, or maybe a gossip msg. + * @ctx: context to allocate return packet from. + * @peer_fd, @gossip_fd: peer and gossip fd. + * @cs: the cryptostate (updated) + * @from_gossipd: true if the msg was from gossipd, otherwise false. + * + * Will call peer_failed_connection_lost() or + * status_failed(STATUS_FAIL_GOSSIP_IO) or return a message. + * + * Usually, you should call handle_gossip_msg if *@from_gossipd is + * true, otherwise if is_peer_error() handle the error, otherwise if + * is_msg_for_gossipd() then send to gossipd, otherwise if is + * is_wrong_channel() send that as a reply. Otherwise it should be + * a valid message. + */ +u8 *peer_or_gossip_sync_read(const tal_t *ctx, + int peer_fd, int gossip_fd, + struct crypto_state *cs, + bool *from_gossipd); + +/** + * is_peer_error - if it's an error, describe if it applies to this channel. + * @ctx: context to allocate return from. + * @msg: the peer message. + * @channel_id: the channel id of the current channel. + * @desc: set to non-NULL if this describes a channel we care about. + * @all_channels: set to true if this applies to all channels. + * + * If @desc is NULL, ignore this message. Otherwise, that's usually passed + * to peer_failed_received_errmsg(). + */ +bool is_peer_error(const tal_t *ctx, const u8 *msg, + const struct channel_id *channel_id, + char **desc, bool *all_channels); + +/** + * is_wrong_channel - if it's a message about a different channel, return true + * @msg: the peer message. + * @channel_id: the channel id of the current channel. + * @actual: set to the actual channel id if this returns false. + * + * Note that this only handles some message types, returning false for others. + */ +bool is_wrong_channel(const u8 *msg, const struct channel_id *expected, + struct channel_id *actual); + + +/** + * handle_peer_gossip_or_error - simple handler for all the above cases. + * @peer_fd, @gossip_fd: peer and gossip fd. + * @cs: the cryptostate (updated) + * @msg: the peer message (only taken if returns true). + * + * This returns true if it handled the packet: a gossip packet (forwarded + * to gossipd), an error packet (causes peer_failed_received_errmsg or + * ignored), or a message about the wrong channel (sends sync error reply). + */ +bool handle_peer_gossip_or_error(int peer_fd, int gossip_fd, + struct crypto_state *cs, + const struct channel_id *channel_id, + const u8 *msg TAKES); + /** * read_peer_msg - read & decode in a peer message, handling common ones. * @ctx: context to allocate return packet from.