mod_ssl's output buffering has been bothering me for a while.
1) it buffers the encrypted output stream (to some extent) coupled with
regular use of FLUSH buckets. This seems redundant/inefficient; the
core output filter should be doing this kind of thing optimally already.
2) it does /not/ do any buffering of the plaintext input brigades. This
leads to some pathological inefficiency as discussed here in the past:
http://marc.info/?l=apache-httpd-dev&m=125070085316400&w=2
The one justification I know for (1) is in the ssl_engine_io.c comment:
/* the first two SSL_writes (of 1024 and 261 bytes)
* need to be in the same packet (vec[0].iov_base)
*/
but I see no evidence this will not happen if the core output filter is
allowed to operate as normal. Maybe that was not true in the psat.
So I'd propose to remove (1), which is simple and add (2), which is not
so simple. I've hacked up (yet another) "coalescing" filter for (2)
which works to coalesce chunked brigades as expected; proof of concept
attached. Would welcome some discussion before I work on this
further...
Regards, Joe
Index: ssl_engine_io.c
===================================================================
--- ssl_engine_io.c (revision 1031534)
+++ ssl_engine_io.c (working copy)
@@ -102,7 +102,6 @@
BIO *pbioWrite;
ap_filter_t *pInputFilter;
ap_filter_t *pOutputFilter;
- int nobuffer; /* non-zero to prevent buffering */
SSLConnRec *config;
} ssl_filter_ctx_t;
@@ -110,9 +109,6 @@
ssl_filter_ctx_t *filter_ctx;
conn_rec *c;
apr_bucket_brigade *bb; /* Brigade used as a buffer. */
- apr_size_t length; /* Number of bytes stored in ->bb brigade. */
- char buffer[AP_IOBUFSIZE]; /* Fixed-size buffer */
- apr_size_t blen; /* Number of bytes of ->buffer used. */
apr_status_t rc;
} bio_filter_out_ctx_t;
@@ -124,36 +120,13 @@
outctx->filter_ctx = filter_ctx;
outctx->c = c;
outctx->bb = apr_brigade_create(c->pool, c->bucket_alloc);
- outctx->blen = 0;
- outctx->length = 0;
return outctx;
}
-static int bio_filter_out_flush(BIO *bio)
+/* Pass the output filter brigade up the filter stack. */
+static int bio_filter_out_pass(bio_filter_out_ctx_t *outctx)
{
- bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);
- apr_bucket *e;
-
- if (!(outctx->blen || outctx->length)) {
- outctx->rc = APR_SUCCESS;
- return 1;
- }
-
- if (outctx->blen) {
- e = apr_bucket_transient_create(outctx->buffer, outctx->blen,
- outctx->bb->bucket_alloc);
- /* we filled this buffer first so add it to the
- * head of the brigade
- */
- APR_BRIGADE_INSERT_HEAD(outctx->bb, e);
- outctx->blen = 0;
- }
-
- outctx->length = 0;
- e = apr_bucket_flush_create(outctx->bb->bucket_alloc);
- APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
-
outctx->rc = ap_pass_brigade(outctx->filter_ctx->pOutputFilter->next,
outctx->bb);
/* Fail if the connection was reset: */
@@ -163,6 +136,17 @@
return (outctx->rc == APR_SUCCESS) ? 1 : -1;
}
+static int bio_filter_out_flush(BIO *bio)
+{
+ bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);
+ apr_bucket *e;
+
+ e = apr_bucket_flush_create(outctx->bb->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
+
+ return bio_filter_out_pass(outctx);
+}
+
static int bio_filter_create(BIO *bio)
{
bio->shutdown = 1;
@@ -194,7 +178,8 @@
static int bio_filter_out_write(BIO *bio, const char *in, int inl)
{
bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);
-
+ apr_bucket *e;
+
/* Abort early if the client has initiated a renegotiation. */
if (outctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
outctx->rc = APR_ECONNABORTED;
@@ -207,77 +192,48 @@
*/
BIO_clear_retry_flags(bio);
- if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer)) &&
- !outctx->filter_ctx->nobuffer) {
- /* the first two SSL_writes (of 1024 and 261 bytes)
- * need to be in the same packet (vec[0].iov_base)
- */
- /* XXX: could use apr_brigade_write() to make code look cleaner
- * but this way we avoid the malloc(APR_BUCKET_BUFF_SIZE)
- * and free() of it later
- */
- memcpy(&outctx->buffer[outctx->blen], in, inl);
- outctx->blen += inl;
+ /* pass along the encrypted data
+ * need to flush since we're using SSL's malloc-ed buffer
+ * which will be overwritten once we leave here
+ */
+ e = apr_bucket_transient_create(in, inl, outctx->bb->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
+
+ if (bio_filter_out_pass(outctx) < 0) {
+ return -1;
}
- else {
- /* pass along the encrypted data
- * need to flush since we're using SSL's malloc-ed buffer
- * which will be overwritten once we leave here
- */
- apr_bucket *bucket = apr_bucket_transient_create(in, inl,
- outctx->bb->bucket_alloc);
- outctx->length += inl;
- APR_BRIGADE_INSERT_TAIL(outctx->bb, bucket);
-
- if (bio_filter_out_flush(bio) < 0) {
- return -1;
- }
- }
-
return inl;
}
static long bio_filter_out_ctrl(BIO *bio, int cmd, long num, void *ptr)
{
long ret = 1;
- char **pptr;
-
bio_filter_out_ctx_t *outctx = (bio_filter_out_ctx_t *)(bio->ptr);
switch (cmd) {
- case BIO_CTRL_RESET:
- outctx->blen = outctx->length = 0;
+ case BIO_CTRL_RESET:
+ case BIO_CTRL_EOF:
+ case BIO_C_SET_BUF_MEM_EOF_RETURN:
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, outctx->c,
+ "output bio: eeeko %d", cmd);
+ ret = 0;
break;
- case BIO_CTRL_EOF:
- ret = (long)((outctx->blen + outctx->length) == 0);
+ case BIO_CTRL_WPENDING:
+ case BIO_CTRL_PENDING:
+ case BIO_CTRL_INFO:
+ ret = 0;
break;
- case BIO_C_SET_BUF_MEM_EOF_RETURN:
- outctx->blen = outctx->length = (apr_size_t)num;
- break;
- case BIO_CTRL_INFO:
- ret = (long)(outctx->blen + outctx->length);
- if (ptr) {
- pptr = (char **)ptr;
- *pptr = (char *)&(outctx->buffer[0]);
- }
- break;
- case BIO_CTRL_GET_CLOSE:
+ case BIO_CTRL_GET_CLOSE:
ret = (long)bio->shutdown;
break;
case BIO_CTRL_SET_CLOSE:
bio->shutdown = (int)num;
break;
- case BIO_CTRL_PENDING:
/* _PENDING is interpreted as "pending to read" - since this
* is a *write* buffer, return zero. */
ret = 0L;
break;
- case BIO_CTRL_WPENDING:
- /* _WPENDING is interpreted as "pending to write" - return the
- * number of bytes in ->bb plus buffer. */
- ret = (long)(outctx->blen + outctx->length);
- break;
case BIO_CTRL_FLUSH:
ret = bio_filter_out_flush(bio);
break;
@@ -928,6 +884,7 @@
static const char ssl_io_filter[] = "SSL/TLS Filter";
static const char ssl_io_buffer[] = "SSL/TLS Buffer";
+static const char ssl_io_coalesce[] = "SSL/TLS Coalescing Filter";
/*
* Close the SSL part of the socket connection
@@ -1372,6 +1329,120 @@
return APR_SUCCESS;
}
+#define COALESCE_BYTES (1024)
+
+struct coalesce_ctx {
+ char buffer[COALESCE_BYTES];
+ apr_size_t bytes; /* number of bytes of buffer used. */
+};
+
+static apr_status_t flush_coalesced_data(ap_filter_t *f, apr_bucket_brigade
*bb,
+ struct coalesce_ctx *ctx)
+{
+ apr_bucket *e;
+
+ if (ctx && ctx->bytes) {
+ e = apr_bucket_transient_create(ctx->buffer, ctx->bytes,
bb->bucket_alloc);
+ APR_BRIGADE_INSERT_HEAD(bb, e);
+
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, f->c,
+ "coalesce: flushing %lu bytes [%.*s]", ctx->bytes,
+ (int)ctx->bytes, ctx->buffer);
+
+ ctx->bytes = 0;
+ }
+
+ return ap_pass_brigade(f->next, bb);
+}
+
+static apr_status_t ssl_io_filter_coalesce(ap_filter_t *f,
+ apr_bucket_brigade *bb)
+{
+ apr_bucket *e, *last = NULL;
+ apr_size_t bytes;
+ struct coalesce_ctx *ctx = f->ctx;
+ apr_status_t rv;
+ int count = 0;
+
+ /* nowt to do, move along */
+ if (APR_BRIGADE_EMPTY(bb)) {
+ return APR_SUCCESS;
+ }
+
+ if (ctx)
+ bytes = ctx->bytes;
+ else
+ bytes = 0;
+
+ /* Iterate through until the end of the brigade. If we can
+ * collect a consecutive sequence of data buckets of count > 1 and
+ * of length smaller < COALESCE_BYTES then we should coalesce
+ * them. */
+ e = APR_BRIGADE_FIRST(bb);
+ while (e != APR_BRIGADE_SENTINEL(bb)
+ && !APR_BUCKET_IS_METADATA(e)
+ && e->length != (apr_size_t)-1
+ && (bytes + e->length) < COALESCE_BYTES) {
+ last = e;
+ count++;
+ bytes += e->length;
+ e = APR_BUCKET_NEXT(e);
+ }
+
+ /* cases:
+ * if there is stuff buffered AND any number of buckets stuff to buffer
+ * OR if there are >1 buckets to buffer
+ */
+ if ((bytes && ctx && ctx->bytes) || (count > 1)) {
+ apr_bucket *next;
+
+ /* coalesce buckets from start to last */
+
+ if (!ctx) {
+ f->ctx = ctx = apr_palloc(f->c->pool, sizeof *ctx);
+ ctx->bytes = 0;
+ }
+
+ e = APR_BRIGADE_FIRST(bb);
+ do {
+ apr_size_t len;
+ const char *data;
+
+ if (e->length) {
+ rv = apr_bucket_read(e, &data, &len, APR_BLOCK_READ);
+ if (rv) {
+ ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, f->c,
+ "coalesce failed to read from data bucket");
+ return rv;
+ }
+
+ memcpy(ctx->buffer + ctx->bytes, data, len);
+ ctx->bytes += len;
+
+ ap_log_cerror(APLOG_MARK, APLOG_TRACE4, 0, f->c,
+ "coalesce: grabbed %lu bytes, now [%.*s]",
+ len, (int)ctx->bytes, ctx->buffer);
+ }
+
+ next = APR_BUCKET_NEXT(e);
+ apr_bucket_delete(e);
+ e = next;
+ } while (e != last);
+ }
+
+ /* if there are any buckets which we could not coalesce
+ * (e.g. metadata), flush anything buffered and the brigade. */
+ if (last != APR_BRIGADE_SENTINEL(bb)) {
+ /* flush it all. */
+ rv = flush_coalesced_data(f, bb, ctx);
+ if (rv) {
+ return rv;
+ }
+ }
+
+ return APR_SUCCESS;
+}
+
static apr_status_t ssl_io_filter_output(ap_filter_t *f,
apr_bucket_brigade *bb)
{
@@ -1439,10 +1510,8 @@
}
else if (AP_BUCKET_IS_EOC(bucket)) {
/* The special "EOC" bucket means a shutdown is needed;
- * - turn off buffering in bio_filter_out_write
* - issue the SSL_shutdown
*/
- filter_ctx->nobuffer = 1;
ssl_filter_io_shutdown(filter_ctx, f->c, 0);
if ((status = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
return status;
@@ -1723,7 +1792,8 @@
filter_ctx->config = myConnConfig(c);
- filter_ctx->nobuffer = 0;
+ ap_add_output_filter(ssl_io_coalesce, NULL, r, c);
+
filter_ctx->pOutputFilter = ap_add_output_filter(ssl_io_filter,
filter_ctx, r, c);
@@ -1752,6 +1822,7 @@
void ssl_io_filter_register(apr_pool_t *p)
{
ap_register_input_filter (ssl_io_filter, ssl_io_filter_input, NULL,
AP_FTYPE_CONNECTION + 5);
+ ap_register_output_filter (ssl_io_coalesce, ssl_io_filter_coalesce, NULL,
AP_FTYPE_CONNECTION + 4);
ap_register_output_filter (ssl_io_filter, ssl_io_filter_output, NULL,
AP_FTYPE_CONNECTION + 5);
ap_register_input_filter (ssl_io_buffer, ssl_io_filter_buffer, NULL,
AP_FTYPE_PROTOCOL);
