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

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.
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 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.

> 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);

-- 
:wq Claudio