Download raw body.
gotwebd: avoid calloc/free per fcgi record
Omar Polo <op@omarpolo.com> wrote:
> Omar Polo <op@omarpolo.com> wrote:
> > Omar Polo <op@omarpolo.com> wrote:
> > > Stefan Sperling <stsp@stsp.name> 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?
>
> i forgot to increment the iov base pointer on partial writes, here's a
> revised diff: (with also some other minor tweaks)
attaching the diff instead of inlining it to avoid issues with
quoted-printable.
P.S.: anybody knows how to correctly inline diffs with mblaze? I don't
really like to add attachments with content-disposition=inline because
they aren't included when replying (at least with the clients i tried)
diff refs/heads/main refs/heads/gwdgc
commit - 7375fc126e0f55289656336c6c8160c46efaba20
commit + 28888d2d39500f7bb0646fb59ca545d5b50a1d31
blob - 845cfc6a1588e7b4e4d605f6892a16fb3ef5b601
blob + f0de1b24e2bf33d38244b293dc7316561566af60
--- gotwebd/fcgi.c
+++ gotwebd/fcgi.c
@@ -20,6 +20,7 @@
#include <sys/queue.h>
#include <sys/socket.h>
#include <sys/types.h>
+#include <sys/uio.h>
#include <errno.h>
#include <event.h>
@@ -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,54 @@ 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;
+ tot += header.padding_len;
}
- 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 (tot > 0) {
+ nw = writev(c->fd, iov, nitems(iov));
if (nw == 0) {
c->sock->client_status = CLIENT_DISCONNECT;
break;
@@ -388,7 +377,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 +385,53 @@ 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);
+ if (nw != tot)
+ log_debug("%s: partial write: %zu vs %zu", __func__,
+ nw, tot);
+
+ tot -= nw;
+ for (i = 0; i < nitems(iov); ++i) {
+ if (nw < iov[i].iov_len) {
+ iov[i].iov_base += 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;
gotwebd: avoid calloc/free per fcgi record