[PATCH 00/16] Fix issues reported by Coverity

[PATCH 00/16] Fix issues reported by Coverity

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

Most of these are formal issues with no actual effect, some are false
positives, but it looks sensible to fix all of them and there's also an
interesting finding in udp_timer().

Stefano Brivio (16):
  treewide: Invalid type in argument to printf format specifier, CWE-686
  passt: Ignoring number of bytes read, CWE-252
  tcp: False "Untrusted loop bound" positive, CWE-606
  treewide: Unchecked return value from library, CWE-252
  tap: Resource leak, CWE-404
  conf, packet: Operands don't affect result, CWE-569
  passt: Improper use of negative value (CWE-394)
  treewide: Argument cannot be negative, CWE-687
  conf: False "Assign instead of compare" positive, CWE-481
  conf, tap: False "Buffer not null terminated" positives, CWE-170
  tcp: Dereference null return value, CWE-476
  tcp_splice: Logically dead code, CWE-561
  tcp, tcp_splice: False "Negative array index read" positives, CWE-129
  tcp: False "Out-of-bounds read" positive, CWE-125
  udp: Out-of-bounds read, CWE-125 in udp_timer()
  arch: Pointer to local outside scope, CWE-562

 arch.c       | 10 +++---
 conf.c       | 15 +++++----
 icmp.c       | 13 +++++---
 netlink.c    | 40 ++++++++++++++---------
 packet.c     |  8 ++---
 passt.c      | 24 ++++++++++----
 pasta.c      | 25 +++++----------
 pcap.c       |  6 ++--
 qrap.c       | 15 ++++++---
 tap.c        | 35 +++++++++++++-------
 tcp.c        | 75 ++++++++++++++++++++++++++-----------------
 tcp_splice.c | 91 ++++++++++++++++++++++++++++++++++------------------
 udp.c        |  5 +--
 util.c       | 11 ++++---
 util.h       |  9 ++++++
 15 files changed, 238 insertions(+), 144 deletions(-)

-- 
2.35.1

[PATCH 01/16] treewide: Invalid type in argument to printf format specifier, CWE-686

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 10273 bytes --]

Harmless except for two bad debugging prints.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 packet.c     |  6 +++---
 pcap.c       |  6 +++---
 tcp.c        | 38 +++++++++++++++++++-------------------
 tcp_splice.c | 14 +++++++-------
 4 files changed, 32 insertions(+), 32 deletions(-)

