From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: [6/7] relayd: read parent_dispatch_pfe() payloads via the imsg getters
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Mon, 8 Jun 2026 15:47:15 +0200

On Sun, Jun 07, 2026 at 09:04:36AM +0200, Rafael Sadowski wrote:
> This is a series of commits for an imsg APIv2 rework in relayd. These
> were committed and tested individually. They can be reviewed in their
> entirety here:
> 
> gothub: https://rsadowski.gothub.org/?action=summary&headref=imsg_v2&path=relayd.git
> PR: https://codeberg.org/rsadowski/relayd/pulls/1/files
> 
> commit a047f4ec20bb24e35de4813ad734b4695e5bbe74
> Author: Rafael Sadowski <rafael@sizeofvoid.org>
> Date:   Sat Jun 6 21:11:22 2026 +0200
> 
>     relayd: read parent_dispatch_pfe() payloads via the imsg getters
>     
>     Use imsg_get_data() for the fixed-size messages and imsg_get_ibuf() for
>     the variable-length IMSG_CTL_RELOAD path, taking the config name from
>     the ibuf via ibuf_data()/ibuf_size().
>     
>     Remove IMSG_SIZE_CHECK and IMSG_DATA_SIZE, no consumer left.
> 
> diff --git a/relayd.c b/relayd.c
> index 92c4320..b678103 100644
> --- a/relayd.c
> +++ b/relayd.c
> @@ -399,36 +399,49 @@ parent_shutdown(struct relayd *env)
>  int
>  parent_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
>  {
> +	struct ibuf		 ibuf;
>  	struct privsep		*ps = p->p_ps;
>  	struct relayd		*env = ps->ps_env;
>  	struct ctl_demote	 demote;
>  	struct ctl_netroute	 crt;
>  	u_int			 v;
>  	char			*str = NULL;
> +	size_t			 s;
>  
>  	switch (imsg->hdr.type) {
>  	case IMSG_DEMOTE:
> -		IMSG_SIZE_CHECK(imsg, &demote);
> -		memcpy(&demote, imsg->data, sizeof(demote));
> +		if (imsg_get_data(imsg, &demote, sizeof(demote)) == -1) {
> +			log_warn("%s: imsg_get_data", __func__);
> +			return (-1);
> +		}
>  		demote.group[sizeof(demote.group) - 1] = '\0';
>  		carp_demote_set(demote.group, demote.level);
>  		break;
>  	case IMSG_RTMSG:
> -		IMSG_SIZE_CHECK(imsg, &crt);
> -		memcpy(&crt, imsg->data, sizeof(crt));
> +		if (imsg_get_data(imsg, &crt, sizeof(crt)) == -1) {
> +			log_warn("%s: imsg_get_data", __func__);
> +			return (-1);
> +		}
>  		crt.host.name[sizeof(crt.host.name) - 1] = '\0';
>  		crt.rt.name[sizeof(crt.rt.name) - 1] = '\0';
>  		crt.rt.label[sizeof(crt.rt.label) - 1] = '\0';
>  		pfe_route(env, &crt);
>  		break;
>  	case IMSG_CTL_RESET:
> -		IMSG_SIZE_CHECK(imsg, &v);
> -		memcpy(&v, imsg->data, sizeof(v));
> +		if (imsg_get_data(imsg, &v, sizeof(v)) == -1) {
> +			log_warn("%s: imsg_get_data", __func__);
> +			return (-1);
> +		}
>  		parent_reload(env, v, NULL);
>  		break;
>  	case IMSG_CTL_RELOAD:
> -		if (IMSG_DATA_SIZE(imsg) > 0)
> -			str = get_string(imsg->data, IMSG_DATA_SIZE(imsg));
> +		if (imsg_get_ibuf(imsg, &ibuf) != -1 &&
> +		    (s = ibuf_size(&ibuf)) > 0) {
> +			if ((str = get_string(ibuf_data(&ibuf), s)) == NULL) {

Another get_string(). Another little horror show.

> +				log_warn("%s: get_string", __func__);
> +				return (-1);
> +			}
> +		}
>  		parent_reload(env, CONFIG_RELOAD, str);
>  		free(str);
>  		break;
> @@ -457,8 +470,10 @@ parent_dispatch_hce(int fd, struct privsep_proc *p, struct imsg *imsg)
>  
>  	switch (imsg->hdr.type) {
>  	case IMSG_SCRIPT:
> -		IMSG_SIZE_CHECK(imsg, &scr);
> -		bcopy(imsg->data, &scr, sizeof(scr));
> +		if (imsg_get_data(imsg, &scr, sizeof(scr)) == -1) {
> +			log_warn("%s: imsg_get_data", __func__);
> +			return (-1);
> +		}
>  		scr.name[sizeof(scr.name) - 1] = '\0';
>  		scr.path[sizeof(scr.path) - 1] = '\0';
>  		scr.retval = script_exec(env, &scr);
> @@ -484,8 +499,11 @@ parent_dispatch_relay(int fd, struct privsep_proc *p, struct imsg *imsg)
>  
>  	switch (imsg->hdr.type) {
>  	case IMSG_BINDANY:
> -		IMSG_SIZE_CHECK(imsg, &bnd);
> -		bcopy(imsg->data, &bnd, sizeof(bnd));
> +		if (imsg_get_data(imsg, &bnd, sizeof(bnd)) == -1) {
> +			log_warn("%s: imsg_get_data", __func__);
> +			return (-1);
> +		}
> +
>  		if (bnd.bnd_proc < 0 || bnd.bnd_proc > env->sc_conf.prefork_relay)
>  			fatalx("%s: invalid relay proc", __func__);
>  		switch (bnd.bnd_proto) {
> diff --git a/relayd.h b/relayd.h
> index 373255e..ac64966 100644
> --- a/relayd.h
> +++ b/relayd.h
> @@ -930,12 +930,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)
> -
>  struct ctl_conn {
>  	TAILQ_ENTRY(ctl_conn)	 entry;
>  	u_int8_t		 flags;
> 

I'm very happy about this last bit of the diff.

-- 
:wq Claudio