[PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers

[David Gibson <david@gibson.dropbear.id.au>]

Forwarding rules for the -[tTuU] options are parsed in fwd_rule_parse()
and fwd_rule_parse_ports().  Currently those use a mix of loosely
recursive descent parsing helpers, consuming input from left to right,
and ad-hoc C parsing - searching for delimeters with strchr() or the like
and breaking down on that basis.

As well as being a slightly awkward mix, the current approach will not work
for adding target addresses to the rules: we can't easily split the
listening from target parts first, because the ':' delimiter could appear
multiple times for multiple port ranges, or in IPv6 addresses.  However,
without splitting the target part first, we can't first split off the
listening address and interface because the '/' delimiter could appear in
the target portion, but not the listening portion, e.g.:
    -t 12345:192.0.1.1/12345

To address this, rewrite the entirety of the parsing so we consume the
input left to right in a more-or-less recursive descent manner.  This means
we no longer rely on certain delimiters having a single meaning over the
whole input, just the next part of it.

Because of the semantics of port specs, we need to make several passes
over the list of comma separated chunks.  Previously, some of the logic was
duplicated between the two passes, making it fragile.  Now, we introduce
a parse_port_chunk() helper which we re-use in each pass to make it clearer
we parse exactly the same way on each pass.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 fwd_rule.c | 262 ++++++++++++++++++++++++++++++-----------------------
 parse.c    |  29 ++++++
 parse.h    |   2 +
 3 files changed, 181 insertions(+), 112 deletions(-)

diff --git a/fwd_rule.c b/fwd_rule.c
index b14df340..8ae21b99 100644
--- a/fwd_rule.c
+++ b/fwd_rule.c
@@ -439,17 +439,96 @@ fail:
 	    fwd_rule_fmt(&rule, rulestr, sizeof(rulestr)));
 }
 
-/*
- * for_each_chunk - Step through delimited chunks of a string
- * @p_:		Pointer to start of each chunk (updated)
- * @ep_:	Pointer to end of each chunk (updated)
- * @s_:		String to step through
- * @sep_:	String of all allowed delimiters
+/**
+ * enum fwd_port_chunk_kind - Kind of port specifier piece
+ */
+enum fwd_port_chunk_kind {
+	CHUNK_ALL,		/* "all" */
+	CHUNK_AUTO,		/* "auto" */
+	CHUNK_EXCLUDE,		/* "~1111-2222" */
+	CHUNK_INCLUDE,		/* "1111-2222" */
+};
+
+/**
+ * parse_port_chunk() - Parse one chunk of a port specifier
+ * @cursor:	Parsing point (see parse.c)
+ * @kindp:	Updated with kind of chunk we parsed
+ * @lrange:	Updated with listening port range (for INCLUDE & EXCLUDE)
+ * @trange:	Updated with target port range (for EXCLUDE)
+ */
+static bool parse_port_chunk(const char **cursor, enum fwd_port_chunk_kind *kindp,
+			     struct port_range *lrange, struct port_range *trange)
+{
+	struct port_range lr = { 0 }, tr = { 0 };
+	enum fwd_port_chunk_kind kind;
+	const char *p = *cursor;
+
+	if (parse_literal(&p, "all")) {
+		kind = CHUNK_ALL;
+	} else if (parse_literal(&p, "auto")) {
+		kind = CHUNK_AUTO;
+	} else if (parse_literal(&p, "~")) {
+		kind = CHUNK_EXCLUDE;
+		if (!parse_port_range(&p, &lr))
+			return false;
+	} else if (parse_port_range(&p, &lr)) {
+		kind = CHUNK_INCLUDE;
+
+		if (parse_literal(&p, ":")) {
+			if (!parse_port_range(&p, &tr))
+				return false;
+		} else {
+			tr = lr;
+		}
+	} else {
+		return false;
+	}
+
+	*kindp = kind;
+	*lrange = lr;
+	if (trange)
+		*trange = tr;
+	*cursor = p;
+	return true;
+}
+
+/**
+ * parse_laddrif() - Parse listening address+ifname
+ * @cursor:	Parsing cursor (see parse.c)
+ * @addr:	Updated with parsed inany address (NULL for *)
+ * @abuf:	Buffer to store address
+ * @ifname:	Updated with parsed interface name ("" if none)
  */
