From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Fri, 18 Sep 2026 17:02:44 +0200

On Fri, Sep 18, 2026 at 04:57:32PM +0200, Rafael Sadowski wrote:
> This is a fist diff on a longer imsg journey in httpd. The goal is to
> remove IMSG_SIZE_CHECK().

Hell yes. this is lovely.
Comments below.
 
> 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__);

fatalx is terminal. So no need to free() something before.

> +	}
> +
>  	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__);
> 

Rest look OK.

-- 
:wq Claudio