From: Crystal Kolipe <kolipe.c@exoticsilicon.com>
Subject: Re: relayd: fix dead store
To: Rafael Sadowski <rafael@sizeofvoid.org>
Cc: tech@openbsd.org
Date: Mon, 29 Dec 2025 10:18:34 +0000

On Mon, Dec 29, 2025 at 10:44:47AM +0100, Rafael Sadowski wrote:
> First c is never used  so check if carp_group_find is NULL and return.

Shouldn't the check be for carp_group_find != NULL, otherwise you invert the
logic?

In the current version, if carp_group_find != NULL, the whole conditional is
skipped and the code falls through to return(0).

If carp_group_find == NULL, then it tries to allocate and then populate
c->group, and on success checks if the group is demoted.

With your patch, crap_group_find == NULL does an immediate return(0), and in
the != NULL case, the code will proceed to do the calloc, populate, and
demotion check dance.

> 
> carp.c:64:7: warning: Although the value stored to 'c' is used in the
> enclosing expression, the value is never actually read from 'c'
> [deadcode.DeadSt ores]
>    64 |         if ((c = carp_group_find(group)) == NULL)
> 
> 
> diff --git a/usr.sbin/relayd/carp.c b/usr.sbin/relayd/carp.c
> index 6a6a55a0fbb..34b6b6785a2 100644
> --- a/usr.sbin/relayd/carp.c
> +++ b/usr.sbin/relayd/carp.c
> @@ -61,29 +61,31 @@ carp_demote_init(char *group, int force)
>  	struct carpgroup	*c;
>  	int			 level;
>  
> -	if ((c = carp_group_find(group)) == NULL) {
> -		if ((c = calloc(1, sizeof(struct carpgroup))) == NULL) {
> -			log_warn("%s: calloc", __func__);
> -			return (-1);
> -		}
> -		if ((c->group = strdup(group)) == NULL) {
> -			log_warn("%s: strdup", __func__);
> -			free(c);
> -			return (-1);
> -		}
> -
> -		/* only demote if this group already is demoted */
> -		if ((level = carp_demote_get(group)) == -1) {
> -			free(c->group);
> -			free(c);
> -			return (-1);
> -		}
> -		if (level > 0 || force)
> -			c->do_demote = 1;
> -
> -		TAILQ_INSERT_TAIL(&carpgroups, c, entry);
> +	if (carp_group_find(group) == NULL) {
                                   ^^
                         should be != to match original behaviour

> +		return (0);
> +	}
> +
> +	if ((c = calloc(1, sizeof(struct carpgroup))) == NULL) {
> +		log_warn("%s: calloc", __func__);
> +		return (-1);
> +	}
> +	if ((c->group = strdup(group)) == NULL) {
> +		log_warn("%s: strdup", __func__);
> +		free(c);
> +		return (-1);
>  	}
>  
> +	/* only demote if this group already is demoted */
> +	if ((level = carp_demote_get(group)) == -1) {
> +		free(c->group);
> +		free(c);
> +		return (-1);
> +	}
> +	if (level > 0 || force)
> +		c->do_demote = 1;
> +
> +	TAILQ_INSERT_TAIL(&carpgroups, c, entry);
> +
>  	return (0);
>  }
>  
>