Download raw body.
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
On Fri, Sep 18, 2026 at 04:57:32PM +0200, Rafael Sadowski wrote:
> This is a fist diff on a longer imsg journey in httpd. The goal is to
> remove IMSG_SIZE_CHECK().
Hell yes. this is lovely.
Comments below.
> commit e08b6a50c4d986afa2b3eda5fb7661fcf7ed90f3
> Author: Rafael Sadowski <rafael@sizeofvoid.org>
> Date: Sun Aug 30 08:50:26 2026 +0200
>
> httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
>
> diff --git a/config.c b/config.c
> index ecaa59a..4001d86 100644
> --- a/config.c
> +++ b/config.c
> @@ -130,8 +130,8 @@ config_getreset(struct httpd *env, struct imsg *imsg)
> {
> unsigned int mode;
>
> - IMSG_SIZE_CHECK(imsg, &mode);
> - memcpy(&mode, imsg->data, sizeof(mode));
> + if (imsg_get_data(imsg, &mode, sizeof(mode)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
>
> config_purge(env, mode);
>
> @@ -621,8 +621,11 @@ config_getserver_config(struct httpd *env, struct server *srv,
> if ((srv_conf = calloc(1, sizeof(*srv_conf))) == NULL)
> return (-1);
>
> - IMSG_SIZE_CHECK(imsg, srv_conf);
> - memcpy(srv_conf, p, sizeof(*srv_conf));
> + if (imsg_get_data(imsg, srv_conf, sizeof(*srv_conf)) == -1) {
> + free(srv_conf);
> + fatalx("%s: imsg_get_data", __func__);
fatalx is terminal. So no need to free() something before.
> + }
> +
> s = sizeof(*srv_conf);
>
> /* Reset these variables to avoid free'ing invalid pointers */
> @@ -804,8 +807,9 @@ config_getserver(struct httpd *env, struct imsg *imsg)
> size_t s;
> int fd;
>
> - IMSG_SIZE_CHECK(imsg, &srv_conf);
> - memcpy(&srv_conf, p, sizeof(srv_conf));
> + if (imsg_get_data(imsg, &srv_conf, sizeof(srv_conf)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
> +
> s = sizeof(srv_conf);
>
> /* Reset these variables to avoid free'ing invalid pointers */
> @@ -928,22 +932,27 @@ config_gettls(struct httpd *env, struct server_config *srv_conf,
> int
> config_getserver_tls(struct httpd *env, struct imsg *imsg)
> {
> + struct ibuf ibuf;
> struct server_config *srv_conf = NULL;
> struct tls_config tls_conf;
> - uint8_t *p = imsg->data;
> + uint8_t *data;
> size_t len;
>
> - IMSG_SIZE_CHECK(imsg, &tls_conf);
> - memcpy(&tls_conf, p, sizeof(tls_conf));
> + if (imsg_get_ibuf(imsg, &ibuf) == -1) {
> + log_warn("%s: imsg_get_ibuf", __func__);
> + return (-1);
> + }
>
> - len = tls_conf.tls_chunk_len;
> + if (ibuf_get(&ibuf, &tls_conf, sizeof(tls_conf)) == -1)
> + fatalx("%s: ibuf_get", __func__);
>
> - if ((IMSG_DATA_SIZE(imsg) - sizeof(tls_conf)) < len) {
> + if (ibuf_size(&ibuf) != tls_conf.tls_chunk_len) {
> log_debug("%s: invalid message length", __func__);
> goto fail;
> }
>
> - p += sizeof(tls_conf);
> + data = ibuf_data(&ibuf);
> + len = tls_conf.tls_chunk_len;
>
> if ((srv_conf = serverconfig_byid(tls_conf.id)) == NULL) {
> log_debug("%s: server not found", __func__);
> @@ -952,32 +961,32 @@ config_getserver_tls(struct httpd *env, struct imsg *imsg)
>
> switch (tls_conf.tls_type) {
> case TLS_CFG_CA:
> - if (config_gettls(env, srv_conf, &tls_conf, "ca", p, len,
> + if (config_gettls(env, srv_conf, &tls_conf, "ca", data, len,
> &srv_conf->tls_ca, &srv_conf->tls_ca_len) != 0)
> goto fail;
> break;
>
> case TLS_CFG_CERT:
> - if (config_gettls(env, srv_conf, &tls_conf, "cert", p, len,
> + if (config_gettls(env, srv_conf, &tls_conf, "cert", data, len,
> &srv_conf->tls_cert, &srv_conf->tls_cert_len) != 0)
> goto fail;
> break;
>
> case TLS_CFG_CRL:
> - if (config_gettls(env, srv_conf, &tls_conf, "crl", p, len,
> + if (config_gettls(env, srv_conf, &tls_conf, "crl", data, len,
> &srv_conf->tls_crl, &srv_conf->tls_crl_len) != 0)
> goto fail;
> break;
>
> case TLS_CFG_KEY:
> - if (config_gettls(env, srv_conf, &tls_conf, "key", p, len,
> + if (config_gettls(env, srv_conf, &tls_conf, "key", data, len,
> &srv_conf->tls_key, &srv_conf->tls_key_len) != 0)
> goto fail;
> break;
>
> case TLS_CFG_OCSP_STAPLE:
> if (config_gettls(env, srv_conf, &tls_conf, "ocsp staple",
> - p, len, &srv_conf->tls_ocsp_staple,
> + data, len, &srv_conf->tls_ocsp_staple,
> &srv_conf->tls_ocsp_staple_len) != 0)
> goto fail;
> break;
> @@ -1029,10 +1038,9 @@ config_getmedia(struct httpd *env, struct imsg *imsg)
> struct privsep *ps = env->sc_ps;
> #endif
> struct media_type media;
> - uint8_t *p = imsg->data;
>
> - IMSG_SIZE_CHECK(imsg, &media);
> - memcpy(&media, p, sizeof(media));
> + if (imsg_get_data(imsg, &media, sizeof(media)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
>
> if (media_add(env->sc_mediatypes, &media) == NULL) {
> log_debug("%s: failed to add media \"%s\"",
> @@ -1085,10 +1093,9 @@ config_getauth(struct httpd *env, struct imsg *imsg)
> struct privsep *ps = env->sc_ps;
> #endif
> struct auth auth;
> - uint8_t *p = imsg->data;
>
> - IMSG_SIZE_CHECK(imsg, &auth);
> - memcpy(&auth, p, sizeof(auth));
> + if (imsg_get_data(imsg, &auth, sizeof(auth)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
>
> if (auth_add(env->sc_auth, &auth) == NULL) {
> log_debug("%s: failed to add auth \"%s[%u]\"",
> diff --git a/httpd.c b/httpd.c
> index 6d8d031..4b2303b 100644
> --- a/httpd.c
> +++ b/httpd.c
> @@ -430,8 +430,8 @@ parent_dispatch_logger(int fd, struct privsep_proc *p, struct imsg *imsg)
>
> switch (imsg->hdr.type) {
> case IMSG_CTL_RESET:
> - IMSG_SIZE_CHECK(imsg, &v);
> - memcpy(&v, imsg->data, sizeof(v));
> + if (imsg_get_data(imsg, &v, sizeof(v)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
> parent_reload(env, v, NULL);
> break;
> case IMSG_CTL_RELOAD:
> diff --git a/httpd.h b/httpd.h
> index 71377cf..99631ee 100644
> --- a/httpd.h
> +++ b/httpd.h
> @@ -164,10 +164,6 @@ struct imsgev {
> short events;
> };
>
> -#define IMSG_SIZE_CHECK(imsg, p) do { \
> - if (IMSG_DATA_SIZE(imsg) < sizeof(*p)) \
> - fatalx("bad length imsg received"); \
> -} while (0)
> #define IMSG_DATA_SIZE(imsg) ((imsg)->hdr.len - IMSG_HEADER_SIZE)
> #define MAX_IMSG_DATA_SIZE (MAX_IMSGSIZE - IMSG_HEADER_SIZE)
>
> diff --git a/logger.c b/logger.c
> index c2eae70..049a5b1 100644
> --- a/logger.c
> +++ b/logger.c
> @@ -140,8 +140,8 @@ logger_open_fd(struct imsg *imsg)
> struct log_file *log;
> uint32_t id;
>
> - IMSG_SIZE_CHECK(imsg, &id);
> - memcpy(&id, imsg->data, sizeof(id));
> + if (imsg_get_data(imsg, &id, sizeof(id)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
>
> TAILQ_FOREACH(log, &log_files, log_entry) {
> if (log->log_id == id) {
> @@ -158,19 +158,37 @@ logger_open_fd(struct imsg *imsg)
> int
> logger_open_priv(struct imsg *imsg)
> {
> + struct ibuf ibuf;
> char path[PATH_MAX];
> char name[PATH_MAX], *p;
> + char *filename = NULL;
> uint32_t id;
> size_t len;
> - int fd;
> + int fd, ret;
> +
> + if (imsg_get_ibuf(imsg, &ibuf) == -1) {
> + log_warn("%s: imsg_get_ibuf", __func__);
> + return (-1);
> + }
>
> /* called from the privileged process */
> - IMSG_SIZE_CHECK(imsg, &id);
> - memcpy(&id, imsg->data, sizeof(id));
> - p = (char *)imsg->data + sizeof(id);
> + if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
> + fatalx("%s: ibuf_get", __func__);
>
> - if ((size_t)snprintf(name, sizeof(name), "/%s", p) >= sizeof(name))
> + if ((len = ibuf_size(&ibuf)) == 0) {
> + log_debug("%s: invalid message length", __func__);
> return (-1);
> + }
> +
> + if ((filename = ibuf_get_string(&ibuf, len)) == NULL) {
> + log_warn("%s: ibuf_get_string", __func__);
> + return (-1);
> + }
> + ret = snprintf(name, sizeof(name), "/%s", filename);
> + free(filename);
> + if (ret < 0 || (size_t)ret >= sizeof(name))
> + return (-1);
> +
> if ((len = strlcpy(path, httpd_env->sc_logdir, sizeof(path))) >=
> sizeof(path))
> return (-1);
> @@ -243,13 +261,20 @@ logger_start(void)
> int
> logger_log(struct imsg *imsg)
> {
> + struct ibuf ibuf;
> char *logline;
> uint32_t id;
> struct server_config *srv_conf;
> struct log_file *log;
>
> - IMSG_SIZE_CHECK(imsg, &id);
> - memcpy(&id, imsg->data, sizeof(id));
> + if (imsg_get_ibuf(imsg, &ibuf) == -1) {
> + log_warn("%s: imsg_get_ibuf", __func__);
> + return (-1);
> + }
> +
> + /* called from the privileged process */
> + if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
> + fatalx("%s: ibuf_get", __func__);
>
> if ((srv_conf = serverconfig_byid(id)) == NULL)
> fatalx("invalid logging requestr");
> @@ -265,7 +290,7 @@ logger_log(struct imsg *imsg)
> }
>
> /* XXX get_string() would sanitize the string, but add a malloc */
> - logline = (char *)imsg->data + sizeof(id);
> + logline = ibuf_data(&ibuf);
>
> /* For debug output */
> log_debug("%s", logline);
> diff --git a/server.c b/server.c
> index bc96722..f3cbb7c 100644
> --- a/server.c
> +++ b/server.c
> @@ -1408,8 +1408,8 @@ server_dispatch_parent(int fd, struct privsep_proc *p, struct imsg *imsg)
> return (-1);
> break;
> case IMSG_TLSTICKET_REKEY:
> - IMSG_SIZE_CHECK(imsg, (&key));
> - memcpy(&key, imsg->data, sizeof(key));
> + if (imsg_get_data(imsg, &key, sizeof(key)) == -1)
> + fatalx("%s: imsg_get_data", __func__);
> /* apply to the right server */
> if ((srv = server_byid(key.tt_id)) == NULL) {
> log_debug("%s: invalid sever id", __func__);
>
Rest look OK.
--
:wq Claudio
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro