From: Claudio Jeker Subject: Re: [7/7+1] relayd: ibuf_get_string / ibuf_get_data To: Rafael Sadowski Cc: tech@openbsd.org Date: Tue, 9 Jun 2026 20:55:41 +0200 On Tue, Jun 09, 2026 at 06:27:20PM +0200, Rafael Sadowski wrote: > 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); > Looks good to me. OK claudio@ Is there a reason why you implemented get_string instead of just using ibuf_get_string()? To me it looks like both return the same result. I wonder how many other reykd use get_string and get_data as well. -- :wq Claudio