gotwebd: unix socket shouldn't be "special"

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? :) 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 <sys/queue.h> #include <sys/socket.h> #include <sys/stat.h> +#include <sys/un.h> #include <net/if.h> #include <netinet/in.h> @@ -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 <v.string> 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);

2024-05-16 13:09 Omar Polo:
gotwebd: unix socket shouldn't be "special"
- 2024-05-16 15:17 Tracey Emery:
  gotwebd: unix socket shouldn't be "special"