The -F option is parsed in conf() along with everything else. However, because it informs what fds we can close at startup, we also have a special case parse of it in close_open_files(). At present we duplicate the parsing and validation code, which is a bit risky. Avoid that with a conf_tap_fd() helper. Signed-off-by: David Gibson --- conf.c | 29 +++++++++++++++++++---------- conf.h | 1 + util.c | 12 +++--------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/conf.c b/conf.c index ca6c859c..32163f56 100644 --- a/conf.c +++ b/conf.c @@ -1153,6 +1153,24 @@ static void conf_sock_listen(const struct ctx *c) die_perror("Couldn't add configuration socket to epoll"); } +/** + * conf_tap_fd() - Read tap fd as supplied by -F command line option + * @arg: Argument to -F command line option + */ +int conf_tap_fd(const char *arg) +{ + long val; + + errno = 0; + val = strtol(arg, NULL, 0); + + if (errno || (val != STDIN_FILENO && val <= STDERR_FILENO) || + val > INT_MAX) + die("Invalid --fd: %s", arg); + + return val; +} + /** * conf() - Process command-line arguments and set configuration * @c: Execution context @@ -1253,7 +1271,6 @@ void conf(struct ctx *c, int argc, char **argv) const char *logfile = NULL; char *runas = NULL; size_t logsize = 0; - long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1506,15 +1523,7 @@ void conf(struct ctx *c, int argc, char **argv) c->fd_control_listen = c->fd_control = -1; break; case 'F': - errno = 0; - fd_tap_opt = strtol(optarg, NULL, 0); - - if (errno || - (fd_tap_opt != STDIN_FILENO && fd_tap_opt <= STDERR_FILENO) || - fd_tap_opt > INT_MAX) - die("Invalid --fd: %s", optarg); - - c->fd_tap = fd_tap_opt; + c->fd_tap = conf_tap_fd(optarg); c->one_off = true; *c->sock_path = 0; break; diff --git a/conf.h b/conf.h index 16f97189..1fa1280e 100644 --- a/conf.h +++ b/conf.h @@ -7,6 +7,7 @@ #define CONF_H enum passt_modes conf_mode(int argc, char *argv[]); +int conf_tap_fd(const char *arg); void conf(struct ctx *c, int argc, char **argv); void conf_listen_handler(struct ctx *c, uint32_t events); void conf_handler(struct ctx *c, uint32_t events); diff --git a/util.c b/util.c index 0aa4d42d..4bc5d6f8 100644 --- a/util.c +++ b/util.c @@ -38,6 +38,7 @@ #include "epoll_ctl.h" #include "pasta.h" #include "serialise.h" +#include "conf.h" #ifdef HAS_GETRANDOM #include #endif @@ -939,15 +940,8 @@ void close_open_files(int argc, char **argv) do { name = getopt_long(argc, argv, "-:F:", optfd, NULL); - if (name == 'F') { - errno = 0; - fd = strtol(optarg, NULL, 0); - - if (errno || - (fd != STDIN_FILENO && fd <= STDERR_FILENO) || - fd > INT_MAX) - die("Invalid --fd: %s", optarg); - } + if (name == 'F') + fd = conf_tap_fd(optarg); } while (name != -1); if (fd == -1) { -- 2.54.0

conf_addr() sets c->ip[46]no_copy_addrs conditional upon being in pasta mode. That sort of makes sense, since address copying is only a thing for pasta mode. However, setting the variables anyway is harmless, and arguably more logically consistent. If we had a way of copying addresses for passt mode (or some future mode), it would still be incorrect to do so in these circumstances. So, make the assignments unconditional. Signed-off-by: David Gibson --- conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/conf.c b/conf.c index 3614776c..d1889e0c 100644 --- a/conf.c +++ b/conf.c @@ -1203,13 +1203,11 @@ static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n) c->ip4.addr = *inany_v4(&addr); c->ip4.prefix_len = prefix_len - 96; c->ip4.addr_fixed = true; - if (c->mode == MODE_PASTA) - c->ip4.no_copy_addrs = true; + c->ip4.no_copy_addrs = true; } else { c->ip6.addr = addr.a6; c->ip6.addr_fixed = true; - if (c->mode == MODE_PASTA) - c->ip6.no_copy_addrs = true; + c->ip6.no_copy_addrs = true; } return is_prefix; -- 2.54.0

Most places we need to parse an integer encoded as a string, we use strtol() or strtoul(). These already work a bit like descent parser helpers, in that they consume as much as they can, but don't require the number parsed to be the whole of the einput string. Make a wrapper to parse unsigned integers as part of our parsing helper. This wrapper handles the mildly fiddly error checking requirements for strtoul(). We replace a number, though not all, of our existing strtoul() uses with the new parse_unsigned(). We also replace a number of strtol() use cases, because, despite using that instead of strtoul() they are only used for non-negative values. In some cases this makes the logic a little more straightforward. In some other cases it will catch some error cases we previously could have missed. Signed-off-by: David Gibson --- Makefile | 1 - conf.c | 51 +++++++++++++++++++++++++-------------------------- fwd_rule.c | 42 ++++++++++++++---------------------------- parse.c | 27 ++++++++++++++++++++++----- parse.h | 1 + 5 files changed, 62 insertions(+), 60 deletions(-) diff --git a/Makefile b/Makefile index bd729c75..85d279c0 100644 --- a/Makefile +++ b/Makefile @@ -225,7 +225,6 @@ pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:bitmap.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.h pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h -pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:parse.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c pesto.cppcheck: $(PESTO_SRCS) $(PESTO_HEADERS) seccomp_pesto.h diff --git a/conf.c b/conf.c index ea299e18..bd357117 100644 --- a/conf.c +++ b/conf.c @@ -321,17 +321,16 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns, die("Both --netns and PID or command given"); if (optind + 1 == argc) { - char *endptr; - long pidval; + const char *p = argv[optind]; + unsigned long pidval; - pidval = strtol(argv[optind], &endptr, 10); - if (!*endptr) { + if (parse_unsigned(&p, 10, &pidval) && parse_eoi(p)) { /* Looks like a pid */ - if (pidval < 0 || pidval > INT_MAX) + if (pidval > INT_MAX) die("Invalid PID %s", argv[optind]); if (snprintf_check(netns, PATH_MAX, - "/proc/%ld/ns/net", pidval)) + "/proc/%lu/ns/net", pidval)) die_perror("Can't build netns path"); if (!*userns) { @@ -354,6 +353,7 @@ static void conf_pasta_ns(int *netns_only, char *userns, char *netns, */ static uint8_t conf_ip4_prefix(const char *arg) { + const char *p = arg; struct in_addr mask; unsigned long len; @@ -362,11 +362,9 @@ static uint8_t conf_ip4_prefix(const char *arg) len = __builtin_popcount(hmask); if ((hmask << len) == 0) return len; - } else { - errno = 0; - len = strtoul(arg, NULL, 0); - if (!errno && len <= 32) - return len; + } else if (parse_unsigned(&p, 0, &len) && parse_eoi(p) && + len <= 32) { + return len; } die("Invalid prefix length: %s", arg); @@ -1159,13 +1157,12 @@ static void conf_sock_listen(const struct ctx *c) */ int conf_tap_fd(const char *arg) { - long val; - - errno = 0; - val = strtol(arg, NULL, 0); + const char *p = arg; + unsigned long val; - if (errno || (val != STDIN_FILENO && val <= STDERR_FILENO) || - val > INT_MAX) + if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || + val > INT_MAX || + (val != STDIN_FILENO && val <= STDERR_FILENO)) die("Invalid --fd: %s", arg); return val; @@ -1288,6 +1285,8 @@ void conf(struct ctx *c, int argc, char **argv) optind = 0; do { + const char *p; + name = getopt_long(argc, argv, optstring, options, NULL); switch (name) { @@ -1368,14 +1367,17 @@ void conf(struct ctx *c, int argc, char **argv) case 12: runas = optarg; break; - case 13: - errno = 0; - logsize = strtol(optarg, NULL, 0); + case 13: { + unsigned long val; - if (logsize < LOGFILE_SIZE_MIN || errno) + p = optarg; + if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || + val < LOGFILE_SIZE_MIN) die("Invalid --log-size: %s", optarg); + logsize = val; break; + } case 14: FPRINTF(stdout, c->mode == MODE_PASTA ? "pasta " : "passt "); @@ -1552,12 +1554,9 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'm': { unsigned long mtu; - char *e; - - errno = 0; - mtu = strtoul(optarg, &e, 0); - if (errno || *e) + p = optarg; + if (!parse_unsigned(&p, 0, &mtu) || !parse_eoi(p)) die("Invalid MTU: %s", optarg); if (mtu > max_mtu) { diff --git a/fwd_rule.c b/fwd_rule.c index d0e90402..d4b422d7 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -41,10 +41,11 @@ static in_port_t fwd_ephemeral_max = NUM_PORTS - 1; */ void fwd_probe_ephemeral(void) { - char *line, *tab, *end; + unsigned long min, max; struct lineread lr; - long min, max; + const char *p; ssize_t len; + char *line; int fd; fd = open(PORT_RANGE_SYSCTL, O_RDONLY | O_CLOEXEC); @@ -57,35 +58,20 @@ void fwd_probe_ephemeral(void) len = lineread_get(&lr, &line); close(fd); - if (len < 0) - goto parse_err; - - tab = strchr(line, '\t'); - if (!tab) - goto parse_err; - *tab = '\0'; - - errno = 0; - min = strtol(line, &end, 10); - if (*end || errno) - goto parse_err; - - errno = 0; - max = strtol(tab + 1, &end, 10); - if (*end || errno) - goto parse_err; - - if (min < 0 || min >= (long)NUM_PORTS || - max < 0 || max >= (long)NUM_PORTS) - goto parse_err; + p = line; + if (len < 0 || + !parse_unsigned(&p, 10, &min) || + !parse_literal(&p, "\t") || + !parse_unsigned(&p, 10, &max) || + !parse_eoi(p) || + min >= NUM_PORTS || + max >= NUM_PORTS) { + warn("Unable to parse %s", PORT_RANGE_SYSCTL); + return; + } fwd_ephemeral_min = min; fwd_ephemeral_max = max; - - return; - -parse_err: - warn("Unable to parse %s", PORT_RANGE_SYSCTL); } /** diff --git a/parse.c b/parse.c index 4e3b527d..9bd0e81f 100644 --- a/parse.c +++ b/parse.c @@ -12,14 +12,12 @@ * Author: David Gibson */ -#include +#include #include -#include +#include #include -#include -#include -#include "common.h" +#include "parse.h" /** * DOC: Theory of Operation @@ -66,3 +64,22 @@ bool parse_eoi(const char *cursor) { return !(*cursor); } + +/* + * parse_unsigned() - Parse an unsigned integer + * @base: Numeric base for string as strtoul(3) + * @valp: On success, updated with parsed value + */ +bool parse_unsigned(const char **cursor, int base, unsigned long *valp) +{ + const char *p = *cursor; + unsigned long val; + + errno = 0; + val = strtoul(p, (char **)&p, base); + if (errno || p == *cursor) + return false; + *valp = val; + *cursor = p; + return true; +} diff --git a/parse.h b/parse.h index eb51a469..4f38c28b 100644 --- a/parse.h +++ b/parse.h @@ -10,5 +10,6 @@ bool parse_literal(const char **cursor, const char *lit); bool parse_eoi(const char *cursor); +bool parse_unsigned(const char **cursor, int base, unsigned long *valp); #endif /* _PARSE_H */ -- 2.54.0

The -a command line option can take either an address prefix, or a bare address. Current parsing of this is pretty awkward, using the special purpose helper inany_prefix_pton(). With the new incremental parsing helpers this can be done more naturally. Rework it to use them. This does requiring extending parse_inany() to parse_inany_() which also reports the format of the address as parse, as opposed to the family of the resulting address. This is so that ::ffff:192.0.1.1/112 will be correctly interpreted the same as 192.0.1.1/16, rather than the nonsensical 192.0.0.1/112. Cc: Jon Maloy Signed-off-by: David Gibson --- Makefile | 1 - conf.c | 57 +++++++++++++++++++++++++++++++++++++------------------- inany.c | 50 ------------------------------------------------- inany.h | 2 -- parse.c | 17 ++++++++++++++--- parse.h | 5 ++++- 6 files changed, 56 insertions(+), 76 deletions(-) diff --git a/Makefile b/Makefile index e2b22ddf..5757aeff 100644 --- a/Makefile +++ b/Makefile @@ -223,7 +223,6 @@ passt-repair.cppcheck: $(PASST_REPAIR_SRCS) $(PASST_REPAIR_HEADERS) seccomp_repa pesto.cppcheck: BASE_CPPFLAGS += -DPESTO pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:bitmap.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.h -pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:inany.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:ip.h pesto.cppcheck: CPPCHECK_FLAGS += --suppress=unusedFunction:serialise.c pesto.cppcheck: CPPCHECK_FLAGS += --suppress=staticFunction:fwd_rule.c diff --git a/conf.c b/conf.c index d1889e0c..c4a36dee 100644 --- a/conf.c +++ b/conf.c @@ -1176,31 +1176,45 @@ int conf_tap_fd(const char *arg) */ static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n) { + unsigned long prefix_len; + const struct in_addr *a4; union inany_addr addr; + sa_family_t parse_af; const char *p = arg; - uint8_t prefix_len; bool is_prefix; - is_prefix = inany_prefix_pton(arg, &addr, &prefix_len); - - if (is_prefix && opt_n) - die("Redundant prefix length specification"); - - if (!is_prefix && - !(parse_inany(&p, &addr) && parse_eoi(p))) - die("Invalid address: %s", arg); - - if (opt_n && inany_v4(&addr)) - prefix_len = opt_n; - else if (!is_prefix) - prefix_len = inany_default_prefix_len(&addr); + if (!parse_inany_(&p, &addr, &parse_af)) + goto bad; + a4 = inany_v4(&addr); + + if ((is_prefix = parse_literal(&p, "/"))) { + /* Prefix length included in -a option */ + if (!parse_unsigned(&p, 10, &prefix_len)) + goto bad; + if (opt_n) + die("Redundant prefix length specification"); + if (parse_af == AF_INET) { + if (prefix_len > 32) + goto bad_prefix; + prefix_len += 96; + } else if (prefix_len > 128) { + goto bad_prefix; + } + } else { + /* Get prefix length from elsewhere */ + if (opt_n && a4) + prefix_len = opt_n; + else + prefix_len = inany_default_prefix_len(&addr); + } - if (inany_is_unspecified(&addr) || inany_is_multicast(&addr) || - inany_is_loopback(&addr) || IN6_IS_ADDR_V4COMPAT(&addr.a6)) - die("Invalid address: %s", arg); + if (!parse_eoi(p) || + !inany_is_unicast(&addr) || + inany_is_loopback(&addr)) + goto bad; - if (inany_v4(&addr)) { - c->ip4.addr = *inany_v4(&addr); + if (a4) { + c->ip4.addr = *a4; c->ip4.prefix_len = prefix_len - 96; c->ip4.addr_fixed = true; c->ip4.no_copy_addrs = true; @@ -1211,6 +1225,11 @@ static bool conf_addr(struct ctx *c, char *arg, uint8_t opt_n) } return is_prefix; + +bad_prefix: + die("Invalid prefix length: %s", arg); +bad: + die("Invalid guest address: %s", arg); } /** diff --git a/inany.c b/inany.c index 154f08b5..120c9387 100644 --- a/inany.c +++ b/inany.c @@ -70,53 +70,3 @@ const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size) return inet_ntop(AF_INET6, &src->a6, dst, size); } - -/** - * inany_prefix_pton() - Parse an IPv[46] address with prefix length - * @src: IPv[46] address and prefix length string in CIDR format - * @dst: Output buffer, filled with parsed address - * @prefix_len: Prefix length, to be filled in IPv6 format - * - * Return: 1 on success, 0 if no parseable address or prefix is found - */ -int inany_prefix_pton(const char *src, union inany_addr *dst, - uint8_t *prefix_len) -{ - char astr[INANY_ADDRSTRLEN] = { 0 }; - size_t alen = strcspn(src, "/"); - const char *pstr = &src[alen + 1]; - const char *p = astr; - unsigned long plen; - char *end; - - if (alen >= INANY_ADDRSTRLEN) - return 0; - - if (src[alen] != '/') - return 0; - - strncpy(astr, src, alen); - - /* Read prefix length */ - errno = 0; - plen = strtoul(pstr, &end, 10); - if (errno || *end || plen > 128) - return 0; - - /* Read address */ - if (inet_pton(AF_INET6, astr, dst)) { - if (inany_v4(dst) && plen < 96) - return 0; - *prefix_len = plen; - return 1; - } - - if (parse_inany(&p, dst) && parse_eoi(p)) { - if (plen > 32) - return 0; - *prefix_len = plen + 96; - return 1; - } - - return 0; -} diff --git a/inany.h b/inany.h index 93d98368..5b176ccf 100644 --- a/inany.h +++ b/inany.h @@ -303,7 +303,5 @@ static inline int inany_from_sockaddr(union inany_addr *dst, in_port_t *port, bool inany_matches(const union inany_addr *a, const union inany_addr *b); const char *inany_ntop(const union inany_addr *src, char *dst, socklen_t size); -int inany_prefix_pton(const char *src, union inany_addr *dst, - uint8_t *prefix_len); #endif /* INANY_H */ diff --git a/parse.c b/parse.c index c8d3a9b0..24178873 100644 --- a/parse.c +++ b/parse.c @@ -185,18 +185,29 @@ static bool parse_ipv6(const char **cursor, struct in6_addr *abuf) } /** - * parse_inany() - Parse an IPv4 or IPv6 address from a string + * parse_inany_() - Parse an IPv4 or IPv6 address from a string * @addr: On success, updated with parsed address + * @parse_af: On success, updated with the format of the parsed address + * + * @parseaf is updated to reflect the string format, not the final address + * family. So "::ffff:192.0.1.1", will set @parseaf to AF_INET6, despite being + * a IPv4-mapped address. */ -bool parse_inany(const char **cursor, union inany_addr *addr) +bool parse_inany_(const char **cursor, union inany_addr *addr, + sa_family_t *parse_af) { struct in_addr a4; - if (parse_ipv6(cursor, &addr->a6)) + if (parse_ipv6(cursor, &addr->a6)) { + if (parse_af) + *parse_af = AF_INET6; return true; + } if (parse_ipv4(cursor, &a4)) { *addr = inany_from_v4(a4); + if (parse_af) + *parse_af = AF_INET; return true; } diff --git a/parse.h b/parse.h index 2820a065..ab1d5adb 100644 --- a/parse.h +++ b/parse.h @@ -27,6 +27,9 @@ bool parse_eoi(const char *cursor); bool parse_unsigned(const char **cursor, int base, unsigned long *valp); bool parse_port_range(const char **cursor, struct port_range *range); bool parse_ipv4(const char **cursor, struct in_addr *abuf); -bool parse_inany(const char **cursor, union inany_addr *addr); +bool parse_inany_(const char **cursor, union inany_addr *addr, + sa_family_t *parse_af); + +#define parse_inany(cursor, addr) parse_inany_((cursor), (addr), NULL) #endif /* _PARSE_H */ -- 2.54.0

parse_port_range() in fwd_rule.c takes an approach similar to that used by the new parsing helpers in parse.c (and is partially inspiration for it), but isn't quite the same. Move it into parse.c, and bring it into line with that file's conventions. Signed-off-by: David Gibson --- common.h | 3 +++ fwd_rule.c | 53 +++-------------------------------------------------- fwd_rule.h | 2 -- parse.c | 28 ++++++++++++++++++++++++++++ parse.h | 13 +++++++++++++ 5 files changed, 47 insertions(+), 52 deletions(-) diff --git a/common.h b/common.h index f2518f66..16b9dc8c 100644 --- a/common.h +++ b/common.h @@ -113,4 +113,7 @@ static inline const char *strerror_(int errnum) #define ntohll(x) (be64toh((x))) #define htonll(x) (htobe64((x))) +/* Number of ports for both TCP and UDP */ +#define NUM_PORTS (1U << 16) + #endif /* COMMON_H */ diff --git a/fwd_rule.c b/fwd_rule.c index d4b422d7..0b85b17e 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -367,53 +367,6 @@ int fwd_rule_add(struct fwd_table *fwd, const struct fwd_rule *new) return 0; } -/** - * port_range() - Represents a non-empty range of ports - * @first: First port number in the range - * @last: Last port number in the range (inclusive) - * - * Invariant: @last >= @first - */ -struct port_range { - in_port_t first, last; -}; - -/** - * parse_port_range() - Parse a range of port numbers '<first>[-<last>]' - * @s: String to parse - * @endptr: Update to the character after the parsed range (similar to - * strtol() etc.) - * @range: Update with the parsed values on success - * - * Return: -EINVAL on parsing error, -ERANGE on out of range port - * numbers, 0 on success - */ -static int parse_port_range(const char *s, const char **endptr, - struct port_range *range) -{ - unsigned long first, last; - char *ep; - - last = first = strtoul(s, &ep, 10); - if (ep == s) /* Parsed nothing */ - return -EINVAL; - if (*ep == '-') { /* we have a last value too */ - const char *lasts = ep + 1; - last = strtoul(lasts, &ep, 10); - if (ep == lasts) /* Parsed nothing */ - return -EINVAL; - } - - if ((last < first) || (last >= NUM_PORTS)) - return -ERANGE; - - range->first = first; - range->last = last; - *endptr = ep; - - return 0; -} - /** * fwd_rule_range_except() - Set up forwarding for a range of ports minus a * bitmap of exclusions @@ -550,7 +503,7 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, if (!parse_literal(&p, "~")) goto bad; - if (parse_port_range(p, &p, &xrange)) + if (!parse_port_range(&p, &xrange)) goto bad; if (p != ep) /* Garbage after the range */ goto bad; @@ -577,12 +530,12 @@ static void fwd_rule_parse_ports(struct fwd_table *fwd, bool del, uint8_t proto, /* Already parsed */ continue; - if (parse_port_range(p, &p, &orig_range)) + if (!parse_port_range(&p, &orig_range)) goto bad; if (parse_literal(&p, ":")) { /* There's a range to map to as well */ - if (parse_port_range(p, &p, &mapped_range)) + if (!parse_port_range(&p, &mapped_range)) goto bad; if ((mapped_range.last - mapped_range.first) != (orig_range.last - orig_range.first)) diff --git a/fwd_rule.h b/fwd_rule.h index ae9a3cb0..435be5bd 100644 --- a/fwd_rule.h +++ b/fwd_rule.h @@ -18,8 +18,6 @@ #include "inany.h" #include "bitmap.h" -/* Number of ports for both TCP and UDP */ -#define NUM_PORTS (1U << 16) #define PORT_BITMAP_SIZE DIV_ROUND_UP(NUM_PORTS, 8) /* Forwarding capability bits */ diff --git a/parse.c b/parse.c index 9bd0e81f..ad210406 100644 --- a/parse.c +++ b/parse.c @@ -17,6 +17,7 @@ #include #include +#include "common.h" #include "parse.h" /** @@ -83,3 +84,30 @@ bool parse_unsigned(const char **cursor, int base, unsigned long *valp) *cursor = p; return true; } + +/** + * parse_port_range() - Parse a range of port numbers '<first>[-<last>]' + * @range: Update with the parsed values on success + */ +bool parse_port_range(const char **cursor, struct port_range *range) +{ + unsigned long first, last; + const char *p = *cursor; + + if (!parse_unsigned(&p, 10, &first)) + return false; + + last = first; + + if (parse_literal(&p, "-")) + if (!parse_unsigned(&p, 10, &last)) + return false; + + if ((last < first) || (last >= NUM_PORTS)) + return false; + + range->first = first; + range->last = last; + *cursor = p; + return true; +} diff --git a/parse.h b/parse.h index 4f38c28b..155b3995 100644 --- a/parse.h +++ b/parse.h @@ -7,9 +7,22 @@ #define PARSE_H #include +#include + +/** + * port_range() - Represents a non-empty range of ports + * @first: First port number in the range + * @last: Last port number in the range (inclusive) + * + * Invariant: @last >= @first + */ +struct port_range { + in_port_t first, last; +}; bool parse_literal(const char **cursor, const char *lit); bool parse_eoi(const char *cursor); bool parse_unsigned(const char **cursor, int base, unsigned long *valp); +bool parse_port_range(const char **cursor, struct port_range *range); #endif /* _PARSE_H */ -- 2.54.0

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 delimiters 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 | 268 +++++++++++++++++++++++++++++++---------------------- parse.c | 29 ++++++ parse.h | 2 + 3 files changed, 187 insertions(+), 112 deletions(-) diff --git a/fwd_rule.c b/fwd_rule.c index b14df340..ca409eaf 100644 --- a/fwd_rule.c +++ b/fwd_rule.c @@ -439,17 +439,102 @@ 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 + * @CHUNK_ALL "all" + * @CHUNK_AUTO "auto" + * @CHUNK_EXCLUDE "~1111[-2222]" + * @CHUNK_INCLUDE "1111[-2222][:3333[-4444]]" + */ +enum fwd_port_chunk_kind { + CHUNK_ALL, + CHUNK_AUTO, + CHUNK_EXCLUDE, + CHUNK_INCLUDE, +}; + +/** + * 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 INCLUDE) + */ +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_addrifname() - Parse 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_addrifname(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 +551,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 +637,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 +650,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 +663,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_addrifname(&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 +684,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 +704,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 24178873..73f8ffb4 100644 --- a/parse.c +++ b/parse.c @@ -18,6 +18,7 @@ #include #include #include +#include #include "common.h" #include "parse.h" @@ -213,3 +214,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 ab1d5adb..257b9c58 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