From: Omar Polo Subject: Re: gotwebd: avoid calloc/free per fcgi record To: Omar Polo Cc: Stefan Sperling , gameoftrees@openbsd.org Date: Fri, 29 Jul 2022 16:53:32 +0200 Omar Polo wrote: > Stefan Sperling wrote: > > On Fri, Jul 29, 2022 at 03:28:09PM +0200, Omar Polo wrote: > > > to send something to the browser we have to go through > > > fcgi_send_response. > > > > > > diff below uses a static buffer in fcgi_send_response (now > > > send_response) to avoid dynamically allocating ~16K for each bit of the > > > reply. > > > > Are you sure this approach is safe? > > Doesn't this introduce a risk where cross-request data leaks could > > become a potential issue? > > I don't think; each request gets its own process so even in case of a > bug we could leak only data previously sent to the same client. i misread the code badly. the processes are preforked and handle one request per time, but then they are re-used for the next one. thanks tracey@ for correcting me on irc. > anyway, i wrote it with a buffer because i thought that the equivalent > with writev would be way more complex. turns out, the version with an > iovec is not really worse (and saves a couple of memcpy too.) fwiw, the writev approach saves us from allocating and from the risk of leaking data. I wanted to get something like this in because we have a lot of calls to fcgi_gen_response with (very small) strings, and allocating ~16K for each of them is quite heavy. > is this better? > > diff refs/heads/main refs/heads/gwdgc > commit - 7375fc126e0f55289656336c6c8160c46efaba20 > commit + fb6991a14b66fa913e4ad8f197e624820d58647e > blob - 845cfc6a1588e7b4e4d605f6892a16fb3ef5b601 > blob + ca55913d0e41cce27db7881a23765de4f0045ab3 > --- gotwebd/fcgi.c > +++ gotwebd/fcgi.c > @@ -20,6 +20,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -39,7 +40,7 @@ size_t fcgi_parse_record(uint8_t *, size_t, struct re > void fcgi_parse_begin_request(uint8_t *, uint16_t, struct request *, > uint16_t); > void fcgi_parse_params(uint8_t *, uint16_t, struct request *, uint16_t); > -void fcgi_send_response(struct request *, struct fcgi_response *); > +void fcgi_send_response(struct request *, int, const void *, size_t); > > void dump_fcgi_record_header(const char *, struct fcgi_record_header *); > void dump_fcgi_begin_request_body(const char *, > @@ -303,39 +304,13 @@ fcgi_timeout(int fd, short events, void *arg) > int > fcgi_gen_binary_response(struct request *c, const uint8_t *data, int len) > { > - struct fcgi_response *resp; > - struct fcgi_record_header *header; > - ssize_t n = 0; > - int i; > - > if (c->sock->client_status == CLIENT_DISCONNECT) > return -1; > > if (data == NULL || len == 0) > return 0; > > - if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) { > - log_warn("%s: cannot calloc fcgi_response", __func__); > - return -1; > - } > - > - header = (struct fcgi_record_header*) resp->data; > - header->version = 1; > - header->type = FCGI_STDOUT; > - header->id = htons(c->id); > - header->padding_len = 0; > - header->reserved = 0; > - > - for (i = 0; i < len; i++) { > - resp->data[i+8] = data[i]; > - n++; > - } > - > - header->content_len = htons(n); > - resp->data_pos = 0; > - resp->data_len = n + sizeof(struct fcgi_record_header); > - fcgi_send_response(c, resp); > - > + fcgi_send_response(c, FCGI_STDOUT, data, len); > return 0; > } > > @@ -347,40 +322,52 @@ fcgi_gen_response(struct request *c, const char *data) > return fcgi_gen_binary_response(c, data, strlen(data)); > } > > -void > -fcgi_send_response(struct request *c, struct fcgi_response *resp) > +static void > +send_response(struct request *c, int type, const uint8_t *data, > + size_t len) > { > - struct fcgi_record_header *header; > + static const uint8_t padding[FCGI_PADDING_SIZE]; > + struct fcgi_record_header header; > + struct iovec iov[3]; > struct timespec ts; > - size_t padded_len, off; > ssize_t nw; > - int err = 0, th = 2000; > + size_t padded_len, tot; > + int i, err = 0, th = 2000; > > ts.tv_sec = 0; > ts.tv_nsec = 50; > > - header = (struct fcgi_record_header*)resp->data; > + memset(&header, 0, sizeof(header)); > + header.version = 1; > + header.type = type; > + header.id = htons(c->id); > + header.content_len = htons(len); > > /* The FastCGI spec suggests to align the output buffer */ > - padded_len = FCGI_ALIGN(resp->data_len); > - if (padded_len > resp->data_len) { > - /* There should always be FCGI_PADDING_SIZE bytes left */ > - if (padded_len > FCGI_RECORD_SIZE) > - log_warn("response too long"); > - header->padding_len = padded_len - resp->data_len; > - resp->data_len = padded_len; > - } > + tot = sizeof(header) + len; > + padded_len = FCGI_ALIGN(tot); > + if (padded_len > tot) > + header.padding_len = padded_len - tot; > > - dump_fcgi_record("resp ", header); > + iov[0].iov_base = &header; > + iov[0].iov_len = sizeof(header); > > + iov[1].iov_base = (void *)data; > + iov[1].iov_len = len; > + > + iov[2].iov_base = (void *)padding; > + iov[2].iov_len = header.padding_len; > + > + dump_fcgi_record("resp ", &header); > + > /* > * XXX: add some simple write heuristics here > * On slower VMs, spotty connections, etc., we don't want to go right to > * disconnect. Let's at least try to write the data a few times before > * giving up. > */ > - for (off = 0; off < resp->data_len; off += nw) { > - nw = write(c->fd, resp->data + off, resp->data_len - off); > + while (iov[0].iov_len > 0 || iov[1].iov_len > 0 || iov[2].iov_len > 0) { > + nw = writev(c->fd, iov, nitems(iov)); > if (nw == 0) { > c->sock->client_status = CLIENT_DISCONNECT; > break; > @@ -388,7 +375,6 @@ fcgi_send_response(struct request *c, struct fcgi_resp > if (nw == -1) { > err++; > if (errno == EAGAIN && err < th) { > - nw = 0; > nanosleep(&ts, NULL); > continue; > } > @@ -397,44 +383,47 @@ fcgi_send_response(struct request *c, struct fcgi_resp > break; > } > > - if (nw != resp->data_len - off) > - log_debug("%s: partial write: %zu vs %zu", __func__, nw, > - resp->data_len - off); > + for (i = 0; i < nitems(iov); ++i) { > + if (iov[i].iov_len >= nw) { > + iov[i].iov_len -= nw; > + break; > + } > + nw -= iov[i].iov_len; > + iov[i].iov_len = 0; > + } > } > +} > > - free(resp); > +void > +fcgi_send_response(struct request *c, int type, const void *data, > + size_t len) > +{ > + while (len > FCGI_CONTENT_SIZE) { > + send_response(c, type, data, len); > + if (c->sock->client_status == CLIENT_DISCONNECT) > + return; > + > + data += FCGI_CONTENT_SIZE; > + len -= FCGI_CONTENT_SIZE; > + } > + > + if (len == 0) > + return; > + > + send_response(c, type, data, len); > } > > void > fcgi_create_end_record(struct request *c) > { > - struct fcgi_response *resp; > - struct fcgi_record_header *header; > - struct fcgi_end_request_body *end_request; > + struct fcgi_end_request_body end_request; > > - if ((resp = calloc(1, sizeof(struct fcgi_response))) == NULL) { > - log_warn("cannot calloc fcgi_response"); > - return; > - } > - header = (struct fcgi_record_header*) resp->data; > - header->version = 1; > - header->type = FCGI_END_REQUEST; > - header->id = htons(c->id); > - header->content_len = htons(sizeof(struct > - fcgi_end_request_body)); > - header->padding_len = 0; > - header->reserved = 0; > - end_request = (struct fcgi_end_request_body *) (resp->data + > - sizeof(struct fcgi_record_header)); > - end_request->app_status = htonl(0); /* script_status */ > - end_request->protocol_status = FCGI_REQUEST_COMPLETE; > - end_request->reserved[0] = 0; > - end_request->reserved[1] = 0; > - end_request->reserved[2] = 0; > - resp->data_pos = 0; > - resp->data_len = sizeof(struct fcgi_end_request_body) + > - sizeof(struct fcgi_record_header); > - fcgi_send_response(c, resp); > + memset(&end_request, 0, sizeof(end_request)); > + end_request.app_status = htonl(0); /* script status */ > + end_request.protocol_status = FCGI_REQUEST_COMPLETE; > + > + fcgi_send_response(c, FCGI_END_REQUEST, &end_request, > + sizeof(end_request)); > } > > void > blob - 789ae140f9fb7ff7c3be4252259d46cc4164ec0f > blob + b164df949130a41810c6bd822a285e903027709d > --- gotwebd/gotwebd.h > +++ gotwebd/gotwebd.h > @@ -134,13 +134,6 @@ struct fcgi_record_header { > uint8_t reserved; > }__attribute__((__packed__)); > > -struct fcgi_response { > - TAILQ_ENTRY(fcgi_response) entry; > - uint8_t data[FCGI_RECORD_SIZE]; > - size_t data_pos; > - size_t data_len; > -}; > - > struct repo_dir { > char *name; > char *owner;