From: Kirill A. Korinsky <kirill@korins.ky>
Subject: Re: relayd: refactor config_getrelayfd
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Thu, 29 Oct 2026 22:26:24 +0100

On Thu, 29 Oct 2026 08:58:26 +0100,
Rafael Sadowski <rafael@sizeofvoid.org> wrote:
> 
> Based on tb@'s feedback below, you can find a suggestion for
> implementing this function properly with correct error handling.
> 
> Unknown fd types and imsgs without an fd are now treated as errors
> instead of being silently ignored.
> 
> As an improvement no orphan cert is created when the relay is unknown.
> 
> BTW the old code loves to trust imsg_get_fd blindly.
> 
> Feedback, OK?
>

It reads way better than current code, thanks.

OK kirill@

> >> config.c:1219:36: warning: 'cert' may be used uninitialized [-Wmaybe-uninitialized]
> >>  1219 |                 cert->cert_ocsp_fd = imsg_get_fd(imsg);
> >>       |                 ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~
> >> config.c:1186:34: note: 'cert' was declared here
> >>  1186 |         struct relay_cert       *cert;
> >>       |                                  ^~~~
> >
> >Maybe we should just ignore these warnings... Adding kludges to badly
> >written code to appease a dumb compiler isn't making the world better
> >place.
> 
> 
> diff --git a/config.c b/config.c
> index d1f8f8c..4690ae9 100644
> --- a/config.c
> +++ b/config.c
> @@ -31,6 +31,9 @@
>  #include "relayd.h"
>  #include "log.h"
>  
> +static struct relay_cert *config_getcert(struct relayd *,
> +		    struct ctl_relayfd *);
> +
>  int
>  config_init(struct relayd *env)
>  {
> @@ -1178,55 +1181,70 @@ config_getrelaytable(struct relayd *env, struct imsg *imsg)
>  	return (-1);
>  }
>  
> +static struct relay_cert *
> +config_getcert(struct relayd *env, struct ctl_relayfd *crfd)
> +{
> +	struct relay_cert	*cert;
> +
> +	if ((cert = cert_find(env, crfd->id)) == NULL) {
> +		if ((cert = cert_add(env, crfd->id)) == NULL)
> +			return (NULL);
> +		cert->cert_relayid = crfd->relayid;
> +	}
> +	return (cert);
> +}
> +
>  int
>  config_getrelayfd(struct relayd *env, struct imsg *imsg)
>  {
>  	struct ctl_relayfd	 crfd;
> -	struct relay		*rlay = NULL;
> +	struct relay		*rlay;
>  	struct relay_cert	*cert;
> +	int			 fd;
>  
>  	if (imsg_get_data(imsg, &crfd, sizeof(crfd)) == -1) {
>  		log_warn("%s: imsg_get_data", __func__);
>  		return (-1);
>  	}
>  
> -	switch (crfd.type) {
> -	case RELAY_FD_CERT:
> -	case RELAY_FD_KEY:
> -	case RELAY_FD_OCSP:
> -		if ((cert = cert_find(env, crfd.id)) == NULL) {
> -			if ((cert = cert_add(env, crfd.id)) == NULL)
> -				return (-1);
> -			cert->cert_relayid = crfd.relayid;
> -		}
> -		/* FALLTHROUGH */
> -	default:
> -		if ((rlay = relay_find(env, crfd.relayid)) == NULL) {
> -			log_debug("%s: unknown relay", __func__);
> -			return (-1);
> -		}
> -		break;
> +	if ((rlay = relay_find(env, crfd.relayid)) == NULL) {
> +		log_debug("%s: unknown relay", __func__);
> +		return (-1);
> +	}
> +
> +	if ((fd = imsg_get_fd(imsg)) == -1) {
> +		log_warnx("%s: no fd for type %d", __func__, crfd.type);
> +		return (-1);
>  	}
>  
>  	switch (crfd.type) {
>  	case RELAY_FD_CERT:
> -		cert->cert_fd = imsg_get_fd(imsg);
> +		if ((cert = config_getcert(env, &crfd)) == NULL)
> +			goto fail;
> +		cert->cert_fd = fd;
>  		break;
>  	case RELAY_FD_KEY:
> -		cert->cert_key_fd = imsg_get_fd(imsg);
> +		if ((cert = config_getcert(env, &crfd)) == NULL)
> +			goto fail;
> +		cert->cert_key_fd = fd;
>  		break;
>  	case RELAY_FD_OCSP:
> -		cert->cert_ocsp_fd = imsg_get_fd(imsg);
> +		if ((cert = config_getcert(env, &crfd)) == NULL)
> +			goto fail;
> +		cert->cert_ocsp_fd = fd;
>  		break;
>  	case RELAY_FD_CACERT:
> -		rlay->rl_tls_ca_fd = imsg_get_fd(imsg);
> +		rlay->rl_tls_ca_fd = fd;
>  		break;
>  	case RELAY_FD_CAFILE:
> -		rlay->rl_tls_cacert_fd = imsg_get_fd(imsg);
> +		rlay->rl_tls_cacert_fd = fd;
>  		break;
>  	case RELAY_FD_CLIENTCACERT:
> -		rlay->rl_tls_client_ca_fd = imsg_get_fd(imsg);
> +		rlay->rl_tls_client_ca_fd = fd;
>  		break;
> +	default:
> +		log_warnx("%s: unknown type %d", __func__, crfd.type);
> +		goto fail;
>  	}
>  
>  	log_debug("%s: %s %d received relay type %d for relay %s", __func__,
> @@ -1234,4 +1252,8 @@ config_getrelayfd(struct relayd *env, struct imsg *imsg)
>  	    crfd.type, rlay->rl_conf.name);
>  
>  	return (0);
> +
> + fail:
> +	close(fd);
> +	return (-1);
>  }
> 

-- 
wbr, Kirill