From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=IL0aNBe5; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 145D05A0626 for ; Fri, 26 Jun 2026 09:10:18 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1782457808; bh=FfE201wL0AeLDUGCiZvMi0F+J7nrxmVllyELLI1CoSY=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=IL0aNBe5lEY0ramaJoY3N1O6HCK/kWxzRtKGcynKSkmjXDd36XbYIIr7vM27EcJKD 6EovzpxrXPldVSFk8NwwHes8KWTQxvg9gmfECxDVMdeZKzbBXuiQfPaaSmqkrTTAoY 29GKqiRnciR0RzsCZxTGBKEwL4XghNkG7wZQFyAyA+2Ml5xMxb185D/4RVhlkvfJ9h rhqUeEpGxc662ndz3MN4Mzu0QjOma8JzcSbQvjuaVwftmCd6m2MgWDoMyuo6+mnLZZ mat8tp1rsSQVO+Y+iR/SfyZROF7qZ9GqHz/Y5fDHvvRE/AvDjM/Pjg8QhEjary77Wi GZVMoNJAzcNBQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gmmym5m2rz4wst; Fri, 26 Jun 2026 17:10:08 +1000 (AEST) From: David Gibson To: passt-dev@passt.top, Stefano Brivio Subject: [PATCH 12/12] fwd_rule: Rewrite forward rule parsing using parse.c helpers Date: Fri, 26 Jun 2026 17:10:03 +1000 Message-ID: <20260626071003.3472194-13-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.54.0 In-Reply-To: <20260626071003.3472194-1-david@gibson.dropbear.id.au> References: <20260626071003.3472194-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: NVH66HH4VJH22VZFGBQHQLJEXXGVUCZQ X-Message-ID-Hash: NVH66HH4VJH22VZFGBQHQLJEXXGVUCZQ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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 #include #include +#include #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