From: Tracey Emery Subject: Re: gotwebd: unix socket shouldn't be "special" To: Omar Polo Cc: gameoftrees@openbsd.org Date: Thu, 16 May 2024 09:17:55 -0600 On Thu, May 16, 2024 at 03:09:53PM +0200, Omar Polo wrote: > one thing i find a bit annoying about gotwebd is how it considers the > unix socket and the tcp sockets differently. Both internally, and in the > config. > > So, for e.g. > > server "localhost" { > repos_path "..." > } > > has an implicit `listen on socket "/var/www/run/gotweb.sock"`, but > > server "localhost" { > listen on localhost port 9000 > } > > has it too. `listen on sock off' is needed, but i would prefer if when > there is an explicit `listen' directive (or more of them), we didn't add > an implicit one too. (the top-level `unix_socket off' has the same > problem IMHO.) > > At the same time, I'd also like to simplify how gotwebd handles the > sockets internally. There's this weird separation between unix sockets > and tcp sockets, where we can instead just reuse our list of addresses > for both (the struct sockaddr_storage covers unix sockets too.) > > I also had to make another change to make this working: relaxing the > addresses handling. There's no harm in having multiple `server' block > listening on the same socket, as gotwebd already has code to do the > matching (see gotwebd/gotweb.c:/^gotweb_get_server). > > To do so I'm adding a new list of de-duplicated addresses in > gotwebd->addresses. I'm also keeping the list of addresses per-server, > since in a follow-up I'd like to use it in gotweb_get_server(). > > The struct socket is loosing the name and srv_name fields, but those > were used only for debugging purposes. > > Doing all of this allows us to greatly simplify gotwebd internals, > remove a lot of lines, and to have a clearer configuration. Two options > are gone now, but I'm not sure how many peoples are using `listen on > socket off' and `unix_socket off'. > > Oh, as a bonus I'm also fixing unix_socket_name that previously was > doing nothing ;) > > Except that by adding some temporary hacks, I'm not sure how to split > this in smaller pieces. > > OK/thoughs/comments? :) Only compile tested and lightly browsed, but ok! > > > diffstat /home/op/w/got > M gotwebd/config.c | 4+ 5- > M gotwebd/gotwebd.conf.5 | 0+ 6- > M gotwebd/gotwebd.h | 1+ 9- > M gotwebd/parse.y | 63+ 84- > M gotwebd/sockets.c | 23+ 108- > > 5 files changed, 91 insertions(+), 212 deletions(-) > > diff /home/op/w/got > commit - eb916dafa2967bc60a8996ea7cc0a23a661ed88e > path + /home/op/w/got > blob - ebe0bac12e5e093bd83e0efed0048485805ea07d > file + gotwebd/config.c > --- gotwebd/config.c > +++ gotwebd/config.c > @@ -50,6 +50,7 @@ config_init(struct gotwebd *env) > env->server_cnt = 0; > TAILQ_INIT(&env->servers); > TAILQ_INIT(&env->sockets); > + TAILQ_INIT(&env->addresses); > > return 0; > } > @@ -88,9 +89,7 @@ config_getserver(struct gotwebd *env, struct imsg *ims > memcpy(srv, p, sizeof(*srv)); > > /* log server info */ > - log_debug("%s: server=%s fcgi_socket=%s unix_socket=%s", __func__, > - srv->name, srv->fcgi_socket ? "yes" : "no", srv->unix_socket ? > - "yes" : "no"); > + log_debug("%s: server=%s", __func__, srv->name); > > TAILQ_INSERT_TAIL(&env->servers, srv, entry); > > @@ -147,8 +146,8 @@ 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 af_type=%s socket_path=%s", > - __func__, sock->conf.name, sock->conf.id, sock->conf.srv_name, > + log_debug("%s: id=%d af_type=%s socket_path=%s", > + __func__, sock->conf.id, > sock->conf.af_type == AF_UNIX ? "unix" : > (sock->conf.af_type == AF_INET ? "inet" : > (sock->conf.af_type == AF_INET6 ? "inet6" : "unknown")), > blob - 2c4a828f3b8cde396a60854daae3bbc690ecdfd3 > file + gotwebd/gotwebd.conf.5 > --- gotwebd/gotwebd.conf.5 > +++ gotwebd/gotwebd.conf.5 > @@ -62,9 +62,6 @@ will be used. > Run the specified number of server processes. > .Xr gotwebd 8 > runs 3 server processes by default. > -.It Ic unix_socket Ar on | off > -Controls whether the servers will listen on unix sockets by default. > -Listening on unix sockets is the default. > .It Ic unix_socket_name Ar path > Set the path to the default unix socket. > Defaults to > @@ -108,8 +105,6 @@ argument may be number or a service name defined in > May be specified multiple times to build up a list of listening sockets. > However, a given combination of address and port may only be used by > one server. > -.It Ic listen on socket off > -Disable use of unix socket. > .It Ic listen on socket Ar path > Set the path to the unix socket used by the server. > .It Ic logo Ar path > @@ -211,7 +206,6 @@ implicit > socket. > .Bd -literal -offset indent > server "localhost" { > - listen on socket off > listen on 127.0.0.1 port 9000 > listen on ::1 port 9000 > } > blob - 9adea01286908db58bde811ebffe397078853fe3 > file + gotwebd/gotwebd.h > --- gotwebd/gotwebd.h > +++ gotwebd/gotwebd.h > @@ -308,11 +308,6 @@ struct server { > int show_repo_description; > int show_repo_cloneurl; > int respect_exportok; > - > - int unix_socket; > - char unix_socket_name[PATH_MAX]; > - > - int fcgi_socket; > }; > TAILQ_HEAD(serverlist, server); > > @@ -324,9 +319,6 @@ enum client_action { > struct socket_conf { > struct address addr; > > - char name[GOTWEBD_MAXTEXT]; > - char srv_name[GOTWEBD_MAXTEXT]; > - > int id; > int af_type; > char unix_socket_name[PATH_MAX]; > @@ -353,6 +345,7 @@ struct passwd; > struct gotwebd { > struct serverlist servers; > struct socketlist sockets; > + struct addresslist addresses; > > const char *gotwebd_conffile; > > @@ -372,7 +365,6 @@ struct gotwebd { > > char httpd_chroot[PATH_MAX]; > > - int unix_socket; > char unix_socket_name[PATH_MAX]; > }; > > blob - 84c27c6aef847eb203aee9d73ec6114c3af9eb15 > file + gotwebd/parse.y > --- gotwebd/parse.y > +++ gotwebd/parse.y > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -94,8 +95,8 @@ int getservice(const char *); > int n; > > int get_addrs(const char *, const char *, struct server *); > -int addr_dup_check(struct addresslist *, struct address *, > - const char *, const char *); > +int get_unix_addr(const char *, struct server *); > +int addr_dup_check(struct addresslist *, struct address *); > int add_addr(struct server *, struct address *); > > typedef struct { > @@ -112,7 +113,7 @@ typedef struct { > %token LOGO_URL SHOW_REPO_OWNER SHOW_REPO_AGE SHOW_REPO_DESCRIPTION > %token MAX_REPOS_DISPLAY REPOS_PATH MAX_COMMITS_DISPLAY ON ERROR > %token SHOW_SITE_OWNER SHOW_REPO_CLONEURL PORT PREFORK RESPECT_EXPORTOK > -%token UNIX_SOCKET UNIX_SOCKET_NAME SERVER CHROOT CUSTOM_CSS SOCKET > +%token UNIX_SOCKET_NAME SERVER CHROOT CUSTOM_CSS SOCKET > %token SUMMARY_COMMITS_DISPLAY SUMMARY_TAGS_DISPLAY > > %token STRING > @@ -201,9 +202,6 @@ main : PREFORK NUMBER { > } > free($2); > } > - | UNIX_SOCKET boolean { > - gotwebd->unix_socket = $2; > - } > | UNIX_SOCKET_NAME STRING { > n = snprintf(gotwebd->unix_socket_name, > sizeof(gotwebd->unix_socket_name), "%s%s", > @@ -328,7 +326,6 @@ serveropts1 : REPOS_PATH STRING { > } > free($3); > free($5); > - new_srv->fcgi_socket = 1; > } > | LISTEN ON listen_addr PORT NUMBER { > char portno[32]; > @@ -345,25 +342,11 @@ serveropts1 : REPOS_PATH STRING { > YYERROR; > } > free($3); > - new_srv->fcgi_socket = 1; > } > | LISTEN ON SOCKET STRING { > - if (strcasecmp($4, "off") == 0) { > - new_srv->unix_socket = 0; > + if (get_unix_addr($4, new_srv) == -1) { > + yyerror("can't listen on %s", $4); > free($4); > - YYACCEPT; > - } > - > - new_srv->unix_socket = 1; > - > - n = snprintf(new_srv->unix_socket_name, > - sizeof(new_srv->unix_socket_name), "%s%s", > - gotwebd->httpd_chroot, $4); > - if (n < 0 || > - (size_t)n >= sizeof(new_srv->unix_socket_name)) { > - yyerror("%s: unix_socket_name truncated", > - __func__); > - free($4); > YYERROR; > } > free($4); > @@ -489,7 +472,6 @@ lookup(char *s) > { "socket", SOCKET }, > { "summary_commits_display", SUMMARY_COMMITS_DISPLAY }, > { "summary_tags_display", SUMMARY_TAGS_DISPLAY }, > - { "unix_socket", UNIX_SOCKET }, > { "unix_socket_name", UNIX_SOCKET_NAME }, > }; > const struct keywords *p; > @@ -823,12 +805,19 @@ int > parse_config(const char *filename, struct gotwebd *env) > { > struct sym *sym, *next; > + struct server *srv; > + int n; > > if (config_init(env) == -1) > fatalx("failed to initialize configuration"); > > gotwebd = env; > > + n = snprintf(env->unix_socket_name, sizeof(env->unix_socket_name), > + "%s%s", D_HTTPD_CHROOT, D_UNIX_SOCKET); > + if (n < 0 || (size_t)n >= sizeof(env->unix_socket_name)) > + fatalx("%s: snprintf", __func__); > + > file = newfile(filename, 0); > if (file == NULL) { > add_default_server(); > @@ -854,13 +843,21 @@ parse_config(const char *filename, struct gotwebd *env > } > } > > - if (errors) > - return (-1); > - > /* just add default server if no config specified */ > if (gotwebd->server_cnt == 0) > add_default_server(); > > + /* add the implicit listen on socket where missing */ > + TAILQ_FOREACH(srv, &gotwebd->servers, entry) { > + if (!TAILQ_EMPTY(&srv->al)) > + continue; > + if (get_unix_addr(env->unix_socket_name, srv) == -1) > + yyerror("can't listen on %s", env->unix_socket_name); > + } > + > + if (errors) > + return (-1); > + > /* setup our listening sockets */ > sockets_parse_sockets(env); > > @@ -879,11 +876,6 @@ conf_new_server(const char *name) > n = strlcpy(srv->name, name, sizeof(srv->name)); > if (n >= sizeof(srv->name)) > fatalx("%s: strlcpy", __func__); > - n = snprintf(srv->unix_socket_name, > - sizeof(srv->unix_socket_name), "%s%s", D_HTTPD_CHROOT, > - D_UNIX_SOCKET); > - if (n < 0 || (size_t)n >= sizeof(srv->unix_socket_name)) > - fatalx("%s: snprintf", __func__); > n = strlcpy(srv->repos_path, D_GOTPATH, > sizeof(srv->repos_path)); > if (n >= sizeof(srv->repos_path)) > @@ -923,9 +915,6 @@ conf_new_server(const char *name) > srv->summary_commits_display = D_MAXSLCOMMDISP; > srv->summary_tags_display = D_MAXSLTAGDISP; > > - srv->unix_socket = 1; > - srv->fcgi_socket = 0; > - > TAILQ_INIT(&srv->al); > TAILQ_INSERT_TAIL(&gotwebd->servers, srv, entry); > gotwebd->server_cnt++; > @@ -1074,13 +1063,34 @@ get_addrs(const char *hostname, const char *servname, > } > > int > -addr_dup_check(struct addresslist *al, struct address *h, const char *new_srv, > - const char *other_srv) > +get_unix_addr(const char *path, struct server *new_srv) > { > + struct address *h; > + struct sockaddr_un *sun; > + > + if ((h = calloc(1, sizeof(*h))) == NULL) > + fatal("%s: calloc", __func__); > + > + h->ai_family = AF_UNIX; > + h->ai_socktype = SOCK_STREAM; > + h->ai_protocol = PF_UNSPEC; > + h->slen = sizeof(*sun); > + > + sun = (struct sockaddr_un *)&h->ss; > + sun->sun_family = AF_UNIX; > + if (strlcpy(sun->sun_path, path, sizeof(sun->sun_path)) >= > + sizeof(sun->sun_path)) { > + log_warnx("socket path too long: %s", sun->sun_path); > + return (-1); > + } > + > + return add_addr(new_srv, h); > +} > + > +int > +addr_dup_check(struct addresslist *al, struct address *h) > +{ > struct address *a; > - void *ia; > - char buf[INET6_ADDRSTRLEN]; > - const char *addrstr; > > TAILQ_FOREACH(a, al, entry) { > if (a->ai_family != h->ai_family || > @@ -1089,39 +1099,6 @@ addr_dup_check(struct addresslist *al, struct address > a->slen != h->slen || > memcmp(&a->ss, &h->ss, a->slen) != 0) > continue; > - > - switch (h->ss.ss_family) { > - case AF_INET: > - ia = &((struct sockaddr_in *)(&h->ss))->sin_addr; > - break; > - case AF_INET6: > - ia = &((struct sockaddr_in6 *)(&h->ss))->sin6_addr; > - break; > - default: > - yyerror("unknown address family: %d", h->ss.ss_family); > - return -1; > - } > - addrstr = inet_ntop(h->ss.ss_family, ia, buf, sizeof(buf)); > - if (addrstr) { > - if (other_srv) { > - yyerror("server %s: duplicate fcgi listen " > - "address %s:%d, already used by server %s", > - new_srv, addrstr, h->port, other_srv); > - } else { > - log_warnx("server: %s: duplicate fcgi listen " > - "address %s:%d", new_srv, addrstr, h->port); > - } > - } else { > - if (other_srv) { > - yyerror("server: %s: duplicate fcgi listen " > - "address, already used by server %s", > - new_srv, other_srv); > - } else { > - log_warnx("server %s: duplicate fcgi listen " > - "address", new_srv); > - } > - } > - > return -1; > } > > @@ -1132,20 +1109,22 @@ int > add_addr(struct server *new_srv, struct address *h) > { > struct server *srv; > + struct address *dup; > > - /* Address cannot be shared between different servers. */ > TAILQ_FOREACH(srv, &gotwebd->servers, entry) { > - if (srv == new_srv) > - continue; > - if (addr_dup_check(&srv->al, h, new_srv->name, srv->name)) > - return -1; > + if (addr_dup_check(&srv->al, h) == -1) { > + free(h); > + return 0; > + } > } > > - /* Tolerate duplicate address lines within the scope of a server. */ > - if (addr_dup_check(&new_srv->al, h, NULL, NULL) == 0) > - TAILQ_INSERT_TAIL(&new_srv->al, h, entry); > - else > - free(h); > + if (addr_dup_check(&gotwebd->addresses, h) == 0) { > + if ((dup = calloc(1, sizeof(*dup))) == NULL) > + fatal("%s: calloc", __func__); > + memcpy(dup, h, sizeof(*dup)); > + TAILQ_INSERT_TAIL(&gotwebd->addresses, dup, entry); > + } > > - return 0; > + TAILQ_INSERT_TAIL(&new_srv->al, h, entry); > + return (0); > } > blob - 156dad3dc6bd9624457ed102668c97c9f301eda3 > file + gotwebd/sockets.c > --- gotwebd/sockets.c > +++ gotwebd/sockets.c > @@ -78,10 +78,8 @@ static int sockets_create_socket(struct address *); > static int sockets_accept_reserve(int, struct sockaddr *, socklen_t *, > int, volatile int *); > > -static struct socket *sockets_conf_new_socket_unix(struct gotwebd *, > - struct server *, int); > -static struct socket *sockets_conf_new_socket_fcgi(struct gotwebd *, > - struct server *, int, struct address *); > +static struct socket *sockets_conf_new_socket(struct gotwebd *, > + int, struct address *); > > int cgi_inflight = 0; > > @@ -132,88 +130,25 @@ sockets(struct gotwebd *env, int fd) > void > sockets_parse_sockets(struct gotwebd *env) > { > - struct server *srv; > struct address *a; > struct socket *new_sock = NULL; > int sock_id = 1; > > - TAILQ_FOREACH(srv, &env->servers, entry) { > - if (srv->unix_socket) { > - new_sock = sockets_conf_new_socket_unix(env, srv, > - sock_id); > - if (new_sock) { > - sock_id++; > - TAILQ_INSERT_TAIL(&env->sockets, new_sock, > - entry); > - } > + TAILQ_FOREACH(a, &env->addresses, entry) { > + new_sock = sockets_conf_new_socket(env, sock_id, a); > + if (new_sock) { > + sock_id++; > + TAILQ_INSERT_TAIL(&env->sockets, > + new_sock, entry); > } > - > - if (srv->fcgi_socket) { > - if (TAILQ_EMPTY(&srv->al)) { > - fatalx("%s: server %s has no IP addresses to " > - "listen for FCGI connections", __func__, > - srv->name); > - } > - TAILQ_FOREACH(a, &srv->al, entry) { > - if (a->ss.ss_family != AF_INET && > - a->ss.ss_family != AF_INET6) > - continue; > - new_sock = sockets_conf_new_socket_fcgi(env, > - srv, sock_id, a); > - if (new_sock) { > - sock_id++; > - TAILQ_INSERT_TAIL(&env->sockets, > - new_sock, entry); > - } > - } > - } > } > } > > static struct socket * > -sockets_conf_new_socket_unix(struct gotwebd *env, struct server *srv, int id) > +sockets_conf_new_socket(struct gotwebd *env, int id, struct address *a) > { > struct socket *sock; > - int n; > - > - if ((sock = calloc(1, sizeof(*sock))) == NULL) > - fatalx("%s: calloc", __func__); > - > - sock->conf.id = id; > - sock->fd = -1; > - sock->conf.af_type = AF_UNIX; > - > - if (strlcpy(sock->conf.unix_socket_name, > - srv->unix_socket_name, > - sizeof(sock->conf.unix_socket_name)) >= > - sizeof(sock->conf.unix_socket_name)) { > - free(sock); > - fatalx("%s: strlcpy", __func__); > - } > - > - n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_parent", > - srv->name); > - if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) { > - free(sock); > - fatalx("%s: snprintf", __func__); > - } > - > - if (strlcpy(sock->conf.srv_name, srv->name, > - sizeof(sock->conf.srv_name)) >= sizeof(sock->conf.srv_name)) { > - free(sock); > - fatalx("%s: strlcpy", __func__); > - } > - > - return sock; > -} > - > -static struct socket * > -sockets_conf_new_socket_fcgi(struct gotwebd *env, struct server *srv, int id, > - struct address *a) > -{ > - struct socket *sock; > struct address *acp; > - int n; > > if ((sock = calloc(1, sizeof(*sock))) == NULL) > fatalx("%s: calloc", __func__); > @@ -222,21 +157,18 @@ sockets_conf_new_socket_fcgi(struct gotwebd *env, stru > sock->fd = -1; > sock->conf.af_type = a->ss.ss_family; > > + if (a->ss.ss_family == AF_UNIX) { > + struct sockaddr_un *sun; > + > + sun = (struct sockaddr_un *)&a->ss; > + if (strlcpy(sock->conf.unix_socket_name, sun->sun_path, > + sizeof(sock->conf.unix_socket_name)) >= > + sizeof(sock->conf.unix_socket_name)) > + fatalx("unix socket path too long: %s", sun->sun_path); > + } > + > sock->conf.fcgi_socket_port = a->port; > > - n = snprintf(sock->conf.name, GOTWEBD_MAXTEXT, "%s_parent", > - srv->name); > - if (n < 0 || (size_t)n >= GOTWEBD_MAXTEXT) { > - free(sock); > - fatalx("%s: snprintf", __func__); > - } > - > - if (strlcpy(sock->conf.srv_name, srv->name, > - sizeof(sock->conf.srv_name)) >= sizeof(sock->conf.srv_name)) { > - free(sock); > - fatalx("%s: strlcpy", __func__); > - } > - > acp = &sock->conf.addr; > > memcpy(&acp->ss, &a->ss, sizeof(acp->ss)); > @@ -438,9 +370,9 @@ sockets_privinit(struct gotwebd *env, struct socket *s > } > > 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", > + log_debug("%s: initializing %s FCGI socket on port %d", > __func__, sock->conf.af_type == AF_INET ? "inet" : "inet6", > - sock->conf.fcgi_socket_port, sock->conf.name); > + sock->conf.fcgi_socket_port); > sock->fd = sockets_create_socket(&sock->conf.addr); > if (sock->fd == -1) { > log_warnx("%s: create FCGI socket failed", __func__); > @@ -454,33 +386,15 @@ sockets_privinit(struct gotwebd *env, struct socket *s > static int > sockets_unix_socket_listen(struct gotwebd *env, struct socket *sock) > { > - struct sockaddr_un sun; > - struct socket *tsock; > int u_fd = -1; > mode_t old_umask, mode; > > - TAILQ_FOREACH(tsock, &env->sockets, entry) { > - if (strcmp(tsock->conf.unix_socket_name, > - sock->conf.unix_socket_name) == 0 && > - tsock->fd != -1) > - return (tsock->fd); > - } > - > u_fd = socket(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK| SOCK_CLOEXEC, 0); > if (u_fd == -1) { > log_warn("%s: socket", __func__); > return -1; > } > > - sun.sun_family = AF_UNIX; > - if (strlcpy(sun.sun_path, sock->conf.unix_socket_name, > - sizeof(sun.sun_path)) >= sizeof(sun.sun_path)) { > - log_warn("%s: %s name too long", __func__, > - sock->conf.unix_socket_name); > - close(u_fd); > - return -1; > - } > - > if (unlink(sock->conf.unix_socket_name) == -1) { > if (errno != ENOENT) { > log_warn("%s: unlink %s", __func__, > @@ -493,7 +407,8 @@ sockets_unix_socket_listen(struct gotwebd *env, struct > old_umask = umask(S_IXUSR|S_IXGRP|S_IWOTH|S_IROTH|S_IXOTH); > mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP; > > - if (bind(u_fd, (struct sockaddr *)&sun, sizeof(sun)) == -1) { > + if (bind(u_fd, (struct sockaddr *)&sock->conf.addr.ss, > + sock->conf.addr.slen) == -1) { > log_warn("%s: bind: %s", __func__, sock->conf.unix_socket_name); > close(u_fd); > (void)umask(old_umask); >