Index | Thread | Search

From:
Florian Obser <florian@openbsd.org>
Subject:
Re: dhcpleased(8): do not peek inside of struct imsg
To:
Theo Buehler <tb@theobuehler.org>
Cc:
tech <tech@openbsd.org>
Date:
Sun, 25 Aug 2024 10:07:00 +0200

Download raw body.

Thread
On 2024-08-24 21:56 +02, Theo Buehler <tb@theobuehler.org> wrote:
> On Sat, Aug 24, 2024 at 07:48:11PM +0200, Florian Obser wrote:
>> Very similar to what I just did to slaacd(8).
>
> Yes, I think I read much of that diff already...
>
>> Tests with more complicated config files that use client id, hostname
>> and / or vendor id would be appreciated.
>> 
>> OK?
>
> It generally looks good, but some nits and a couple of bugs below.
> Most importantly IMSG_RECONF_C_ID will error when it shouldn't.

oops, thanks! good catch

> Some ints should have <= 0 checks instead of == 0 cheks in my opinion,
> rest is cosmetic.

I went with changing the type to size_t to match what imsg_get_len(3)
gives us. Since that function can't fail, it feels odd to do an error
check that looks like checking for a failed function call.

>
> I'd need to make a second pass with a fresh head

here is an updated diff for your fresh head ;)

diff --git control.c control.c
index d29e3fb1639..ff5fa9d6af9 100644
--- control.c
+++ control.c
@@ -224,6 +224,8 @@ control_dispatch_imsg(int fd, short event, void *bula)
 	struct imsg	 imsg;
 	ssize_t		 n;
 	int		 verbose;
+	uint32_t	 if_index, type;
+	pid_t		 pid;
 
 	if ((c = control_connbyfd(fd)) == NULL) {
 		log_warnx("%s: fd %d: not found", __func__, fd);
@@ -252,40 +254,47 @@ control_dispatch_imsg(int fd, short event, void *bula)
 		if (n == 0)
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+		pid = imsg_get_pid(&imsg);
+
+		switch (type) {
 		case IMSG_CTL_RELOAD:
-			frontend_imsg_compose_main(imsg.hdr.type, 0, NULL, 0);
+			frontend_imsg_compose_main(type, 0, NULL, 0);
 			break;
 		case IMSG_CTL_LOG_VERBOSE:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(verbose))
+			if (imsg_get_data(&imsg, &verbose,
+			    sizeof(verbose)) == -1)
 				break;
-			c->iev.ibuf.pid = imsg.hdr.pid;
+
+			c->iev.ibuf.pid = pid;
 			/* Forward to all other processes. */
-			frontend_imsg_compose_main(imsg.hdr.type, imsg.hdr.pid,
-			    imsg.data, IMSG_DATA_SIZE(imsg));
-			frontend_imsg_compose_engine(imsg.hdr.type, 0,
-			    imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg));
+			frontend_imsg_compose_main(type, pid, &verbose,
+			    sizeof(verbose));
+			frontend_imsg_compose_engine(type, 0, pid, &verbose,
+			    sizeof(verbose));
 
-			memcpy(&verbose, imsg.data, sizeof(verbose));
 			log_setverbose(verbose);
 			break;
 		case IMSG_CTL_SHOW_INTERFACE_INFO:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(uint32_t))
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
 				break;
-			c->iev.ibuf.pid = imsg.hdr.pid;
-			frontend_imsg_compose_engine(imsg.hdr.type, 0,
-			    imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg));
+
+			c->iev.ibuf.pid = pid;
+			frontend_imsg_compose_engine(type, 0, pid, &if_index,
+			    sizeof(if_index));
 			break;
 		case IMSG_CTL_SEND_REQUEST:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(uint32_t))
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
 				break;
-			c->iev.ibuf.pid = imsg.hdr.pid;
+
+			c->iev.ibuf.pid = pid;
 			frontend_imsg_compose_engine(IMSG_REQUEST_REBOOT, 0,
-			    imsg.hdr.pid, imsg.data, IMSG_DATA_SIZE(imsg));
+			    pid, &if_index, sizeof(&if_index));
 			break;
 		default:
-			log_debug("%s: error handling imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: error handling imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -299,9 +308,8 @@ 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));
 }
diff --git dhcpleased.c dhcpleased.c
index b5f65046894..5b0eb018f9c 100644
--- dhcpleased.c
+++ dhcpleased.c
@@ -436,7 +436,7 @@ main_dispatch_frontend(int fd, short event, void *bula)
 	struct imsg_ifinfo	 imsg_ifinfo;
 	ssize_t			 n;
 	int			 shut = 0;
-	uint32_t		 if_index;
+	uint32_t		 if_index, type;
 #ifndef	SMALL
 	int			 verbose;
 #endif	/* SMALL */
@@ -462,12 +462,14 @@ main_dispatch_frontend(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 		case IMSG_OPEN_BPFSOCK:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_OPEN_BPFSOCK wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			open_bpfsock(if_index);
 			break;
 #ifndef	SMALL
@@ -478,25 +480,24 @@ main_dispatch_frontend(int fd, short event, void *bula)
 				log_warnx("configuration reloaded");
 			break;
 		case IMSG_CTL_LOG_VERBOSE:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(verbose))
-				fatalx("%s: IMSG_CTL_LOG_VERBOSE wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&verbose, imsg.data, sizeof(verbose));
+			if (imsg_get_data(&imsg, &verbose,
+			    sizeof(verbose)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			log_setverbose(verbose);
 			break;
 #endif	/* SMALL */
 		case IMSG_UPDATE_IF:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_ifinfo))
-				fatalx("%s: IMSG_UPDATE_IF wrong length: %lu",
-				    __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_ifinfo, imsg.data, sizeof(imsg_ifinfo));
+			if (imsg_get_data(&imsg, &imsg_ifinfo,
+			    sizeof(imsg_ifinfo)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			read_lease_file(&imsg_ifinfo);
 			main_imsg_compose_engine(IMSG_UPDATE_IF, -1,
 			    &imsg_ifinfo, sizeof(imsg_ifinfo));
 			break;
 		default:
-			log_debug("%s: error handling imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: error handling imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -517,6 +518,7 @@ main_dispatch_engine(int fd, short event, void *bula)
 	struct imsgbuf			*ibuf;
 	struct imsg			 imsg;
 	ssize_t				 n;
+	uint32_t			 type;
 	int				 shut = 0;
 
 	ibuf = &iev->ibuf;
@@ -540,15 +542,16 @@ main_dispatch_engine(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 		case IMSG_CONFIGURE_INTERFACE: {
 			struct imsg_configure_interface imsg_interface;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
-				fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_interface, imsg.data,
-			    sizeof(imsg_interface));
+
+			if (imsg_get_data(&imsg, &imsg_interface,
+			    sizeof(imsg_interface)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
 				fatalx("%s: too many routes in imsg", __func__);
 			configure_interface(&imsg_interface);
@@ -556,14 +559,13 @@ main_dispatch_engine(int fd, short event, void *bula)
 		}
 		case IMSG_DECONFIGURE_INTERFACE: {
 			struct imsg_configure_interface imsg_interface;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
-				fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_interface, imsg.data,
-			    sizeof(imsg_interface));
+
+			if (imsg_get_data(&imsg, &imsg_interface,
+			    sizeof(imsg_interface)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
 				fatalx("%s: too many routes in imsg", __func__);
+
 			deconfigure_interface(&imsg_interface);
 			main_imsg_compose_frontend(IMSG_CLOSE_UDPSOCK, -1,
 			    &imsg_interface.if_index,
@@ -572,48 +574,44 @@ main_dispatch_engine(int fd, short event, void *bula)
 		}
 		case IMSG_WITHDRAW_ROUTES: {
 			struct imsg_configure_interface imsg_interface;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_interface))
-				fatalx("%s: IMSG_CONFIGURE_INTERFACE wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_interface, imsg.data,
-			    sizeof(imsg_interface));
+
+			if (imsg_get_data(&imsg, &imsg_interface,
+			    sizeof(imsg_interface)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			if (imsg_interface.routes_len >= MAX_DHCP_ROUTES)
 				fatalx("%s: too many routes in imsg", __func__);
+
 			if (imsg_interface.routes_len > 0)
 				configure_routes(RTM_DELETE, &imsg_interface);
 			break;
 		}
 		case IMSG_PROPOSE_RDNS: {
 			struct imsg_propose_rdns	 rdns;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
-				fatalx("%s: IMSG_PROPOSE_RDNS wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&rdns, imsg.data, sizeof(rdns));
+
+			if (imsg_get_data(&imsg, &rdns, sizeof(rdns)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			if ((2 + rdns.rdns_count * sizeof(struct in_addr)) >
 			    sizeof(struct sockaddr_rtdns))
 				fatalx("%s: rdns_count too big: %d", __func__,
 				    rdns.rdns_count);
+
 			propose_rdns(&rdns);
 			break;
 		}
 		case IMSG_WITHDRAW_RDNS: {
 			struct imsg_propose_rdns	 rdns;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(rdns))
-				fatalx("%s: IMSG_WITHDRAW_RDNS wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&rdns, imsg.data, sizeof(rdns));
+
+			if (imsg_get_data(&imsg, &rdns, sizeof(rdns)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			if (rdns.rdns_count != 0)
 				fatalx("%s: expected rdns_count == 0: %d",
 				    __func__, rdns.rdns_count);
+
 			propose_rdns(&rdns);
 			break;
 		}
 		default:
-			log_debug("%s: error handling imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: error handling imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -672,6 +670,17 @@ 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)
@@ -1293,4 +1302,52 @@ config_clear(struct dhcpleased_conf *conf)
 
 	free(conf);
 }
+
+#define	I2S(x) case x: return #x
+
+const char*
+i2s(uint32_t type)
+{
+	static char	unknown[sizeof("IMSG_4294967295")];
+
+	switch (type) {
+	I2S(IMSG_NONE);
+	I2S(IMSG_CTL_LOG_VERBOSE);
+	I2S(IMSG_CTL_SHOW_INTERFACE_INFO);
+	I2S(IMSG_CTL_SEND_REQUEST);
+	I2S(IMSG_CTL_RELOAD);
+	I2S(IMSG_CTL_END);
+	I2S(IMSG_RECONF_CONF);
+	I2S(IMSG_RECONF_IFACE);
+	I2S(IMSG_RECONF_VC_ID);
+	I2S(IMSG_RECONF_C_ID);
+	I2S(IMSG_RECONF_H_NAME);
+	I2S(IMSG_RECONF_END);
+	I2S(IMSG_SEND_DISCOVER);
+	I2S(IMSG_SEND_REQUEST);
+	I2S(IMSG_SOCKET_IPC);
+	I2S(IMSG_OPEN_BPFSOCK);
+	I2S(IMSG_BPFSOCK);
+	I2S(IMSG_UDPSOCK);
+	I2S(IMSG_CLOSE_UDPSOCK);
+	I2S(IMSG_ROUTESOCK);
+	I2S(IMSG_CONTROLFD);
+	I2S(IMSG_STARTUP);
+	I2S(IMSG_UPDATE_IF);
+	I2S(IMSG_REMOVE_IF);
+	I2S(IMSG_DHCP);
+	I2S(IMSG_CONFIGURE_INTERFACE);
+	I2S(IMSG_DECONFIGURE_INTERFACE);
+	I2S(IMSG_PROPOSE_RDNS);
+	I2S(IMSG_WITHDRAW_RDNS);
+	I2S(IMSG_WITHDRAW_ROUTES);
+	I2S(IMSG_REPROPOSE_RDNS);
+	I2S(IMSG_REQUEST_REBOOT);
+	default:
+		snprintf(unknown, sizeof(unknown), "IMSG_%u", type);
+		return unknown;
+	}
+}
+#undef	I2S
+
 #endif /* SMALL */
diff --git dhcpleased.h dhcpleased.h
index 41749fc5031..d97b3e4f367 100644
--- dhcpleased.h
+++ dhcpleased.h
@@ -158,7 +158,6 @@
 
 #define	MAX_SERVERS	16	/* max servers that can be ignored per if */
 
-#define	IMSG_DATA_SIZE(imsg)	((imsg).hdr.len - IMSG_HEADER_SIZE)
 #define	DHCP_SNAME_LEN		64
 #define	DHCP_FILE_LEN		128
 
@@ -252,9 +251,9 @@ struct iface_conf {
 	SIMPLEQ_ENTRY(iface_conf)	 entry;
 	char				 name[IF_NAMESIZE];
 	uint8_t				*vc_id;
-	int				 vc_id_len;
+	size_t				 vc_id_len;
 	uint8_t				*c_id;
-	int				 c_id_len;
+	size_t				 c_id_len;
 	char				*h_name;
 	int				 ignore;
 	struct in_addr			 ignore_servers[MAX_SERVERS];
@@ -304,12 +303,14 @@ struct imsg_req_dhcp {
 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
 void			 config_clear(struct dhcpleased_conf *);
 struct dhcpleased_conf	*config_new_empty(void);
 void			 merge_config(struct dhcpleased_conf *, struct
 			     dhcpleased_conf *);
 const char	*sin_to_str(struct sockaddr_in *);
+const char	*i2s(uint32_t);
 
 /* frontend.c */
 struct iface_conf	*find_iface_conf(struct iface_conf_head *, char *);
@@ -323,6 +324,7 @@ void	print_config(struct dhcpleased_conf *);
 struct dhcpleased_conf	*parse_config(const char *);
 int			 cmdline_symset(char *);
 #else
-#define	sin_to_str(x...)	""
+#define	sin_to_str(x)	""
+#define	i2s(x)		""
 #endif	/* SMALL */
 
diff --git engine.c engine.c
index 289dffdf20a..ec54aa979f3 100644
--- engine.c
+++ engine.c
@@ -127,7 +127,7 @@ void			 engine_dispatch_frontend(int, short, void *);
 void			 engine_dispatch_main(int, short, void *);
 #ifndef	SMALL
 void			 send_interface_info(struct dhcpleased_iface *, pid_t);
-void			 engine_showinfo_ctl(struct imsg *, uint32_t);
+void			 engine_showinfo_ctl(pid_t, uint32_t);
 #endif	/* SMALL */
 void			 engine_update_iface(struct imsg_ifinfo *);
 struct dhcpleased_iface	*get_dhcpleased_iface_by_id(uint32_t);
@@ -286,7 +286,7 @@ engine_dispatch_frontend(int fd, short event, void *bula)
 #ifndef	SMALL
 	int				 verbose;
 #endif	/* SMALL */
-	uint32_t			 if_index;
+	uint32_t			 if_index, type;
 
 	if (event & EV_READ) {
 		if ((n = imsg_read(ibuf)) == -1 && errno != EAGAIN)
@@ -307,29 +307,29 @@ engine_dispatch_frontend(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 #ifndef	SMALL
 		case IMSG_CTL_LOG_VERBOSE:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(verbose))
-				fatalx("%s: IMSG_CTL_LOG_VERBOSE wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&verbose, imsg.data, sizeof(verbose));
+			if (imsg_get_data(&imsg, &verbose,
+			    sizeof(verbose)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			log_setverbose(verbose);
 			break;
 		case IMSG_CTL_SHOW_INTERFACE_INFO:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_CTL_SHOW_INTERFACE_INFO wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
-			engine_showinfo_ctl(&imsg, if_index);
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
+			engine_showinfo_ctl(imsg_get_pid(&imsg), if_index);
 			break;
 		case IMSG_REQUEST_REBOOT:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_CTL_SEND_DISCOVER wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			iface = get_dhcpleased_iface_by_id(if_index);
 			if (iface != NULL) {
 				switch (iface->state) {
@@ -351,18 +351,19 @@ engine_dispatch_frontend(int fd, short event, void *bula)
 			break;
 #endif	/* SMALL */
 		case IMSG_REMOVE_IF:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_REMOVE_IF wrong length: %lu",
-				    __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			remove_dhcpleased_iface(if_index);
 			break;
 		case IMSG_DHCP: {
 			struct imsg_dhcp	imsg_dhcp;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_dhcp))
-				fatalx("%s: IMSG_DHCP wrong length: %lu",
-				    __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_dhcp, imsg.data, sizeof(imsg_dhcp));
+
+			if (imsg_get_data(&imsg, &imsg_dhcp,
+			    sizeof(imsg_dhcp)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			iface = get_dhcpleased_iface_by_id(imsg_dhcp.if_index);
 			if (iface != NULL)
 				parse_dhcp(iface, &imsg_dhcp);
@@ -373,8 +374,7 @@ engine_dispatch_frontend(int fd, short event, void *bula)
 				send_rdns_proposal(iface);
 			break;
 		default:
-			log_debug("%s: unexpected imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: unexpected imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -400,6 +400,7 @@ engine_dispatch_main(int fd, short event, void *bula)
 	struct imsgbuf			*ibuf = &iev->ibuf;
 	struct imsg_ifinfo		 imsg_ifinfo;
 	ssize_t				 n;
+	uint32_t			 type;
 	int				 shut = 0;
 
 	if (event & EV_READ) {
@@ -421,7 +422,9 @@ engine_dispatch_main(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 		case IMSG_SOCKET_IPC:
 			/*
 			 * Setup pipe and event handler to the frontend
@@ -453,12 +456,12 @@ engine_dispatch_main(int fd, short event, void *bula)
 
 			break;
 		case IMSG_UPDATE_IF:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_ifinfo))
-				fatalx("%s: IMSG_UPDATE_IF wrong length: %lu",
-				    __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_ifinfo, imsg.data, sizeof(imsg_ifinfo));
+			if (imsg_get_data(&imsg, &imsg_ifinfo,
+			    sizeof(imsg_ifinfo)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			if (imsg_ifinfo.lease[LEASE_SIZE - 1] != '\0')
 				fatalx("Invalid lease");
+
 			engine_update_iface(&imsg_ifinfo);
 			break;
 #ifndef SMALL
@@ -472,15 +475,14 @@ engine_dispatch_main(int fd, short event, void *bula)
 			SIMPLEQ_INIT(&nconf->iface_list);
 			break;
 		case IMSG_RECONF_IFACE:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(struct
-			    iface_conf))
-				fatalx("%s: IMSG_RECONF_IFACE wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
 			if ((iface_conf = malloc(sizeof(struct iface_conf)))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf, imsg.data, sizeof(struct
-			    iface_conf));
+
+			if (imsg_get_data(&imsg, iface_conf,
+			    sizeof(struct iface_conf)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			iface_conf->vc_id = NULL;
 			iface_conf->vc_id_len = 0;
 			iface_conf->c_id = NULL;
@@ -491,44 +493,48 @@ engine_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_VC_ID:
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_VC_ID without "
+				fatalx("IMSG_RECONF_VC_ID without "
 				    "IMSG_RECONF_IFACE");
-			if (IMSG_DATA_SIZE(imsg) > 255 + 2)
-				fatalx("%s: IMSG_RECONF_VC_ID wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			if ((iface_conf->vc_id = malloc(IMSG_DATA_SIZE(imsg)))
+			if ((iface_conf->vc_id_len = imsg_get_len(&imsg))
+			    > 255 + 2 || iface_conf->vc_id_len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->vc_id = malloc(iface_conf->vc_id_len))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf->vc_id, imsg.data,
-			    IMSG_DATA_SIZE(imsg));
-			iface_conf->vc_id_len = IMSG_DATA_SIZE(imsg);
+			if (imsg_get_data(&imsg, iface_conf->vc_id,
+			    iface_conf->vc_id_len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			break;
 		case IMSG_RECONF_C_ID:
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_C_ID without "
+				fatalx("IMSG_RECONF_C_ID without "
 				    "IMSG_RECONF_IFACE");
-			if (IMSG_DATA_SIZE(imsg) > 255 + 2)
-				fatalx("%s: IMSG_RECONF_C_ID wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			if ((iface_conf->c_id = malloc(IMSG_DATA_SIZE(imsg)))
+			if ((iface_conf->c_id_len = imsg_get_len(&imsg))
+			    > 255 + 2 || iface_conf->c_id_len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->c_id = malloc(iface_conf->c_id_len))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf->c_id, imsg.data,
-			    IMSG_DATA_SIZE(imsg));
-			iface_conf->c_id_len = IMSG_DATA_SIZE(imsg);
+			if (imsg_get_data(&imsg, iface_conf->c_id,
+			    iface_conf->c_id_len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			break;
-		case IMSG_RECONF_H_NAME:
+		case IMSG_RECONF_H_NAME: {
+			size_t	len;
+
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_H_NAME without "
+				fatalx("IMSG_RECONF_H_NAME without "
 				    "IMSG_RECONF_IFACE");
-			if (((char *)imsg.data)[IMSG_DATA_SIZE(imsg) - 1] !=
-			    '\0')
-				fatalx("Invalid hostname");
-			if (IMSG_DATA_SIZE(imsg) > 256)
-				fatalx("Invalid hostname");
-			if ((iface_conf->h_name = strdup(imsg.data)) == NULL)
+			if ((len = imsg_get_len(&imsg)) > 256 || len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->h_name = malloc(len)) == NULL)
 				fatal(NULL);
+			if (imsg_get_data(&imsg, iface_conf->h_name, len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if (iface_conf->h_name[len - 1] != '\0')
+				fatalx("Invalid hostname");
 			break;
+		}
 		case IMSG_RECONF_END: {
 			struct dhcpleased_iface	*iface;
 			int			*ifaces;
@@ -562,8 +568,7 @@ engine_dispatch_main(int fd, short event, void *bula)
 		}
 #endif /* SMALL */
 		default:
-			log_debug("%s: unexpected imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: unexpected imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -605,22 +610,14 @@ send_interface_info(struct dhcpleased_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 dhcpleased_iface			*iface;
 
-	switch (imsg->hdr.type) {
-	case IMSG_CTL_SHOW_INTERFACE_INFO:
-		if ((iface = get_dhcpleased_iface_by_id(if_index)) != NULL)
-			send_interface_info(iface, imsg->hdr.pid);
-		else
-			engine_imsg_compose_frontend(IMSG_CTL_END,
-			    imsg->hdr.pid, NULL, 0);
-		break;
-	default:
-		log_debug("%s: error handling imsg", __func__);
-		break;
-	}
+	if ((iface = get_dhcpleased_iface_by_id(if_index)) != NULL)
+		send_interface_info(iface, pid);
+	else
+		engine_imsg_compose_frontend(IMSG_CTL_END, pid, NULL, 0);
 }
 #endif	/* SMALL */
 
diff --git frontend.c frontend.c
index fdc4ae8ca16..d0d651dfb28 100644
--- frontend.c
+++ frontend.c
@@ -238,6 +238,7 @@ frontend_dispatch_main(int fd, short event, void *bula)
 	struct imsgbuf			*ibuf = &iev->ibuf;
 	struct iface			*iface;
 	ssize_t				 n;
+	uint32_t			 type;
 	int				 shut = 0, bpfsock, if_index, udpsock;
 
 	if (event & EV_READ) {
@@ -259,7 +260,9 @@ frontend_dispatch_main(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 		case IMSG_SOCKET_IPC:
 			/*
 			 * Setup pipe and event handler to the engine
@@ -291,10 +294,10 @@ frontend_dispatch_main(int fd, short event, void *bula)
 				fatalx("%s: expected to receive imsg "
 				    "bpf fd but didn't receive any",
 				    __func__);
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_BPFSOCK wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			set_bpfsock(bpfsock, if_index);
 			break;
 		case IMSG_UDPSOCK:
@@ -302,10 +305,10 @@ frontend_dispatch_main(int fd, short event, void *bula)
 				fatalx("%s: expected to receive imsg "
 				    "udpsocket fd but didn't receive any",
 				    __func__);
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_UDPSOCK wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			if ((iface = get_iface_by_id(if_index)) == NULL) {
 				close(fd);
 				break;
@@ -316,10 +319,10 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			iface->udpsock = udpsock;
 			break;
 		case IMSG_CLOSE_UDPSOCK:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(if_index))
-				fatalx("%s: IMSG_UDPSOCK wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			memcpy(&if_index, imsg.data, sizeof(if_index));
+			if (imsg_get_data(&imsg, &if_index,
+			    sizeof(if_index)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			if ((iface = get_iface_by_id(if_index)) != NULL &&
 			    iface->udpsock != -1) {
 				close(iface->udpsock);
@@ -348,15 +351,14 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			SIMPLEQ_INIT(&nconf->iface_list);
 			break;
 		case IMSG_RECONF_IFACE:
-			if (IMSG_DATA_SIZE(imsg) != sizeof(struct
-			    iface_conf))
-				fatalx("%s: IMSG_RECONF_IFACE wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
 			if ((iface_conf = malloc(sizeof(struct iface_conf)))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf, imsg.data, sizeof(struct
-			    iface_conf));
+
+			if (imsg_get_data(&imsg, iface_conf,
+			    sizeof(struct iface_conf)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+
 			iface_conf->vc_id = NULL;
 			iface_conf->vc_id_len = 0;
 			iface_conf->c_id = NULL;
@@ -367,44 +369,48 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			break;
 		case IMSG_RECONF_VC_ID:
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_VC_ID without "
+				fatalx("IMSG_RECONF_VC_ID without "
 				    "IMSG_RECONF_IFACE");
-			if (IMSG_DATA_SIZE(imsg) > 255 + 2)
-				fatalx("%s: IMSG_RECONF_VC_ID wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			if ((iface_conf->vc_id = malloc(IMSG_DATA_SIZE(imsg)))
+			if ((iface_conf->vc_id_len = imsg_get_len(&imsg))
+			    > 255 + 2 || iface_conf->vc_id_len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->vc_id = malloc(iface_conf->vc_id_len))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf->vc_id, imsg.data,
-			    IMSG_DATA_SIZE(imsg));
-			iface_conf->vc_id_len = IMSG_DATA_SIZE(imsg);
+			if (imsg_get_data(&imsg, iface_conf->vc_id,
+			    iface_conf->vc_id_len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			break;
 		case IMSG_RECONF_C_ID:
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_C_ID without "
+				fatalx("IMSG_RECONF_C_ID without "
 				    "IMSG_RECONF_IFACE");
-			if (IMSG_DATA_SIZE(imsg) > 255 + 2)
-				fatalx("%s: IMSG_RECONF_C_ID wrong length: "
-				    "%lu", __func__, IMSG_DATA_SIZE(imsg));
-			if ((iface_conf->c_id = malloc(IMSG_DATA_SIZE(imsg)))
+			if ((iface_conf->c_id_len = imsg_get_len(&imsg))
+			    > 255 + 2 || iface_conf->c_id_len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->c_id = malloc(iface_conf->c_id_len))
 			    == NULL)
 				fatal(NULL);
-			memcpy(iface_conf->c_id, imsg.data,
-			    IMSG_DATA_SIZE(imsg));
-			iface_conf->c_id_len = IMSG_DATA_SIZE(imsg);
+			if (imsg_get_data(&imsg, iface_conf->c_id,
+			    iface_conf->c_id_len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 			break;
-		case IMSG_RECONF_H_NAME:
+		case IMSG_RECONF_H_NAME: {
+			size_t	len;
+
 			if (iface_conf == NULL)
-				fatal("IMSG_RECONF_H_NAME without "
+				fatalx("IMSG_RECONF_H_NAME without "
 				    "IMSG_RECONF_IFACE");
-			if (((char *)imsg.data)[IMSG_DATA_SIZE(imsg) - 1] !=
-			    '\0')
-				fatalx("Invalid hostname");
-			if (IMSG_DATA_SIZE(imsg) > 256)
-				fatalx("Invalid hostname");
-			if ((iface_conf->h_name = strdup(imsg.data)) == NULL)
+			if ((len = imsg_get_len(&imsg)) > 256 || len == 0)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if ((iface_conf->h_name = malloc(len)) == NULL)
 				fatal(NULL);
+			if (imsg_get_data(&imsg, iface_conf->h_name, len) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
+			if (iface_conf->h_name[len - 1] != '\0')
+				fatalx("Invalid hostname");
 			break;
+		}
 		case IMSG_RECONF_END: {
 			int	 i;
 			int	*ifaces;
@@ -442,8 +448,7 @@ frontend_dispatch_main(int fd, short event, void *bula)
 			break;
 #endif	/* SMALL */
 		default:
-			log_debug("%s: error handling imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: error handling imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);
@@ -465,6 +470,7 @@ frontend_dispatch_engine(int fd, short event, void *bula)
 	struct imsg		 imsg;
 	struct iface		*iface;
 	ssize_t			 n;
+	uint32_t		 type;
 	int			 shut = 0;
 
 	if (event & EV_READ) {
@@ -486,7 +492,9 @@ frontend_dispatch_engine(int fd, short event, void *bula)
 		if (n == 0)	/* No more messages. */
 			break;
 
-		switch (imsg.hdr.type) {
+		type = imsg_get_type(&imsg);
+
+		switch (type) {
 #ifndef	SMALL
 		case IMSG_CTL_END:
 		case IMSG_CTL_SHOW_INTERFACE_INFO:
@@ -495,12 +503,10 @@ frontend_dispatch_engine(int fd, short event, void *bula)
 #endif	/* SMALL */
 		case IMSG_SEND_DISCOVER: {
 			struct imsg_req_dhcp	 imsg_req_dhcp;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
-				fatalx("%s: IMSG_SEND_DISCOVER wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_req_dhcp, imsg.data,
-			    sizeof(imsg_req_dhcp));
+
+			if (imsg_get_data(&imsg, &imsg_req_dhcp,
+			    sizeof(imsg_req_dhcp)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 
 			iface = get_iface_by_id(imsg_req_dhcp.if_index);
 
@@ -513,12 +519,10 @@ frontend_dispatch_engine(int fd, short event, void *bula)
 		}
 		case IMSG_SEND_REQUEST: {
 			struct imsg_req_dhcp	 imsg_req_dhcp;
-			if (IMSG_DATA_SIZE(imsg) != sizeof(imsg_req_dhcp))
-				fatalx("%s: IMSG_SEND_REQUEST wrong "
-				    "length: %lu", __func__,
-				    IMSG_DATA_SIZE(imsg));
-			memcpy(&imsg_req_dhcp, imsg.data,
-			    sizeof(imsg_req_dhcp));
+
+			if (imsg_get_data(&imsg, &imsg_req_dhcp,
+			    sizeof(imsg_req_dhcp)) == -1)
+				fatalx("%s: invalid %s", __func__, i2s(type));
 
 			iface = get_iface_by_id(imsg_req_dhcp.if_index);
 
@@ -530,8 +534,7 @@ frontend_dispatch_engine(int fd, short event, void *bula)
 			break;
 		}
 		default:
-			log_debug("%s: error handling imsg %d", __func__,
-			    imsg.hdr.type);
+			log_debug("%s: error handling imsg %d", __func__, type);
 			break;
 		}
 		imsg_free(&imsg);


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