-#define for_each_chunk(p_, ep_, s_, sep_)			\
-	for ((p_) = (s_);					\
-	     (ep_) = (p_) + strcspn((p_), (sep_)), *(p_);	\
-	     (p_) = *(ep_) ? (ep_) + 1 : (ep_))
+static bool parse_laddrif(const char **cursor,
+			  const union inany_addr **addr,
+			  union inany_addr *abuf,
+			  char *ifname)
+{
+	union inany_addr atmp = inany_any6;
+	char iftmp[IFNAMSIZ] = {0};
+	const char *p;
+
+	if (p = *cursor,
+	    parse_inany(&p, &atmp)		&&
+	    parse_ifspec(&p, iftmp)		&&
+	    parse_literal(&p, "/")) {
+		/* Specific listening address */
+		*addr = abuf;
+	} else if (p = *cursor,
+		   parse_literal(&p, "*"),
+		   parse_ifspec(&p, iftmp)	&&
+		   parse_literal(&p, "/")) {
+		/* Missing or "*" address */
+		*addr = NULL;
+	} else {
+		return false;
+	}
+
+	*abuf = atmp;
+	memcpy(ifname, iftmp, IFNAMSIZ);
+	*cursor = p;
+	return true;
+}
 
 /**
  * fwd_rule_parse_ports() - Parse port range(s) specifier
@@ -466,87 +545,70 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto,
 				 const char *spec)
 {
 	uint8_t exclude[PORT_BITMAP_SIZE] = { 0 };
+	enum fwd_port_chunk_kind kind;
+	struct port_range lrange;
 	bool exclude_only = true;
-	const char *p, *ep;
 	uint8_t flags = 0;
+	const char *p;
 	unsigned i;
 
-	/* Parse excluded ranges and "auto" in the first pass */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range xrange;
-
-		/* Include range, parse later */
-		if (parse_literal(&p, "all") || isdigit(*p))
-			continue;
-
-		if (parse_literal(&p, "auto")) {
-			if (p != ep) /* Garbage after the keyword */
-				goto bad;
+	/* Consider excluded ranges and "auto" in the first pass */
+	p = spec;
+	do {
+		if (!parse_port_chunk(&p, &kind, &lrange, NULL))
+			goto bad;
 
+		switch (kind) {
+		case CHUNK_AUTO:
 			if (!(fwd->caps & FWD_CAP_SCAN)) {
 				die(
 "'auto' port forwarding is only allowed for pasta");
 			}
-
 			flags |= FWD_SCAN;
-			continue;
-		}
-
-		/* Should be an exclude range */
-		if (!parse_literal(&p, "~"))
-			goto bad;
-
-		if (!parse_port_range(&p, &xrange))
-			goto bad;
-		if (p != ep) /* Garbage after the range */
-			goto bad;
-
-		for (i = xrange.first; i <= xrange.last; i++)
-			bitmap_set(exclude, i);
-	}
-
-	/* Now process base ranges, skipping exclusions */
-	for_each_chunk(p, ep, spec, ",") {
-		struct port_range orig_range, mapped_range;
-
-		/* Handle "all" like exclude only */
-		if (parse_literal(&p, "all")) {
-			if (p != ep) /* Garbage after the keyword */
-				goto bad;
+			break;
 
-			continue;
+		case CHUNK_EXCLUDE:
+			for (i = lrange.first; i <= lrange.last; i++)
+				bitmap_set(exclude, i);
+			break;
+		default:
+			; /* Handled later */
 		}
+	} while (parse_literal(&p, ","));
 
-		if (!isdigit(*p))
-			/* Already parsed */
-			continue;
+	/* Consider included ranges in next pass */
+	p = spec;
+	do {
+		struct port_range trange;
 
-		if (!parse_port_range(&p, &orig_range))
+		if (!parse_port_chunk(&p, &kind, &lrange, &trange))
 			goto bad;
 
-		exclude_only = false;
+		switch (kind) {
+		case CHUNK_AUTO:	/* already handled */
+		case CHUNK_EXCLUDE:	/* already handled */
+		case CHUNK_ALL:		/* handled later */
+			continue;
 
-		if (parse_literal(&p, ":")) {
-			/* There's a range to map to as well */
-			if (!parse_port_range(&p, &mapped_range))
-				goto bad;
-			if ((mapped_range.last - mapped_range.first) !=
-			    (orig_range.last - orig_range.first))
+		case CHUNK_INCLUDE:
+			exclude_only = false;
+			if (trange.last - trange.first !=
+			    lrange.last - lrange.first)
 				goto bad;
-		} else {
-			mapped_range = orig_range;
-		}
 
-		if (p != ep) /* Garbage after the ranges */
+			fwd_rule_range_except(fwd, del, proto, addr, ifname,
+					      lrange.first, lrange.last,
+					      exclude, trange.first, flags);
+			break;
+		default:
 			goto bad;
+		}
+	} while (parse_literal(&p, ","));
 
-		fwd_rule_range_except(fwd, del, proto, addr, ifname,
-				      orig_range.first, orig_range.last,
-				      exclude,
-				      mapped_range.first, flags);
-	}
+	if (!parse_eoi(p))
+		goto bad; /* trailing garbage */
 
