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?
>
> 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 <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,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;