From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: [7/7+1] relayd: ibuf_get_string / ibuf_get_data
To: tech@openbsd.org, Claudio Jeker <cjeker@diehard.n-r-g.com>
Date: Tue, 9 Jun 2026 13:13:12 +0200

Hi,

the diff below converts Claudio's feedback (thanks) on the imsg rework
diff. It's much easier for me to stack this diff on top. The main idea
and feedback is to get rid of the ibuf_data call.

This diff replaced the get_string()/get_data() helpers, which took a raw
(ptr, len) and forced callers to use ibuf_data(), with
ibuf_get_string()/ibuf_get_data() that read straight from the ibuf and
advance the cursor. This removes the manual ibuf_skip() and the
ibuf_data() calls.

ibuf_get_string() keeps the "empty 0-length string" behaviour (returns
"" for len 0).

One behaviour change worth calling out: the old get_string() truncated
at the first non-printable byte; the new helper copies the bytes as-is.
The payloads here come from the parent over privsep imsg, not from the
network. Let me know if that filtering was intentional and should be
kept. I don't think it's necessary.


Built and tested: the regress suite passes.


diff --git a/config.c b/config.c
index 7ca37d5..7cb161b 100644
--- a/config.c
+++ b/config.c
@@ -361,8 +361,8 @@ config_gettable(struct relayd *env, struct imsg *imsg)
 		return (-1);
 	}
 	if ((sb = ibuf_size(&ibuf)) > 0) {
-		if ((tb->sendbuf = get_string(ibuf_data(&ibuf), sb)) == NULL) {
-			log_warn("%s: get_string", __func__);
+		if ((tb->sendbuf = ibuf_get_string(&ibuf, sb)) == NULL) {
+			log_warn("%s: ibuf_get_string", __func__);
 			free(tb);
 			return (-1);
 		}
@@ -732,9 +732,9 @@ config_getproto(struct relayd *env, struct imsg *imsg)
 	}
 	if ((s = ibuf_size(&ibuf)) > 0) {
 		proto->style = NULL;
-		if ((proto->style = get_string(ibuf_data(&ibuf), s - 1))
+		if ((proto->style = ibuf_get_string(&ibuf, s - 1))
 		    == NULL) {
-			log_warn("%s: get_string", __func__);
+			log_warn("%s: ibuf_get_string", __func__);
 			free(proto);
 			return (-1);
 		}
@@ -790,11 +790,7 @@ config_getrule(struct relayd *env, struct imsg *imsg)
 		/* Also accept "empty" 0-length strings */		\
 		if (ibuf_size(&ibuf) < (size_t)len ||			\
 		    (rule->rule_kv[_n].kv_##_f =			\
-		    get_string(ibuf_data(&ibuf), len)) == NULL) {	\
-			free(rule);					\
-			return (-1);					\
-		}							\
-		if (len > 0 && ibuf_skip(&ibuf, len) == -1) {		\
+		    ibuf_get_string(&ibuf, len)) == NULL) {		\
 			free(rule);					\
 			return (-1);					\
 		}							\
@@ -1104,8 +1100,7 @@ config_getrelay(struct relayd *env, struct imsg *imsg)
 		goto fail;
 	}
 	if (s > 0) {
-		if ((rlay->rl_tls_cakey = get_data(ibuf_data(&ibuf), s))
-		    == NULL)
+		if ((rlay->rl_tls_cakey = ibuf_get_data(&ibuf, s)) == NULL)
 			goto fail;
 	}
 
diff --git a/relayd.c b/relayd.c
index b678103..340ceeb 100644
--- a/relayd.c
+++ b/relayd.c
@@ -437,8 +437,8 @@ parent_dispatch_pfe(int fd, struct privsep_proc *p, struct imsg *imsg)
 	case IMSG_CTL_RELOAD:
 		if (imsg_get_ibuf(imsg, &ibuf) != -1 &&
 		    (s = ibuf_size(&ibuf)) > 0) {
-			if ((str = get_string(ibuf_data(&ibuf), s)) == NULL) {
-				log_warn("%s: get_string", __func__);
+			if ((str = ibuf_get_string(&ibuf, s)) == NULL) {
+				log_warn("%s: ibuf_get_string", __func__);
 				return (-1);
 			}
 		}
@@ -1806,28 +1806,42 @@ socket_rlimit(int maxfd)
 		fatal("%s: failed to set resource limit", __func__);
 }
 
+/*
+ * Read len bytes from ibuf and return them as a NUL-terminated string.
+ * A len of 0 is valid and yields an empty string.
+ */
 char *
-get_string(u_int8_t *ptr, size_t len)
+ibuf_get_string(struct ibuf *ibuf, size_t len)
 {
-	size_t	 i;
+	char	*s;
+	if (ibuf == NULL)
+		return (NULL);
 
-	for (i = 0; i < len; i++)
-		if (!(isprint((unsigned char)ptr[i]) ||
-		    isspace((unsigned char)ptr[i])))
-			break;
+	if ((s = calloc(1, len + 1)) == NULL)
+		return (NULL);
 
-	return strndup(ptr, i);
+	if (len > 0 && ibuf_get(ibuf, s, len) == -1) {
+		free(s);
+		return (NULL);
+	}
+	return (s);
 }
 
 void *
-get_data(u_int8_t *ptr, size_t len)
+ibuf_get_data(struct ibuf *ibuf, size_t len)
 {
 	u_int8_t	*data;
 
+	if (ibuf == NULL || len == 0)
+		return (NULL);
+
 	if ((data = malloc(len)) == NULL)
 		return (NULL);
-	memcpy(data, ptr, len);
 
+	if (ibuf_get(ibuf, data, len) == -1) {
+		free(data);
+		return (NULL);
+	}
 	return (data);
 }
 
diff --git a/relayd.h b/relayd.h
index bd29888..be2f811 100644
--- a/relayd.h
+++ b/relayd.h
@@ -1336,8 +1336,8 @@ void		 imsg_event_add(struct imsgev *);
 int		 imsg_compose_event(struct imsgev *, u_int16_t, u_int32_t,
 		    pid_t, int, void *, u_int16_t);
 void		 socket_rlimit(int);
-char		*get_string(u_int8_t *, size_t);
-void		*get_data(u_int8_t *, size_t);
+char		*ibuf_get_string(struct ibuf *, size_t);
+void		*ibuf_get_data(struct ibuf *, size_t);
 int		 sockaddr_cmp(struct sockaddr *, struct sockaddr *, int);
 struct in6_addr *prefixlen2mask6(u_int8_t, u_int32_t *);
 u_int32_t	 prefixlen2mask(u_int8_t);