From: Omar Polo <op@omarpolo.com>
Subject: gotwebd: handle short reads and timeouts
To: gameoftrees@openbsd.org
Date: Fri, 10 Mar 2023 18:23:26 +0100

Diff below fixes two issue:

 - the second hunk guards the only gotweb_free_transport call for a
   NULL transport.  If we hit the timeout *before*
   gotweb_process_request c->t is NULL and we crash

 - the other hunks deal with short reads.  fcgi_request reads in a
   buffer one or more fcgi records and parses all it has read.  If a
   short reads happens, we still have to read part of a record, and we
   currently fail to handle that case because:

   1. fcgi_request is not scheduled again; so the timeout hits and we
      crash in gotweb_free_transport; so register the event handler
      with EV_PERSIST.  (alternatively we could call event_add() in
      fcgi_request)

   2. drop the parsed record at every successful loop invocation,
      otherwise we may fail to make space for the next record.

Many, many thanks to stsp for helping debugging this :)

diff /home/op/w/got
commit - 8c9ae19cb1bac895dd2fd156f76df95c53ec375b
path + /home/op/w/got
blob - dd6dbe5f5b29eee48c7bcc972741e3a471d95c6b
file + gotwebd/fcgi.c
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -95,14 +95,14 @@ fcgi_request(int fd, short events, void *arg)
 			c->buf_pos += parsed;
 			c->buf_len -= parsed;
 		}
-	} while (parsed > 0 && c->buf_len > 0);
 
-	/* Make space for further reads */
-	if (parsed != 0)
-		if (c->buf_len > 0) {
+		/* drop the parsed record */
+		if (parsed != 0 && c->buf_len > 0) {
 			bcopy(c->buf + c->buf_pos, c->buf, c->buf_len);
 			c->buf_pos = 0;
 		}
+	} while (parsed > 0 && c->buf_len > 0);
+
 	return;
 fail:
 	fcgi_cleanup_request(c);
@@ -504,7 +504,8 @@ fcgi_cleanup_request(struct request *c)
 
 	close(c->fd);
 	template_free(c->tp);
-	gotweb_free_transport(c->t);
+	if (c->t != NULL)
+		gotweb_free_transport(c->t);
 	free(c);
 }
 
blob - cfba5dfb0e7368b0b37738d2eb71b53d437db169
file + gotwebd/sockets.c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -637,7 +637,7 @@ sockets_socket_accept(int fd, short event, void *arg)
 	c->request_started = 0;
 	c->sock->client_status = CLIENT_CONNECT;
 
-	event_set(&c->ev, s, EV_READ, fcgi_request, c);
+	event_set(&c->ev, s, EV_READ|EV_PERSIST, fcgi_request, c);
 	event_add(&c->ev, NULL);
 
 	evtimer_set(&c->tmo, fcgi_timeout, c);