"Omar Polo" <op@omarpolo.com> wrote:
> Hello,
> 
> I'd like to simplify the way we're processing the output in gotwebd now.
> Explanations on the commit message, but the main reason I'm doing this
> is to avoid a race in the current code where we can send a
> GOTWEBD_IMSG_REQ_DONE while there's still data in the pipe between the
> process and... well, we send a truncated, corrupted page.

A small fix on top of this to unbreak a case i hadn't tested:

>  -static void
>  gotweb_dispatch_server(int fd, short event, void *arg)
>  {
>  	struct imsgev		*iev = arg;
> @@ -1518,8 +1491,10 @@ gotweb_dispatch_server(int fd, short event, void *arg)
>  						    request_id);
>  					}
>  				}
> -				free_request(c);
> -				send_request_done(iev, request_id);
> +
> +				if (c->client_status == CLIENT_REQUEST)
> +					fcgi_create_end_record(c);

this is wrong.  we should call fcgi_create_end_record in all cases.
worst situation, the client is already disconnected (we got an error
before) and this becomes a no-op.

> +				fcgi_cleanup_request(c);
>  			}
>  			break;
>  		default:

updated diff below.  i'm now running with this in my instance.


commit da0676db1d192955f75cda32679eba2e926022a6 (main)
from: Omar Polo <op@omarpolo.com>
date: Mon Aug  4 08:12:15 2025 UTC
 
 gotwebd: avoid the pipe; forward the socket directly
 
 Instead of keeping a pipe between the sockets and the gotwebd
 process, just forward the socket directly.  This needs a little bit
 of care because once we enter process_request() the socket (possibly)
 no longer there, so on the socket side use the client_status as a
 flag to signal the caller that we're done.  I hope to improve this
 in follow-ups.
 
 The motivation behind this is to avoid a (possible, but quite easily
 triggerable) race in the current code where the other side might
 process GOTWEBD_IMSG_REQ_DONE before draining the pipe, truncating
 the output page.
 
diff 98958d75e928ddd95151c462562d1b91b172f7e9 da0676db1d192955f75cda32679eba2e926022a6
commit - 98958d75e928ddd95151c462562d1b91b172f7e9
commit + da0676db1d192955f75cda32679eba2e926022a6
blob - 98b144aa064f722540e70cb344a42e3e59abf96e
blob + 53d51f463017b20b82843d793916c4830ff32b4b
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -99,6 +99,17 @@ fcgi_request(int fd, short events, void *arg)
 	 */
 	do {
 		parsed = fcgi_parse_record(c->buf + c->buf_pos, c->buf_len, c);
+
+		/*
+		 * When we start to actually process the entry, we
+		 * send the request to the gotweb process, so we're
+		 * done.
+		 */
+		if (c->client_status == CLIENT_DISCONNECT) {
+			fcgi_cleanup_request(c);
+			return;
+		}
+
 		if (parsed != 0) {
 			c->buf_pos += parsed;
 			c->buf_len -= parsed;
@@ -184,46 +195,12 @@ fcgi_parse_begin_request(uint8_t *buf, uint16_t n,
 }
 
 static void
-fcgi_forward_response(int fd, short event, void *arg)
-{
-	const struct got_error *err = NULL;
-	struct request *c = arg;
-	uint8_t outbuf[GOTWEBD_CACHESIZE];
-	ssize_t r;
-
-	if ((event & EV_READ) == 0)
-		return;
-
-	r = read(fd, outbuf, sizeof(outbuf));
-	if (r == 0)
-		return;
-
-	if (r == -1) {
-		log_warn("read response");
-		return;
-	} else {
-		err = got_poll_write_full_timeout(c->fd, outbuf, r, 1);
-		if (err)
-			log_warnx("forward response: %s", err->msg);
-	}
-
-	event_add(c->resp_event, NULL);
-}
-
-static void
 process_request(struct request *c)
 {
 	struct gotwebd *env = gotwebd_env;
-	int ret, i, pipe[2];
+	int ret, i;
 	struct request ic;
-	struct event *resp_event = NULL;
 
-	if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC | SOCK_NONBLOCK,
-	    PF_UNSPEC, pipe) == -1) {
-		log_warn("socketpair");
-		return;
-	}
-
 	memcpy(&ic, c, sizeof(ic));
 
 	/* Don't leak pointers from our address space to another process. */
@@ -238,32 +215,16 @@ process_request(struct request *c)
 	for (i = 0; i < nitems(c->priv_fd); i++)
 		ic.priv_fd[i] = -1;
 	ic.fd = -1;
-	ic.resp_fd = -1;
 
-	resp_event = calloc(1, sizeof(*resp_event));
-	if (resp_event == NULL) {
-		log_warn("calloc");
-		close(pipe[0]);
-		close(pipe[1]);
-		return;
-	}
-
 	ret = imsg_compose_event(env->iev_gotweb, GOTWEBD_IMSG_REQ_PROCESS,
-	    GOTWEBD_PROC_SERVER, getpid(), pipe[0], &ic, sizeof(ic));
+	    GOTWEBD_PROC_SERVER, -1, c->fd, &ic, sizeof(ic));
 	if (ret == -1) {
 		log_warn("imsg_compose_event");
-		close(pipe[0]);
-		close(pipe[1]);
-		free(resp_event);
-		return;
+		close(c->fd);
 	}
