From eb0603bd131b1918a983769c8bc7fcebb1c2954a Mon Sep 17 00:00:00 2001 From: Rusty Russell Date: Thu, 8 Feb 2018 11:57:24 +1030 Subject: [PATCH] wireaddr: rework port parsing for weird addresses. We save wireaddr to databases as a string (which is pretty dumb) but it turned out that my local node saved '[::ffff:127.0.0.1]:49150' which our parser can't parse. Thus I've reworked the parser to make fewer assumptions: parse_ip_port() is renamed to separate_address_and_port() and is now far more accepting of different forms, and returns failure only on grossly malformed strings. Otherwise it overwrites its *port arg only if there's a port specified. I also made it static. Then fromwire_wireaddr() hands the resulting address to inet_pton to figure out if it's actually valid. Cc: William Casarin Signed-off-by: Rusty Russell --- common/test/run-ip_port_parsing.c | 29 ++++++---- common/wireaddr.c | 89 ++++++++++++++++++------------- common/wireaddr.h | 1 - 3 files changed, 71 insertions(+), 48 deletions(-) diff --git a/common/test/run-ip_port_parsing.c b/common/test/run-ip_port_parsing.c index cc1f7472b..6b8665563 100644 --- a/common/test/run-ip_port_parsing.c +++ b/common/test/run-ip_port_parsing.c @@ -2,6 +2,7 @@ #include #include +#include /* AUTOGENERATED MOCKS START */ /* Generated stub for fromwire */ @@ -31,36 +32,44 @@ int main(void) char *ip; u16 port; + /* Grossly invalid. */ + assert(!separate_address_and_port(ctx, "[", &ip, &port)); + assert(!separate_address_and_port(ctx, "[123", &ip, &port)); + assert(!separate_address_and_port(ctx, "[::1]:8f", &ip, &port)); + assert(!separate_address_and_port(ctx, "127.0.0.1:8f", &ip, &port)); + assert(!separate_address_and_port(ctx, "127.0.0.1:0", &ip, &port)); + assert(!separate_address_and_port(ctx, "127.0.0.1:ff", &ip, &port)); + /* ret = getaddrinfo("[::1]:80", NULL, NULL, &res); */ - assert(parse_ip_port(ctx, "[::1]:80", &ip, &port)); + assert(separate_address_and_port(ctx, "[::1]:80", &ip, &port)); assert(streq(ip, "::1")); assert(port == 80); - assert(!parse_ip_port(ctx, "ip6-localhost", &ip, &port)); + port = 0; + assert(separate_address_and_port(ctx, "ip6-localhost", &ip, &port)); assert(streq(ip, "ip6-localhost")); assert(port == 0); - assert(!parse_ip_port(ctx, "::1", &ip, &port)); + assert(separate_address_and_port(ctx, "::1", &ip, &port)); assert(streq(ip, "::1")); assert(port == 0); - assert(parse_ip_port(ctx, "192.168.1.1:8000", &ip, &port)); + assert(separate_address_and_port(ctx, "192.168.1.1:8000", &ip, &port)); assert(streq(ip, "192.168.1.1")); assert(port == 8000); - assert(!parse_ip_port(ctx, "192.168.2.255", &ip, &port)); + port = 0; + assert(separate_address_and_port(ctx, "192.168.2.255", &ip, &port)); assert(streq(ip, "192.168.2.255")); assert(port == 0); // unusual but possibly valid case - assert(!parse_ip_port(ctx, "[::1]", &ip, &port)); + assert(separate_address_and_port(ctx, "[::1]", &ip, &port)); assert(streq(ip, "::1")); assert(port == 0); // service names not supported yet - assert(!parse_ip_port(ctx, "[::1]:http", &ip, &port)); - assert(streq(ip, "::1")); - assert(port == 0); + assert(!separate_address_and_port(ctx, "[::1]:http", &ip, &port)); // localhost hostnames for backward compat parse_wireaddr("localhost", &addr, 200); @@ -79,6 +88,8 @@ int main(void) ip = fmt_wireaddr(ctx, &addr); assert(streq(ip, "[2001:db8:85a3::8a2e:370:7334]:9777")); + assert(parse_wireaddr("[::ffff:127.0.0.1]:49150", &addr, 1)); + assert(addr.port == 49150); tal_free(ctx); return 0; } diff --git a/common/wireaddr.c b/common/wireaddr.c index 6be18fa29..4985e36d3 100644 --- a/common/wireaddr.c +++ b/common/wireaddr.c @@ -65,6 +65,51 @@ char *fmt_wireaddr(const tal_t *ctx, const struct wireaddr *a) } REGISTER_TYPE_TO_STRING(wireaddr, fmt_wireaddr); +/* Valid forms: + * + * [anything]: + * anything-without-colons-or-left-brace: + * anything-without-colons + * string-with-multiple-colons + * + * Returns false if it wasn't one of these forms. If it returns true, + * it only overwrites *port if it was specified by above. + */ +static bool separate_address_and_port(tal_t *ctx, const char *arg, + char **addr, u16 *port) +{ + char *portcolon; + + if (strstarts(arg, "[")) { + char *end = strchr(arg, ']'); + if (!end) + return false; + /* Copy inside [] */ + *addr = tal_strndup(ctx, arg + 1, end - arg - 1); + portcolon = strchr(end+1, ':'); + } else { + portcolon = strchr(arg, ':'); + if (portcolon) { + /* Disregard if there's more than one : or if it's at + the start or end */ + if (portcolon != strrchr(arg, ':') + || portcolon == arg + || portcolon[1] == '\0') + portcolon = NULL; + } + if (portcolon) + *addr = tal_strndup(ctx, arg, portcolon - arg); + else + *addr = tal_strdup(ctx, arg); + } + + if (portcolon) { + char *endp; + *port = strtol(portcolon + 1, &endp, 10); + return *port != 0 && *endp == '\0'; + } + return true; +} bool parse_wireaddr(const char *arg, struct wireaddr *addr, u16 defport) { @@ -78,13 +123,15 @@ bool parse_wireaddr(const char *arg, struct wireaddr *addr, u16 defport) res = false; port = defport; - if (!parse_ip_port(tmpctx, arg, &ip, &port)) - port = defport; + if (!separate_address_and_port(tmpctx, arg, &ip, &port)) { + tal_free(tmpctx); + return false; + } /* FIXME: change arg to addr[:port] and use getaddrinfo? */ - if (streq(arg, "localhost")) + if (streq(ip, "localhost")) ip = "127.0.0.1"; - else if (streq(arg, "ip6-localhost")) + else if (streq(ip, "ip6-localhost")) ip = "::1"; memset(&addr->addr, 0, sizeof(addr->addr)); @@ -106,37 +153,3 @@ bool parse_wireaddr(const char *arg, struct wireaddr *addr, u16 defport) tal_free(tmpctx); return res; } - -// NOTE: arg is assumed to be an ipv4/6 addr string with optional port -bool parse_ip_port(tal_t *ctx, const char *arg, char **ip, u16 *port) { - *port = 0; - *ip = tal_strdup(ctx, arg); - - bool ipv6 = strchr(*ip, '.') == NULL; - bool has_brackets = strchr(*ip, '['); - bool has_colon = strchr(*ip, ':'); - - // we have an ip addr with no port - if ((ipv6 && !has_brackets) || (!ipv6 && !has_colon)) - return false; - - /* IPv6 can have [ ], trim them here */ - if (ipv6 && strstarts(*ip, "[") && strends(*ip, "]")) { - (*ip)++; - (*ip)[strlen(*ip)-1] = '\0'; - return false; - } - - // we have a port, let's go to it - const char *last_colon = strrchr(*ip, ':'); - assert(last_colon); - - // chop off port - (*ip)[last_colon - *ip - ipv6] = '\0'; - - // skip over first [ if ipv6 - if (ipv6) (*ip)++; - - *port = atoi(last_colon + 1); - return *port != 0; -} diff --git a/common/wireaddr.h b/common/wireaddr.h index 4b1893749..69f1a9225 100644 --- a/common/wireaddr.h +++ b/common/wireaddr.h @@ -41,7 +41,6 @@ struct wireaddr { void towire_wireaddr(u8 **pptr, const struct wireaddr *addr); bool fromwire_wireaddr(const u8 **cursor, size_t *max, struct wireaddr *addr); -bool parse_ip_port(tal_t *ctx, const char *arg, char **ip, u16 *port); bool parse_wireaddr(const char *arg, struct wireaddr *addr, u16 port); char *fmt_wireaddr(const tal_t *ctx, const struct wireaddr *a);