[PATCH] Make assertions actually useful

[PATCH] Make assertions actually useful

[David Gibson]

There are some places in passt/pasta which #include <assert.h> and make
various assertions.  If we hit these something has already gone wrong, but
they're there so that we a useful message instead of cryptic misbehaviour
if assumptions we thought were correct turn out not to be.

Except.. the glibc implementation of assert() uses syscalls that aren't in
our seccomp filter, so we'll get a SIGSYS before it actually prints the
message.  Work around this by adding our own ASSERT() implementation using
our existing err() function to log the message, and an abort().  The
abort() probably also won't work exactly right with seccomp, but once we've
printed the message, dying with a SIGSYS works just as well as dying with
a SIGABRT.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 inany.h      |  6 ++----
 lineread.c   | 10 +++++-----
 tcp.c        |  9 ++++-----
 tcp_splice.c |  3 +--
 udp.c        |  3 +--
 util.c       |  1 -
 util.h       | 20 +++++++++++++++++++-
 7 files changed, 32 insertions(+), 20 deletions(-)

diff --git a/inany.h b/inany.h
index 52d43f2..7ff32f0 100644
--- a/inany.h
+++ b/inany.h
@@ -6,8 +6,6 @@
  *           IPv6 or IPv4 (encoded as IPv4-mapped IPv6 addresses)
  */
 
-#include <assert.h>
-
 /** union inany_addr - Represents either an IPv4 or IPv6 address
  * @a6:			Address as an IPv6 address, may be IPv4-mapped
  * @v4mapped.zero:	All zero-bits for an IPv4 address
@@ -63,7 +61,7 @@ static inline void inany_from_af(union inany_addr *aa, int af, const void *addr)
 		aa->v4mapped.a4 = *((struct in_addr *)addr);
 	} else {
 		/* Not valid to call with other address families */
-		assert(0);
+		ASSERT(0);
 	}
 }
 
@@ -89,6 +87,6 @@ static inline void inany_from_sockaddr(union inany_addr *aa, in_port_t *port,
 		*port = ntohs(sa4->sin_port);
 	} else {
 		/* Not valid to call with other address families */
-		assert(0);
+		ASSERT(0);
 	}
 }
diff --git a/lineread.c b/lineread.c
index 59021e6..6ffb14e 100644
--- a/lineread.c
+++ b/lineread.c
@@ -16,10 +16,10 @@
 #include <fcntl.h>
 #include <string.h>
 #include <stdbool.h>
-#include <assert.h>
 #include <unistd.h>
 
 #include "lineread.h"
+#include "util.h"
 
 /**
  * lineread_init() - Prepare for line by line file reading without allocation
@@ -44,10 +44,10 @@ static int peek_line(struct lineread *lr, bool eof)
 	char *nl;
 
 	/* Sanity checks (which also document invariants) */
-	assert(lr->count >= 0);
-	assert(lr->next_line >= 0);
-	assert(lr->next_line + lr->count >= lr->next_line);
-	assert(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
+	ASSERT(lr->count >= 0);
+	ASSERT(lr->next_line >= 0);
+	ASSERT(lr->next_line + lr->count >= lr->next_line);
+	ASSERT(lr->next_line + lr->count <= LINEREAD_BUFFER_SIZE);
 
 	nl = memchr(lr->buf + lr->next_line, '\n', lr->count);
 
diff --git a/tcp.c b/tcp.c
index 4744ac5..475c23f 100644
--- a/tcp.c
+++ b/tcp.c
@@ -288,7 +288,6 @@
 #include <sys/uio.h>
 #include <unistd.h>
 #include <time.h>
-#include <assert.h>
 
 #include <linux/tcp.h> /* For struct tcp_info */
 
@@ -602,7 +601,7 @@ static inline struct tcp_tap_conn *conn_at_idx(int index)
 {
 	if ((index < 0) || (index >= TCP_MAX_CONNS))
 		return NULL;
-	assert(!(CONN(index)->c.spliced));
+	ASSERT(!(CONN(index)->c.spliced));
 	return CONN(index);
 }
 
@@ -2809,7 +2808,7 @@ static void tcp_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	socklen_t sl;
 	int s;
 
-	assert(ref.r.p.tcp.tcp.listen);
+	ASSERT(ref.r.p.tcp.tcp.listen);
 
 	if (c->tcp.conn_count >= TCP_MAX_CONNS)
 		return;
@@ -3051,7 +3050,7 @@ static void tcp_ns_sock_init4(const struct ctx *c, in_port_t port)
 	struct in_addr loopback = { htonl(INADDR_LOOPBACK) };
 	int s;
 
-	assert(c->mode == MODE_PASTA);
+	ASSERT(c->mode == MODE_PASTA);
 
 	s = sock_l4(c, AF_INET, IPPROTO_TCP, &loopback, NULL, port, tref.u32);
 	if (s >= 0)
@@ -3075,7 +3074,7 @@ static void tcp_ns_sock_init6(const struct ctx *c, in_port_t port)
 				     .tcp.index = idx };
 	int s;
 
-	assert(c->mode == MODE_PASTA);
+	ASSERT(c->mode == MODE_PASTA);
 
 	s = sock_l4(c, AF_INET6, IPPROTO_TCP, &in6addr_loopback, NULL, port,
 		    tref.u32);
