From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: initialize IPv4 and IPv6 sockets in the same way
To: gameoftrees@openbsd.org
Date: Tue, 16 Aug 2022 10:51:22 -0600

On Tue, Aug 16, 2022 at 06:45:27PM +0200, Stefan Sperling wrote:
> Simplify gotwebd's socket initialization code.
> 
> There is no reason to treat IPv6 differently, it's just another address
> family. We might have a dual-stack setup, or IPv4-only, or IPv6-only,
> and all these cases ought to work.
> 
> At present, the configuration file accepts only one "listen on" statement
> per server. Which means the only way to run dual-stack is to specify the
> name of an interface which has both IPv4 and IPv6 addresses configured,
> instead of specifying a set of IP addresses.
> This patch is a first step towards lifting this limitation.
> 
> Ok?

ok

>  
> diff 73ffdfc038e3a5f5bf130d7c5754428ff92f69e4 4419b431948d0a7a36543168bce3b3e253ae08ec
> commit - 73ffdfc038e3a5f5bf130d7c5754428ff92f69e4
> commit + 4419b431948d0a7a36543168bce3b3e253ae08ec
> blob - 2ff6fae01c54ac33ce96e4a1a48f018b553cdc75
> blob + 29bdf6301687d8ed6e4ae5f52d25ea757efd3176
> --- gotwebd/config.c
> +++ gotwebd/config.c
> @@ -212,11 +212,11 @@ config_getsock(struct gotwebd *env, struct imsg *imsg)
>  		sock->pack_fds[i] = -1;
>  
>  	/* log new socket info */
> -	log_debug("%s: name=%s id=%d server=%s child_id=%d parent_id=%d "
> -	    "type=%s ipv4=%d ipv6=%d socket_path=%s",
> +	log_debug("%s: name=%s id=%d server=%s af_type=%s socket_path=%s",
>  	    __func__, sock->conf.name, sock->conf.id, sock->conf.srv_name,
> -	    sock->conf.child_id, sock->conf.parent_id, sock->conf.type ?
> -	    "fcgi" : "unix", sock->conf.ipv4, sock->conf.ipv6,
> +	    sock->conf.af_type == AF_UNIX ? "unix" :
> +	    (sock->conf.af_type == AF_INET ? "inet" :
> +	    (sock->conf.af_type == AF_INET6 ? "inet6" : "unknown")),
>  	    strlen(sock->conf.unix_socket_name) ?
>  	    sock->conf.unix_socket_name : "none");
>  
> blob - 0f238f4dcbdecd4cd7a5224cea8dd35c9bd25215
> blob + 71be6a49025d52f2768a5f9a8f625964d6a23e38
> --- gotwebd/gotwebd.h
> +++ gotwebd/gotwebd.h
> @@ -288,11 +288,6 @@ enum client_action {
>  	CLIENT_DISCONNECT,
>  };
>  
> -enum sock_type {
> -	UNIX,
> -	FCGI,
> -};
> -
>  struct socket_conf {
>  	struct addresslist	al;
>  
> @@ -300,13 +295,7 @@ struct socket_conf {
>  	char		 srv_name[GOTWEBD_MAXTEXT];
>  
>  	int		 id;
> -	int		 child_id;
> -	int		 parent_id;
> -
> -	int		 ipv4;
> -	int		 ipv6;
> -
> -	int		 type;
> +	int		 af_type;
>  	char		 unix_socket_name[PATH_MAX];
>  	in_port_t	 fcgi_socket_port;
>  };
> blob - a8601823f7e0c6d8d1c751a2980c6b4976f7003e
> blob + bde51dbff1e8cbc18fd20d36fe3701aa4ab66e53
> --- gotwebd/sockets.c
> +++ gotwebd/sockets.c
> @@ -68,7 +68,6 @@ void	 sockets_run(struct privsep *, struct privsep_pro
>  void	 sockets_launch(void);
>  void	 sockets_purge(struct gotwebd *);
>  void	 sockets_accept_paused(int, short, void *);
> -void	 sockets_dup_new_socket(struct socket *, struct socket *);
>  void	 sockets_rlimit(int);
>  
>  
> @@ -78,9 +77,8 @@ int	 sockets_create_socket(struct addresslist *, in_po
>  int	 sockets_accept_reserve(int, struct sockaddr *, socklen_t *, int,
>  	    volatile int *);
>  
> -struct socket
> -	*sockets_conf_new_socket(struct gotwebd *, struct server *, int, int,
> -	    int);
> +struct socket *sockets_conf_new_socket(struct gotwebd *, struct server *,
> +    int, int);
>  
>  int cgi_inflight = 0;
>  
> @@ -119,97 +117,49 @@ void
>  sockets_parse_sockets(struct gotwebd *env)
>  {
>  	struct server *srv;
> -	struct socket *sock, *new_sock = NULL;
> -	struct address *a;
> +	struct socket *new_sock = NULL;
>  	int sock_id = 0, ipv4 = 0, ipv6 = 0;
>  
>  	TAILQ_FOREACH(srv, &env->servers, entry) {
>  		if (srv->unix_socket) {
>  			sock_id++;
>  			new_sock = sockets_conf_new_socket(env, srv,
> -			    sock_id, UNIX, 0);
> +			    sock_id, AF_UNIX);
> +			/* Should always succeed. */
>  			TAILQ_INSERT_TAIL(&env->sockets, new_sock, entry);
>  		}
>  
>  		if (srv->fcgi_socket) {
>  			sock_id++;
> -			new_sock = sockets_conf_new_socket(env, srv,
> -			    sock_id, FCGI, 0);
> -			TAILQ_INSERT_TAIL(&env->sockets, new_sock, entry);
> +			new_sock = sockets_conf_new_socket(env, srv, sock_id,
> +			    AF_INET);
> +			if (new_sock) {
> +				TAILQ_INSERT_TAIL(&env->sockets, new_sock,
> +				    entry);
> +				ipv4 = 1;
> +				sock_id++;
> +			}
>  
> -			/* add ipv6 children */
> -			TAILQ_FOREACH(sock, &env->sockets, entry) {
> -				ipv4 = ipv6 = 0;
> -
> -				TAILQ_FOREACH(a, &sock->conf.al, entry) {
> -					if (a->ss.ss_family == AF_INET)
> -						ipv4 = 1;
> -					if (a->ss.ss_family == AF_INET6)
> -						ipv6 = 1;
> -				}
> -
> -				/* create ipv6 sock */
> -				if (ipv4 == 1 && ipv6 == 1) {
> -					sock_id++;
> -					sock->conf.child_id = sock_id;
> -					new_sock = sockets_conf_new_socket(env,
> -					    srv, sock_id, FCGI, 1);
> -					sockets_dup_new_socket(sock, new_sock);
> -					TAILQ_INSERT_TAIL(&env->sockets,
> -					    new_sock, entry);
> -					continue;
> -				}
> +			new_sock = sockets_conf_new_socket(env, srv, sock_id,
> +			    AF_INET6);
> +			if (new_sock) {
> +				TAILQ_INSERT_TAIL(&env->sockets,
> +				    new_sock, entry);
> +				ipv6 = 1;
>  			}
> -		}
> -	}
> -}
>  
> -void
> -sockets_dup_new_socket(struct socket *p_sock, struct socket *sock)
> -{
> -	struct address *a, *acp;
> -	int n;
> -
> -	sock->conf.parent_id = p_sock->conf.id;
> -	sock->conf.ipv4 = 0;
> -	sock->conf.ipv6 = 1;
> -
> -	memcpy(&sock->conf.srv_name, p_sock->conf.srv_name,
> -	    sizeof(sock->conf.srv_name));
> -
> -	n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_child",
> -	    p_sock->conf.srv_name);
> -	if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) {
> -		free(p_sock);
> -		free(sock);
> -		fatalx("%s: snprintf", __func__);
> -	}
> -
> -	TAILQ_FOREACH(a, &p_sock->conf.al, entry) {
> -		if (a->ss.ss_family == AF_INET)
> -			continue;
> -
> -		if ((acp = calloc(1, sizeof(*acp))) == NULL)
> -			fatal("%s: calloc", __func__);
> -		memcpy(&acp->ss, &a->ss, sizeof(acp->ss));
> -		acp->ipproto = a->ipproto;
> -		acp->prefixlen = a->prefixlen;
> -		acp->port = a->port;
> -		if (strlen(a->ifname) != 0) {
> -			if (strlcpy(acp->ifname, a->ifname,
> -			    sizeof(acp->ifname)) >= sizeof(acp->ifname)) {
> -				fatalx("%s: interface name truncated",
> -				    __func__);
> +			if (ipv4 == 0 && ipv6 == 0) {
> +				fatalx("%s: have no IP addresses to listen "
> +				    "for FCGI connections", __func__);
>  			}
>  		}
> -
> -		TAILQ_INSERT_TAIL(&sock->conf.al, acp, entry);
>  	}
>  }
>  
> +/* Returns NULL if no addresses in the requested family are configured. */
>  struct socket *
>  sockets_conf_new_socket(struct gotwebd *env, struct server *srv, int id,
> -    int type, int is_dup)
> +    int af_type)
>  {
>  	struct socket *sock;
>  	struct address *a, *acp;
> @@ -220,14 +170,11 @@ sockets_conf_new_socket(struct gotwebd *env, struct se
>  
>  	TAILQ_INIT(&sock->conf.al);
>  
> -	sock->conf.parent_id = 0;
>  	sock->conf.id = id;
> -
>  	sock->fd = -1;
> +	sock->conf.af_type = af_type;
>  
> -	sock->conf.type = type;
> -
> -	if (type == UNIX) {
> +	if (af_type == AF_UNIX) {
>  		if (strlcpy(sock->conf.unix_socket_name,
>  		    srv->unix_socket_name,
>  		    sizeof(sock->conf.unix_socket_name)) >=
> @@ -235,14 +182,10 @@ sockets_conf_new_socket(struct gotwebd *env, struct se
>  			free(sock);
>  			fatalx("%s: strlcpy", __func__);
>  		}
> -	} else
> -		sock->conf.ipv4 = 1;
> +	}
>  
>  	sock->conf.fcgi_socket_port = srv->fcgi_socket_port;
>  
> -	if (is_dup)
> -		goto done;
> -
>  	n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_parent",
>  	    srv->name);
>  	if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) {
> @@ -257,6 +200,9 @@ sockets_conf_new_socket(struct gotwebd *env, struct se
>  	}
>  
>  	TAILQ_FOREACH(a, &srv->al, entry) {
> +		if (a->ss.ss_family != af_type)
> +			continue;
> +
>  		if ((acp = calloc(1, sizeof(*acp))) == NULL) {
>  			free(sock);
>  			fatal("%s: calloc", __func__);
> @@ -275,7 +221,13 @@ sockets_conf_new_socket(struct gotwebd *env, struct se
>  
>  		TAILQ_INSERT_TAIL(&sock->conf.al, acp, entry);
>  	}
> -done:
> +
> +	if ((af_type == AF_INET || af_type == AF_INET6) &&
> +	    TAILQ_EMPTY(&sock->conf.al)) {
> +		free(sock);
> +		return NULL;
> +	}
> +
>  	return (sock);
>  }
>  
> @@ -412,7 +364,7 @@ sockets_privinit(struct gotwebd *env, struct socket *s
>  {
>  	struct privsep *ps = env->gotwebd_ps;
>  
> -	if (sock->conf.type == UNIX) {
> +	if (sock->conf.af_type == AF_UNIX) {
>  		log_debug("%s: initializing unix socket %s", __func__,
>  		    sock->conf.unix_socket_name);
>  		sock->fd = sockets_unix_socket_listen(ps, sock);
> @@ -422,9 +374,10 @@ sockets_privinit(struct gotwebd *env, struct socket *s
>  		}
>  	}
>  
> -	if (sock->conf.type == FCGI) {
> -		log_debug("%s: initializing fcgi socket for %s", __func__,
> -		    sock->conf.name);
> +	if (sock->conf.af_type == AF_INET || sock->conf.af_type == AF_INET6) {
> +		log_debug("%s: initializing %s FCGI socket on port %d for %s",
> +		    __func__, sock->conf.af_type == AF_INET ? "inet" : "inet6",
> +		    sock->conf.fcgi_socket_port, sock->conf.name);
>  		sock->fd = sockets_create_socket(&sock->conf.al,
>  		    sock->conf.fcgi_socket_port);
>  		if (sock->fd == -1) {
> 

-- 

Tracey Emery