Commit Diff

Commit:
2c3e53dac6faed4d9502bd3310b4837f0d3112cf
From:
Omar Polo <op@omarpolo.com>
Date:
Message:
give each server process its own socket for the executor this fixes a bug introduced with the prefork mechanics: every server process shared the same socket, and this would cause a race condition when multiple server processes asked for a script cgi being executed. This gives each server process its own socket to talk to the executor, so the race cannot happen.
Actions:
Patch | Tree
commit - fda7b99fc7f19b04eced114983268cfe3eb46c99
commit + 2c3e53dac6faed4d9502bd3310b4837f0d3112cf
blob - f1b4fa5df52ee194cc7c703fe826a2dfa6855599
blob + 5ff75159db1183a7bc7f6fcdf58020329571049d
--- ex.c
+++ ex.c
@@ -19,6 +19,7 @@
 #include <err.h>
 #include <errno.h>
 
+#include <event.h>
 #include <fcntl.h>
 #include <libgen.h>
 #include <limits.h>
@@ -397,14 +398,49 @@ childerr:
 	_exit(1);
 }
 
-int
-executor_main()
+static void
+handle_fork_req(int fd, short ev, void *data)
 {
 	char *spath, *relpath, *addr, *ruser, *cissuer, *chash;
-        struct vhost *vhost;
+	struct vhost *vhost;
 	struct iri iri;
 	time_t notbefore, notafter;
 	int d;
+
+	if (!recv_iri(fd, &iri)
+	    || !recv_string(fd, &spath)
+	    || !recv_string(fd, &relpath)
+	    || !recv_string(fd, &addr)
+	    || !recv_string(fd, &ruser)
+	    || !recv_string(fd, &cissuer)
+	    || !recv_string(fd, &chash)
+	    || !recv_time(fd, &notbefore)
+	    || !recv_time(fd, &notafter)
+	    || !recv_vhost(fd, &vhost))
+		fatal("failure in handling fork request");
+
+	d = launch_cgi(&iri, spath, relpath, addr, ruser, cissuer, chash,
+	    notbefore, notafter, vhost);
+	if (!send_fd(fd, d))
+		fatal("failure in sending the fd to the server: %s",
+		    strerror(errno));
+	close(d);
+
+	free_recvd_iri(&iri);
+	free(spath);
+	free(relpath);
+	free(addr);
+	free(ruser);
+	free(cissuer);
+	free(chash);
+}
+
+int
+executor_main(void)
+{
+	struct vhost	*vhost;
+	struct event	 evs[PROC_MAX];
+	int		 i;
 
 #ifdef __OpenBSD__
 	for (vhost = hosts; vhost->domain != NULL; ++vhost) {
@@ -419,39 +455,15 @@ executor_main()
 		err(1, "pledge");
 #endif
 
-	while (!hupped) {
-		if (!recv_iri(exfd, &iri)
-		    || !recv_string(exfd, &spath)
-		    || !recv_string(exfd, &relpath)
-		    || !recv_string(exfd, &addr)
-		    || !recv_string(exfd, &ruser)
-		    || !recv_string(exfd, &cissuer)
-		    || !recv_string(exfd, &chash)
-		    || !recv_time(exfd, &notbefore)
-		    || !recv_time(exfd, &notafter)
-		    || !recv_vhost(exfd, &vhost))
-			break;
+	event_init();
 
-		d = launch_cgi(&iri, spath, relpath, addr, ruser, cissuer, chash,
-		    notbefore, notafter, vhost);
-		if (!send_fd(exfd, d))
-			break;
-		close(d);
-
-		free_recvd_iri(&iri);
-		free(spath);
-		free(relpath);
-		free(addr);
-		free(ruser);
-		free(cissuer);
-		free(chash);
+	for (i = 0; i < conf.prefork; ++i) {
+		event_set(&evs[i], servpipes[i], EV_READ | EV_PERSIST,
+		    handle_fork_req, NULL);
+		event_add(&evs[i], NULL);
 	}
 
-	if (hupped)
-		_exit(0);
+	event_dispatch();
 
-	/* kill all process in my group.  This means the listener and
-	 * every pending CGI script. */
-	kill(0, SIGINT);
 	return 1;
 }
blob - daa8a30114c2cb7b84a4bb5d3a1a862b4898cf8e
blob + 7c76bb56f5fe6ba219d9b91817c7df8b323b0584
--- gmid.1
+++ gmid.1
@@ -159,6 +159,7 @@ This increases the performance and prevents delays whe
 a server.
 .Nm
 runs 3 server processes by default, when not in config-less mode.
+The maximum number allowed is 16.
 .It Ic protocols Ar string
 Specify the TLS protocols to enable.
 Refer to
blob - d14891ab6dfa1a4a68e79ee7c51da7fe78924c50
blob + 3671763da3eb448807659c929d683f2f7e4cce2f
--- gmid.c
+++ gmid.c
@@ -29,7 +29,7 @@ volatile sig_atomic_t hupped;
 
 struct vhost hosts[HOSTSLEN];
 
-int exfd, logfd, sock4, sock6;
+int exfd, logfd, sock4, sock6, servpipes[PROC_MAX];
 
 struct imsgbuf logpibuf, logcibuf;
 
@@ -297,31 +297,6 @@ drop_priv(void)
 
 	if (getuid() == 0)
 		log_warn(NULL, "not a good idea to run a network daemon as root");
-}
-
-static int
-spawn_listeners(int *p)
-{
-	int i;
-
-	close(p[0]);
-	exfd = p[1];
-
-	for (i = 0; i < conf.prefork -1; ++i) {
-		switch (fork()) {
-		case -1:
-			fatal("fork: %s", strerror(errno));
-		case 0:		/* child */
-			setproctitle("server(%d)", i+1);
-			return listener_main();
-		}
-	}
-
-	if (conf.prefork == 0)
-		setproctitle("server");
-	else
-		setproctitle("server(%d)", conf.prefork);
-	return listener_main();
 }
 
 static void
@@ -359,13 +334,13 @@ logger_init(void)
 	}
 }
 
