Download raw body.
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
Second attempt, since the first one still had a bug. I overlooked the
fact that in 'config_getserver', the 'imsg' data was passed through
again to 'config_getserver_config' and read there once more. With the
switch to the new API, the message is retrieved by 'imsg_get_data' and
cannot be retrieved again. This has now been fixed.
Our regression didn't find that. I have an idea.
commit 0fba93bae28cb57b9c1c598ff41835ca84617961
Author: Rafael Sadowski <rafael@sizeofvoid.org>
Date: Sat Oct 17 08:39:35 2026 +0200
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro
diff --git a/config.c b/config.c
index ddf09f2..957da66 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);
@@ -744,7 +744,8 @@ 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);
+ if (imsg_get_len(imsg) < sizeof(*srv_conf))
+ fatalx("%s: bad length imsg received", __func__);
memcpy(srv_conf, p, sizeof(*srv_conf));
s = sizeof(*srv_conf);
@@ -927,7 +928,8 @@ config_getserver(struct httpd *env, struct imsg *imsg)
size_t s;
int fd;
- IMSG_SIZE_CHECK(imsg, &srv_conf);
+ if (imsg_get_len(imsg) < sizeof(srv_conf))
+ fatalx("%s: bad length imsg received", __func__);
memcpy(&srv_conf, p, sizeof(srv_conf));
s = sizeof(srv_conf);
@@ -1052,22 +1054,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__);
@@ -1076,32 +1083,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;
@@ -1153,10 +1160,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\"",
@@ -1209,10 +1215,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 abfbb5c..7ebdc84 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_get_type(imsg)) {
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_SHUTDOWN:
diff --git a/httpd.h b/httpd.h
index 1e54479..e238e04 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 3b9a7d9..c75e7b7 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,36 @@ 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;
- /* called from the privileged process */
- IMSG_SIZE_CHECK(imsg, &id);
- memcpy(&id, imsg->data, sizeof(id));
- p = (char *)imsg->data + sizeof(id);
+ if (imsg_get_ibuf(imsg, &ibuf) == -1) {
+ log_warn("%s: imsg_get_ibuf", __func__);
+ return (-1);
+ }
+
+ if (ibuf_get(&ibuf, &id, sizeof(id)) == -1)
+ fatalx("%s: ibuf_get", __func__);
+
+ if ((len = ibuf_size(&ibuf)) == 0) {
+ log_debug("%s: invalid message length", __func__);
+ return (-1);
+ }
- if ((size_t)snprintf(name, sizeof(name), "/%s", p) >= sizeof(name))
+ 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 +260,19 @@ 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);
+ }
+
+ 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 +288,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 9975a1c..1ce17f8 100644
--- a/server.c
+++ b/server.c
@@ -1421,8 +1421,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__);
httpd: convert imsg handling to imsg_get_ and remove IMSG_SIZE_CHECK macro