Re: [7/7+1] relayd: ibuf_get_string / ibuf_get_data

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