cppcheck 2.14.1 complains about the rport variable not being in as small as scope as it could be. It's also only used once, so we might as well just open code the calculation for it. Signed-off-by: David Gibson --- udp.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/udp.c b/udp.c index 3abafc99..61e106a9 100644 --- a/udp.c +++ b/udp.c @@ -277,10 +277,9 @@ static void udp_invert_portmap(struct udp_fwd_ports *fwd) "Forward and reverse delta arrays must have same size"); for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) { in_port_t delta = fwd->f.delta[i]; - in_port_t rport = i + delta; if (delta) - fwd->rdelta[rport] = NUM_PORTS - delta; + fwd->rdelta[i + delta] = NUM_PORTS - delta; } } -- 2.45.2

We globally disabled this, with a justification lumped together with several checks about braces. They don't really go together, the others are essentially a stylistic choice which doesn't match our style. Omitting brackets on macro parameters can lead to real and hard to track down bugs if an expression is ever passed to the macro instead of a plain identifier. We've only gotten away with the macros which trigger the warning, because of other conventions its been unlikely to invoke them with anything other than a simple identifier. Fix the macros, and enable the warning for the future. Signed-off-by: David Gibson --- Makefile | 2 -- tap.c | 37 +++++++++++++++++++------------------ tcp.c | 8 ++++---- tcp_splice.c | 4 ++-- 4 files changed, 25 insertions(+), 26 deletions(-) diff --git a/Makefile b/Makefile index 22f05813..4d00f48c 100644 --- a/Makefile +++ b/Makefile @@ -192,7 +192,6 @@ docs: README.md # - llvmlibc-restrict-system-libc-headers # TODO: this is Linux-only for the moment, nice to fix eventually # -# - bugprone-macro-parentheses # - google-readability-braces-around-statements # - hicpp-braces-around-statements # - readability-braces-around-statements @@ -269,7 +268,6 @@ clang-tidy: $(SRCS) $(HEADERS) -clang-analyzer-valist.Uninitialized,\ -cppcoreguidelines-init-variables,\ -bugprone-assignment-in-if-condition,\ - -bugprone-macro-parentheses,\ -google-readability-braces-around-statements,\ -hicpp-braces-around-statements,\ -readability-braces-around-statements,\ diff --git a/tap.c b/tap.c index 2ea08491..26084b48 100644 --- a/tap.c +++ b/tap.c @@ -674,18 +674,18 @@ resume: continue; } -#define L4_MATCH(iph, uh, seq) \ - (seq->protocol == iph->protocol && \ - seq->source == uh->source && seq->dest == uh->dest && \ - seq->saddr.s_addr == iph->saddr && seq->daddr.s_addr == iph->daddr) +#define L4_MATCH(iph, uh, seq) \ + ((seq)->protocol == (iph)->protocol && \ + (seq)->source == (uh)->source && (seq)->dest == (uh)->dest && \ + (seq)->saddr.s_addr == (iph)->saddr && (seq)->daddr.s_addr == (iph)->daddr) #define L4_SET(iph, uh, seq) \ do { \ - seq->protocol = iph->protocol; \ - seq->source = uh->source; \ - seq->dest = uh->dest; \ - seq->saddr.s_addr = iph->saddr; \ - seq->daddr.s_addr = iph->daddr; \ + (seq)->protocol = (iph)->protocol; \ + (seq)->source = (uh)->source; \ + (seq)->dest = (uh)->dest; \ + (seq)->saddr.s_addr = (iph)->saddr; \ + (seq)->daddr.s_addr = (iph)->daddr; \ } while (0) if (seq && L4_MATCH(iph, uh, seq) && seq->p.count < UIO_MAXIOV) @@ -848,18 +848,19 @@ resume: } #define L4_MATCH(ip6h, proto, uh, seq) \ - (seq->protocol == proto && \ - seq->source == uh->source && seq->dest == uh->dest && \ - IN6_ARE_ADDR_EQUAL(&seq->saddr, saddr) && \ - IN6_ARE_ADDR_EQUAL(&seq->daddr, daddr)) + ((seq)->protocol == (proto) && \ + (seq)->source == (uh)->source && \ + (seq)->dest == (uh)->dest && \ + IN6_ARE_ADDR_EQUAL(&(seq)->saddr, saddr) && \ + IN6_ARE_ADDR_EQUAL(&(seq)->daddr, daddr)) #define L4_SET(ip6h, proto, uh, seq) \ do { \ - seq->protocol = proto; \ - seq->source = uh->source; \ - seq->dest = uh->dest; \ - seq->saddr = *saddr; \ - seq->daddr = *daddr; \ + (seq)->protocol = (proto); \ + (seq)->source = (uh)->source; \ + (seq)->dest = (uh)->dest; \ + (seq)->saddr = *saddr; \ + (seq)->daddr = *daddr; \ } while (0) if (seq && L4_MATCH(ip6h, proto, uh, seq) && diff --git a/tcp.c b/tcp.c index ff1198dd..0dccde9c 100644 --- a/tcp.c +++ b/tcp.c @@ -326,7 +326,7 @@ #define WINDOW_DEFAULT 14600 /* RFC 6928 */ #ifdef HAS_SND_WND -# define KERNEL_REPORTS_SND_WND(c) (c->tcp.kernel_snd_wnd) +# define KERNEL_REPORTS_SND_WND(c) ((c)->tcp.kernel_snd_wnd) #else # define KERNEL_REPORTS_SND_WND(c) (0 && (c)) #endif @@ -373,9 +373,9 @@ #define CONN_V4(conn) (!!inany_v4(&(conn)->faddr)) #define CONN_V6(conn) (!CONN_V4(conn)) #define CONN_IS_CLOSING(conn) \ - ((conn->events & ESTABLISHED) && \ - (conn->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD))) -#define CONN_HAS(conn, set) ((conn->events & (set)) == (set)) + (((conn)->events & ESTABLISHED) && \ + ((conn)->events & (SOCK_FIN_RCVD | TAP_FIN_RCVD))) +#define CONN_HAS(conn, set) (((conn)->events & (set)) == (set)) static const char *tcp_event_str[] __attribute((__unused__)) = { "SOCK_ACCEPTED", "TAP_SYN_RCVD", "ESTABLISHED", "TAP_SYN_ACK_SENT", diff --git a/tcp_splice.c b/tcp_splice.c index b8f64a95..3f9d395a 100644 --- a/tcp_splice.c +++ b/tcp_splice.c @@ -73,9 +73,9 @@ static int ns_sock_pool6 [TCP_SOCK_POOL_SIZE]; /* Pool of pre-opened pipes */ static int splice_pipe_pool [TCP_SPLICE_PIPE_POOL_SIZE][2]; -#define CONN_V6(x) (x->flags & SPLICE_V6) +#define CONN_V6(x) ((x)->flags & SPLICE_V6) #define CONN_V4(x) (!CONN_V6(x)) -#define CONN_HAS(conn, set) ((conn->events & (set)) == (set)) +#define CONN_HAS(conn, set) (((conn)->events & (set)) == (set)) #define CONN(idx) (&FLOW(idx)->tcp_splice) /* Display strings for connection events */ -- 2.45.2

