iprange: rework range parsing using u_parse_*() functions

Now that we have primitives for parsing IPv4 ranges, let's use them to
simplify parse_iprange().

Try u_parse_ip4cidr() first. In case of failure, try u_parse_ip4range().
If any of them succeeds, verify that there aren't spurious data
following the range definition. If everything is valid, either load the
range or disable the module (if the range is 0.0.0.0/0).

The diff is a bit ugly, but the implementation should be much clearer.

Signed-off-by: Guillaume Nault <g.nault@alphalink.fr>

1 files changed, 63 insertions, 95 deletions

diff --git a/accel-pppd/iprange.c b/accel-pppd/iprange.c
index 40c938e2..58321bd3 100644
--- a/accel-pppd/iprange.c
+++ b/accel-pppd/iprange.c
@@ -29,12 +29,6 @@ static pthread_mutex_t iprange_lock = PTHREAD_MUTEX_INITIALIZER;
 static bool conf_disable = false;
 static LIST_HEAD(client_ranges);
 
-/* Maximum IPv4 address length with CIDR notation but no extra 0,
- * e.g. "0xff.0xff.0xff.0xff/32".
- */
-#define CIDR_MAXLEN 22
-
-
 static void free_ranges(struct list_head *head)
 {
 	struct iprange_t *range;
@@ -46,115 +40,89 @@ static void free_ranges(struct list_head *head)
 	}
 }
 
+/* Parse a [client-ip-iprange] configuration entry.
+ * Ranges can be defined in CIDR notation ("192.0.2.0/24") or by specifying an
+ * upper bound for the last IPv4 byte, after a '-' character ("192.0.2.0-255").
+ * For simplicity, only mention the CIDR notation in error messages.
+ */
 static int parse_iprange(const char *str, struct iprange_t **range)
 {
-	char ipstr[CIDR_MAXLEN + 1] = { 0 };
-	struct iprange_t *_range;
-	struct in_addr addr;
-	char *suffix_str;
-	uint32_t ipmin;
-	uint32_t ipmax;
-	bool is_cidr;
-
-	/* Extra spaces and comments must have already been removed */
-	if (strpbrk(str, " \t#")) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " invalid space or comment character found\n",
-			  str);
-		return -1;
-	}
+	struct iprange_t *new_range;
+	struct in_addr base_addr;
+	const char *ptr;
+	uint32_t ip_min;
+	uint32_t ip_max;
+	uint8_t suffix;
+	size_t len;
 
 	if (!strcmp(str, "disable"))
 		goto disable;
 
-	strncpy(ipstr, str, CIDR_MAXLEN + 1);
-	if (ipstr[CIDR_MAXLEN] != '\0') {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " line too long\n",
-			  str);
-		return -1;
-	}
-
-	suffix_str = strpbrk(ipstr, "-/");
-	if (!suffix_str) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " unrecognised range format\n",
-			  str);
-		return -1;
-	}
-
-	is_cidr = *suffix_str == '/';
-	*suffix_str = '\0';
-	++suffix_str;
-
-	if (!u_parse_ip4addr(ipstr, &addr)) {
-		log_error("iprange: impossible to parse range \"%s\":"
-			  " invalid IPv4 address \"%s\"\n",
-			  str, ipstr);
-		return -1;
-	}
-	ipmin = ntohl(addr.s_addr);
+	ptr = str;
 