-
 static int
-serve(int argc, char **argv, int *p)
+serve(int argc, char **argv)
 {
-	char path[PATH_MAX];
+	char	path[PATH_MAX];
+	int	i, p[2];
 
-	if (config_path == NULL) {
+        if (config_path == NULL) {
 		if (hostname == NULL)
 			hostname = "localhost";
 		if (certs_dir == NULL)
@@ -395,28 +370,35 @@ serve(int argc, char **argv, int *p)
 	 * to put private certs inside the chroot. */
 	setup_tls();
 
-	switch (fork()) {
-	case -1:
-		fatal("fork: %s", strerror(errno));
+	for (i = 0; i < conf.prefork; ++i) {
+		if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC,
+		    PF_UNSPEC, p) == -1)
+			fatal("socketpair: %s", strerror(errno));
 
-	case 0:			/* child */
-		return spawn_listeners(p);
-
-	default:		/* parent */
-		setproctitle("executor");
-		close(p[1]);
-		exfd = p[0];
-		drop_priv();
-		unblock_signals();
-		return executor_main();
+		switch (fork()) {
+		case -1:
+			fatal("fork: %s", strerror(errno));
+		case 0:		/* child */
+			close(p[0]);
+			exfd = p[1];
+			setproctitle("server");
+			_exit(listener_main());
+		default:
+			servpipes[i] = p[0];
+			close(p[1]);
+		}
 	}
+
+	setproctitle("executor");
+	drop_priv();
+	unblock_signals();
+	_exit(executor_main());
 }
 
 int
 main(int argc, char **argv)
 {
-	int ch, p[2];
-	int conftest = 0, configless = 0;
+	int ch, conftest = 0, configless = 0;
 	int old_ipv6, old_port;
 
 	init_config();
@@ -482,7 +464,7 @@ main(int argc, char **argv)
 	if (config_path == NULL) {
 		configless = 1;
 		conf.foreground = 1;
-		conf.prefork = 0;
+		conf.prefork = 1;
 		conf.verbose++;
 	}
 
@@ -509,10 +491,6 @@ main(int argc, char **argv)
 			err(1, "daemon");
 	}
 
-	if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC,
-	    PF_UNSPEC, p) == -1)
-		err(1, "socketpair");
-
 	if (config_path != NULL)
 		parse_conf(config_path);
 
@@ -524,7 +502,7 @@ main(int argc, char **argv)
 		sock6 = make_socket(conf.port, AF_INET6);
 
 	if (configless)
-		return serve(argc, argv, p);
+		return serve(argc, argv);
 
 	/* wait a sighup and reload the daemon */
 	for (;;) {
@@ -535,12 +513,9 @@ main(int argc, char **argv)
 		case -1:
 			fatal("fork: %s", strerror(errno));
 		case 0:
-			return serve(argc, argv, p);
+			return serve(argc, argv);
 		}
 
-		close(p[0]);
-		close(p[1]);
-
 		wait_sighup();
 		unblock_signals();
 		log_info(NULL, "reloading configuration %s", config_path);
@@ -568,9 +543,5 @@ main(int argc, char **argv)
 			sock4 = make_socket(conf.port, AF_INET);
 		if (sock6 == -1 && conf.ipv6)
 			sock6 = make_socket(conf.port, AF_INET6);
-
-		if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC,
-		    PF_UNSPEC, p) == -1)
-			fatal("socketpair: %s", strerror(errno));
 	}
 }
blob - e926f9c5b69fa2e933874dbb5c5a69e6f6ea0a5d
blob + b858ebc97cbf83139224a9c5d91c53ade5b33194
--- gmid.h
+++ gmid.h
@@ -55,6 +55,8 @@
 /* maximum hostname and label length, +1 for the NUL-terminator */
 #define DOMAIN_NAME_LEN	(253+1)
 #define LABEL_LEN	(63+1)
+
+#define PROC_MAX	16
 
 struct location {
 	const char	*match;
@@ -120,6 +122,8 @@ extern struct imsgbuf logpibuf, logcibuf;
 
 extern volatile sig_atomic_t hupped;
 
+extern int servpipes[PROC_MAX];
+
 struct iri {
 	char		*schema;
 	char		*host;
blob - 06fdf250bcc88b65db050fde6b1d7e2160329cbb
blob + 717bcdcd6f01bf609b59e1d4cc6bef832e73b4fa
--- parse.y
+++ parse.y
@@ -299,7 +299,7 @@ check_strip_no(int n)
 int
 check_prefork_num(int n)
 {
-	if (n <= 0)
+	if (n <= 0 || n >= PROC_MAX)
 		yyerror("invalid prefork number %d", n);
 	return n;
 }