+	c->fd = -1;
 
-	event_set(resp_event, pipe[1], EV_READ, fcgi_forward_response, c);
-	event_add(resp_event, NULL);
-
-	c->resp_fd = pipe[1];
-	c->resp_event = resp_event;
-	c->client_status = CLIENT_REQUEST;
+	c->client_status = CLIENT_DISCONNECT;
 }
 
 void
@@ -494,8 +455,6 @@ fcgi_cleanup_request(struct request *c)
 {
 	cgi_inflight--;
 
-	sockets_del_request(c);
-
 	if (evtimer_initialized(&c->tmo))
 		evtimer_del(&c->tmo);
 	if (event_initialized(&c->ev))
@@ -503,8 +462,6 @@ fcgi_cleanup_request(struct request *c)
 
 	if (c->fd != -1)
 		close(c->fd);
-	if (c->resp_fd != -1)
-		close(c->resp_fd);
 	if (c->tp != NULL)
 		template_free(c->tp);
 	if (c->t != NULL)
blob - 86caae0a2edf5c2a511c9da7f3b02a08a8586dff
blob + 1d2b63de026c502f7440177d20c1b3e81c019ae6
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -143,20 +143,6 @@ gotweb_reply_file(struct request *c, const char *ctype
 	return gotweb_reply(c, 200, ctype, NULL);
 }
 
-static void
-free_request(struct request *c)
-{
-	if (c->fd != -1)
-		close(c->fd);
-	if (c->tp != NULL)
-		template_free(c->tp);
-	if (c->t != NULL)
-		gotweb_free_transport(c->t);
-	free(c->buf);
-	free(c->outbuf);
-	free(c);
-}
-
 static struct socket *
 gotweb_get_socket(int sock_id)
 {
@@ -225,14 +211,14 @@ recv_request(struct imsg *imsg)
 	c->tp = template(c, fcgi_write, c->outbuf, GOTWEBD_CACHESIZE);
 	if (c->tp == NULL) {
 		log_warn("gotweb init template");
-		free_request(c);
+		fcgi_cleanup_request(c);
 		return NULL;
 	}
 
 	c->sock = gotweb_get_socket(c->sock_id);
 	if (c->sock == NULL) {
 		log_warn("socket id '%d' not found", c->sock_id);
-		free_request(c);
+		fcgi_cleanup_request(c);
 		return NULL;
 	}
 
@@ -240,7 +226,7 @@ recv_request(struct imsg *imsg)
 	error = gotweb_init_transport(&c->t);
 	if (error) {
 		log_warnx("gotweb init transport: %s", error->msg);
-		free_request(c);
+		fcgi_cleanup_request(c);
 		return NULL;
 	}
 
@@ -248,7 +234,7 @@ recv_request(struct imsg *imsg)
 	srv = gotweb_get_server(c->server_name);
 	if (srv == NULL) {
 		log_warnx("server '%s' not found", c->server_name);
-		free_request(c);
+		fcgi_cleanup_request(c);
 		return NULL;
 	}
 	c->srv = srv;
@@ -1396,8 +1382,6 @@ gotweb_shutdown(void)
 		free(gotwebd_env->iev_server);
 	}
 
-	sockets_purge(gotwebd_env);
-
 	while (!TAILQ_EMPTY(&gotwebd_env->servers)) {
 		struct server *srv;
 
@@ -1465,17 +1449,6 @@ gotweb_launch(struct gotwebd *env)
 }
 
 static void
-send_request_done(struct imsgev *iev, int request_id)
-{
-	struct gotwebd		*env = gotwebd_env;
-
-	if (imsg_compose_event(env->iev_server, GOTWEBD_IMSG_REQ_DONE,
-	    GOTWEBD_PROC_GOTWEB, getpid(), -1,
-	    &request_id, sizeof(request_id)) == -1)
-		log_warn("imsg_compose_event");
-}
-
-static void
 gotweb_dispatch_server(int fd, short event, void *arg)
 {
 	struct imsgev		*iev = arg;
@@ -1518,8 +1491,9 @@ gotweb_dispatch_server(int fd, short event, void *arg)
 						    request_id);
 					}
 				}
