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

On Tue, Jun 09, 2026 at 09:17:08PM +0200, Rafael Sadowski wrote:
> On Tue Jun 09, 2026 at 08:55:41PM +0200, Claudio Jeker wrote:
> > 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.
> 
> Because I just saw the "str = strndup(ibuf_data(buf), len);" and not the
> "buf->rpos += len;" For sure I'll use ibuf_get_string form libutil instead.

Cool. Diff still has my OK.
 
> > 
> > I wonder how many other reykd use get_string and get_data as well.
> 
> I'll have a look at it. But first, there is more in relayd/imsg space.

No rush.
 
> Rafael
> 
> diff --git a/config.c b/config.c
> index 7ca37d5..c4a12e4 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 = get_data(&ibuf, s)) == NULL)
>  			goto fail;
>  	}
>  
> diff --git a/relayd.c b/relayd.c
> index b678103..201144c 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,21 @@ socket_rlimit(int maxfd)
>  		fatal("%s: failed to set resource limit", __func__);
>  }
>  
> -char *
> -get_string(u_int8_t *ptr, size_t len)
> -{
> -	size_t	 i;
> -
> -	for (i = 0; i < len; i++)
> -		if (!(isprint((unsigned char)ptr[i]) ||
> -		    isspace((unsigned char)ptr[i])))
> -			break;
> -
> -	return strndup(ptr, i);
> -}
> -
>  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..ac771fb 100644
> --- a/relayd.h
> +++ b/relayd.h
> @@ -1336,8 +1336,7 @@ 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);
> +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);
> 

-- 
:wq Claudio