commit 05ea464b5805d35f00f67170d9cbf8bdede98178 from: Omar Polo via: Thomas Adam date: Wed Aug 06 12:48:03 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. ok stsp@ commit - e650a842f69a783ed41a073df81eac7e8490b003 commit + 05ea464b5805d35f00f67170d9cbf8bdede98178 blob - 812c20aad494316656997ec684ccd4c433e60775 blob + 16c1a4c345629c57a011c73d856df48152069120 --- gotwebd/fcgi.c +++ gotwebd/fcgi.c @@ -101,6 +101,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_REQUEST) { + fcgi_cleanup_request(c); + return; + } + if (parsed != 0) { c->buf_pos += parsed; c->buf_len -= parsed; @@ -186,51 +197,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; - int sock_flags = SOCK_STREAM | SOCK_NONBLOCK; -#ifdef SOCK_CLOEXEC - sock_flags |= SOCK_CLOEXEC; -#endif - - if (socketpair(AF_UNIX, sock_flags, - PF_UNSPEC, pipe) == -1) { - log_warn("socketpair"); - return; - } - memcpy(&ic, c, sizeof(ic)); /* Don't leak pointers from our address space to another process. */ @@ -245,31 +217,15 @@ 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); } - - event_set(resp_event, pipe[1], EV_READ, fcgi_forward_response, c); - event_add(resp_event, NULL); + c->fd = -1; - c->resp_fd = pipe[1]; - c->resp_event = resp_event; c->client_status = CLIENT_REQUEST; } @@ -501,8 +457,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)) @@ -510,8 +464,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 - a0493b015524864c549b80f78d5dbc16010da137 blob + 51af7b098c7c71632f8484cec3d8a71d8ce38066 --- gotwebd/gotweb.c +++ gotwebd/gotweb.c @@ -141,20 +141,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) { @@ -223,14 +209,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; } @@ -238,7 +224,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; } @@ -246,7 +232,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; @@ -1394,8 +1380,6 @@ gotweb_shutdown(void) free(gotwebd_env->iev_server); } - sockets_purge(gotwebd_env); - while (!TAILQ_EMPTY(&gotwebd_env->servers)) { struct server *srv; @@ -1460,17 +1444,6 @@ gotweb_launch(struct gotwebd *env) fatal("unveil"); event_add(&env->iev_server->ev, NULL); -} - -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 @@ -1516,8 +1489,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 - 74f8973d272e4a432bec49a9dfae9404a4747e51 blob + c76f6606ef9cb25bd7c9a8911738fe8872edff91 --- gotwebd/gotwebd.c +++ gotwebd/gotwebd.c @@ -689,7 +689,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 - 96fe4275aaf6124f0d00e89c23c7e05cbf6adcf6 blob + 50a67b39462d9648bf9b82b20af518700d68d49a --- gotwebd/sockets.c +++ gotwebd/sockets.c @@ -78,95 +78,13 @@ static struct socket *sockets_conf_new_socket(struct g int, struct address *); 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); @@ -295,53 +213,7 @@ sockets_launch(struct gotwebd *env) fatal("pledge"); #endif event_add(&env->iev_gotweb->ev, NULL); - -} - -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 @@ -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); @@ -770,7 +635,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; @@ -778,7 +642,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); @@ -786,7 +649,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--;