gossip: fix address descriptor handling.

1. The code to skip over padding didn't take into account max.
2. It also didn't use symbolic names.
3. We are not supposed to fail on unknown addresses, just stop parsing.
4. We don't use the read_ip/write_ip code, so get rid of it.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

gossipd/routing.c

@@ -427,109 +427,6 @@ static bool add_channel_direction(struct routing_state *rstate,
 	return true;
 }
 
-/* BOLT #7:
- *
- * The following `address descriptor` types are defined:
- *
- * * `0`: padding.  data = none (length 0).
- * * `1`: ipv4. data = `[4:ipv4_addr][2:port]` (length 6)
- * * `2`: ipv6. data = `[16:ipv6_addr][2:port]` (length 18)
- */
-
-/* FIXME: Don't just take first one, depends whether we have IPv6 ourselves */
-/* Returns false iff it was malformed */
-bool read_ip(const tal_t *ctx, const u8 *addresses, char **hostname,
-		    int *port)
-{
-	size_t len = tal_count(addresses);
-	const u8 *p = addresses;
-	char tempaddr[INET6_ADDRSTRLEN];
-	be16 portnum;
-
-	*hostname = NULL;
-	while (len) {
-		u8 type = *p;
-		p++;
-		len--;
-
-		switch (type) {
-		case 0:
-			break;
-		case 1:
-			/* BOLT #7:
-			 *
-			 * The receiving node SHOULD fail the connection if
-			 * `addrlen` is insufficient to hold the address
-			 * descriptors of the known types.
-			 */
-			if (len < 6)
-				return false;
-			inet_ntop(AF_INET, p, tempaddr, sizeof(tempaddr));
-			memcpy(&portnum, p + 4, sizeof(portnum));
-			*hostname = tal_strdup(ctx, tempaddr);
-			*port = be16_to_cpu(portnum);
-			return true;
-		case 2:
-			if (len < 18)
-				return false;
-			inet_ntop(AF_INET6, p, tempaddr, sizeof(tempaddr));
-			memcpy(&portnum, p + 16, sizeof(portnum));
-			*hostname = tal_strdup(ctx, tempaddr);
-			*port = be16_to_cpu(portnum);
-			return true;
-		default:
-			/* BOLT #7:
-			 *
-			 * The receiving node SHOULD ignore the first `address
-			 * descriptor` which does not match the types defined
-			 * above.
-			 */
-			return true;
-		}
-	}
-
-	/* Not a fatal error. */
-	return true;
-}
-
-/* BOLT #7:
- *
- * The creating node SHOULD fill `addresses` with an address descriptor for
- * each public network address which expects incoming connections, and MUST
- * set `addrlen` to the number of bytes in `addresses`.  Non-zero typed
- * address descriptors MUST be placed in ascending order; any number of
- * zero-typed address descriptors MAY be placed anywhere, but SHOULD only be
- * used for aligning fields following `addresses`.
- *
- * The creating node MUST NOT create a type 1 or type 2 address descriptor
- * with `port` equal to zero, and SHOULD ensure `ipv4_addr` and `ipv6_addr`
- * are routable addresses.  The creating node MUST NOT include more than one
- * `address descriptor` of the same type.
- */
-/* FIXME: handle case where we have both ipv6 and ipv4 addresses! */
-u8 *write_ip(const tal_t *ctx, const char *srcip, int port)
-{
-	u8 *address;
-	be16 portnum = cpu_to_be16(port);
-
-	if (!port)
-		return tal_arr(ctx, u8, 0);
-
-	if (!strchr(srcip, ':')) {
-		address = tal_arr(ctx, u8, 7);
-		address[0] = 1;
-		inet_pton(AF_INET, srcip, address+1);
-		memcpy(address + 5, &portnum, sizeof(portnum));
-		return address;
-	} else {
-		address = tal_arr(ctx, u8, 18);
-		address[0] = 2;
-		inet_pton(AF_INET6, srcip, address+1);
-		memcpy(address + 17, &portnum, sizeof(portnum));
-		return address;
-	}
-}
-
 /* Verify the signature of a channel_update message */
 static bool check_channel_update(const struct pubkey *node_key,
 				 const secp256k1_ecdsa_signature *node_sig,
@@ -731,20 +628,32 @@ void handle_channel_update(struct routing_state *rstate, const u8 *update, size_
 	tal_free(tmpctx);
 }
 
-static struct ipaddr *read_addresses(const tal_t *ctx, u8 *ser)
+static struct ipaddr *read_addresses(const tal_t *ctx, const u8 *ser)
 {
 	const u8 *cursor = ser;
 	size_t max = tal_len(ser);
 	struct ipaddr *ipaddrs = tal_arr(ctx, struct ipaddr, 0);
 	int numaddrs = 0;
-	while (cursor < ser + max) {
+	while (cursor && cursor < ser + max) {
-		numaddrs++;
+		struct ipaddr ipaddr;
-		tal_resize(&ipaddrs, numaddrs);
+
-		fromwire_ipaddr(&cursor, &max, &ipaddrs[numaddrs-1]);
+		/* BOLT #7:
-		if (cursor == NULL) {
+		 *
+		 * The receiving node SHOULD ignore the first `address
+		 * descriptor` which does not match the types defined
+		 * above.
+		 */
+		if (!fromwire_ipaddr(&cursor, &max, &ipaddr)) {
+			if (!cursor)
 				/* Parsing address failed */
 				return tal_free(ipaddrs);
+			/* Unknown type, stop there. */
+			break;
 		}
+
+		tal_resize(&ipaddrs, numaddrs+1);
+		ipaddrs[numaddrs] = ipaddr;
+		numaddrs++;
 	}
 	return ipaddrs;
 }

gossipd/routing.h

@@ -115,9 +115,6 @@ struct node_connection *get_connection_by_scid(const struct routing_state *rstat
 					       const struct short_channel_id *schanid,
 					      const u8 direction);
 
-bool read_ip(const tal_t *ctx, const u8 *addresses, char **hostname, int *port);
-u8 *write_ip(const tal_t *ctx, const char *srcip, int port);
-
 /* Handlers for incoming messages */
 void handle_channel_announcement(struct routing_state *rstate, const u8 *announce, size_t len);
 void handle_channel_update(struct routing_state *rstate, const u8 *update, size_t len);

gossipd/test/run-find_route-specific.c

@@ -26,7 +26,7 @@ bool fromwire_channel_announcement(const tal_t *ctx UNNEEDED, const void *p UNNE
 bool fromwire_channel_update(const void *p UNNEEDED, size_t *plen UNNEEDED, secp256k1_ecdsa_signature *signature UNNEEDED, struct sha256_double *chain_hash UNNEEDED, struct short_channel_id *short_channel_id UNNEEDED, u32 *timestamp UNNEEDED, u16 *flags UNNEEDED, u16 *cltv_expiry_delta UNNEEDED, u64 *htlc_minimum_msat UNNEEDED, u32 *fee_base_msat UNNEEDED, u32 *fee_proportional_millionths UNNEEDED)
 { fprintf(stderr, "fromwire_channel_update called!\n"); abort(); }
 /* Generated stub for fromwire_ipaddr */
-void fromwire_ipaddr(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct ipaddr *addr UNNEEDED)
+bool fromwire_ipaddr(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct ipaddr *addr UNNEEDED)
 { fprintf(stderr, "fromwire_ipaddr called!\n"); abort(); }
 /* Generated stub for fromwire_node_announcement */
 bool fromwire_node_announcement(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, size_t *plen UNNEEDED, secp256k1_ecdsa_signature *signature UNNEEDED, u8 **features UNNEEDED, u32 *timestamp UNNEEDED, struct pubkey *node_id UNNEEDED, u8 rgb_color[3] UNNEEDED, u8 alias[32] UNNEEDED, u8 **addresses UNNEEDED)

gossipd/test/run-find_route.c

@@ -19,7 +19,7 @@ bool fromwire_channel_announcement(const tal_t *ctx UNNEEDED, const void *p UNNE
 bool fromwire_channel_update(const void *p UNNEEDED, size_t *plen UNNEEDED, secp256k1_ecdsa_signature *signature UNNEEDED, struct sha256_double *chain_hash UNNEEDED, struct short_channel_id *short_channel_id UNNEEDED, u32 *timestamp UNNEEDED, u16 *flags UNNEEDED, u16 *cltv_expiry_delta UNNEEDED, u64 *htlc_minimum_msat UNNEEDED, u32 *fee_base_msat UNNEEDED, u32 *fee_proportional_millionths UNNEEDED)
 { fprintf(stderr, "fromwire_channel_update called!\n"); abort(); }
 /* Generated stub for fromwire_ipaddr */
-void fromwire_ipaddr(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct ipaddr *addr UNNEEDED)
+bool fromwire_ipaddr(const u8 **cursor UNNEEDED, size_t *max UNNEEDED, struct ipaddr *addr UNNEEDED)
 { fprintf(stderr, "fromwire_ipaddr called!\n"); abort(); }
 /* Generated stub for fromwire_node_announcement */
 bool fromwire_node_announcement(const tal_t *ctx UNNEEDED, const void *p UNNEEDED, size_t *plen UNNEEDED, secp256k1_ecdsa_signature *signature UNNEEDED, u8 **features UNNEEDED, u32 *timestamp UNNEEDED, struct pubkey *node_id UNNEEDED, u8 rgb_color[3] UNNEEDED, u8 alias[32] UNNEEDED, u8 **addresses UNNEEDED)

lightningd/gossip_msg.c

@@ -9,7 +9,11 @@ void fromwire_gossip_getnodes_entry(const tal_t *ctx, const u8 **pptr, size_t *m
 
 	entry->addresses = tal_arr(ctx, struct ipaddr, numaddresses);
 	for (i=0; i<numaddresses; i++) {
-		fromwire_ipaddr(pptr, max, entry->addresses);
+		/* Gossipd doesn't hand us addresses we can't understand. */
+		if (!fromwire_ipaddr(pptr, max, entry->addresses)) {
+			fromwire_fail(pptr, max);
+			return;
+		}
 	}
 }
 void towire_gossip_getnodes_entry(u8 **pptr, const struct gossip_getnodes_entry *entry)

wire/fromwire.c

@@ -170,28 +170,39 @@ void fromwire_ripemd160(const u8 **cursor, size_t *max, struct ripemd160 *ripemd
 	fromwire(cursor, max, ripemd, sizeof(*ripemd));
 }
 
-void fromwire_ipaddr(const u8 **cursor, size_t *max, struct ipaddr *addr)
+/* BOLT #7:
+ *
+ * The following `address descriptor` types are defined:
+ *
+ * * `0`: padding.  data = none (length 0).
+ * * `1`: ipv4. data = `[4:ipv4_addr][2:port]` (length 6)
+ * * `2`: ipv6. data = `[16:ipv6_addr][2:port]` (length 18)
+ */
+/* FIXME: Tor addresses! */
+
+/* Returns false if we didn't parse it, and *cursor == NULL if malformed. */
+bool fromwire_ipaddr(const u8 **cursor, size_t *max, struct ipaddr *addr)
 {
-	/* Skip any eventual padding */
+	addr->type = fromwire_u8(cursor, max);
-	while (**cursor == 0) {
-		(*cursor)++;
-	}
 
-	addr->type = **cursor;
-	(*cursor)++;
 	switch (addr->type) {
-	case 1:
+	case ADDR_TYPE_IPV4:
 		addr->addrlen = 4;
 		break;
-	case 2:
+	case ADDR_TYPE_IPV6:
 		addr->addrlen = 16;
 		break;
 	default:
-		fromwire_fail(cursor, max);
+		return false;
-		return;
 	}
 	fromwire(cursor, max, addr->addr, addr->addrlen);
 	addr->port = fromwire_u16(cursor, max);
+
+	/* Skip any post-padding */
+	while (*max && (*cursor)[0] == ADDR_TYPE_PADDING)
+		fromwire_u8(cursor, max);
+
+	return *cursor != NULL;
 }
 
 void fromwire_u8_array(const u8 **cursor, size_t *max, u8 *arr, size_t num)

wire/wire.h

@@ -86,7 +86,7 @@ void fromwire_sha256_double(const u8 **cursor, size_t *max,
 			    struct sha256_double *sha256d);
 void fromwire_preimage(const u8 **cursor, size_t *max, struct preimage *preimage);
 void fromwire_ripemd160(const u8 **cursor, size_t *max, struct ripemd160 *ripemd);
-void fromwire_ipaddr(const u8 **cursor, size_t *max, struct ipaddr *addr);
+bool fromwire_ipaddr(const u8 **cursor, size_t *max, struct ipaddr *addr);
 void fromwire_pad(const u8 **cursor, size_t *max, size_t num);
 
 void fromwire_u8_array(const u8 **cursor, size_t *max, u8 *arr, size_t num);