-				free_request(c);
-				send_request_done(iev, request_id);
+
+				fcgi_create_end_record(c);
+				fcgi_cleanup_request(c);
 			}
 			break;
 		default:
blob - b3a5e3687066935bb40b1fc0ced2997768dfa1ed
blob + 01affdd22c9241eabef71fd1fc28c0ec9c226800
--- gotwebd/gotwebd.c
+++ gotwebd/gotwebd.c
@@ -690,7 +690,6 @@ gotwebd_shutdown(void)
 		TAILQ_REMOVE(&gotwebd_env->servers, srv, entry);
 		free(srv);
 	}
-	sockets_purge(gotwebd_env);
 	free(gotwebd_env);
 
 	log_warnx("gotwebd terminating");
blob - 1973199772dbd6e269f7b884a5607f7e7d7f0414
blob + 888f71c5b60cbc3697f03efa568436627738f7dc
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -135,7 +135,6 @@ enum imsg_type {
 	GOTWEBD_IMSG_CTL_PIPE,
 	GOTWEBD_IMSG_CTL_START,
 	GOTWEBD_IMSG_REQ_PROCESS,
-	GOTWEBD_IMSG_REQ_DONE,
 };
 
 struct imsgev {
@@ -253,7 +252,6 @@ struct request {
 	uint16_t			 id;
 	int				 fd;
 	int				 priv_fd[PRIV_FDS__MAX];
-	int				 resp_fd;
 	struct event			*resp_event;
 	int				 sock_id;
 	uint32_t			 request_id;
@@ -463,9 +461,7 @@ void sockets(struct gotwebd *, int);
 void sockets_parse_sockets(struct gotwebd *);
 void sockets_socket_accept(int, short, void *);
 int sockets_privinit(struct gotwebd *, struct socket *, uid_t, gid_t);
-void sockets_purge(struct gotwebd *);
 void sockets_rlimit(int);
-void sockets_del_request(struct request *);
 
 /* gotweb.c */
 void gotweb_index_navs(struct request *, struct gotweb_url *, int *,
blob - 75d96baa9f14d17f1efe37982699d03c6a4d1ce3
blob + 0ddf1e382eb72e1f455b15c2ed1acf5408748f83
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -79,94 +79,12 @@ static struct socket *sockets_conf_new_socket(struct g
 
 int cgi_inflight = 0;
 
-/* Request hash table needs some spare room to avoid collisions. */
-struct requestlist requests[GOTWEBD_MAXCLIENTS * 4];
-static SIPHASH_KEY requests_hash_key;
-
-static void
-requests_init(void)
-{
-	int i;
-
-	arc4random_buf(&requests_hash_key, sizeof(requests_hash_key));
-
-	for (i = 0; i < nitems(requests); i++)
-		TAILQ_INIT(&requests[i]);
-}
-
-static uint64_t
-request_hash(uint32_t request_id)
-{
-	return SipHash24(&requests_hash_key, &request_id, sizeof(request_id));
-}
-
-static void
-add_request(struct request *c)
-{
-	uint64_t slot = request_hash(c->request_id) % nitems(requests);
-	TAILQ_INSERT_HEAD(&requests[slot], c, entry);
-	client_cnt++;
-}
-
 void
-sockets_del_request(struct request *c)
-{
-	uint64_t slot = request_hash(c->request_id) % nitems(requests);
-	TAILQ_REMOVE(&requests[slot], c, entry);
-	client_cnt--;
-}
-
-static struct request *
-find_request(uint32_t request_id)
-{
-	uint64_t slot;
-	struct request *c;
-
-	slot = request_hash(request_id) % nitems(requests);
-	TAILQ_FOREACH(c, &requests[slot], entry) {
-		if (c->request_id == request_id)
-			return c;
-	}
-
-	return NULL;
-}
-
-static void
-requests_purge(void)
-{
-	uint64_t slot;
-	struct request *c;
-
-	for (slot = 0; slot < nitems(requests); slot++) {
-		while (!TAILQ_EMPTY(&requests[slot])) {
-			c = TAILQ_FIRST(&requests[slot]);
-			fcgi_cleanup_request(c);
-		}
-	}
-}
-
-static uint32_t
-get_request_id(void)
-{
-	int duplicate = 0;
-	uint32_t id;
-
-	do {
-		id = arc4random();
-		duplicate = (find_request(id) != NULL);
-	} while (duplicate || id == 0);
-
-	return id;
-}
-
-void
 sockets(struct gotwebd *env, int fd)
 {
 	struct event	 sighup, sigint, sigusr1, sigchld, sigterm;
 	struct event_base *evb;
 
-	requests_init();
-
 	evb = event_init();
 
 	sockets_rlimit(-1);
@@ -298,53 +216,7 @@ sockets_launch(struct gotwebd *env)
 
 }
 
-void
-sockets_purge(struct gotwebd *env)
-{
-	struct socket *sock, *tsock;
-
-	/* shutdown and remove sockets */
-	TAILQ_FOREACH_SAFE(sock, &env->sockets, entry, tsock) {
-		if (event_initialized(&sock->ev))
-			event_del(&sock->ev);
-		if (evtimer_initialized(&sock->evt))
-			evtimer_del(&sock->evt);
-		if (evtimer_initialized(&sock->pause))
-			evtimer_del(&sock->pause);
-		if (sock->fd != -1)
-			close(sock->fd);
-		TAILQ_REMOVE(&env->sockets, sock, entry);
-		free(sock);
-	}
-}
-
 static void
-request_done(struct imsg *imsg)
-{
-	struct request *c;
-	uint32_t request_id;
-	size_t datalen = imsg->hdr.len - IMSG_HEADER_SIZE;
-
-	if (datalen != sizeof(request_id)) {
-		log_warn("IMSG_REQ_DONE with bad data length");
-		return;
-	}
-
-	memcpy(&request_id, imsg->data, sizeof(request_id));
-
-	c = find_request(request_id);
-	if (c == NULL) {
-		log_warnx("no request to clean up found for ID %u",
-		    request_id);
-		return;
-	}
-
-	if (c->client_status == CLIENT_REQUEST)
-		fcgi_create_end_record(c);
-	fcgi_cleanup_request(c);
-}
-
-static void
 server_dispatch_gotweb(int fd, short event, void *arg)
 {
 	struct imsgev		*iev = arg;
@@ -373,9 +245,6 @@ server_dispatch_gotweb(int fd, short event, void *arg)
 			break;
 
 		switch (imsg.hdr.type) {
-		case GOTWEBD_IMSG_REQ_DONE:
-			request_done(&imsg);
-			break;
 		default:
 			fatalx("%s: unknown imsg type %d", __func__,
 			    imsg.hdr.type);
@@ -514,8 +383,6 @@ sockets_sighdlr(int sig, short event, void *arg)
 static void
 sockets_shutdown(void)
 {
-	sockets_purge(gotwebd_env);
-
 	/* clean servers */
 	while (!TAILQ_EMPTY(&gotwebd_env->servers)) {
 		struct server *srv;
@@ -533,8 +400,6 @@ sockets_shutdown(void)
 		free(h);
 	}
 
-	requests_purge();
-
 	imsgbuf_clear(&gotwebd_env->iev_parent->ibuf);
 	free(gotwebd_env->iev_parent);
 	imsgbuf_clear(&gotwebd_env->iev_gotweb->ibuf);
@@ -760,7 +625,6 @@ sockets_socket_accept(int fd, short event, void *arg)
 
 	c->buf = buf;
 	c->fd = s;
-	c->resp_fd = -1;
 	c->sock = sock;
 	memcpy(c->priv_fd, gotwebd_env->priv_fd, sizeof(c->priv_fd));
 	c->sock_id = sock->conf.id;
@@ -768,7 +632,6 @@ sockets_socket_accept(int fd, short event, void *arg)
 	c->buf_len = 0;
 	c->request_started = 0;
 	c->client_status = CLIENT_CONNECT;
-	c->request_id = get_request_id();
 
 	event_set(&c->ev, s, EV_READ, fcgi_request, c);
 	event_add(&c->ev, NULL);
@@ -776,7 +639,6 @@ sockets_socket_accept(int fd, short event, void *arg)
 	evtimer_set(&c->tmo, fcgi_timeout, c);
 	evtimer_add(&c->tmo, &timeout);
 
-	add_request(c);
 	return;
 err:
 	cgi_inflight--;