From: Theo Buehler Subject: Re: dhcpleased(8): do not peek inside of struct imsg To: tech Date: Sat, 24 Aug 2024 21:56:26 +0200 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. Some ints should have <= 0 checks instead of == 0 cheks in my opinion, rest is cosmetic. I'd need to make a second pass with a fresh head > > diff --git control.c control.c > index d29e3fb1639..5bce89d3cc3 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,46 @@ 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); you usually left an empty line here > + 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 +307,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..6f67079c90a 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 > > @@ -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 *); > @@ -324,5 +325,6 @@ struct dhcpleased_conf *parse_config(const char *); > int cmdline_symset(char *); > #else > #define sin_to_str(x...) "" > +#define i2s(x...) "" missed this in slaacd and doesn't really matter much: these macros should only take a single argument. > #endif /* SMALL */ > > diff --git engine.c engine.c > index 289dffdf20a..657cb9266a8 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) vc_id_len is an int. I'd fatal on <= 0 rather than == 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) this should be > 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..c01d59b7055 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) again <= 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) again > 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. >