On Thu, Jul 30, 2026 at 09:59:27AM +0200, Rafael Sadowski wrote:
> Use imsg_get_data() with size validation instead of casting imsg->data
> directly and imsg_get_type/len/id/pid() instead of imsg->hdr.* access.
> 
> Tested with all options. OK?

OK claudio@

Some comments inline.
 
> diff --git a/relayctl.c b/relayctl.c
> index e228ae8..405143c 100644
> --- a/relayctl.c
> +++ b/relayctl.c
> @@ -292,7 +292,8 @@ monitor_host_status(struct imsg *imsg)
>  {
>  	struct ctl_status	 cs;
>  
> -	memcpy(&cs, imsg->data, sizeof(cs));
> +	if (imsg_get_data(imsg, &cs, sizeof(cs)) == -1)
> +		errx(1, "imsg_get_data ctl status error");
>  	printf("\tid: %u\n", cs.id);
>  	printf("\tstate: ");
>  	switch (cs.up) {
> @@ -313,7 +314,8 @@ monitor_id(struct imsg *imsg)
>  {
>  	struct ctl_id		 id;
>  
> -	memcpy(&id, imsg->data, sizeof(id));
> +	if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
> +		errx(1, "imsg_get_data ctl id error");
>  	printf("\tid: %u\n", id.id);
>  	if (strlen(id.name))
>  		printf("\tname: %s\n", id.name);
> @@ -328,9 +330,9 @@ monitor(struct imsg *imsg)
>  
>  	now = time(NULL);
>  
> -	imn = monitor_lookup(imsg->hdr.type);
> -	printf("%s: imsg type %u len %u peerid %u pid %d\n", imn->name,
> -	    imsg->hdr.type, imsg->hdr.len, imsg->hdr.peerid, imsg->hdr.pid);
> +	imn = monitor_lookup(imsg_get_type(imsg));
> +	printf("%s: imsg type %u len %zu peerid %u pid %d\n", imn->name,
> +	    imsg_get_type(imsg), imsg_get_len(imsg), imsg_get_id(imsg), imsg_get_pid(imsg));

Line got too long.

>  	printf("\ttimestamp: %lld, %s", (long long)now, ctime(&now));
>  	if (imn->type == -1)
>  		done = 1;
> @@ -343,102 +345,110 @@ monitor(struct imsg *imsg)
>  int
>  show_summary_msg(struct imsg *imsg, int type)
>  {
> -	struct rdr		*rdr;
> -	struct table		*table;
> -	struct host		*host;
> -	struct relay		*rlay;
> -	struct router		*rt;
> -	struct netroute		*nr;
> +	struct rdr		 rdr;
> +	struct table		 table;
> +	struct host		 host;
> +	struct relay		 rlay;
> +	struct router		 rt;
> +	struct netroute		 nr;
>  	struct ctl_stats	 stats[PROC_MAX_INSTANCES];
>  	char			 name[HOST_NAME_MAX + 1];
>  
> -	switch (imsg->hdr.type) {
> +	switch (imsg_get_type(imsg)) {
>  	case IMSG_CTL_RDR:
>  		if (!(type == SHOW_SUM || type == SHOW_RDRS))
>  			break;
> -		rdr = imsg->data;
> +		if (imsg_get_data(imsg, &rdr, sizeof(rdr)) == -1)
> +			errx(1, "imsg_get_data rdr error");
>  		printf("%-4u\t%-8s\t%-24s\t%-7s\t%s\n",
> -		    rdr->conf.id, "redirect", rdr->conf.name, "",
> -		    print_rdr_status(rdr->conf.flags));
> +		    rdr.conf.id, "redirect", rdr.conf.name, "",
> +		    print_rdr_status(rdr.conf.flags));

This and various other structs use embedded strings. In general if you
have such a struct and it crosses a privilege boundary then you can't
trust that the string is actually NUL terminated in the reciever.
One way to solve this is to force a '\0' in the last byte of rdr.conf.name
after the imsg_get_data call. e.g.
	rdr.conf.name[sizeof(rdr.conf.name) - 1] = '\0';

There is also imsg_get_strbuf() for this but using it is a bit harder when
the string is embedded since you need to chop up the message.

Now in this case relayd sends the data to relayctl and so this is less of
an issue here. Still something to be careful with.
The IMSG_CTL_TABLE, IMSG_CTL_HOST, IMSG_CTL_RELAY, and IMSG_CTL_ROUTER
have the same issue.

-- 
:wq Claudio