From: Rafael Sadowski <rafael@sizeofvoid.org>
Subject: Re: [7/7+1] relayd: ibuf_get_string / ibuf_get_data
To: tech@openbsd.org
Date: Tue, 9 Jun 2026 18:27:20 +0200

On Tue Jun 09, 2026 at 01:49:38PM +0200, Claudio Jeker wrote:
> On Tue, Jun 09, 2026 at 01:13:12PM +0200, Rafael Sadowski wrote:
> > 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.
>  
> ibuf_get_string() is a function that is part of -lutil so you're living
> dangerously here. I would prefer if you stay away from ibuf_* names.

Oh yes, the name should be the same as it used to be. Thanks!

> I realized that ibuf_get_string(3) is a strange beast of a function and
> using it correctly is tricky. Since it is not quite right I may change the
> function (right now it is only used by bgpd and xlock).
> 
> Btw. your ibuf_get_string function is doing mostly the same as
> the libutil ibuf_get_string(). You could still do the isprint() check
> but that would happen after pulling out the string from the ibuf.

I took a look at it, but I don't want to use it the way it is :)
I see no reason for the isprint||isspace.

> 
> I dislike the ibuf == NULL checks in those functions. Only
> ibuf_free() has such a check since it makes sense there everything else
> should fail hard.
> 

I can understand that. New diff below

diff --git a/config.c b/config.c
index 7ca37d5..84ee3dc 100644
--- a/config.c
+++ b/config.c
@@ -361,7 +361,7 @@ 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) {
+		if ((tb->sendbuf = get_string(&ibuf, sb)) == NULL) {
 			log_warn("%s: get_string", __func__);
 			free(tb);
 			return (-1);
@@ -732,7 +732,7 @@ 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 = get_string(&ibuf, s - 1))
 		    == NULL) {
 			log_warn("%s: get_string", __func__);
 			free(proto);
@@ -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) {		\
+		    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 = get_data(&ibuf, s)) == NULL)
 			goto fail;
 	}
 
diff --git a/relayd.c b/relayd.c
index b678103..5a556e7 100644
--- a/relayd.c
+++ b/relayd.c
@@ -437,7 +437,7 @@ 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) {
+			if ((str = get_string(&ibuf, s)) == NULL) {
 				log_warn("%s: get_string", __func__);
 				return (-1);
 			}
@@ -1806,28 +1806,40 @@ 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)
+get_string(struct ibuf *ibuf, size_t len)
 {
-	size_t	 i;
+	char	*s;
 
-	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)
+get_data(struct ibuf *ibuf, size_t len)
 {
 	u_int8_t	*data;
 
+	if (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..90e0b08 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		*get_string(struct ibuf *, size_t);
+void		*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);