-	/* Finally handle "all" and exclude only specs */
+	/* Finally handle "all" and exclude only cases */
 	if (exclude_only) {
 		fwd_port_map_ephemeral(exclude);
 
@@ -569,10 +631,11 @@ bad:
 void fwd_rule_parse(char optname, bool del, const char *optarg,
 		    struct fwd_table *fwd)
 {
-	char buf[BUFSIZ], *spec, *ifname = NULL;
-	union inany_addr addr_buf = inany_any6;
-	const union inany_addr *addr = &addr_buf;
+	const union inany_addr *addr;
+	union inany_addr addr_buf;
+	char ifname[IFNAMSIZ];
 	uint8_t proto;
+	const char *p;
 
 	if (optname == 't' || optname == 'T')
 		proto = IPPROTO_TCP;
@@ -581,7 +644,8 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 	else
 		assert(0);
 
-	if (!strcmp(optarg, "none")) {
+	if (p = optarg,
+	    parse_literal(&p, "none") && parse_eoi(p)) {
 		unsigned i;
 
 		for (i = 0; i < fwd->count; i++) {
@@ -593,45 +657,19 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 		return;
 	}
 
-	strncpy(buf, optarg, sizeof(buf) - 1);
-
-	if ((spec = strchr(buf, '/'))) {
-		const char *p = buf;
-
-		*spec = 0;
-		spec++;
-
-		if (optname != 't' && optname != 'u')
+	if (p = optarg,
+	    parse_laddrif(&p, &addr, &addr_buf, ifname)) {
+		if (optname == 'T' || optname == 'U')
 			die("Listening address not allowed for -%c %s",
 			    optname, optarg);
-
-		if ((ifname = strchr(buf, '%'))) {
-			*ifname = 0;
-			ifname++;
-
-			/* spec is already advanced one past the '/',
-			 * so the length of the given ifname is:
-			 * (spec - ifname - 1)
-			 */
-			if (spec - ifname - 1 >= IFNAMSIZ) {
-				die("Interface name '%s' is too long (max %u)",
-				    ifname, IFNAMSIZ - 1);
-			}
-		}
-
-		if (ifname == buf + 1 || /* Interface without address */
-		    !strcmp(buf, "*")) /* Explicit wildcard address */
-			addr = NULL;
-		else if (!parse_inany(&p, &addr_buf) && parse_eoi(p))
-			die("Bad forwarding address '%s'", buf);
 	} else {
-		spec = buf;
-
+		/* No address or ifname */
 		addr = NULL;
+		ifname[0] = '\0';
 	}
 
 	if (optname == 'T' || optname == 'U') {
-		assert(!addr && !ifname);
+		assert(!addr && !*ifname);
 
 		if (!(fwd->caps & FWD_CAP_IFNAME)) {
 			warn(
@@ -640,18 +678,18 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 
 			if (fwd->caps & FWD_CAP_IPV4) {
 				fwd_rule_parse_ports(fwd, del, proto,
-						     &inany_loopback4, NULL,
-						     spec);
+						     &inany_loopback4, NULL, p);
 			}
 			if (fwd->caps & FWD_CAP_IPV6) {
 				fwd_rule_parse_ports(fwd, del, proto,
-						     &inany_loopback6, NULL,
-						     spec);
+						     &inany_loopback6, NULL, p);
 			}
 			return;
 		}
 
-		ifname = "lo";
+		static_assert(sizeof("lo") <= sizeof(ifname),
+			      "ifname buffer too small");
+		strcpy(ifname, "lo");
 	}
 
 	/* No need for dual stack if we only have one IP version */
@@ -660,13 +698,13 @@ void fwd_rule_parse(char optname, bool del, const char *optarg,
 	else if (!addr && !(fwd->caps & FWD_CAP_IPV6))
 		addr = &inany_any4;
 
-	if (ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
+	if (*ifname && !(fwd->caps & FWD_CAP_IFNAME)) {
 		die(
 "Device binding for '-%c %s' unsupported (requires kernel 5.7+)",
 		    optname, optarg);
 	}
 
-	fwd_rule_parse_ports(fwd, del, proto, addr, ifname, spec);
+	fwd_rule_parse_ports(fwd, del, proto, addr, *ifname ? ifname : NULL, p);
 }
 
 /**
diff --git a/parse.c b/parse.c
index 3e0dbd45..d83ea9f9 100644
--- a/parse.c
+++ b/parse.c
@@ -18,6 +18,7 @@
 #include <string.h>
 #include <limits.h>
 #include <arpa/inet.h>
+#include <net/if.h>
 
 #include "common.h"
 #include "parse.h"
@@ -212,3 +213,31 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
 
 	return false;
 }
+
+/**
+ * parse_ifspec() - Parse a interface name specifier (starting with %)
+ * @ifname:	On success updated with parsed name (must have IFNAMSIZ space)
+ *
+ * This will accept a missing specifier (empty string), setting ifname to ""
+ */
+bool parse_ifspec(const char **cursor, char *ifname)
+{
+	const char *p = *cursor;
+	size_t len;
+
+	if (!parse_literal(&p, "%")) {
+		/* No interface specifier */
+		ifname[0] = '\0';
+		return true;
+	}
+
+	/* ifnames can have anything that's not '/' or whitespace */
+	len = strcspn(p, "/ \f\n\r\t\v");
+	if (!len || len >= IFNAMSIZ)
+		return false;
+
+	memcpy(ifname, p, len);
+	ifname[len] = '\0';
+	*cursor = p + len;
+	return true;
+}
diff --git a/parse.h b/parse.h
index 08b038cf..1079f5bf 100644
--- a/parse.h
+++ b/parse.h
@@ -32,4 +32,6 @@ bool parse_inany_(const char **cursor, union inany_addr *addr,
 
 #define parse_inany(cursor, addr)	parse_inany_((cursor), (addr), NULL)
 
+bool parse_ifspec(const char **cursor, char *ifname);
+
 #endif /* _PARSE_H */
-- 
2.54.0