diff --git a/packet.c b/packet.c
index d003640..fa9e9b4 100644
--- a/packet.c
+++ b/packet.c
@@ -53,12 +53,12 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 	}
 
 	if (len > UINT16_MAX) {
-		trace("add packet length %lu, %s:%i", func, line);
+		trace("add packet length %lu, %s:%i", len, func, line);
 		return;
 	}
 
 	if ((unsigned int)((intptr_t)start - (intptr_t)p->buf) > UINT32_MAX) {
-		trace("add packet start %p, buffer start %lu, %s:%i",
+		trace("add packet start %p, buffer start %p, %s:%i",
 		      start, p->buf, func, line);
 		return;
 	}
@@ -111,7 +111,7 @@ void *packet_get_do(const struct pool *p, size_t index, size_t offset,
 
 	if (len + offset > p->pkt[index].len) {
 		if (func) {
-			trace("data length %lu, offset %lu from length %lu, "
+			trace("data length %lu, offset %lu from length %u, "
 			      "%s:%i", len, offset, p->pkt[index].len,
 			      func, line);
 		}
diff --git a/pcap.c b/pcap.c
index 296bbb5..64beb34 100644
--- a/pcap.c
+++ b/pcap.c
@@ -88,7 +88,7 @@ void pcap(const char *pkt, size_t len)
 	h.caplen = h.len = len;
 
 	if (write(pcap_fd, &h, sizeof(h)) < 0 || write(pcap_fd, pkt, len) < 0)
-		debug("Cannot log packet, length %u", len);
+		debug("Cannot log packet, length %lu", len);
 }
 
 /**
@@ -123,7 +123,7 @@ void pcapm(const struct msghdr *mh)
 
 	return;
 fail:
-	debug("Cannot log packet, length %u", iov->iov_len - 4);
+	debug("Cannot log packet, length %lu", iov->iov_len - 4);
 }
 
 /**
@@ -161,7 +161,7 @@ void pcapmm(const struct mmsghdr *mmh, unsigned int vlen)
 	}
 	return;
 fail:
-	debug("Cannot log packet, length %u", iov->iov_len - 4);
+	debug("Cannot log packet, length %lu", iov->iov_len - 4);
 }
 
 /**
diff --git a/tcp.c b/tcp.c
index 2194067..1409c53 100644
--- a/tcp.c
+++ b/tcp.c
@@ -848,7 +848,7 @@ static void tcp_timer_ctl(const struct ctx *c, struct tcp_conn *conn)
 		it.it_value.tv_sec = ACT_TIMEOUT;
 	}
 
-	debug("TCP: index %i, timer expires in %u.%03us", conn - tc,
+	debug("TCP: index %li, timer expires in %lu.%03lus", conn - tc,
 	      it.it_value.tv_sec, it.it_value.tv_nsec / 1000 / 1000);
 
 	timerfd_settime(conn->timer, 0, &it, NULL);
@@ -868,14 +868,14 @@ static void conn_flag_do(const struct ctx *c, struct tcp_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP: index %i: %s dropped", (conn) - tc,
+		debug("TCP: index %li: %s dropped", conn - tc,
 		      tcp_flag_str[fls(~flag)]);
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP: index %i: %s", (conn) - tc,
+		debug("TCP: index %li: %s", conn - tc,
 		      tcp_flag_str[fls(flag)]);
 	}
 
@@ -924,12 +924,12 @@ static void conn_event_do(const struct ctx *c, struct tcp_conn *conn,
 		new += 5;
 
 	if (prev != new) {
-		debug("TCP: index %i, %s: %s -> %s", (conn) - tc,
+		debug("TCP: index %li, %s: %s -> %s", conn - tc,
 		      num == -1 	       ? "CLOSED" : tcp_event_str[num],
 		      prev == -1	       ? "CLOSED" : tcp_state_str[prev],
 		      (new == -1 || num == -1) ? "CLOSED" : tcp_state_str[new]);
 	} else {
-		debug("TCP: index %i, %s", (conn) - tc,
+		debug("TCP: index %li, %s", conn - tc,
 		      num == -1 	       ? "CLOSED" : tcp_event_str[num]);
 	}
 
@@ -1371,8 +1371,8 @@ static void tcp_hash_insert(const struct ctx *c, struct tcp_conn *conn,
 	tc_hash[b] = conn;
 	conn->hash_bucket = b;
 
-	debug("TCP: hash table insert: index %i, sock %i, bucket: %i, next: %p",
-	      conn - tc, conn->sock, b, CONN_OR_NULL(conn->next_index));
+	debug("TCP: hash table insert: index %li, sock %i, bucket: %i, next: "
+	      "%p", conn - tc, conn->sock, b, CONN_OR_NULL(conn->next_index));
 }
 
 /**
@@ -1395,7 +1395,7 @@ static void tcp_hash_remove(const struct tcp_conn *conn)
 		}
 	}
 
-	debug("TCP: hash table remove: index %i, sock %i, bucket: %i, new: %p",
+	debug("TCP: hash table remove: index %li, sock %i, bucket: %i, new: %p",
 	      conn - tc, conn->sock, b,
 	      prev ? CONN_OR_NULL(prev->next_index) : tc_hash[b]);
 }
@@ -1421,7 +1421,7 @@ static void tcp_hash_update(struct tcp_conn *old, struct tcp_conn *new)
 		}
 	}
 
-	debug("TCP: hash table update: old index %i, new index %i, sock %i, "
+	debug("TCP: hash table update: old index %li, new index %li, sock %i, "
 	      "bucket: %i, old: %p, new: %p",
 	      old - tc, new - tc, new->sock, b, old, new);
 }
@@ -1461,7 +1461,7 @@ static void tcp_table_compact(struct ctx *c, struct tcp_conn *hole)
 	struct tcp_conn *from, *to;
 
 	if ((hole - tc) == --c->tcp.conn_count) {
-		debug("TCP: hash table compaction: index %i (%p) was max index",
+		debug("TCP: hash table compaction: maximum index was %li (%p)",
 		      hole - tc, hole);
 		memset(hole, 0, sizeof(*hole));
 		return;
@@ -1475,7 +1475,7 @@ static void tcp_table_compact(struct ctx *c, struct tcp_conn *hole)
 
 	tcp_epoll_ctl(c, to);
 
-	debug("TCP: hash table compaction: old index %i, new index %i, "
+	debug("TCP: hash table compaction: old index %li, new index %li, "
 	      "sock %i, from: %p, to: %p",
 	      from - tc, to - tc, from->sock, from, to);
 
@@ -1500,7 +1500,7 @@ static void tcp_conn_destroy(struct ctx *c, struct tcp_conn *conn)
 static void tcp_rst_do(struct ctx *c, struct tcp_conn *conn);
 #define tcp_rst(c, conn)						\
 	do {								\
-		debug("TCP: index %i, reset at %s:%i", conn - tc,	\
+		debug("TCP: index %li, reset at %s:%i", conn - tc,	\
 		      __func__, __LINE__);				\
 		tcp_rst_do(c, conn);					\
 	} while (0)
@@ -2357,7 +2357,7 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_conn *conn)
 
 	if (SEQ_LT(already_sent, 0)) {
 		/* RFC 761, section 2.1. */
-		trace("TCP: ACK sequence gap: ACK for %lu, sent: %lu",
+		trace("TCP: ACK sequence gap: ACK for %u, sent: %u",
 		      conn->seq_ack_from_tap, conn->seq_to_tap);
 		conn->seq_to_tap = conn->seq_ack_from_tap;
 		already_sent = 0;
@@ -2589,7 +2589,7 @@ static void tcp_data_from_tap(struct ctx *c, struct tcp_conn *conn,
 	}
 
 	if (retr) {
-		trace("TCP: fast re-transmit, ACK: %lu, previous sequence: %lu",
+		trace("TCP: fast re-transmit, ACK: %u, previous sequence: %u",
 		      max_ack_seq, conn->seq_to_tap);
 		conn->seq_ack_from_tap = max_ack_seq;
 		conn->seq_to_tap = max_ack_seq;
@@ -2956,17 +2956,17 @@ static void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 		conn_flag(c, conn, ~ACK_TO_TAP_DUE);
 	} else if (conn->flags & ACK_FROM_TAP_DUE) {
 		if (!(conn->events & ESTABLISHED)) {
-			debug("TCP: index %i, handshake timeout", conn - tc);
+			debug("TCP: index %li, handshake timeout", conn - tc);
 			tcp_rst(c, conn);
 		} else if (CONN_HAS(conn, SOCK_FIN_SENT | TAP_FIN_ACKED)) {
-			debug("TCP: index %i, FIN timeout", conn - tc);
+			debug("TCP: index %li, FIN timeout", conn - tc);
 			tcp_rst(c, conn);
 		} else if (conn->retrans == TCP_MAX_RETRANS) {
-			debug("TCP: index %i, maximum retransmissions exceeded",
+			debug("TCP: index %li, retransmissions count exceeded",
 			      conn - tc);
 			tcp_rst(c, conn);
 		} else {
-			debug("TCP: index %i, ACK timeout, retry", conn - tc);
+			debug("TCP: index %li, ACK timeout, retry", conn - tc);
 			conn->retrans++;
 			conn->seq_to_tap = conn->seq_ack_from_tap;
 			tcp_data_from_sock(c, conn);
@@ -2984,7 +2984,7 @@ static void tcp_timer_handler(struct ctx *c, union epoll_ref ref)
 		 */
 		timerfd_settime(conn->timer, 0, &new, &old);
 		if (old.it_value.tv_sec == ACT_TIMEOUT) {
-			debug("TCP: index %i, activity timeout", conn - tc);
+			debug("TCP: index %li, activity timeout", conn - tc);
 			tcp_rst(c, conn);
 		}
 	}
diff --git a/tcp_splice.c b/tcp_splice.c
index 3f2ef2e..24a3b4b 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -173,14 +173,14 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP (spliced): index %i: %s dropped", (conn) - tc,
+		debug("TCP (spliced): index %li: %s dropped", conn - tc,
 		      tcp_splice_flag_str[fls(~flag)]);
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP (spliced): index %i: %s", (conn) - tc,
+		debug("TCP (spliced): index %li: %s", conn - tc,
 		      tcp_splice_flag_str[fls(flag)]);
 	}
 
@@ -253,14 +253,14 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->events &= event;
-		debug("TCP (spliced): index %i, ~%s", conn - tc,
+		debug("TCP (spliced): index %li, ~%s", conn - tc,
 		      tcp_splice_event_str[fls(~event)]);
 	} else {
 		if (conn->events & event)
 			return;
 
 		conn->events |= event;
-		debug("TCP (spliced): index %i, %s", conn - tc,
+		debug("TCP (spliced): index %li, %s", conn - tc,
 		      tcp_splice_event_str[fls(event)]);
 	}
 
@@ -286,7 +286,7 @@ static void tcp_table_splice_compact(struct ctx *c,
 	struct tcp_splice_conn *move;
 
 	if ((hole - tc) == --c->tcp.splice_conn_count) {
-		debug("TCP (spliced): index %i (max) removed", hole - tc);
+		debug("TCP (spliced): index %li (max) removed", hole - tc);
 		return;
 	}
 
@@ -300,7 +300,7 @@ static void tcp_table_splice_compact(struct ctx *c,
 	move->pipe_b_a[0] = move->pipe_b_a[1] = -1;
 	move->flags = move->events = 0;
 
-	debug("TCP (spliced): index %i moved to %i", move - tc, hole - tc);
+	debug("TCP (spliced): index %li moved to %li", move - tc, hole - tc);
 	tcp_splice_epoll_ctl(c, hole);
 	if (tcp_splice_epoll_ctl(c, hole))
 		conn_flag(c, hole, CLOSING);
@@ -338,7 +338,7 @@ static void tcp_splice_destroy(struct ctx *c, struct tcp_splice_conn *conn)
 
 	conn->events = CLOSED;
 	conn->flags = 0;
-	debug("TCP (spliced): index %i, CLOSED", conn - tc);
+	debug("TCP (spliced): index %li, CLOSED", conn - tc);
 
 	tcp_table_splice_compact(c, conn);
 }
-- 
@@ -173,14 +173,14 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP (spliced): index %i: %s dropped", (conn) - tc,
+		debug("TCP (spliced): index %li: %s dropped", conn - tc,
 		      tcp_splice_flag_str[fls(~flag)]);
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP (spliced): index %i: %s", (conn) - tc,
+		debug("TCP (spliced): index %li: %s", conn - tc,
 		      tcp_splice_flag_str[fls(flag)]);
 	}
 
@@ -253,14 +253,14 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->events &= event;
-		debug("TCP (spliced): index %i, ~%s", conn - tc,
+		debug("TCP (spliced): index %li, ~%s", conn - tc,
 		      tcp_splice_event_str[fls(~event)]);
 	} else {
 		if (conn->events & event)
 			return;
 
 		conn->events |= event;
-		debug("TCP (spliced): index %i, %s", conn - tc,
+		debug("TCP (spliced): index %li, %s", conn - tc,
 		      tcp_splice_event_str[fls(event)]);
 	}
 
@@ -286,7 +286,7 @@ static void tcp_table_splice_compact(struct ctx *c,
 	struct tcp_splice_conn *move;
 
 	if ((hole - tc) == --c->tcp.splice_conn_count) {
-		debug("TCP (spliced): index %i (max) removed", hole - tc);
+		debug("TCP (spliced): index %li (max) removed", hole - tc);
 		return;
 	}
 
@@ -300,7 +300,7 @@ static void tcp_table_splice_compact(struct ctx *c,
 	move->pipe_b_a[0] = move->pipe_b_a[1] = -1;
 	move->flags = move->events = 0;
 
-	debug("TCP (spliced): index %i moved to %i", move - tc, hole - tc);
+	debug("TCP (spliced): index %li moved to %li", move - tc, hole - tc);
 	tcp_splice_epoll_ctl(c, hole);
 	if (tcp_splice_epoll_ctl(c, hole))
 		conn_flag(c, hole, CLOSING);
@@ -338,7 +338,7 @@ static void tcp_splice_destroy(struct ctx *c, struct tcp_splice_conn *conn)
 
 	conn->events = CLOSED;
 	conn->flags = 0;
-	debug("TCP (spliced): index %i, CLOSED", conn - tc);
+	debug("TCP (spliced): index %li, CLOSED", conn - tc);
 
 	tcp_table_splice_compact(c, conn);
 }
-- 
2.35.1

[PATCH 02/16] passt: Ignoring number of bytes read, CWE-252

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

Harmless, assuming sane kernel behaviour. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 passt.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/passt.c b/passt.c
index c469fe8..06c3d73 100644
--- a/passt.c
+++ b/passt.c
@@ -195,6 +195,7 @@ static void seccomp(const struct ctx *c)
  */
 static void check_root(void)
 {
+	const char root_uid_map[] = "         0          0 4294967295";
 	struct passwd *pw;
 	char buf[BUFSIZ];
 	int fd;
@@ -205,8 +206,8 @@ static void check_root(void)
 	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
 		return;
 
-	if (read(fd, buf, BUFSIZ) > 0 &&
-	    strcmp(buf, "         0          0 4294967295")) {
+	if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
+	    strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
 		close(fd);
 		return;
 	}
-- 
@@ -195,6 +195,7 @@ static void seccomp(const struct ctx *c)
  */
 static void check_root(void)
 {
+	const char root_uid_map[] = "         0          0 4294967295";
 	struct passwd *pw;
 	char buf[BUFSIZ];
 	int fd;
@@ -205,8 +206,8 @@ static void check_root(void)
 	if ((fd = open("/proc/self/uid_map", O_RDONLY | O_CLOEXEC)) < 0)
 		return;
 
-	if (read(fd, buf, BUFSIZ) > 0 &&
-	    strcmp(buf, "         0          0 4294967295")) {
+	if (read(fd, buf, BUFSIZ) != sizeof(root_uid_map) ||
+	    strncmp(buf, root_uid_map, sizeof(root_uid_map) - 1)) {
 		close(fd);
 		return;
 	}
-- 
2.35.1

[PATCH 03/16] tcp: False "Untrusted loop bound" positive, CWE-606

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

Field doff in struct tcp_hdr is 4 bits wide, so optlen in
tcp_tap_handler() is already bound, but make that explicit.
Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 tcp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tcp.c b/tcp.c
index 1409c53..858eb41 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2716,6 +2716,8 @@ int tcp_tap_handler(struct ctx *c, int af, const void *addr,
 		return 1;
 
 	optlen = th->doff * 4UL - sizeof(*th);
+	/* Static checkers might fail to see this: */
+	optlen = MIN(optlen, ((1UL << 4) /* from doff width */ - 6) * 4UL);
 	opts = packet_get(p, 0, sizeof(*th), optlen, NULL);
 
 	conn = tcp_hash_lookup(c, af, addr, htons(th->source), htons(th->dest));
-- 
@@ -2716,6 +2716,8 @@ int tcp_tap_handler(struct ctx *c, int af, const void *addr,
 		return 1;
 
 	optlen = th->doff * 4UL - sizeof(*th);
+	/* Static checkers might fail to see this: */
+	optlen = MIN(optlen, ((1UL << 4) /* from doff width */ - 6) * 4UL);
 	opts = packet_get(p, 0, sizeof(*th), optlen, NULL);
 
 	conn = tcp_hash_lookup(c, af, addr, htons(th->source), htons(th->dest));
-- 
2.35.1

[PATCH 04/16] treewide: Unchecked return value from library, CWE-252

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 15272 bytes --]

All instances were harmless, but it might be useful to have some
debug messages here and there. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 icmp.c       | 13 +++++++++----
 netlink.c    | 40 ++++++++++++++++++++++++---------------
 qrap.c       | 13 +++++++++----
 tap.c        | 19 ++++++++++++-------
 tcp.c        | 19 ++++++++++++-------
 tcp_splice.c | 53 +++++++++++++++++++++++++++++++++++++++-------------
 udp.c        |  3 ++-
 util.c       | 11 +++++++----
 8 files changed, 116 insertions(+), 55 deletions(-)

diff --git a/icmp.c b/icmp.c
index 0eb5bfe..8abc94b 100644
--- a/icmp.c
+++ b/icmp.c
@@ -160,6 +160,9 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 		if (!ih)
 			return 1;
 
+		if (ih->type != ICMP_ECHO && ih->type != ICMP_ECHOREPLY)
+			return 1;
+
 		sa.sin_port = ih->un.echo.id;
 
 		iref.icmp.id = id = ntohs(ih->un.echo.id);
@@ -179,8 +182,9 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 		bitmap_set(icmp_act[V4], id);
 
 		sa.sin_addr = *(struct in_addr *)addr;
-		sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
-		       (struct sockaddr *)&sa, sizeof(sa));
+		if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
+			   (struct sockaddr *)&sa, sizeof(sa)) < 0)
+			debug("ICMP: failed to relay request to socket");
 	} else if (af == AF_INET6) {
 		union icmp_epoll_ref iref = { .icmp.v6 = 1 };
 		struct sockaddr_in6 sa = {
@@ -216,8 +220,9 @@ int icmp_tap_handler(const struct ctx *c, int af, const void *addr,
 		bitmap_set(icmp_act[V6], id);
 
 		sa.sin6_addr = *(struct in6_addr *)addr;
-		sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
-		       (struct sockaddr *)&sa, sizeof(sa));
+		if (sendto(s, ih, sizeof(*ih) + plen, MSG_NOSIGNAL,
+			   (struct sockaddr *)&sa, sizeof(sa)) < 1)
+			debug("ICMPv6: failed to relay request to socket");
 	}
 
 	return 1;
diff --git a/netlink.c b/netlink.c
index 5902dc4..78c1186 100644
--- a/netlink.c
+++ b/netlink.c
@@ -60,7 +60,8 @@ ns:
 		return 0;
 
 #ifdef NETLINK_GET_STRICT_CHK
-	setsockopt(*s, SOL_NETLINK, NETLINK_GET_STRICT_CHK, &y, sizeof(y));
+	if (setsockopt(*s, SOL_NETLINK, NETLINK_GET_STRICT_CHK, &y, sizeof(y)))
+		debug("netlink: cannot set NETLINK_GET_STRICT_CHK on %i", *s);
 #endif
 
 	ns_enter((struct ctx *)arg);
@@ -152,7 +153,8 @@ unsigned int nl_get_ext_if(int *v4, int *v6)
 	char buf[BUFSIZ];
 	long *word, tmp;
 	uint8_t *vmap;
-	size_t n, na;
+	ssize_t n;
+	size_t na;
 	int *v;
 
 	if (*v4 == IP_VERSION_PROBE) {
@@ -168,7 +170,9 @@ v6:
 		return 0;
 	}
 
-	n = nl_req(0, buf, &req, sizeof(req));
+	if ((n = nl_req(0, buf, &req, sizeof(req))) < 0)
+		return 0;
+
 	nh = (struct nlmsghdr *)buf;
 
 	for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
@@ -289,7 +293,8 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 	struct rtattr *rta;
 	struct rtmsg *rtm;
 	char buf[BUFSIZ];
-	size_t n, na;
+	ssize_t n;
+	size_t na;
 
 	if (set) {
 		if (af == AF_INET6) {
@@ -323,8 +328,7 @@ void nl_route(int ns, unsigned int ifi, sa_family_t af, void *gw)
 		req.nlh.nlmsg_flags |= NLM_F_DUMP;
 	}
 
-	n = nl_req(ns, buf, &req, req.nlh.nlmsg_len);
-	if (set)
+	if (set || (n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0)
 		return;
 
 	nh = (struct nlmsghdr *)buf;
@@ -398,7 +402,8 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
 	char buf[BUFSIZ];
-	size_t n, na;
+	ssize_t n;
+	size_t na;
 
 	if (set) {
 		if (af == AF_INET6) {
@@ -430,8 +435,7 @@ void nl_addr(int ns, unsigned int ifi, sa_family_t af,
 		req.nlh.nlmsg_flags |= NLM_F_DUMP;
 	}
 
-	n = nl_req(ns, buf, &req, req.nlh.nlmsg_len);
-	if (set)
+	if (set || (n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0)
 		return;
 
 	nh = (struct nlmsghdr *)buf;
@@ -504,14 +508,17 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 	struct nlmsghdr *nh;
 	struct rtattr *rta;
 	char buf[BUFSIZ];
-	size_t n, na;
+	ssize_t n;
+	size_t na;
 
 	if (!MAC_IS_ZERO(mac)) {
 		req.nlh.nlmsg_len = sizeof(req);
 		memcpy(req.set.mac, mac, ETH_ALEN);
 		req.rta.rta_type = IFLA_ADDRESS;
 		req.rta.rta_len = RTA_LENGTH(ETH_ALEN);
-		nl_req(ns, buf, &req, req.nlh.nlmsg_len);
+		if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
+			return;
+
 		up = 0;
 	}
 
@@ -520,17 +527,20 @@ void nl_link(int ns, unsigned int ifi, void *mac, int up, int mtu)
 		req.set.mtu.mtu = mtu;
 		req.rta.rta_type = IFLA_MTU;
 		req.rta.rta_len = RTA_LENGTH(sizeof(unsigned int));
-		nl_req(ns, buf, &req, req.nlh.nlmsg_len);
+		if (nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
+			return;
+
 		up = 0;
 	}
 
-	if (up)
-		nl_req(ns, buf, &req, req.nlh.nlmsg_len);
+	if (up && nl_req(ns, buf, &req, req.nlh.nlmsg_len) < 0)
+		return;
 
 	if (change)
 		return;
 
-	n = nl_req(ns, buf, &req, req.nlh.nlmsg_len);
+	if ((n = nl_req(ns, buf, &req, req.nlh.nlmsg_len)) < 0)
+		return;
 
 	nh = (struct nlmsghdr *)buf;
 	for ( ; NLMSG_OK(nh, n); nh = NLMSG_NEXT(nh, n)) {
diff --git a/qrap.c b/qrap.c
index 3ee73a7..50eea89 100644
--- a/qrap.c
+++ b/qrap.c
@@ -234,8 +234,10 @@ int main(int argc, char **argv)
 valid_args:
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		s = socket(AF_UNIX, SOCK_STREAM, 0);
-		setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-		setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
+		if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
+			perror("setsockopt SO_RCVTIMEO");
+		if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
+			perror("setsockopt SO_SNDTIMEO");
 
 		if (s < 0) {
 			perror("socket");
@@ -263,8 +265,11 @@ valid_args:
 	}
 
 	tv.tv_usec = 0;
-	setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
-	setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
+	if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
+		perror("setsockopt, SO_RCVTIMEO reset");
+	if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
+		perror("setsockopt, SO_SNDTIMEO reset");
+
 	fprintf(stderr, "Connected to %s\n", addr.sun_path);
 
 	if (dup2(s, (int)fd) < 0) {
diff --git a/tap.c b/tap.c
index f8222a2..e4dd804 100644
--- a/tap.c
+++ b/tap.c
@@ -84,7 +84,8 @@ int tap_send(const struct ctx *c, const void *data, size_t len, int vnet_pre)
 		} else {
 			uint32_t vnet_len = htonl(len);
 
-			send(c->fd_tap, &vnet_len, 4, flags);
+			if (send(c->fd_tap, &vnet_len, 4, flags) < 0)
+				return -1;
 		}
 
 		return send(c->fd_tap, data, len, flags);
@@ -150,7 +151,8 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto,
 			ih->checksum = csum_unaligned(ih, len, 0);
 		}
 
-		tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1);
+		if (tap_send(c, buf, len + sizeof(*iph) + sizeof(*eh), 1) < 0)
+			debug("tap: failed to send %lu bytes (IPv4)", len);
 	} else {
 		struct ipv6hdr *ip6h = (struct ipv6hdr *)(eh + 1);
 		char *data = (char *)(ip6h + 1);
@@ -201,7 +203,8 @@ void tap_ip_send(const struct ctx *c, const struct in6_addr *src, uint8_t proto,
 			ip6h->flow_lbl[2] = (flow >> 0) & 0xff;
 		}
 
-		tap_send(c, buf, len + sizeof(*ip6h) + sizeof(*eh), 1);
+		if (tap_send(c, buf, len + sizeof(*ip6h) + sizeof(*eh), 1) < 1)
+			debug("tap: failed to send %lu bytes (IPv6)", len);
 	}
 }
 
@@ -862,11 +865,13 @@ static void tap_sock_unix_new(struct ctx *c)
 
 	c->fd_tap = accept4(c->fd_tap_listen, NULL, NULL, 0);
 
-	if (!c->low_rmem)
-		setsockopt(c->fd_tap, SOL_SOCKET, SO_RCVBUF, &v, sizeof(v));
+	if (!c->low_rmem &&
+	    setsockopt(c->fd_tap, SOL_SOCKET, SO_RCVBUF, &v, sizeof(v)))
+		trace("tap: failed to set SO_RCVBUF to %i", v);
 
-	if (!c->low_wmem)
-		setsockopt(c->fd_tap, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v));
+	if (!c->low_wmem &&
+	    setsockopt(c->fd_tap, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v)))
+		trace("tap: failed to set SO_SNDBUF to %i", v);
 
 	ev.data.fd = c->fd_tap;
 	ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP;
diff --git a/tcp.c b/tcp.c
index 858eb41..92cefab 100644
--- a/tcp.c
+++ b/tcp.c
@@ -1053,11 +1053,11 @@ void tcp_sock_set_bufsize(const struct ctx *c, int s)
 	if (s == -1)
 		return;
 
-	if (!c->low_rmem)
-		setsockopt(s, SOL_SOCKET, SO_RCVBUF, &v, sizeof(v));
+	if (!c->low_rmem && setsockopt(s, SOL_SOCKET, SO_RCVBUF, &v, sizeof(v)))
+		trace("TCP: failed to set SO_RCVBUF to %i", v);
 
-	if (!c->low_wmem)
-		setsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v));
+	if (!c->low_wmem && setsockopt(s, SOL_SOCKET, SO_SNDBUF, &v, sizeof(v)))
+		trace("TCP: failed to set SO_SNDBUF to %i", v);
 }
 
 /**
@@ -1547,7 +1547,8 @@ static void tcp_l2_buf_flush_part(const struct ctx *c,
 
 	missing = end - sent;
 	p = (char *)iov->iov_base + iov->iov_len - missing;
-	send(c->fd_tap, p, missing, MSG_NOSIGNAL);
+	if (send(c->fd_tap, p, missing, MSG_NOSIGNAL))
+		debug("TCP: failed to flush %lu missing bytes to tap", missing);
 }
 
 /**
@@ -2010,6 +2011,7 @@ static void tcp_clamp_window(const struct ctx *c, struct tcp_conn *conn,
 			     unsigned wnd)
 {
 	uint32_t prev_scaled = conn->wnd_from_tap << conn->ws_from_tap;
+	int s = conn->sock;
 
 	wnd <<= conn->ws_from_tap;
 	wnd = MIN(MAX_WINDOW, wnd);
@@ -2025,7 +2027,9 @@ static void tcp_clamp_window(const struct ctx *c, struct tcp_conn *conn,
 	}
 
 	conn->wnd_from_tap = MIN(wnd >> conn->ws_from_tap, USHRT_MAX);
-	setsockopt(conn->sock, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd));
+	if (setsockopt(s, SOL_TCP, TCP_WINDOW_CLAMP, &wnd, sizeof(wnd)))
+		trace("TCP: failed to set TCP_WINDOW_CLAMP on socket %i", s);
+
 	conn_flag(c, conn, WND_CLAMPED);
 }
 
@@ -2209,7 +2213,8 @@ static void tcp_conn_from_tap(struct ctx *c, int af, const void *addr,
 	conn->wnd_to_tap = WINDOW_DEFAULT;
 
 	mss = tcp_conn_tap_mss(c, conn, opts, optlen);
-	setsockopt(s, SOL_TCP, TCP_MAXSEG, &mss, sizeof(mss));
+	if (setsockopt(s, SOL_TCP, TCP_MAXSEG, &mss, sizeof(mss)))
+		trace("TCP: failed to set TCP_MAXSEG on socket %i", s);
 	MSS_SET(conn, mss);
 
 	tcp_get_tap_ws(conn, opts, optlen);
diff --git a/tcp_splice.c b/tcp_splice.c
index 24a3b4b..84df8ed 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -376,8 +376,15 @@ static int tcp_splice_connect_finish(const struct ctx *c,
 			return -EIO;
 		}
 
-		fcntl(conn->pipe_a_b[0], F_SETPIPE_SZ, c->tcp.pipe_size);
-		fcntl(conn->pipe_b_a[0], F_SETPIPE_SZ, c->tcp.pipe_size);
+		if (fcntl(conn->pipe_a_b[0], F_SETPIPE_SZ, c->tcp.pipe_size)) {
+			trace("TCP (spliced): cannot set a->b pipe size to %lu",
+			      c->tcp.pipe_size);
+		}
+
+		if (fcntl(conn->pipe_b_a[0], F_SETPIPE_SZ, c->tcp.pipe_size)) {
+			trace("TCP (spliced): cannot set b->a pipe size to %lu",
+			      c->tcp.pipe_size);
+		}
 	}
 
 	if (!(conn->events & ESTABLISHED))
@@ -428,7 +435,11 @@ static int tcp_splice_connect(const struct ctx *c, struct tcp_splice_conn *conn,
 	if (s < 0)
 		tcp_sock_set_bufsize(c, conn->b);
 
-	setsockopt(conn->b, SOL_TCP, TCP_QUICKACK, &((int){ 1 }), sizeof(int));
+	if (setsockopt(conn->b, SOL_TCP, TCP_QUICKACK,
+		       &((int){ 1 }), sizeof(int))) {
+		trace("TCP (spliced): failed to set TCP_QUICKACK on socket %i",
+		      conn->b);
+	}
 
 	if (CONN_V6(conn)) {
 		sa = (struct sockaddr *)&addr6;
@@ -565,8 +576,11 @@ void tcp_sock_handler_splice(struct ctx *c, union epoll_ref ref,
 		if ((s = accept4(ref.r.s, NULL, NULL, SOCK_NONBLOCK)) < 0)
 			return;
 
-		setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }),
-			   sizeof(int));
+		if (setsockopt(s, SOL_TCP, TCP_QUICKACK, &((int){ 1 }),
+			       sizeof(int))) {
+			trace("TCP (spliced): failed to set TCP_QUICKACK on %i",
+			      s);
+		}
 
 		conn = CONN(c->tcp.splice_conn_count++);
 		conn->a = s;
@@ -817,10 +831,17 @@ static void tcp_splice_pipe_refill(const struct ctx *c)
 			continue;
 		}
 
-		fcntl(splice_pipe_pool[i][0][0], F_SETPIPE_SZ,
-		      c->tcp.pipe_size);
-		fcntl(splice_pipe_pool[i][1][0], F_SETPIPE_SZ,
-		      c->tcp.pipe_size);
+		if (fcntl(splice_pipe_pool[i][0][0], F_SETPIPE_SZ,
+			  c->tcp.pipe_size)) {
+			trace("TCP (spliced): cannot set a->b pipe size to %lu",
+			      c->tcp.pipe_size);
+		}
+
+		if (fcntl(splice_pipe_pool[i][1][0], F_SETPIPE_SZ,
+			  c->tcp.pipe_size)) {
+			trace("TCP (spliced): cannot set b->a pipe size to %lu",
+			      c->tcp.pipe_size);
+		}
 	}
 }
 
@@ -850,15 +871,21 @@ void tcp_splice_timer(struct ctx *c)
 
 		if ( (conn->flags & RCVLOWAT_SET_A) &&
 		    !(conn->flags & RCVLOWAT_ACT_A)) {
-			setsockopt(conn->a, SOL_SOCKET, SO_RCVLOWAT,
-				   &((int){ 1 }), sizeof(int));
+			if (setsockopt(conn->a, SOL_SOCKET, SO_RCVLOWAT,
+				       &((int){ 1 }), sizeof(int))) {
+				trace("TCP (spliced): can't set SO_RCVLOWAT on "
+				      "%i", conn->a);
+			}
 			conn_flag(c, conn, ~RCVLOWAT_SET_A);
 		}
 
 		if ( (conn->flags & RCVLOWAT_SET_B) &&
 		    !(conn->flags & RCVLOWAT_ACT_B)) {
-			setsockopt(conn->b, SOL_SOCKET, SO_RCVLOWAT,
-				   &((int){ 1 }), sizeof(int));
+			if (setsockopt(conn->b, SOL_SOCKET, SO_RCVLOWAT,
+				       &((int){ 1 }), sizeof(int))) {
+				trace("TCP (spliced): can't set SO_RCVLOWAT on "
+				      "%i", conn->b);
+			}
 			conn_flag(c, conn, ~RCVLOWAT_SET_B);
 		}
 
diff --git a/udp.c b/udp.c
index 1c0fdc6..cbd3ac8 100644
--- a/udp.c
+++ b/udp.c
@@ -938,7 +938,8 @@ void udp_sock_handler(const struct ctx *c, union epoll_ref ref, uint32_t events,
 
 			last_mh->msg_iov = &last_mh->msg_iov[i];
 
-			sendmsg(c->fd_tap, last_mh, MSG_NOSIGNAL);
+			if (sendmsg(c->fd_tap, last_mh, MSG_NOSIGNAL) < 0)
+				debug("UDP: %li bytes to tap missing", missing);
 
 			*iov_base -= first_offset;
 			break;
diff --git a/util.c b/util.c
index 81cf744..a4d2cd1 100644
--- a/util.c
+++ b/util.c
@@ -154,7 +154,8 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
 	if (log_opt & LOG_PERROR)
 		fprintf(stderr, "%s", buf + sizeof("<0>"));
 
-	send(log_sock, buf, n, 0);
+	if (send(log_sock, buf, n, 0))
+		fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
 }
 
 #define IPV6_NH_OPT(nh)							\
@@ -239,7 +240,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, uint16_t port,
 	};
 	const struct sockaddr *sa;
 	struct epoll_event ev;
-	int fd, sl, one = 1;
+	int fd, sl, y = 1;
 
 	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP &&
 	    proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6)
@@ -287,10 +288,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, uint16_t port,
 		sa = (const struct sockaddr *)&addr6;
 		sl = sizeof(addr6);
 
-		setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one));
+		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &y, sizeof(y)))
+			debug("Failed to set IPV6_V6ONLY on socket %i", fd);
 	}
 
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one));
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
+		debug("Failed to set IPV6_V6ONLY on socket %i", fd);
 
 	if (bind(fd, sa, sl) < 0) {
 		/* We'll fail to bind to low ports if we don't have enough
-- 
@@ -154,7 +154,8 @@ void passt_vsyslog(int pri, const char *format, va_list ap)
 	if (log_opt & LOG_PERROR)
 		fprintf(stderr, "%s", buf + sizeof("<0>"));
 
-	send(log_sock, buf, n, 0);
+	if (send(log_sock, buf, n, 0))
+		fprintf(stderr, "Failed to send %i bytes to syslog\n", n);
 }
 
 #define IPV6_NH_OPT(nh)							\
@@ -239,7 +240,7 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, uint16_t port,
 	};
 	const struct sockaddr *sa;
 	struct epoll_event ev;
-	int fd, sl, one = 1;
+	int fd, sl, y = 1;
 
 	if (proto != IPPROTO_TCP && proto != IPPROTO_UDP &&
 	    proto != IPPROTO_ICMP && proto != IPPROTO_ICMPV6)
@@ -287,10 +288,12 @@ int sock_l4(const struct ctx *c, int af, uint8_t proto, uint16_t port,
 		sa = (const struct sockaddr *)&addr6;
 		sl = sizeof(addr6);
 
-		setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &one, sizeof(one));
+		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &y, sizeof(y)))
+			debug("Failed to set IPV6_V6ONLY on socket %i", fd);
 	}
 
-	setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one));
+	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
+		debug("Failed to set IPV6_V6ONLY on socket %i", fd);
 
 	if (bind(fd, sa, sl) < 0) {
 		/* We'll fail to bind to low ports if we don't have enough
-- 
2.35.1

[PATCH 05/16] tap: Resource leak, CWE-404

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 tap.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tap.c b/tap.c
index e4dd804..8310891 100644
--- a/tap.c
+++ b/tap.c
@@ -899,8 +899,11 @@ static int tap_ns_tun(void *arg)
 	if (ns_enter(c) ||
 	    (tun_ns_fd = open("/dev/net/tun", flags)) < 0 ||
 	    ioctl(tun_ns_fd, TUNSETIFF, &ifr) ||
-	    !(c->pasta_ifi = if_nametoindex(c->pasta_ifn)))
+	    !(c->pasta_ifi = if_nametoindex(c->pasta_ifn))) {
+		if (tun_ns_fd != -1)
+			close(tun_ns_fd);
 		tun_ns_fd = -1;
+	}
 
 	return 0;
 }
-- 
@@ -899,8 +899,11 @@ static int tap_ns_tun(void *arg)
 	if (ns_enter(c) ||
 	    (tun_ns_fd = open("/dev/net/tun", flags)) < 0 ||
 	    ioctl(tun_ns_fd, TUNSETIFF, &ifr) ||
-	    !(c->pasta_ifi = if_nametoindex(c->pasta_ifn)))
+	    !(c->pasta_ifi = if_nametoindex(c->pasta_ifn))) {
+		if (tun_ns_fd != -1)
+			close(tun_ns_fd);
 		tun_ns_fd = -1;
+	}
 
 	return 0;
 }
-- 
2.35.1

[PATCH 06/16] conf, packet: Operands don't affect result, CWE-569

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1521 bytes --]

Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 conf.c   | 7 +++++--
 packet.c | 2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/conf.c b/conf.c
index ea51de4..ca44b30 100644
--- a/conf.c
+++ b/conf.c
@@ -369,6 +369,7 @@ static int conf_ns_opt(struct ctx *c,
 	int ufd = -1, nfd = -1, try, ret, netns_only_reset = c->netns_only;
 	char userns[PATH_MAX] = { 0 }, netns[PATH_MAX];
 	char *endptr;
+	long pid_arg;
 	pid_t pid;
 
 	if (c->netns_only && *conf_userns) {
@@ -379,10 +380,12 @@ static int conf_ns_opt(struct ctx *c,
 	/* It might be a PID, a netns path, or a netns name */
 	for (try = 0; try < 3; try++) {
 		if (try == 0) {
-			pid = strtol(optarg, &endptr, 10);
-			if (*endptr || pid > INT_MAX)
+			pid_arg = strtol(optarg, &endptr, 10);
+			if (*endptr || pid_arg < 0 || pid_arg > INT_MAX)
 				continue;
 
+			pid = pid_arg;
+
 			if (!*conf_userns && !c->netns_only) {
 				ret = snprintf(userns, PATH_MAX,
 					       "/proc/%i/ns/user", pid);
diff --git a/packet.c b/packet.c
index fa9e9b4..3358c2c 100644
--- a/packet.c
+++ b/packet.c
@@ -57,7 +57,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 		return;
 	}
 
-	if ((unsigned int)((intptr_t)start - (intptr_t)p->buf) > UINT32_MAX) {
+	if ((uint64_t)((uintptr_t)start - (uintptr_t)p->buf) > UINT32_MAX) {
 		trace("add packet start %p, buffer start %p, %s:%i",
 		      start, p->buf, func, line);
 		return;
-- 
@@ -57,7 +57,7 @@ void packet_add_do(struct pool *p, size_t len, const char *start,
 		return;
 	}
 
-	if ((unsigned int)((intptr_t)start - (intptr_t)p->buf) > UINT32_MAX) {
+	if ((uint64_t)((uintptr_t)start - (uintptr_t)p->buf) > UINT32_MAX) {
 		trace("add packet start %p, buffer start %p, %s:%i",
 		      start, p->buf, func, line);
 		return;
-- 
2.35.1

[PATCH 07/16] passt: Improper use of negative value (CWE-394)

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 passt.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/passt.c b/passt.c
index 06c3d73..8781a7f 100644
--- a/passt.c
+++ b/passt.c
@@ -421,13 +421,22 @@ int main(int argc, char **argv)
 
 	pcap_init(&c);
 
-	if (!c.foreground)
+	if (!c.foreground) {
 		/* NOLINTNEXTLINE(android-cloexec-open): see __daemon() */
-		devnull_fd = open("/dev/null", O_RDWR);
+		if ((devnull_fd = open("/dev/null", O_RDWR)) < 0) {
+			perror("/dev/null open");
+			exit(EXIT_FAILURE);
+		}
+	}
 
-	if (*c.pid_file)
-		pidfile_fd = open(c.pid_file, O_CREAT | O_WRONLY | O_CLOEXEC,
-				  S_IRUSR | S_IWUSR);
+	if (*c.pid_file) {
+		if ((pidfile_fd = open(c.pid_file,
+				       O_CREAT | O_WRONLY | O_CLOEXEC,
+				       S_IRUSR | S_IWUSR)) < 0) {
+			perror("PID file open");
+			exit(EXIT_FAILURE);
+		}
+	}
 
 	if (sandbox(&c)) {
 		err("Failed to sandbox process, exiting\n");
-- 
@@ -421,13 +421,22 @@ int main(int argc, char **argv)
 
 	pcap_init(&c);
 
-	if (!c.foreground)
+	if (!c.foreground) {
 		/* NOLINTNEXTLINE(android-cloexec-open): see __daemon() */
-		devnull_fd = open("/dev/null", O_RDWR);
+		if ((devnull_fd = open("/dev/null", O_RDWR)) < 0) {
+			perror("/dev/null open");
+			exit(EXIT_FAILURE);
+		}
+	}
 
-	if (*c.pid_file)
-		pidfile_fd = open(c.pid_file, O_CREAT | O_WRONLY | O_CLOEXEC,
-				  S_IRUSR | S_IWUSR);
+	if (*c.pid_file) {
+		if ((pidfile_fd = open(c.pid_file,
+				       O_CREAT | O_WRONLY | O_CLOEXEC,
+				       S_IRUSR | S_IWUSR)) < 0) {
+			perror("PID file open");
+			exit(EXIT_FAILURE);
+		}
+	}
 
 	if (sandbox(&c)) {
 		err("Failed to sandbox process, exiting\n");
-- 
2.35.1

[PATCH 08/16] treewide: Argument cannot be negative, CWE-687

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 3607 bytes --]

Actually harmless. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 pasta.c | 25 ++++++++-----------------
 qrap.c  | 10 +++++-----
 tap.c   |  5 +++++
 util.h  |  9 +++++++++
 4 files changed, 27 insertions(+), 22 deletions(-)

diff --git a/pasta.c b/pasta.c
index 18df5d2..cd37d16 100644
--- a/pasta.c
+++ b/pasta.c
@@ -120,33 +120,24 @@ static int pasta_setup_ns(void *arg)
 {
 	struct pasta_setup_ns_arg *a = (struct pasta_setup_ns_arg *)arg;
 	char *shell;
-	int fd;
 
 	if (!a->c->netns_only) {
 		char buf[BUFSIZ];
 
 		snprintf(buf, BUFSIZ, "%i %i %i", 0, a->euid, 1);
 
-		fd = open("/proc/self/uid_map", O_WRONLY | O_CLOEXEC);
-		if (write(fd, buf, strlen(buf)) < 0)
-			warn("Cannot set uid_map in namespace");
-		close(fd);
+		FWRITE("/proc/self/uid_map", buf,
+		       "Cannot set uid_map in namespace");
 
-		fd = open("/proc/self/setgroups", O_WRONLY | O_CLOEXEC);
-		if (write(fd, "deny", sizeof("deny")) < 0)
-			warn("Cannot write to setgroups in namespace");
-		close(fd);
+		FWRITE("/proc/self/setgroups", "deny",
+		       "Cannot write to setgroups in namespace");
 
-		fd = open("/proc/self/gid_map", O_WRONLY | O_CLOEXEC);
-		if (write(fd, buf, strlen(buf)) < 0)
-			warn("Cannot set gid_map in namespace");
-		close(fd);
+		FWRITE("/proc/self/gid_map", buf,
+		       "Cannot set gid_map in namespace");
 	}
 
-	fd = open("/proc/sys/net/ipv4/ping_group_range", O_WRONLY | O_CLOEXEC);
-	if (write(fd, "0 0", strlen("0 0")) < 0)
-		warn("Cannot set ping_group_range, ICMP requests might fail");
-	close(fd);
+	FWRITE("/proc/sys/net/ipv4/ping_group_range", "0 0",
+	       "Cannot set ping_group_range, ICMP requests might fail");
 
 	shell = getenv("SHELL") ? getenv("SHELL") : "/bin/sh";
 	if (strstr(shell, "/bash"))
diff --git a/qrap.c b/qrap.c
index 50eea89..17cc472 100644
--- a/qrap.c
+++ b/qrap.c
@@ -234,16 +234,16 @@ int main(int argc, char **argv)
 valid_args:
 	for (i = 1; i < UNIX_SOCK_MAX; i++) {
 		s = socket(AF_UNIX, SOCK_STREAM, 0);
-		if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
-			perror("setsockopt SO_RCVTIMEO");
-		if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
-			perror("setsockopt SO_SNDTIMEO");
-
 		if (s < 0) {
 			perror("socket");
 			exit(EXIT_FAILURE);
 		}
 
+		if (setsockopt(s, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)))
+			perror("setsockopt SO_RCVTIMEO");
+		if (setsockopt(s, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)))
+			perror("setsockopt SO_SNDTIMEO");
+
 		snprintf(addr.sun_path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
 		if (connect(s, (const struct sockaddr *)&addr, sizeof(addr)))
 			perror("connect");
diff --git a/tap.c b/tap.c
index 8310891..8110577 100644
--- a/tap.c
+++ b/tap.c
@@ -803,6 +803,11 @@ static void tap_sock_unix_init(struct ctx *c)
 			snprintf(path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
 
 		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
+		if (ex < 0) {
+			perror("UNIX domain socket check");
+			exit(EXIT_FAILURE);
+		}
+
 		ret = connect(ex, (const struct sockaddr *)&addr, sizeof(addr));
 		if (!ret || (errno != ENOENT && errno != ECONNREFUSED)) {
 			if (*c->sock_path) {
diff --git a/util.h b/util.h
index 91ce3e0..4ed7d94 100644
--- a/util.h
+++ b/util.h
@@ -58,6 +58,15 @@ void trace_init(int enable);
 #define TMPDIR		"/tmp"
 #endif
 
+#define FWRITE(path, buf, str)						\
+	do {								\
+		int fd = open(path, O_WRONLY | O_CLOEXEC);		\
+		if (fd < 0 || write(fd, buf, strlen(buf)))		\
+			warn(str);					\
+		if (fd >= 0)						\
+			close(fd);					\
+	} while (0)
+
 #define V4		0
 #define V6		1
 #define IP_VERSIONS	2
-- 
@@ -58,6 +58,15 @@ void trace_init(int enable);
 #define TMPDIR		"/tmp"
 #endif
 
+#define FWRITE(path, buf, str)						\
+	do {								\
+		int fd = open(path, O_WRONLY | O_CLOEXEC);		\
+		if (fd < 0 || write(fd, buf, strlen(buf)))		\
+			warn(str);					\
+		if (fd >= 0)						\
+			close(fd);					\
+	} while (0)
+
 #define V4		0
 #define V6		1
 #define IP_VERSIONS	2
-- 
2.35.1

[PATCH 09/16] conf: False "Assign instead of compare" positive, CWE-481

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 671 bytes --]

This really just needs to be an assignment before line_read() --
turn it into a for loop. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/conf.c b/conf.c
index ca44b30..2412fc6 100644
--- a/conf.c
+++ b/conf.c
@@ -288,7 +288,7 @@ static void get_dns(struct ctx *c)
 	if ((fd = open("/etc/resolv.conf", O_RDONLY | O_CLOEXEC)) < 0)
 		goto out;
 
-	while (!(*buf = 0) && line_read(buf, BUFSIZ, fd)) {
+	for (*buf = 0; line_read(buf, BUFSIZ, fd); *buf = 0) {
 		if (!dns_set && strstr(buf, "nameserver ") == buf) {
 			p = strrchr(buf, ' ');
 			if (!p)
-- 
@@ -288,7 +288,7 @@ static void get_dns(struct ctx *c)
 	if ((fd = open("/etc/resolv.conf", O_RDONLY | O_CLOEXEC)) < 0)
 		goto out;
 
-	while (!(*buf = 0) && line_read(buf, BUFSIZ, fd)) {
+	for (*buf = 0; line_read(buf, BUFSIZ, fd); *buf = 0) {
 		if (!dns_set && strstr(buf, "nameserver ") == buf) {
 			p = strrchr(buf, ' ');
 			if (!p)
-- 
2.35.1

[PATCH 10/16] conf, tap: False "Buffer not null terminated" positives, CWE-170

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1882 bytes --]

Those strings are actually guaranteed to be NULL-terminated. Reported
by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 conf.c | 6 +++---
 tap.c  | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/conf.c b/conf.c
index 2412fc6..c1c8058 100644
--- a/conf.c
+++ b/conf.c
@@ -1035,7 +1035,7 @@ void conf(struct ctx *c, int argc, char **argv)
 				usage(argv[0]);
 			}
 
-			ret = snprintf(c->sock_path, sizeof(c->sock_path), "%s",
+			ret = snprintf(c->sock_path, UNIX_SOCK_MAX - 1, "%s",
 				       optarg);
 			if (ret <= 0 || ret >= (int)sizeof(c->pcap)) {
 				err("Invalid socket path: %s", optarg);
@@ -1048,9 +1048,9 @@ void conf(struct ctx *c, int argc, char **argv)
 				usage(argv[0]);
 			}
 
-			ret = snprintf(c->pasta_ifn, sizeof(c->pasta_ifn), "%s",
+			ret = snprintf(c->pasta_ifn, IFNAMSIZ - 1, "%s",
 				       optarg);
-			if (ret <= 0 || ret >= (int)sizeof(c->pasta_ifn)) {
+			if (ret <= 0 || ret >= (int)IFNAMSIZ - 1) {
 				err("Invalid interface name: %s", optarg);
 				usage(argv[0]);
 			}
diff --git a/tap.c b/tap.c
index 8110577..04ceade 100644
--- a/tap.c
+++ b/tap.c
@@ -798,9 +798,9 @@ static void tap_sock_unix_init(struct ctx *c)
 		char *path = addr.sun_path;
 
 		if (*c->sock_path)
-			strncpy(path, c->sock_path, UNIX_PATH_MAX);
+			memcpy(path, c->sock_path, UNIX_PATH_MAX);
 		else
-			snprintf(path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
+			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
 
 		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
 		if (ex < 0) {
@@ -899,7 +899,7 @@ static int tap_ns_tun(void *arg)
 	int flags = O_RDWR | O_NONBLOCK | O_CLOEXEC;
 	struct ctx *c = (struct ctx *)arg;
 
-	strncpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ);
+	memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ);
 
 	if (ns_enter(c) ||
 	    (tun_ns_fd = open("/dev/net/tun", flags)) < 0 ||
-- 
@@ -798,9 +798,9 @@ static void tap_sock_unix_init(struct ctx *c)
 		char *path = addr.sun_path;
 
 		if (*c->sock_path)
-			strncpy(path, c->sock_path, UNIX_PATH_MAX);
+			memcpy(path, c->sock_path, UNIX_PATH_MAX);
 		else
-			snprintf(path, UNIX_PATH_MAX, UNIX_SOCK_PATH, i);
+			snprintf(path, UNIX_PATH_MAX - 1, UNIX_SOCK_PATH, i);
 
 		ex = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0);
 		if (ex < 0) {
@@ -899,7 +899,7 @@ static int tap_ns_tun(void *arg)
 	int flags = O_RDWR | O_NONBLOCK | O_CLOEXEC;
 	struct ctx *c = (struct ctx *)arg;
 
-	strncpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ);
+	memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ);
 
 	if (ns_enter(c) ||
 	    (tun_ns_fd = open("/dev/net/tun", flags)) < 0 ||
-- 
2.35.1

[PATCH 11/16] tcp: Dereference null return value, CWE-476

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

Not an issue with a sane kernel behaviour. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index 92cefab..1820e19 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2729,7 +2729,7 @@ int tcp_tap_handler(struct ctx *c, int af, const void *addr,
 
 	/* New connection from tap */
 	if (!conn) {
-		if (th->syn && !th->ack)
+		if (opts && th->syn && !th->ack)
 			tcp_conn_from_tap(c, af, addr, th, opts, optlen, now);
 		return 1;
 	}
-- 
@@ -2729,7 +2729,7 @@ int tcp_tap_handler(struct ctx *c, int af, const void *addr,
 
 	/* New connection from tap */
 	if (!conn) {
-		if (th->syn && !th->ack)
+		if (opts && th->syn && !th->ack)
 			tcp_conn_from_tap(c, af, addr, th, opts, optlen, now);
 		return 1;
 	}
-- 
2.35.1

[PATCH 12/16] tcp_splice: Logically dead code, CWE-561

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 tcp_splice.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/tcp_splice.c b/tcp_splice.c
index 84df8ed..7c19d99 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -139,9 +139,6 @@ static void tcp_splice_conn_epoll_events(uint16_t events,
 {
 	*a = *b = 0;
 
-	if (events & CLOSED)
-		return;
-
 	if (events & ESTABLISHED) {
 		if (!(events & B_FIN_SENT))
 			*a = EPOLLIN | EPOLLRDHUP;
@@ -649,8 +646,8 @@ swap:
 	}
 
 	while (1) {
-		int retry_write = 0, more = 0;
 		ssize_t readlen, to_write = 0, written;
+		int more = 0;
 
 retry:
 		readlen = splice(from, NULL, pipes[1], NULL, c->tcp.pipe_size,
@@ -715,9 +712,6 @@ eintr:
 			if (never_read)
 				break;
 
-			if (retry_write--)
-				goto retry;
-
 			if (to == conn->a)
 				conn_event(c, conn, A_OUT_WAIT);
 			else
-- 
@@ -139,9 +139,6 @@ static void tcp_splice_conn_epoll_events(uint16_t events,
 {
 	*a = *b = 0;
 
-	if (events & CLOSED)
-		return;
-
 	if (events & ESTABLISHED) {
 		if (!(events & B_FIN_SENT))
 			*a = EPOLLIN | EPOLLRDHUP;
@@ -649,8 +646,8 @@ swap:
 	}
 
 	while (1) {
-		int retry_write = 0, more = 0;
 		ssize_t readlen, to_write = 0, written;
+		int more = 0;
 
 retry:
 		readlen = splice(from, NULL, pipes[1], NULL, c->tcp.pipe_size,
@@ -715,9 +712,6 @@ eintr:
 			if (never_read)
 				break;
 
-			if (retry_write--)
-				goto retry;
-
 			if (to == conn->a)
 				conn_event(c, conn, A_OUT_WAIT);
 			else
-- 
2.35.1

[PATCH 13/16] tcp, tcp_splice: False "Negative array index read" positives, CWE-129

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 2610 bytes --]

A flag or event bit is always set by callers. Reported by Coverity.

Signed-by-off: Stefano Brivio <sbrivio(a)redhat.com>
---
 tcp.c        | 12 ++++++++----
 tcp_splice.c | 24 ++++++++++++++++--------
 2 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/tcp.c b/tcp.c
index 1820e19..13a108e 100644
--- a/tcp.c
+++ b/tcp.c
@@ -868,15 +868,19 @@ static void conn_flag_do(const struct ctx *c, struct tcp_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP: index %li: %s dropped", conn - tc,
-		      tcp_flag_str[fls(~flag)]);
+		if (fls(~flag) >= 0) {
+			debug("TCP: index %li: %s dropped", conn - tc,
+			      tcp_flag_str[fls(~flag)]);
+		}
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP: index %li: %s", conn - tc,
-		      tcp_flag_str[fls(flag)]);
+		if (fls(flag) >= 0) {
+			debug("TCP: index %li: %s", conn - tc,
+			      tcp_flag_str[fls(flag)]);
+		}
 	}
 
 	if (flag == STALLED || flag == ~STALLED)
diff --git a/tcp_splice.c b/tcp_splice.c
index 7c19d99..1e24986 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -170,15 +170,19 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP (spliced): index %li: %s dropped", conn - tc,
-		      tcp_splice_flag_str[fls(~flag)]);
+		if (fls(~flag) >= 0) {
+			debug("TCP (spliced): index %li: %s dropped", conn - tc,
+			      tcp_splice_flag_str[fls(~flag)]);
+		}
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP (spliced): index %li: %s", conn - tc,
-		      tcp_splice_flag_str[fls(flag)]);
+		if (fls(flag) >= 0) {
+			debug("TCP (spliced): index %li: %s", conn - tc,
+			      tcp_splice_flag_str[fls(flag)]);
+		}
 	}
 
 	if (flag == CLOSING)
@@ -250,15 +254,19 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->events &= event;
-		debug("TCP (spliced): index %li, ~%s", conn - tc,
-		      tcp_splice_event_str[fls(~event)]);
+		if (fls(~event) >= 0) {
+			debug("TCP (spliced): index %li, ~%s", conn - tc,
+			      tcp_splice_event_str[fls(~event)]);
+		}
 	} else {
 		if (conn->events & event)
 			return;
 
 		conn->events |= event;
-		debug("TCP (spliced): index %li, %s", conn - tc,
-		      tcp_splice_event_str[fls(event)]);
+		if (fls(event) >= 0) {
+			debug("TCP (spliced): index %li, %s", conn - tc,
+			      tcp_splice_event_str[fls(event)]);
+		}
 	}
 
 	if (tcp_splice_epoll_ctl(c, conn))
-- 
@@ -170,15 +170,19 @@ static void conn_flag_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->flags &= flag;
-		debug("TCP (spliced): index %li: %s dropped", conn - tc,
-		      tcp_splice_flag_str[fls(~flag)]);
+		if (fls(~flag) >= 0) {
+			debug("TCP (spliced): index %li: %s dropped", conn - tc,
+			      tcp_splice_flag_str[fls(~flag)]);
+		}
 	} else {
 		if (conn->flags & flag)
 			return;
 
 		conn->flags |= flag;
-		debug("TCP (spliced): index %li: %s", conn - tc,
-		      tcp_splice_flag_str[fls(flag)]);
+		if (fls(flag) >= 0) {
+			debug("TCP (spliced): index %li: %s", conn - tc,
+			      tcp_splice_flag_str[fls(flag)]);
+		}
 	}
 
 	if (flag == CLOSING)
@@ -250,15 +254,19 @@ static void conn_event_do(const struct ctx *c, struct tcp_splice_conn *conn,
 			return;
 
 		conn->events &= event;
-		debug("TCP (spliced): index %li, ~%s", conn - tc,
-		      tcp_splice_event_str[fls(~event)]);
+		if (fls(~event) >= 0) {
+			debug("TCP (spliced): index %li, ~%s", conn - tc,
+			      tcp_splice_event_str[fls(~event)]);
+		}
 	} else {
 		if (conn->events & event)
 			return;
 
 		conn->events |= event;
-		debug("TCP (spliced): index %li, %s", conn - tc,
-		      tcp_splice_event_str[fls(event)]);
+		if (fls(event) >= 0) {
+			debug("TCP (spliced): index %li, %s", conn - tc,
+			      tcp_splice_event_str[fls(event)]);
+		}
 	}
 
 	if (tcp_splice_epoll_ctl(c, conn))
-- 
2.35.1

[PATCH 14/16] tcp: False "Out-of-bounds read" positive, CWE-125

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 997 bytes --]

Reported by Coverity: it doesn't see that tcp{4,6}_l2_buf_used are
set to zero by tcp_l2_data_buf_flush(), repeat that explicitly here.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 tcp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tcp.c b/tcp.c
index 13a108e..ad10688 100644
--- a/tcp.c
+++ b/tcp.c
@@ -2394,9 +2394,13 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_conn *conn)
 	iov_sock[0].iov_len = already_sent;
 
 	if (( v4 && tcp4_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp4_l2_buf)) ||
-	    (!v4 && tcp6_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp6_l2_buf)))
+	    (!v4 && tcp6_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp6_l2_buf))) {
 		tcp_l2_data_buf_flush(c);
 
+		/* Silence Coverity CWE-125 false positive */
+		tcp4_l2_buf_used = tcp6_l2_buf_used = 0;
+	}
+
 	for (i = 0, iov = iov_sock + 1; i < fill_bufs; i++, iov++) {
 		if (v4)
 			iov->iov_base = &tcp4_l2_buf[tcp4_l2_buf_used + i].data;
-- 
@@ -2394,9 +2394,13 @@ static int tcp_data_from_sock(struct ctx *c, struct tcp_conn *conn)
 	iov_sock[0].iov_len = already_sent;
 
 	if (( v4 && tcp4_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp4_l2_buf)) ||
-	    (!v4 && tcp6_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp6_l2_buf)))
+	    (!v4 && tcp6_l2_buf_used + fill_bufs > ARRAY_SIZE(tcp6_l2_buf))) {
 		tcp_l2_data_buf_flush(c);
 
+		/* Silence Coverity CWE-125 false positive */
+		tcp4_l2_buf_used = tcp6_l2_buf_used = 0;
+	}
+
 	for (i = 0, iov = iov_sock + 1; i < fill_bufs; i++, iov++) {
 		if (v4)
 			iov->iov_base = &tcp4_l2_buf[tcp4_l2_buf_used + i].data;
-- 
2.35.1

[PATCH 15/16] udp: Out-of-bounds read, CWE-125 in udp_timer()

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

Not an actual issue due to how it's typically stored, but udp_act
can also be used for ports 65528-65535. Reported by Coverity.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 udp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/udp.c b/udp.c
index cbd3ac8..86d806a 100644
--- a/udp.c
+++ b/udp.c
@@ -180,7 +180,7 @@ enum udp_act_type {
 };
 
 /* Activity-based aging for bindings */
-static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][USHRT_MAX / 8];
+static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][(USHRT_MAX + 1) / 8];
 
 /* Static buffers */
 
-- 
@@ -180,7 +180,7 @@ enum udp_act_type {
 };
 
 /* Activity-based aging for bindings */
-static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][USHRT_MAX / 8];
+static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][(USHRT_MAX + 1) / 8];
 
 /* Static buffers */
 
-- 
2.35.1

[PATCH 16/16] arch: Pointer to local outside scope, CWE-562

[Stefano Brivio]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

Reported by Coverity: if we fail to run the AVX2 version, once
execve() fails, we had already replaced argv[0] with the new
stack-allocated path string, and that's then passed back to
main(). Use a static variable instead.

Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com>
---
 arch.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch.c b/arch.c
index b8e1db5..ae21d59 100644
--- a/arch.c
+++ b/arch.c
@@ -22,6 +22,8 @@
  * @argv:	Arguments from command line
  */
 #ifdef __x86_64__
+static char avx2_path[PATH_MAX];
+
 void arch_avx2_exec(char **argv)
 {
 	char *p = strstr(argv[0], ".avx2");
@@ -29,11 +31,9 @@ void arch_avx2_exec(char **argv)
 	if (p) {
 		*p = 0;
 	} else if (__builtin_cpu_supports("avx2")) {
-		char path[PATH_MAX];
-
-		snprintf(path, PATH_MAX, "%s.avx2", argv[0]);
-		argv[0] = path;
-		execve(path, argv, environ);
+		snprintf(avx2_path, PATH_MAX, "%s.avx2", argv[0]);
+		argv[0] = avx2_path;
+		execve(avx2_path, argv, environ);
 		perror("Can't run AVX2 build, using non-AVX2 version");
 	}
 }
-- 
@@ -22,6 +22,8 @@
  * @argv:	Arguments from command line
  */
 #ifdef __x86_64__
+static char avx2_path[PATH_MAX];
+
 void arch_avx2_exec(char **argv)
 {
 	char *p = strstr(argv[0], ".avx2");
@@ -29,11 +31,9 @@ void arch_avx2_exec(char **argv)
 	if (p) {
 		*p = 0;
 	} else if (__builtin_cpu_supports("avx2")) {
-		char path[PATH_MAX];
-
-		snprintf(path, PATH_MAX, "%s.avx2", argv[0]);
-		argv[0] = path;
-		execve(path, argv, environ);
+		snprintf(avx2_path, PATH_MAX, "%s.avx2", argv[0]);
+		argv[0] = avx2_path;
+		execve(avx2_path, argv, environ);
 		perror("Can't run AVX2 build, using non-AVX2 version");
 	}
 }
-- 
2.35.1