-
-	/* If is_cidr is set, range is given with CIDR notation,
-	 * e.g. "192.0.2.0/24".
-	 * If unset, range is an IP address where the last octet is replaced by
-	 * an octet range, e.g. "192.0.2.0-255".
-	 */
-	if (is_cidr) {
-		long int prefix_len;
+	/* Try IPv4 CIDR notation first */
+	len = u_parse_ip4cidr(ptr, &base_addr, &suffix);
+	if (len) {
+		uint32_t addr_hbo;
 		uint32_t mask;
 
-		if (u_readlong(&prefix_len, suffix_str, 0, 32)) {
-			log_error("iprange: impossible to parse range \"%s\":"
-				  " invalid CIDR prefix length \"/%s\"\n",
-				  str, suffix_str);
-			return -1;
-		}
+		/* Cast to uint64_t to avoid undefined 32 bits shift on 32 bits
+		 * integer if 'suffix' is 0.
+		 */
+		mask = (uint64_t)0xffffffff << (32 - suffix);
+		addr_hbo = ntohl(base_addr.s_addr);
+		ip_min = addr_hbo & mask;
+		ip_max = addr_hbo | ~mask;
+
+		if (ip_min != addr_hbo) {
+			struct in_addr min_addr = { .s_addr = htonl(ip_min) };
+			char ipbuf[INET_ADDRSTRLEN];
 
-		/* Interpret /0 as disable request */
-		if (prefix_len == 0) {
-			if (ipmin != INADDR_ANY)
-				log_warn("iprange: %s is equivalent to 0.0.0.0/0 and disables the iprange module\n",
-					 str);
-			goto disable;
+			log_warn("iprange: network %s is equivalent to %s/%hhu\n",
+				 str, u_ip4str(&min_addr, ipbuf), suffix);
 		}
+		goto addrange;
+	}
 
-		mask = INADDR_BROADCAST << (32 - prefix_len);
-		if (ipmin != (ipmin & mask)) {
-			char buf[INET_ADDRSTRLEN] = { 0 };
+	/* Not an IPv4 CIDR, try the IPv4 range notation */
+	len = u_parse_ip4range(ptr, &base_addr, &suffix);
+	if (len) {
+		ip_min = ntohl(base_addr.s_addr);
+		ip_max = (ip_min & 0xffffff00) | suffix;
+		goto addrange;
+	}
 
-			ipmin &= mask;
-			addr.s_addr = htonl(ipmin);
-			log_warn("iprange: first IP of range %s will be %s\n",
-				 str, inet_ntop(AF_INET, &addr, buf,
-						sizeof(buf)));
-		}
+	log_error("iprange: parsing range \"%s\" failed:"
+		  " expecting an IPv4 network prefix in CIDR notation\n",
+		  str);
 
-		ipmax = ipmin | ~mask;
-	} else {
-		long int max;
+	return -1;
 
-		if (u_readlong(&max, suffix_str, ipmin & 0xff, 255)) {
-			log_error("iprange: impossible to parse range \"%s\":"
-				  " invalid upper bound \"-%s\"\n",
-				  str, suffix_str);
-			return -1;
-		}
+addrange:
+	ptr += len;
 
-		ipmax = (ipmin & 0xffffff00) | max;
+	if (!u_parse_endstr(ptr)) {
+		log_error("iprange: parsing range \"%s\" failed:"
+			  " unexpected data at \"%s\"\n",
+			  str, ptr);
+		return -1;
 	}
 
-	_range = _malloc(sizeof(*_range));
-	if (!_range) {
-		log_error("iprange: impossible to allocate range \"%s\":"
-			  " memory allocation failed\n", str);
+	if (ip_min == INADDR_ANY && ip_max == INADDR_BROADCAST)
+		goto disable;
+
+	new_range = _malloc(sizeof(*new_range));
+	if (!new_range) {
+		log_error("iprange: impossible to load range \"%s\":"
+			  " memory allocation failed\n",
+			  str);
 		return -1;
 	}
 
-	_range->begin = ipmin;
-	_range->end = ipmax;
-	*range = _range;
+	new_range->begin = ip_min;
+	new_range->end = ip_max;
+
+	*range = new_range;
 
 	return 0;
 
@@ -175,7 +143,7 @@ static bool load_ranges(struct list_head *list, const char *conf_sect)
 
 	list_for_each_entry(opt, &s->items, entry) {
 		/* Ignore parsing errors, parse_iprange() already logs suitable
-		 * error message.
+		 * error messages.
 		 */
 		if (parse_iprange(opt->name, &r) < 0)
 			continue;