commit 4cd25209651f224be8c34d6006ef689963ce37d5
from: Omar Polo <op@omarpolo.com>
date: Thu Oct 07 10:47:02 2021 UTC

one FastCGI connection per client

FastCGI is designed to multiplex requests over a single connection, so
ideally the server can open only one connection per worker to the
FastCGI application and that's that.

Doing this kind of multiplexing makes the code harder to follow and
easier to break/leak etc on the gmid side however.  OpenBSD' httpd
seems to open one connection per client, so why can't we too?

One connection per request is still way better (lighter) than using
CGI, and we can avoid all the pitfalls of the multiplexing (keeping
track of "live ids", properly shut down etc...)

commit - 3096da4ef4418dc57f3e0b1fb1f89dceb2ca426a
commit + 4cd25209651f224be8c34d6006ef689963ce37d5
blob - 59943d75405ccea4ffd8ce6ece65376ae1fb2bbb
blob + d89026a80f709b253a27880d697b288bf61de822
--- ex.c
+++ ex.c
@@ -420,7 +420,7 @@ handle_imsg_fcgi_req(struct imsgbuf *ibuf, struct imsg
 	else
 		fd = fcgi_open_sock(f);
 
-	imsg_compose(ibuf, IMSG_FCGI_FD, id, 0, fd, NULL, 0);
+	imsg_compose(ibuf, IMSG_FCGI_FD, imsg->hdr.peerid, 0, fd, NULL, 0);
 	imsg_flush(ibuf);
 }
 
