From: "Omar Polo" Subject: less pipes in gotwebd To: gameoftrees@openbsd.org Date: Sun, 03 Aug 2025 17:04:01 +0200 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. commit 5c6ad43490b5b32f4c7c1c195a76073cbdf503fa (main) from: Omar Polo date: Sun Aug 3 15:03:20 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 5c6ad43490b5b32f4c7c1c195a76073cbdf503fa commit - 98958d75e928ddd95151c462562d1b91b172f7e9 commit + 5c6ad43490b5b32f4c7c1c195a76073cbdf503fa 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 + 79d541d9296b56ef5b9c7f62d543296a73e5dfbc --- 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,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); + 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--;