From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
To: tech@openbsd.org
Date: Fri, 18 Sep 2026 16:57:32 +0200

This is a fist diff on a longer imsg journey in httpd. The goal is to
remove IMSG_SIZE_CHECK().

commit e08b6a50c4d986afa2b3eda5fb7661fcf7ed90f3
Author: Rafael Sadowski <rafael@sizeofvoid.org>
Date:   Sun Aug 30 08:50:26 2026 +0200

    httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro

diff --git a/config.c b/config.c
index ecaa59a..4001d86 100644
--- a/config.c
+++ b/config.c
@@ -130,8 +130,8 @@ config_getreset(struct httpd *env, struct imsg *imsg)
 {
 	unsigned int	 mode;
 
-	IMSG_SIZE_CHECK(imsg, &mode);
-	memcpy(&mode, imsg->data, sizeof(mode));
+	if (imsg_get_data(imsg, &mode, sizeof(mode)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	config_purge(env, mode);
 
@@ -621,8 +621,11 @@ config_getserver_config(struct httpd *env, struct server *srv,
 	if ((srv_conf = calloc(1, sizeof(*srv_conf))) == NULL)
 		return (-1);
 
-	IMSG_SIZE_CHECK(imsg, srv_conf);
-	memcpy(srv_conf, p, sizeof(*srv_conf));
+	if (imsg_get_data(imsg, srv_conf, sizeof(*srv_conf)) == -1) {
+		free(srv_conf);
+		fatalx("%s: imsg_get_data", __func__);
+	}
+
 	s = sizeof(*srv_conf);
 
 	/* Reset these variables to avoid free'ing invalid pointers */
@@ -804,8 +807,9 @@ config_getserver(struct httpd *env, struct imsg *imsg)
 	size_t			 s;
 	int			 fd;
 
-	IMSG_SIZE_CHECK(imsg, &srv_conf);
-	memcpy(&srv_conf, p, sizeof(srv_conf));
+	if (imsg_get_data(imsg, &srv_conf, sizeof(srv_conf)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
+
 	s = sizeof(srv_conf);
 
 	/* Reset these variables to avoid free'ing invalid pointers */
@@ -928,22 +932,27 @@ config_gettls(struct httpd *env, struct server_config *srv_conf,
 int
 config_getserver_tls(struct httpd *env, struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	struct server_config	*srv_conf = NULL;
 	struct tls_config	 tls_conf;
-	uint8_t			*p = imsg->data;
+	uint8_t			*data;
 	size_t			 len;
 
-	IMSG_SIZE_CHECK(imsg, &tls_conf);
-	memcpy(&tls_conf, p, sizeof(tls_conf));
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
 
-	len = tls_conf.tls_chunk_len;
+	if (ibuf_get(&ibuf, &tls_conf, sizeof(tls_conf)) == -1)
+		fatalx("%s: ibuf_get", __func__);
 
-	if ((IMSG_DATA_SIZE(imsg) - sizeof(tls_conf)) < len) {
+	if (ibuf_size(&ibuf) != tls_conf.tls_chunk_len) {
 		log_debug("%s: invalid message length", __func__);
 		goto fail;
 	}
 
-	p += sizeof(tls_conf);
+	data = ibuf_data(&ibuf);
+	len = tls_conf.tls_chunk_len;
 
 	if ((srv_conf = serverconfig_byid(tls_conf.id)) == NULL) {
 		log_debug("%s: server not found", __func__);
@@ -952,32 +961,32 @@ config_getserver_tls(struct httpd *env, struct imsg *imsg)
 
 	switch (tls_conf.tls_type) {
 	case TLS_CFG_CA:
-		if (config_gettls(env, srv_conf, &tls_conf, "ca", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "ca", data, len,
 		    &srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_CERT:
-		if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "cert", data, len,
 		    &srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_CRL:
-		if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "crl", data, len,
 		    &srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_KEY:
-		if (config_gettls(env, srv_conf, &tls_conf, "key", p, len,
+		if (config_gettls(env, srv_conf, &tls_conf, "key", data, len,
 		    &srv_conf->tls_key, &srv_conf->tls_key_len) != 0)
 			goto fail;
 		break;
 
 	case TLS_CFG_OCSP_STAPLE:
 		if (config_gettls(env, srv_conf, &tls_conf, "ocsp staple",
-		    p, len, &srv_conf->tls_ocsp_staple,
+		    data, len, &srv_conf->tls_ocsp_staple,
 		    &srv_conf->tls_ocsp_staple_len) != 0)
 			goto fail;
 		break;
@@ -1029,10 +1038,9 @@ config_getmedia(struct httpd *env, struct imsg *imsg)
 	struct privsep		*ps = env->sc_ps;
 #endif
 	struct media_type	 media;
-	uint8_t			*p = imsg->data;
 
-	IMSG_SIZE_CHECK(imsg, &media);
-	memcpy(&media, p, sizeof(media));
+	if (imsg_get_data(imsg, &media, sizeof(media)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	if (media_add(env->sc_mediatypes, &media) == NULL) {
 		log_debug("%s: failed to add media \"%s\"",
@@ -1085,10 +1093,9 @@ config_getauth(struct httpd *env, struct imsg *imsg)
 	struct privsep		*ps = env->sc_ps;
 #endif
 	struct auth		 auth;
-	uint8_t			*p = imsg->data;
 
-	IMSG_SIZE_CHECK(imsg, &auth);
-	memcpy(&auth, p, sizeof(auth));
+	if (imsg_get_data(imsg, &auth, sizeof(auth)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	if (auth_add(env->sc_auth, &auth) == NULL) {
 		log_debug("%s: failed to add auth \"%s[%u]\"",
diff --git a/httpd.c b/httpd.c
index 6d8d031..4b2303b 100644
--- a/httpd.c
+++ b/httpd.c
@@ -430,8 +430,8 @@ parent_dispatch_logger(int fd, struct privsep_proc *p, struct imsg *imsg)
 
 	switch (imsg->hdr.type) {
 	case IMSG_CTL_RESET:
-		IMSG_SIZE_CHECK(imsg, &v);
-		memcpy(&v, imsg->data, sizeof(v));
+		if (imsg_get_data(imsg, &v, sizeof(v)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		parent_reload(env, v, NULL);
 		break;
 	case IMSG_CTL_RELOAD:
diff --git a/httpd.h b/httpd.h
index 71377cf..99631ee 100644
--- a/httpd.h
+++ b/httpd.h
@@ -164,10 +164,6 @@ struct imsgev {
 	short			 events;
 };
 
-#define IMSG_SIZE_CHECK(imsg, p) do {				\
-	if (IMSG_DATA_SIZE(imsg) < sizeof(*p))			\
-		fatalx("bad length imsg received");		\
-} while (0)
 #define IMSG_DATA_SIZE(imsg)	((imsg)->hdr.len - IMSG_HEADER_SIZE)
 #define MAX_IMSG_DATA_SIZE	(MAX_IMSGSIZE - IMSG_HEADER_SIZE)
 
diff --git a/logger.c b/logger.c
index c2eae70..049a5b1 100644
--- a/logger.c
+++ b/logger.c
@@ -140,8 +140,8 @@ logger_open_fd(struct imsg *imsg)
 	struct log_file		*log;
 	uint32_t		 id;
 
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
+	if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
+		fatalx("%s: imsg_get_data", __func__);
 
 	TAILQ_FOREACH(log, &log_files, log_entry) {
 		if (log->log_id == id) {
@@ -158,19 +158,37 @@ logger_open_fd(struct imsg *imsg)
 int
 logger_open_priv(struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	char			 path[PATH_MAX];
 	char			 name[PATH_MAX], *p;
+	char			*filename = NULL;
 	uint32_t		 id;
 	size_t			 len;
-	int			 fd;
+	int			 fd, ret;
+
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
 
 	/* called from the privileged process */
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
-	p = (char *)imsg->data + sizeof(id);
+	if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
+		fatalx("%s: ibuf_get", __func__);
 
-	if ((size_t)snprintf(name, sizeof(name), "/%s", p) >= sizeof(name))
+	if ((len = ibuf_size(&ibuf)) == 0) {
+		log_debug("%s: invalid message length", __func__);
 		return (-1);
+	}
+
+	if ((filename = ibuf_get_string(&ibuf, len)) == NULL) {
+		log_warn("%s: ibuf_get_string", __func__);
+		return (-1);
+	}
+	ret = snprintf(name, sizeof(name), "/%s", filename);
+	free(filename);
+	if (ret < 0 || (size_t)ret >= sizeof(name))
+		return (-1);
+
 	if ((len = strlcpy(path, httpd_env->sc_logdir, sizeof(path))) >=
 	    sizeof(path))
 		return (-1);
@@ -243,13 +261,20 @@ logger_start(void)
 int
 logger_log(struct imsg *imsg)
 {
+	struct ibuf		 ibuf;
 	char			*logline;
 	uint32_t		 id;
 	struct server_config	*srv_conf;
 	struct log_file		*log;
 
-	IMSG_SIZE_CHECK(imsg, &id);
-	memcpy(&id, imsg->data, sizeof(id));
+	if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+		log_warn("%s: imsg_get_ibuf", __func__);
+		return (-1);
+	}
+
+	/* called from the privileged process */
+	if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
+		fatalx("%s: ibuf_get", __func__);
 
 	if ((srv_conf = serverconfig_byid(id)) == NULL)
 		fatalx("invalid logging requestr");
@@ -265,7 +290,7 @@ logger_log(struct imsg *imsg)
 	}
 
 	/* XXX get_string() would sanitize the string, but add a malloc */
-	logline = (char *)imsg->data + sizeof(id);
+	logline = ibuf_data(&ibuf);
 
 	/* For debug output */
 	log_debug("%s", logline);
diff --git a/server.c b/server.c
index bc96722..f3cbb7c 100644
--- a/server.c
+++ b/server.c
@@ -1408,8 +1408,8 @@ server_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
 			return (-1);
 		break;
 	case IMSG_TLSTICKET_REKEY:
-		IMSG_SIZE_CHECK(imsg, (&key));
-		memcpy(&key, imsg->data, sizeof(key));
+		if (imsg_get_data(imsg, &key, sizeof(key)) == -1)
+			fatalx("%s: imsg_get_data", __func__);
 		/* apply to the right server */
 		if ((srv = server_byid(key.tt_id)) == NULL) {
 			log_debug("%s: invalid sever id", __func__);