config: refactor parsing of static RA routes

Adds comments and makes static RA routes parser easier to read and
maintain. Also logs concrete values in case some routes are invalid.
Fixes memory leak in case of invalid RA route (line 395 in the original
`config.c`).

Signed-off-by: Dávid Benko <[email protected]>

Author: dbnk0

src/config.c

@@ -372,45 +372,52 @@ static int parse_ra_flags(uint8_t *flags, struct blob_attr *attr)
 
 /* Returns 0 on success, -1 on invalid value, -2 on memory error
 (`routes` is unmodified in that case) */
-static int parse_ra_staticroute(struct blob_attr *attr, struct odhcpd_ip6prefix **routes,
-	size_t *routes_cnt)
+static int parse_append_ra_staticroute(struct blob_attr *attr,
+	struct odhcpd_ip6prefix **routes, size_t *routes_cnt)
 {
 	if (blobmsg_type(attr) != BLOBMSG_TYPE_STRING ||
-		!blobmsg_check_attr(attr, false))
+	    !blobmsg_check_attr(attr, false)) {
 		return -1;
+	}
 
 	const char *str = blobmsg_get_string(attr);
-	char *astr = malloc(strlen(str) + 1);
-	char *delim;
-	int l;
 	struct odhcpd_ip6prefix prefix;
 
-	if (!astr || !strcpy(astr, str))
-		return -2;
+	/* Parse the address and prefix length */
+	if (odhcpd_parse_addr6_prefix(str, &prefix.addr, &prefix.len) < 0) {
+		return -1;
+	}
 
-	if ((delim = strchr(astr, '/')) == NULL || (*(delim++) = 0) ||
-		sscanf(delim, "%i", &l) == 0 || l < 1 || l > 128 ||
-		inet_pton(AF_INET6, astr, &prefix.addr) == 0 ||
-		IN6_IS_ADDR_UNSPECIFIED(&prefix.addr)) {
+	/*
+	 * Validate prefix length
+	 * Default route (::/0) is managed elsewhere.
+	 */
+	if (prefix.len == 0 || prefix.len > 128) {
 		return -1;
-	} else {
-		prefix.len = l;
-
-		struct odhcpd_ip6prefix *tmp = realloc(*routes, (*routes_cnt + 1) *
-			sizeof(**routes));
-		if (!tmp) {
-			if (astr)
-				free(astr);
-			return -2;
-		}
+	}
 
-		*routes = tmp;
-		(*routes)[*routes_cnt] = prefix;
-		++(*routes_cnt);
+	/*
+	 * Validate address
+	 * Zero-address (::) specifies default route and is managed elsewhere,
+	 * loopback and link-local addresses are not routable.
+	 */
+	if (IN6_IS_ADDR_UNSPECIFIED(&prefix.addr) ||
+	    IN6_IS_ADDR_LOOPBACK(&prefix.addr) ||
+	    IN6_IS_ADDR_LINKLOCAL(&prefix.addr)) {
+		return -1;
 	}
 
-	if (astr)
-		free(astr);
+	/* Append the prefix to the list */
+	struct odhcpd_ip6prefix *tmp = realloc(*routes, (*routes_cnt + 1) *
+					       sizeof(**routes));
+	if (!tmp) {
+		return -2;
+	}
+
+	*routes = tmp;
+	(*routes)[*routes_cnt] = prefix;
+	++(*routes_cnt);
+
 	return 0;
 }
 
@@ -461,15 +468,28 @@ static void set_config(struct uci_section *s)
 		unsigned rem;
 
 		blobmsg_for_each_attr(cur, c, rem) {
-			int ec = parse_ra_staticroute(cur, &(config.ra_static_routes),
+			/* Parse and append the route */
+			int ec = parse_append_ra_staticroute(
+				cur, &(config.ra_static_routes),
 				&(config.ra_static_routes_cnt));
 
 			if (ec == -1) {
-				syslog(LOG_ERR, "Invalid %s value",
-					odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name);
+				/* Invalid value */
+				char default_str[] = "?";
+				char *str = default_str;
+				if (blobmsg_type(cur) == BLOBMSG_TYPE_STRING &&
+				    blobmsg_check_attr(cur, false)) {
+					str = blobmsg_get_string(cur);
+				}
+				syslog(LOG_WARNING, "Invalid %s value (%s), "
+				       "ignoring this route",
+				       odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name,
+				       str);
 			} else if (ec == -2) {
+				/* Memory error */
 				syslog(LOG_ERR, "Memory allocation failed for %s",
-					odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name);
+				       odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name);
+				break;
 			}
 		}
 	}
@@ -1388,21 +1408,34 @@ int config_parse_interface(void *data, size_t len, const char *name, bool overwr
 
 	if ((c = tb[IFACE_ATTR_RA_DNS]))
 		iface->ra_dns = blobmsg_get_bool(c);
-	
+
 	if ((c = tb[IFACE_ATTR_RA_STATICROUTE])) {
 		struct blob_attr *cur;
 		unsigned rem;
 
 		blobmsg_for_each_attr(cur, c, rem) {
-			int ec = parse_ra_staticroute(cur, &(iface->ra_static_routes),
+			/* Parse and append the route */
+			int ec = parse_append_ra_staticroute(
+				cur, &(iface->ra_static_routes),
 				&(iface->ra_static_routes_cnt));
 
 			if (ec == -1) {
-				syslog(LOG_ERR, "Invalid %s value configured for interface '%s'",
-					iface_attrs[IFACE_ATTR_RA_STATICROUTE].name, iface->name);
+				/* Invalid value */
+				char default_str[] = "?";
+				char *str = default_str;
+				if (blobmsg_type(cur) == BLOBMSG_TYPE_STRING &&
+				    blobmsg_check_attr(cur, false)) {
+					str = blobmsg_get_string(cur);
+				}
+				syslog(LOG_WARNING, "Invalid %s value (%s) for "
+				       "interface '%s', ignoring this route",
+				       odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name,
+				       str, iface->name);
 			} else if (ec == -2) {
-				syslog(LOG_ERR, "Memory allocation failed for %s on interface '%s'",
-					iface_attrs[IFACE_ATTR_RA_STATICROUTE].name, iface->name);
+				/* Memory error */
+				syslog(LOG_ERR, "Memory allocation failed for %s",
+				       odhcpd_attrs[ODHCPD_ATTR_RA_STATICROUTE].name);
+				break;
 			}
 		}
 	}