Download raw body.
dhcpleased(8): do not peek inside of struct imsg
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.
>
dhcpleased(8): do not peek inside of struct imsg