In conf() we parse a MAC address in two places, for the --ns-mac-addr and the -M options. As well as duplicating code, the logic for this parsing has several bugs: * The most serious is that if the given string is shorter than a MAC address should be, we'll access past the end of it. * We don't check the endptr supplied by strtol() which means we could ignore certain erroneous contents * We never check the separator characters between each octet * We ignore certain sorts of garbage that follow the MAC address Correct all these bugs in a new parse_mac() helper. Signed-off-by: David Gibson --- conf.c | 53 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 36 insertions(+), 17 deletions(-) diff --git a/conf.c b/conf.c index 72ca1fc8..fdeb9db7 100644 --- a/conf.c +++ b/conf.c @@ -1124,6 +1124,39 @@ static void conf_open_files(struct ctx *c) c->pidfile_fd = pidfile_open(c->pidfile); } +/** + * parse_mac - Parse a MAC address from a string + * @mac: Binary MAC address, initialised on success + * @str: String to parse + * + * Parses @str as an Ethernet MAC address stored in @mac on success. Exits on + * failure. + */ +static void parse_mac(unsigned char mac[ETH_ALEN], const char *str) +{ + size_t i; + + if (strlen(str) != (ETH_ALEN * 3 - 1)) + goto fail; + + for (i = 0; i < ETH_ALEN; i++) { + const char *octet = str + 3 * i; + unsigned long b; + char *end; + + errno = 0; + b = strtoul(octet, &end, 16); + if (b > UCHAR_MAX || errno || end != octet + 2 || + *end != ((i == ETH_ALEN - 1) ? '\0' : ':')) + goto fail; + mac[i] = b; + } + return; + +fail: + die("Invalid MAC address: %s", str); +} + /** * conf() - Process command-line arguments and set configuration * @c: Execution context @@ -1200,9 +1233,9 @@ void conf(struct ctx *c, int argc, char **argv) unsigned int ifi4 = 0, ifi6 = 0; const char *logfile = NULL; const char *optstring; - int name, ret, b, i; size_t logsize = 0; char *runas = NULL; + int name, ret; uid_t uid; gid_t gid; @@ -1243,14 +1276,7 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode != MODE_PASTA) die("--ns-mac-addr is for pasta mode only"); - for (i = 0; i < ETH_ALEN; i++) { - errno = 0; - b = strtol(optarg + (intptr_t)i * 3, NULL, 16); - if (b < 0 || b > UCHAR_MAX || errno) - die("Invalid MAC address: %s", optarg); - - c->mac_guest[i] = b; - } + parse_mac(c->mac_guest, optarg); break; case 5: if (c->mode != MODE_PASTA) @@ -1510,14 +1536,7 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'M': - for (i = 0; i < ETH_ALEN; i++) { - errno = 0; - b = strtol(optarg + (intptr_t)i * 3, NULL, 16); - if (b < 0 || b > UCHAR_MAX || errno) - die("Invalid MAC address: %s", optarg); - - c->mac[i] = b; - } + parse_mac(c->mac, optarg); break; case 'g': if (c->mode == MODE_PASTA) -- 2.45.2

timespec_diff_ms() returns an int representing a duration in milliseconds. This will overflow in about 25 days when an int is 32 bits. The way we use this function, we're probably not going to get a result that long, but it's not outrageously implausible. Use a long for safety. Signed-off-by: David Gibson --- util.c | 2 +- util.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/util.c b/util.c index 5e854a26..44461801 100644 --- a/util.c +++ b/util.c @@ -216,7 +216,7 @@ void sock_probe_mem(struct ctx *c) * * Return: difference in milliseconds */ -int timespec_diff_ms(const struct timespec *a, const struct timespec *b) +long timespec_diff_ms(const struct timespec *a, const struct timespec *b) { if (a->tv_nsec < b->tv_nsec) { return (b->tv_nsec - a->tv_nsec) / 1000000 + diff --git a/util.h b/util.h index cf9c4b66..eebb027b 100644 --- a/util.h +++ b/util.h @@ -147,7 +147,7 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto, const void *bind_addr, const char *ifname, uint16_t port, uint32_t data); void sock_probe_mem(struct ctx *c); -int timespec_diff_ms(const struct timespec *a, const struct timespec *b); +long timespec_diff_ms(const struct timespec *a, const struct timespec *b); void bitmap_set(uint8_t *map, unsigned bit); void bitmap_clear(uint8_t *map, unsigned bit); bool bitmap_isset(const uint8_t *map, unsigned bit); -- 2.45.2