blob - 26da55cc33e4a0ecb4aaec6391320f5f8f6208b4
blob + 5e2ba5770fcd420374804073b8b738615c5bddfb
--- fcgi.c
+++ fcgi.c
@@ -127,16 +127,13 @@ struct fcgi_end_req_body {
 #define FCGI_MPXS_CONNS	"FCGI_MPXS_CONNS"
 
 static int
-prepare_header(struct fcgi_header *h, int type, int id, size_t size,
+prepare_header(struct fcgi_header *h, int type, size_t size,
     size_t padding)
 {
+	int id = 1;
+
 	memset(h, 0, sizeof(*h));
 
-	/*
-	 * id=0 is reserved for status messages.
-	 */
-	id++;
-
 	h->version = FCGI_VERSION_1;
         h->type = type;
 	h->req_id1 = (id >> 8);
@@ -149,16 +146,12 @@ prepare_header(struct fcgi_header *h, int type, int id
 }
 
 static int
-fcgi_begin_request(struct bufferevent *bev, int id)
+fcgi_begin_request(struct bufferevent *bev)
 {
 	struct fcgi_begin_req_record r;
 
-	if (id > UINT16_MAX)
-		return -1;
-
 	memset(&r, 0, sizeof(r));
-	prepare_header(&r.header, FCGI_BEGIN_REQUEST, id,
-	    sizeof(r.body), 0);
+	prepare_header(&r.header, FCGI_BEGIN_REQUEST, sizeof(r.body), 0);
 	assert(sizeof(r.body) == FCGI_HEADER_LEN);
 
 	r.body.role1 = 0;
@@ -171,7 +164,7 @@ fcgi_begin_request(struct bufferevent *bev, int id)
 }
 
 static int
-fcgi_send_param(struct bufferevent *bev, int id, const char *name,
+fcgi_send_param(struct bufferevent *bev, const char *name,
     const char *value)
 {
 	struct fcgi_header	h;
@@ -195,7 +188,7 @@ fcgi_send_param(struct bufferevent *bev, int id, const
 	s[6] = ((vallen >>  8) & 0xFF);
 	s[7] = ( vallen        & 0xFF);
 
-	prepare_header(&h, FCGI_PARAMS, id, size, padlen);
+	prepare_header(&h, FCGI_PARAMS, size, padlen);
 
 	if (bufferevent_write(bev, &h, sizeof(h))   == -1 ||
 	    bufferevent_write(bev, s, sizeof(s))    == -1 ||
@@ -208,39 +201,25 @@ fcgi_send_param(struct bufferevent *bev, int id, const
 }
 
 static int
-fcgi_end_param(struct bufferevent *bev, int id)
+fcgi_end_param(struct bufferevent *bev)
 {
 	struct fcgi_header h;
 
-	prepare_header(&h, FCGI_PARAMS, id, 0, 0);
+	prepare_header(&h, FCGI_PARAMS, 0, 0);
 	if (bufferevent_write(bev, &h, sizeof(h)) == -1)
 		return -1;
 
-	prepare_header(&h, FCGI_STDIN, id, 0, 0);
+	prepare_header(&h, FCGI_STDIN, 0, 0);
 	if (bufferevent_write(bev, &h, sizeof(h)) == -1)
 		return -1;
 
 	return 0;
 }
 
-void
-fcgi_abort_request(struct client *c)
-{
-	struct fcgi *f;
-	struct fcgi_header h;
-
-	f = &fcgi[c->fcgi];
-
-	prepare_header(&h, FCGI_ABORT_REQUEST, c->id, 0, 0);
-
-	if (bufferevent_write(f->bev, &h, sizeof(h)) == -1)
-		fcgi_close_backend(f);
-}
-
 static inline int
 recid(struct fcgi_header *h)
 {
-	return h->req_id0 + (h->req_id1 << 8) - 1;
+	return h->req_id0 + (h->req_id1 << 8);
 }
 
 static inline int
@@ -250,24 +229,12 @@ reclen(struct fcgi_header *h)
 }
 
 void
-fcgi_close_backend(struct fcgi *f)
-{
-	bufferevent_free(f->bev);
-	f->bev = NULL;
-	close(fcgi->fd);
-	fcgi->fd = -1;
-	fcgi->pending = 0;
-	fcgi->s = FCGI_OFF;
-}
-
-void
 fcgi_read(struct bufferevent *bev, void *d)
 {
-	struct fcgi		*fcgi = d;
+	struct client		*c = d;
 	struct evbuffer		*src = EVBUFFER_INPUT(bev);
 	struct fcgi_header	 hdr;
 	struct fcgi_end_req_body end;
-	struct client		*c;
 	size_t			 len;
 
 #if DEBUG_FCGI
@@ -292,11 +259,10 @@ fcgi_read(struct bufferevent *bev, void *d)
 
 		memcpy(&hdr, EVBUFFER_DATA(src), sizeof(hdr));
 
-		c = try_client_by_id(recid(&hdr));
-		if (c == NULL) {
+		if (recid(&hdr) != 1) {
 			log_err(NULL,
-			    "got invalid client id %d from fcgi backend %d",
-			    recid(&hdr), fcgi->id);
+			    "got invalid client id %d from fcgi backend",
+			    recid(&hdr));
 			goto err;
 		}
 
@@ -322,8 +288,6 @@ fcgi_read(struct bufferevent *bev, void *d)
 			bufferevent_read(bev, &end, sizeof(end));
 
 			/* TODO: do something with the status? */
-			fcgi->pending--;
-			c->fcgi = -1;
 			c->type = REQUEST_DONE;
 			client_write(c->bev, c);
 			break;
@@ -345,15 +309,10 @@ fcgi_read(struct bufferevent *bev, void *d)
 		}
 
 		evbuffer_drain(src, hdr.padding);
-
-		if (fcgi->pending == 0 && shutting_down) {
-			fcgi_error(bev, EVBUFFER_EOF, fcgi);
-			return;
-		}
 	}
 
 err:
-	fcgi_error(bev, EVBUFFER_ERROR, fcgi);
+	fcgi_error(bev, EVBUFFER_ERROR, c);
 }
 
 void
@@ -368,31 +327,21 @@ fcgi_write(struct bufferevent *bev, void *d)
 void
 fcgi_error(struct bufferevent *bev, short err, void *d)
 {
-	struct fcgi	*fcgi = d;
-	struct client	*c;
-	size_t		 i;
+	struct client	*c = d;
 
 	if (!(err & (EVBUFFER_ERROR|EVBUFFER_EOF)))
 		log_warn(NULL, "unknown event error (%x): %s",
 		    err, strerror(errno));
 
-	for (i = 0; i < MAX_USERS; ++i) {
-		c = &clients[i];
-
-		if (c->fcgi != fcgi->id)
-			continue;
-
-		if (c->code != 0)
-			client_close(c);
-		else
-			start_reply(c, CGI_ERROR, "CGI error");
-	}
-
-	fcgi_close_backend(fcgi);
+	c->type = REQUEST_DONE;
+	if (c->code != 0)
+		client_close(c);
+	else
+		start_reply(c, CGI_ERROR, "CGI error");
 }
 
 void
-fcgi_req(struct fcgi *f, struct client *c)
+fcgi_req(struct client *c)
 {
 	char		 addr[NI_MAXHOST], buf[22];
 	int		 e;
@@ -400,8 +349,6 @@ fcgi_req(struct fcgi *f, struct client *c)
 	struct tm	 tminfo;
 	struct envlist	*p;
 
-	f->pending++;
-
 	e = getnameinfo((struct sockaddr*)&c->addr, sizeof(c->addr),
 	    addr, sizeof(addr),
 	    NULL, 0,
@@ -410,50 +357,50 @@ fcgi_req(struct fcgi *f, struct client *c)
 		fatal("getnameinfo failed: %s (%s)",
 		    gai_strerror(e), strerror(errno));
 
-	fcgi_begin_request(f->bev, c->id);
-	fcgi_send_param(f->bev, c->id, "GATEWAY_INTERFACE", "CGI/1.1");
-	fcgi_send_param(f->bev, c->id, "GEMINI_URL_PATH", c->iri.path);
-	fcgi_send_param(f->bev, c->id, "QUERY_STRING", c->iri.query);
-	fcgi_send_param(f->bev, c->id, "REMOTE_ADDR", addr);
-	fcgi_send_param(f->bev, c->id, "REMOTE_HOST", addr);
-	fcgi_send_param(f->bev, c->id, "REQUEST_METHOD", "");
-	fcgi_send_param(f->bev, c->id, "SERVER_NAME", c->iri.host);
-	fcgi_send_param(f->bev, c->id, "SERVER_PROTOCOL", "GEMINI");
-	fcgi_send_param(f->bev, c->id, "SERVER_SOFTWARE", GMID_VERSION);
+	fcgi_begin_request(c->cgibev);
+	fcgi_send_param(c->cgibev, "GATEWAY_INTERFACE", "CGI/1.1");
+	fcgi_send_param(c->cgibev, "GEMINI_URL_PATH", c->iri.path);
+	fcgi_send_param(c->cgibev, "QUERY_STRING", c->iri.query);
+	fcgi_send_param(c->cgibev, "REMOTE_ADDR", addr);
+	fcgi_send_param(c->cgibev, "REMOTE_HOST", addr);
+	fcgi_send_param(c->cgibev, "REQUEST_METHOD", "");
+	fcgi_send_param(c->cgibev, "SERVER_NAME", c->iri.host);
+	fcgi_send_param(c->cgibev, "SERVER_PROTOCOL", "GEMINI");
+	fcgi_send_param(c->cgibev, "SERVER_SOFTWARE", GMID_VERSION);
 
 	if (tls_peer_cert_provided(c->ctx)) {
-		fcgi_send_param(f->bev, c->id, "AUTH_TYPE", "CERTIFICATE");
-		fcgi_send_param(f->bev, c->id, "REMOTE_USER",
+		fcgi_send_param(c->cgibev, "AUTH_TYPE", "CERTIFICATE");
+		fcgi_send_param(c->cgibev, "REMOTE_USER",
 		    tls_peer_cert_subject(c->ctx));
-		fcgi_send_param(f->bev, c->id, "TLS_CLIENT_ISSUER",
+		fcgi_send_param(c->cgibev, "TLS_CLIENT_ISSUER",
 		    tls_peer_cert_issuer(c->ctx));
-		fcgi_send_param(f->bev, c->id, "TLS_CLIENT_HASH",
+		fcgi_send_param(c->cgibev, "TLS_CLIENT_HASH",
 		    tls_peer_cert_hash(c->ctx));
-		fcgi_send_param(f->bev, c->id, "TLS_VERSION",
+		fcgi_send_param(c->cgibev, "TLS_VERSION",
 		    tls_conn_version(c->ctx));
-		fcgi_send_param(f->bev, c->id, "TLS_CIPHER",
+		fcgi_send_param(c->cgibev, "TLS_CIPHER",
 		    tls_conn_cipher(c->ctx));
 
 		snprintf(buf, sizeof(buf), "%d",
 		    tls_conn_cipher_strength(c->ctx));
-		fcgi_send_param(f->bev, c->id, "TLS_CIPHER_STRENGTH", buf);
+		fcgi_send_param(c->cgibev, "TLS_CIPHER_STRENGTH", buf);
 
 		tim = tls_peer_cert_notbefore(c->ctx);
 		strftime(buf, sizeof(buf), "%FT%TZ",
 		    gmtime_r(&tim, &tminfo));
-		fcgi_send_param(f->bev, c->id, "TLS_CLIENT_NOT_BEFORE", buf);
+		fcgi_send_param(c->cgibev, "TLS_CLIENT_NOT_BEFORE", buf);
 
 		tim = tls_peer_cert_notafter(c->ctx);
 		strftime(buf, sizeof(buf), "%FT%TZ",
 		    gmtime_r(&tim, &tminfo));
-		fcgi_send_param(f->bev, c->id, "TLS_CLIENT_NOT_AFTER", buf);
+		fcgi_send_param(c->cgibev, "TLS_CLIENT_NOT_AFTER", buf);
 
 		TAILQ_FOREACH(p, &c->host->params, envs) {
-			fcgi_send_param(f->bev, c->id, p->name, p->value);
+			fcgi_send_param(c->cgibev, p->name, p->value);
 		}
 	} else
-		fcgi_send_param(f->bev, c->id, "AUTH_TYPE", "");
+		fcgi_send_param(c->cgibev, "AUTH_TYPE", "");
 
-	if (fcgi_end_param(f->bev, c->id) == -1)
-		fcgi_error(f->bev, EVBUFFER_ERROR, f);
+	if (fcgi_end_param(c->cgibev) == -1)
+		fcgi_error(c->cgibev, EVBUFFER_ERROR, c);
 }
blob - d9d07ff1e9c7fe814928fde45c9cbccefe87a241
blob + c8d1b57df6cabd0e171e8e51531ee6b87868df98
--- gmid.c
+++ gmid.c
@@ -332,9 +332,6 @@ free_config(void)
 		fcgi[i].path = NULL;
 		fcgi[i].port = NULL;
 		fcgi[i].prog = NULL;
-
-		fcgi[i].pending = 0;
-		fcgi[i].s = FCGI_OFF;
 	}
 
 	tls_free(ctx);
blob - 53fbca92887ec87d403105d8c5d1c9c8886c02eb
blob + 43c684fec0d931c7f6845d1a37fb8ee624a2db59
--- gmid.h
+++ gmid.h
@@ -79,17 +79,6 @@ struct fcgi {
 	char		*path;
 	char		*port;
 	char		*prog;
-	int		 fd;
-
-	struct bufferevent *bev;
-
-	/* number of pending clients */
-	int		 pending;
-
-#define FCGI_OFF	0
-#define FCGI_INFLIGHT	1
-#define FCGI_READY	2
-	int		 s;
 };
 extern struct fcgi fcgi[FCGI_MAX];
 
@@ -230,7 +219,6 @@ struct client {
 	int		 fd, pfd;
 	struct dirent	**dir;
 	int		 dirlen, diroff;
-	int		 fcgi;
 
 	/* big enough to store STATUS + SPACE + META + CRLF */
 	char		 sbuf[1029];
@@ -366,12 +354,10 @@ int		 recv_fd(int);
 int		 executor_main(struct imsgbuf*);
 
 /* fcgi.c */
-void		 fcgi_abort_request(struct client *);
-void		 fcgi_close_backend(struct fcgi *);
 void		 fcgi_read(struct bufferevent *, void *);
 void		 fcgi_write(struct bufferevent *, void *);
 void		 fcgi_error(struct bufferevent *, short, void *);
-void		 fcgi_req(struct fcgi *, struct client *);
+void		 fcgi_req(struct client *);
 
 /* sandbox.c */
 void		 sandbox_server_process(void);
blob - 879a2b49e341c1fb3f2530612adc27b90fae8168
blob + 66202c460bbf1104741d192306a0c680e81a48c2
--- server.c
+++ server.c
@@ -612,24 +612,14 @@ apply_fastcgi(struct client *c)
 	if ((id = vhost_fastcgi(c->host, c->iri.path)) == -1)
 		return 0;
 
-	switch ((f = &fcgi[id])->s) {
-	case FCGI_OFF:
-		f->s = FCGI_INFLIGHT;
-		log_info(c, "opening fastcgi connection for (%s,%s,%s)",
-		    f->path, f->port, f->prog);
-		imsg_compose(&exibuf, IMSG_FCGI_REQ, 0, 0, -1,
-		    &id, sizeof(id));
-		imsg_flush(&exibuf);
-                /* fallthrough */
-	case FCGI_INFLIGHT:
-                c->fcgi = id;
-		break;
-	case FCGI_READY:
-                c->fcgi = id;
-		fcgi_req(f, c);
-		break;
-	}
+	f = &fcgi[id];
+
+	log_debug(c, "opening fastcgi connection for (%s,%s,%s)",
+	    f->path, f->port, f->prog);
 
+	imsg_compose(&exibuf, IMSG_FCGI_REQ, c->id, 0, -1,
+	    &id, sizeof(id));
+	imsg_flush(&exibuf);
 	return 1;
 }
 
@@ -1057,9 +1047,6 @@ static void
 client_error(struct bufferevent *bev, short error, void *d)
 {
 	struct client	*c = d;
-
-	if (c->type == REQUEST_FCGI)
-		fcgi_abort_request(c);
 
 	c->type = REQUEST_DONE;
 
@@ -1178,9 +1165,6 @@ client_close(struct client *c)
 	 * this point.
 	 */
 
-	if (c->type == REQUEST_FCGI)
-		fcgi_abort_request(c);
-
 	if (c->cgibev != NULL) {
 		bufferevent_disable(c->cgibev, EVBUFFER_READ|EVBUFFER_WRITE);
 		bufferevent_free(c->cgibev);
@@ -1317,7 +1301,6 @@ do_accept(int sock, short et, void *d)
 			c->pfd = -1;
 			c->dir = NULL;
 			c->addr = addr;
-			c->fcgi = -1;
 
 			event_once(c->fd, EV_READ|EV_WRITE, handle_handshake,
 			    c, NULL);
@@ -1370,51 +1353,37 @@ static void
 handle_imsg_fcgi_fd(struct imsgbuf *ibuf, struct imsg *imsg, size_t len)
 {
 	struct client	*c;
-	struct fcgi	*f;
-	int		 i, id;
+	int		 id;
 
 	id = imsg->hdr.peerid;
-	f = &fcgi[id];
 
-	if ((f->fd = imsg->fd) == -1)
-		f->s = FCGI_OFF;
-	else {
-		mark_nonblock(f->fd);
+	if ((c = try_client_by_id(id)) == NULL) {
+		if (imsg->fd != -1)
+			close(imsg->fd);
+		return;
+	}
 
-		f->s = FCGI_READY;
-
-		f->bev = bufferevent_new(f->fd, fcgi_read, fcgi_write,
-		    fcgi_error, f);
-		if (f->bev == NULL) {
-			close(f->fd);
-			log_err(NULL, "%s: failed to allocate client buffer",
-			    __func__);
-			f->s = FCGI_OFF;
-		}
-
-		bufferevent_enable(f->bev, EV_READ|EV_WRITE);
+	if ((c->pfd = imsg->fd) == -1) {
+		start_reply(c, CGI_ERROR, "CGI error");
+		return;
 	}
 
-	for (i = 0; i < MAX_USERS; ++i) {
-		c = &clients[i];
-		if (c->fd == -1)
-			continue;
-		if (c->fcgi != id)
-			continue;
+	mark_nonblock(c->pfd);
 
-		if (f->s == FCGI_OFF) {
-			c->fcgi = -1;
-			start_reply(c, TEMP_FAILURE, "internal server error");
-		} else
-			fcgi_req(f, c);
+	c->cgibev = bufferevent_new(c->pfd, fcgi_read, fcgi_write,
+	    fcgi_error, c);
+	if (c->cgibev == NULL) {
+		start_reply(c, TEMP_FAILURE, "internal server error");
+		return;
 	}
+
+	bufferevent_enable(c->cgibev, EV_READ|EV_WRITE);
+	fcgi_req(c);
 }
 
 static void
 handle_imsg_quit(struct imsgbuf *ibuf, struct imsg *imsg, size_t len)
 {
-	size_t i;
-
 	(void)imsg;
 	(void)len;
 
@@ -1432,16 +1401,6 @@ handle_imsg_quit(struct imsgbuf *ibuf, struct imsg *im
 		signal_del(&siginfo);
 	event_del(&imsgev);
 	signal_del(&sigusr2);
-
-	for (i = 0; i < FCGI_MAX; ++i) {
-		if (fcgi[i].path == NULL && fcgi[i].prog == NULL)
-			break;
-
-		if (fcgi[i].bev == NULL || fcgi[i].pending != 0)
-			continue;
-
-		fcgi_close_backend(&fcgi[i]);
-	}
 }
 
 static void