From: Florian Obser <florian@openbsd.org>
Subject: Re: slaacd(8): do not peek inside of struct imsg
To: Theo Buehler <tb@theobuehler.org>
Cc: tech <tech@openbsd.org>
Date: Sat, 24 Aug 2024 11:55:21 +0200

On 2024-08-24 10:59 +02, Theo Buehler <tb@theobuehler.org> wrote:
> PS: control_imsg_relay() and engine_showinfo_ctl() still reach into imsg.

Thanks! Two diffs for that, they are somewhat unrelated to each other.

OK?

commit e0c8ae3f49fabc017f5c3d5f724fdf747e2e9356
Author: Florian Obser <florian@narrans.de>
Date:   Sat Aug 24 11:48:15 2024 +0200

    Stop peeking into struct imsg when relaying control messages.

diff --git control.c control.c
index 11a4f2ee608..efb469146e4 100644
--- control.c
+++ control.c
@@ -296,10 +296,9 @@ control_imsg_relay(struct imsg *imsg)
 {
 	struct ctl_conn	*c;
 
-	if ((c = control_connbypid(imsg->hdr.pid)) == NULL)
+	if ((c = control_connbypid(imsg_get_pid(imsg))) == NULL)
 		return (0);
 
-	return (imsg_compose_event(&c->iev, imsg->hdr.type, 0, imsg->hdr.pid,
-	    -1, imsg->data, IMSG_DATA_SIZE(*imsg)));
+	return (imsg_forward_event(&c->iev, imsg));
 }
 #endif	/* SMALL */
diff --git slaacd.c slaacd.c
index 43245529543..cc95fb28c9f 100644
--- slaacd.c
+++ slaacd.c
@@ -587,6 +587,16 @@ imsg_compose_event(struct imsgev *iev, uint16_t type, uint32_t peerid,
 	return (ret);
 }
 
+int
+imsg_forward_event(struct imsgev *iev, struct imsg *imsg)
+{
+	int	ret;
+
+	if ((ret = imsg_forward(&iev->ibuf, imsg)) != -1)
+		imsg_event_add(iev);
+
+	return (ret);
+}
 static int
 main_imsg_send_ipc_sockets(struct imsgbuf *frontend_buf,
     struct imsgbuf *engine_buf)
diff --git slaacd.h slaacd.h
index 910392bfaf2..60baa7b3766 100644
--- slaacd.h
+++ slaacd.h
@@ -27,8 +27,6 @@
 
 #define	MAX_RDNS_COUNT		8 /* max nameserver in a RTM_PROPOSAL */
 
-#define	IMSG_DATA_SIZE(imsg)	((imsg).hdr.len - IMSG_HEADER_SIZE)
-
 struct imsgev {
 	struct imsgbuf	 ibuf;
 	void		(*handler)(int, short, void *);
@@ -204,6 +202,7 @@ struct imsg_dup_addr {
 void		imsg_event_add(struct imsgev *);
 int		imsg_compose_event(struct imsgev *, uint16_t, uint32_t, pid_t,
 		    int, void *, uint16_t);
+int		imsg_forward_event(struct imsgev *, struct imsg *);
 #ifndef	SMALL
 const char	*sin6_to_str(struct sockaddr_in6 *);
 const char	*i2s(uint32_t);


commit 048a63bd100fda9d4161c6fe83c6bb3511add80f
Author: Florian Obser <florian@narrans.de>
Date:   Sat Aug 24 11:51:01 2024 +0200

    Simplify engine_showinfo_ctl()
    
    It only handles one imsg type these days, so it doesn't need to peek
    into struct imsg at all.

diff --git engine.c engine.c
index 818c2d75bf4..06083e24445 100644
--- engine.c
+++ engine.c
@@ -242,7 +242,7 @@ void			 engine_dispatch_frontend(int, short, void *);
 void			 engine_dispatch_main(int, short, void *);
 #ifndef	SMALL
 void			 send_interface_info(struct slaacd_iface *, pid_t);
-void			 engine_showinfo_ctl(struct imsg *, uint32_t);
+void			 engine_showinfo_ctl(pid_t, uint32_t);
 void			 debug_log_ra(struct imsg_ra *);
 int			 in6_mask2prefixlen(struct in6_addr *);
 #endif	/* SMALL */
@@ -499,7 +499,7 @@ engine_dispatch_frontend(int fd, short event, void *bula)
 			    sizeof(if_index)) == -1)
 				fatalx("%s: invalid %s", __func__, i2s(type));
 
-			engine_showinfo_ctl(&imsg, if_index);
+			engine_showinfo_ctl(imsg_get_pid(&imsg), if_index);
 			break;
 #endif	/* SMALL */
 		case IMSG_REMOVE_IF:
@@ -862,26 +862,18 @@ send_interface_info(struct slaacd_iface *iface, pid_t pid)
 }
 
 void
-engine_showinfo_ctl(struct imsg *imsg, uint32_t if_index)
+engine_showinfo_ctl(pid_t pid, uint32_t if_index)
 {
 	struct slaacd_iface			*iface;
 
-	switch (imsg->hdr.type) {
-	case IMSG_CTL_SHOW_INTERFACE_INFO:
-		if (if_index == 0) {
-			LIST_FOREACH (iface, &slaacd_interfaces, entries)
-				send_interface_info(iface, imsg->hdr.pid);
-		} else {
-			if ((iface = get_slaacd_iface_by_id(if_index)) != NULL)
-				send_interface_info(iface, imsg->hdr.pid);
-		}
-		engine_imsg_compose_frontend(IMSG_CTL_END, imsg->hdr.pid, NULL,
-		    0);
-		break;
-	default:
-		log_debug("%s: error handling imsg", __func__);
-		break;
+	if (if_index == 0) {
+		LIST_FOREACH (iface, &slaacd_interfaces, entries)
+			send_interface_info(iface, pid);
+	} else {
+		if ((iface = get_slaacd_iface_by_id(if_index)) != NULL)
+			send_interface_info(iface, pid);
 	}
+	engine_imsg_compose_frontend(IMSG_CTL_END, pid, NULL, 0);
 }
 
 #endif	/* SMALL */


-- 
In my defence, I have been left unsupervised.