[PATCH v2 0/6] Fix issues and false positives from static analysis
This series fixes minor issues found by static analysis tools in the main passt code and in documentation examples. The core code changes add missing fd initialisations and guard unsigned arithmetic against potential underflow. We also fix buffer handling, type mismatches, and resource leaks in the documentation example code. v2: - udp_vu, tap: Use if-guards instead of assert(), to handle the error paths properly and let the static checker follow the logic - doc/migration: Use snprintf() instead of strncpy() for clarity and guaranteed null-termination (David Gibson) - Drop va_list initialisation patch (Stefano Brivio, David Gibson) Jon Maloy (6): passt: Initialise listening socket fds to -1 udp_vu: Check iov_tail_clone() return before assigning to msg_iovlen tap: Guard IPv6 tail size subtraction against underflow doc/migration: Use snprintf() for socket path and fix argv access doc/migration: Fix buffer type mismatch in recv() call doc/platform-requirements: Close leaked sockets in test_close_dup() doc/migration/source.c | 2 +- doc/migration/target.c | 10 ++++------ doc/platform-requirements/udp-close-dup.c | 3 +++ passt.c | 3 +++ tap.c | 5 ++++- udp_vu.c | 8 ++++++-- 6 files changed, 21 insertions(+), 10 deletions(-) -- 2.52.0
iov_tail_clone() returns ssize_t and can return -1 if the destination
iov array is too small. Its return value was assigned directly to
msg.msg_iovlen which is size_t, wrapping a negative value to a large
unsigned number passed to recvmsg().
Check for failure and return early, letting the caller rewind the
virtqueue.
Signed-off-by: Jon Maloy
fd_tap_listen, fd_control_listen, and fd_repair_listen are file
descriptors and should be initialised to -1 rather than the implicit
0, consistent with the other fd fields in passt_ctx.
Signed-off-by: Jon Maloy
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes
the expected payload length. IOV_PEEK_HEADER() guarantees at least
sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned
subtraction. A too-small tail would indicate a malformed packet, so
skip it.
Signed-off-by: Jon Maloy
Replace strcpy() with snprintf() when copying the helper socket path
into sun_path, preventing a potential buffer overflow from a long
argument and guaranteeing null-termination.
In target.c, move the argc check before accessing argv[4], so we
don't dereference past the end of argv.
Signed-off-by: Jon Maloy
s1 and send_s were not closed before returning, leaking file
descriptors across the loop iterations in main().
Signed-off-by: Jon Maloy
The compound literal passed to recv() was int (4 bytes) but only
1 byte is received. Use int8_t to match the length, consistent with
the other recv() calls in this file and in source.c.
Signed-off-by: Jon Maloy
On Thu, Jul 09, 2026 at 05:56:54PM -0400, Jon Maloy wrote:
Replace strcpy() with snprintf() when copying the helper socket path into sun_path, preventing a potential buffer overflow from a long argument and guaranteeing null-termination.
In target.c, move the argc check before accessing argv[4], so we don't dereference past the end of argv.
Signed-off-by: Jon Maloy
Reviewed-by: David Gibson
--- v2: Use snprintf() instead of strncpy() for clarity and guaranteed null-termination, as suggested by David Gibson --- doc/migration/source.c | 2 +- doc/migration/target.c | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/doc/migration/source.c b/doc/migration/source.c index d44ebf1f..8aad0259 100644 --- a/doc/migration/source.c +++ b/doc/migration/source.c @@ -47,7 +47,7 @@ int main(int argc, char **argv) return -1; }
- strcpy(a_helper.sun_path, argv[4]); + snprintf(a_helper.sun_path, sizeof(a_helper.sun_path), "%s", argv[4]); getaddrinfo(argv[1], argv[2], &hints, &r);
/* Connect socket to server and send some data */ diff --git a/doc/migration/target.c b/doc/migration/target.c index f7d31083..e2469837 100644 --- a/doc/migration/target.c +++ b/doc/migration/target.c @@ -38,11 +38,6 @@ int main(int argc, char **argv) struct cmsghdr *cmsg = CMSG_FIRSTHDR(&msg); struct addrinfo *r;
- (void)argc; - - strcpy(a_helper.sun_path, argv[4]); - getaddrinfo(argv[1], argv[2], &hints, &r); - if (argc != 7) { fprintf(stderr, "%s DST_ADDR DST_PORT SRC_PORT HELPER_PATH SSEQ RSEQ\n", @@ -50,6 +45,9 @@ int main(int argc, char **argv) return -1; }
+ snprintf(a_helper.sun_path, sizeof(a_helper.sun_path), "%s", argv[4]); + getaddrinfo(argv[1], argv[2], &hints, &r); + /* Prepare socket, bind to source port */ s = socket(r->ai_family, SOCK_STREAM, IPPROTO_TCP); setsockopt(s, SOL_SOCKET, SO_REUSEADDR, &((int){ 1 }), sizeof(int)); -- 2.52.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, Jul 09, 2026 at 05:56:53PM -0400, Jon Maloy wrote:
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes the expected payload length. IOV_PEEK_HEADER() guarantees at least sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned subtraction. A too-small tail would indicate a malformed packet, so skip it.
Signed-off-by: Jon Maloy
--- v2: Use if-guard instead of assert(), to avoid runtime cost in per-packet path and to let the static checker follow the logic.
Why would the if be cheaper than an assert()? The IOV_PEEK_HEADER() already checks the length, so this check is definitely redundant - it exists only for the benefit of static checkers.
--- tap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tap.c b/tap.c index 6d93c7ce..6fd5f595 100644 --- a/tap.c +++ b/tap.c @@ -986,7 +986,10 @@ resume: if (!ip6h) continue;
- check = iov_tail_size(&data) - sizeof(*ip6h); + check = iov_tail_size(&data); + if (check < sizeof(*ip6h)) + continue; + check -= sizeof(*ip6h);
saddr = &ip6h->saddr; daddr = &ip6h->daddr; -- 2.52.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, Jul 09, 2026 at 05:56:52PM -0400, Jon Maloy wrote:
iov_tail_clone() returns ssize_t and can return -1 if the destination iov array is too small. Its return value was assigned directly to msg.msg_iovlen which is size_t, wrapping a negative value to a large unsigned number passed to recvmsg().
Check for failure and return early, letting the caller rewind the virtqueue.
Signed-off-by: Jon Maloy
Reviewed-by: David Gibson
--- v2: Use if-guard instead of assert(), since the error path is a legitimate (if unlikely) condition handled by the caller. --- udp_vu.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/udp_vu.c b/udp_vu.c index e4fb1057..edd9cae7 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -67,11 +67,15 @@ static ssize_t udp_vu_sock_recv(struct iov_tail *payload, size_t *cnt, int s) struct iovec msg_iov[VIRTQUEUE_MAX_SIZE]; struct msghdr msg = { 0 }; size_t iov_used; + ssize_t iovlen; ssize_t dlen;
+ iovlen = iov_tail_clone(msg_iov, ARRAY_SIZE(msg_iov), payload); + if (iovlen < 0) + return -1; + msg.msg_iov = msg_iov; - msg.msg_iovlen = iov_tail_clone(msg.msg_iov, ARRAY_SIZE(msg_iov), - payload); + msg.msg_iovlen = iovlen;
/* read data from the socket */ dlen = recvmsg(s, &msg, 0); -- 2.52.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On 2026-07-09 21:36, David Gibson wrote:
On Thu, Jul 09, 2026 at 05:56:53PM -0400, Jon Maloy wrote:
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes the expected payload length. IOV_PEEK_HEADER() guarantees at least sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned subtraction. A too-small tail would indicate a malformed packet, so skip it.
Signed-off-by: Jon Maloy
--- v2: Use if-guard instead of assert(), to avoid runtime cost in per-packet path and to let the static checker follow the logic.
I had the idea that the compiler may be smart enough to optimize it out, since it is logically redundant, as you note. /jon
Why would the if be cheaper than an assert()? The IOV_PEEK_HEADER() already checks the length, so this check is definitely redundant - it exists only for the benefit of static checkers.
--- tap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tap.c b/tap.c index 6d93c7ce..6fd5f595 100644 --- a/tap.c +++ b/tap.c @@ -986,7 +986,10 @@ resume: if (!ip6h) continue;
- check = iov_tail_size(&data) - sizeof(*ip6h); + check = iov_tail_size(&data); + if (check < sizeof(*ip6h)) + continue; + check -= sizeof(*ip6h);
saddr = &ip6h->saddr; daddr = &ip6h->daddr; -- 2.52.0
On Fri, Jul 10, 2026 at 09:40:03AM -0400, Jon Maloy wrote:
On 2026-07-09 21:36, David Gibson wrote:
On Thu, Jul 09, 2026 at 05:56:53PM -0400, Jon Maloy wrote:
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes the expected payload length. IOV_PEEK_HEADER() guarantees at least sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned subtraction. A too-small tail would indicate a malformed packet, so skip it.
Signed-off-by: Jon Maloy
--- v2: Use if-guard instead of assert(), to avoid runtime cost in per-packet path and to let the static checker follow the logic.
I had the idea that the compiler may be smart enough to optimize it out, since it is logically redundant, as you note.
I mean, maybe, although I think that's moderately unlikely. But assert() is nothing but a macro that says essentially if (!condition) abort(); So if the compiler can optimise away the if, it can optimise away the assert() as well.
/jon
Why would the if be cheaper than an assert()? The IOV_PEEK_HEADER() already checks the length, so this check is definitely redundant - it exists only for the benefit of static checkers.
--- tap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tap.c b/tap.c index 6d93c7ce..6fd5f595 100644 --- a/tap.c +++ b/tap.c @@ -986,7 +986,10 @@ resume: if (!ip6h) continue; - check = iov_tail_size(&data) - sizeof(*ip6h); + check = iov_tail_size(&data); + if (check < sizeof(*ip6h)) + continue; + check -= sizeof(*ip6h); saddr = &ip6h->saddr; daddr = &ip6h->daddr; -- 2.52.0
-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On 2026-07-13 00:09, David Gibson wrote:
On Fri, Jul 10, 2026 at 09:40:03AM -0400, Jon Maloy wrote:
On 2026-07-09 21:36, David Gibson wrote:
On Thu, Jul 09, 2026 at 05:56:53PM -0400, Jon Maloy wrote:
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes the expected payload length. IOV_PEEK_HEADER() guarantees at least sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned subtraction. A too-small tail would indicate a malformed packet, so skip it.
Signed-off-by: Jon Maloy
--- v2: Use if-guard instead of assert(), to avoid runtime cost in per-packet path and to let the static checker follow the logic.
I had the idea that the compiler may be smart enough to optimize it out, since it is logically redundant, as you note.
I mean, maybe, although I think that's moderately unlikely. But assert() is nothing but a macro that says essentially if (!condition) abort(); So if the compiler can optimise away the if, it can optimise away the assert() as well.
Yes. But the analyzer will still mark it as an issue, which it won't if there is an if-clause. /jon
/jon
Why would the if be cheaper than an assert()? The IOV_PEEK_HEADER() already checks the length, so this check is definitely redundant - it exists only for the benefit of static checkers.
--- tap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tap.c b/tap.c index 6d93c7ce..6fd5f595 100644 --- a/tap.c +++ b/tap.c @@ -986,7 +986,10 @@ resume: if (!ip6h) continue; - check = iov_tail_size(&data) - sizeof(*ip6h); + check = iov_tail_size(&data); + if (check < sizeof(*ip6h)) + continue; + check -= sizeof(*ip6h); saddr = &ip6h->saddr; daddr = &ip6h->daddr; -- 2.52.0
On Mon, Jul 13, 2026 at 06:17:17AM -0400, Jon Maloy wrote:
On 2026-07-13 00:09, David Gibson wrote:
On Fri, Jul 10, 2026 at 09:40:03AM -0400, Jon Maloy wrote:
On 2026-07-09 21:36, David Gibson wrote:
On Thu, Jul 09, 2026 at 05:56:53PM -0400, Jon Maloy wrote:
In tap6_handler(), iov_tail_size(&data) - sizeof(*ip6h) computes the expected payload length. IOV_PEEK_HEADER() guarantees at least sizeof(*ip6h) bytes, but add an explicit check to guard the unsigned subtraction. A too-small tail would indicate a malformed packet, so skip it.
Signed-off-by: Jon Maloy
--- v2: Use if-guard instead of assert(), to avoid runtime cost in per-packet path and to let the static checker follow the logic.
I had the idea that the compiler may be smart enough to optimize it out, since it is logically redundant, as you note.
I mean, maybe, although I think that's moderately unlikely. But assert() is nothing but a macro that says essentially if (!condition) abort(); So if the compiler can optimise away the if, it can optimise away the assert() as well.
Yes. But the analyzer will still mark it as an issue, which it won't if there is an if-clause.
Um.. what? I'm struggling to think of any circumstance in which that would be the case. -- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson
On Thu, 9 Jul 2026 17:56:50 -0400
Jon Maloy
This series fixes minor issues found by static analysis tools in the main passt code and in documentation examples.
The core code changes add missing fd initialisations and guard unsigned arithmetic against potential underflow.
We also fix buffer handling, type mismatches, and resource leaks in the documentation example code.
v2: - udp_vu, tap: Use if-guards instead of assert(), to handle the error paths properly and let the static checker follow the logic - doc/migration: Use snprintf() instead of strncpy() for clarity and guaranteed null-termination (David Gibson) - Drop va_list initialisation patch (Stefano Brivio, David Gibson)
Applied up to 2/6. For the rest, I would rather wait for the discussion around 3/6 to come to an end, also because I guess patches 4/6 to 6/6 aren't that much of a priority. I haven't seen any other report of static checkers running on documentation examples and actually it doesn't make a lot of sense to me to do so. -- Stefano
participants (3)
-
David Gibson
-
Jon Maloy
-
Stefano Brivio