diff --git a/tcp_splice.c b/tcp_splice.c
index 72b1672..1d624f1 100644
--- a/tcp_splice.c
+++ b/tcp_splice.c
@@ -46,7 +46,6 @@
 #include <sys/epoll.h>
 #include <sys/types.h>
 #include <sys/socket.h>
-#include <assert.h>
 
 #include "util.h"
 #include "passt.h"
@@ -519,7 +518,7 @@ bool tcp_splice_conn_from_sock(struct ctx *c, union epoll_ref ref,
 	union inany_addr aany;
 	in_port_t port;
 
-	assert(c->mode == MODE_PASTA);
+	ASSERT(c->mode == MODE_PASTA);
 
 	inany_from_sockaddr(&aany, &port, sa);
 	a4 = inany_v4(&aany);
diff --git a/udp.c b/udp.c
index dbfdb61..9c63a8c 100644
--- a/udp.c
+++ b/udp.c
@@ -108,7 +108,6 @@
 #include <sys/uio.h>
 #include <unistd.h>
 #include <time.h>
-#include <assert.h>
 
 #include "checksum.h"
 #include "util.h"
@@ -255,7 +254,7 @@ static void udp_invert_portmap(struct udp_port_fwd *fwd)
 {
 	int i;
 
-	assert(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta));
+	ASSERT(ARRAY_SIZE(fwd->f.delta) == ARRAY_SIZE(fwd->rdelta));
 	for (i = 0; i < ARRAY_SIZE(fwd->f.delta); i++) {
 		in_port_t delta = fwd->f.delta[i];
 
diff --git a/util.c b/util.c
index d9858f2..c5ee1c0 100644
--- a/util.c
+++ b/util.c
@@ -23,7 +23,6 @@
 #include <time.h>
 #include <errno.h>
 #include <stdbool.h>
-#include <assert.h>
 
 #include "util.h"
 #include "passt.h"
diff --git a/util.h b/util.h
index 3baec91..a367f96 100644
--- a/util.h
+++ b/util.h
@@ -6,6 +6,11 @@
 #ifndef UTIL_H
 #define UTIL_H
 
+#include <stdlib.h>
+#include <stdarg.h>
+
+#include "log.h"
+
 #define VERSION_BLOB							       \
 	VERSION "\n"							       \
 	"Copyright Red Hat\n"						       \
@@ -47,6 +52,18 @@
 #define STRINGIFY(x)	#x
 #define STR(x)		STRINGIFY(x)
 
+#define ASSERT(expr)							\
+	do {								\
+		if (!(expr)) {						\
+			err("ASSERTION FAILED in %s (%s:%d): %s",	\
+			    __func__, __FILE__, __LINE__, STRINGIFY(expr)); \
+			/* This may actually SIGSYS, due to seccomp,	\
+			 * but that will still get the job done		\
+			 */						\
+			abort();					\
+		}							\
+	} while (0)
+
 #ifdef P_tmpdir
 #define TMPDIR		P_tmpdir
 #else
@@ -163,7 +180,8 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 
 #include <net/if.h>
 #include <limits.h>
-#include <stdarg.h>
+#include <stdint.h>
+#include <netinet/ip6.h>
 
 #include "packet.h"
 
-- 
@@ -6,6 +6,11 @@
 #ifndef UTIL_H
 #define UTIL_H
 
+#include <stdlib.h>
+#include <stdarg.h>
+
+#include "log.h"
+
 #define VERSION_BLOB							       \
 	VERSION "\n"							       \
 	"Copyright Red Hat\n"						       \
@@ -47,6 +52,18 @@
 #define STRINGIFY(x)	#x
 #define STR(x)		STRINGIFY(x)
 
+#define ASSERT(expr)							\
+	do {								\
+		if (!(expr)) {						\
+			err("ASSERTION FAILED in %s (%s:%d): %s",	\
+			    __func__, __FILE__, __LINE__, STRINGIFY(expr)); \
+			/* This may actually SIGSYS, due to seccomp,	\
+			 * but that will still get the job done		\
+			 */						\
+			abort();					\
+		}							\
+	} while (0)
+
 #ifdef P_tmpdir
 #define TMPDIR		P_tmpdir
 #else
@@ -163,7 +180,8 @@ int do_clone(int (*fn)(void *), char *stack_area, size_t stack_size, int flags,
 
 #include <net/if.h>
 #include <limits.h>
-#include <stdarg.h>
+#include <stdint.h>
+#include <netinet/ip6.h>
 
 #include "packet.h"
 
-- 
2.39.0

Re: [PATCH] Make assertions actually useful

[Stefano Brivio]

On Mon, 16 Jan 2023 14:15:27 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> There are some places in passt/pasta which #include <assert.h> and make
> various assertions.  If we hit these something has already gone wrong, but
> they're there so that we a useful message instead of cryptic misbehaviour
> if assumptions we thought were correct turn out not to be.
> 
> Except.. the glibc implementation of assert() uses syscalls that aren't in
> our seccomp filter, so we'll get a SIGSYS before it actually prints the
> message.  Work around this by adding our own ASSERT() implementation using
> our existing err() function to log the message, and an abort().  The
> abort() probably also won't work exactly right with seccomp, but once we've
> printed the message, dying with a SIGSYS works just as well as dying with
> a SIGABRT.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Applied.

-- 
Stefano