httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Doug MacEachern <do...@covalent.net>
Subject [patch] major ssl problem
Date Fri, 23 Nov 2001 22:39:21 GMT
ssl_io_filter_Output currently does this:

            /* read filter */
            apr_bucket_read(bucket, &data, &len, APR_BLOCK_READ);

            /* write SSL */
            n = ssl_io_hook_write(ctx->pssl, (unsigned char *)data, len);
            => above writes into a single memory buffer of at least size len

            if ((ret = churn_output(ctx)) != APR_SUCCESS) {
            => above creates a single transient bucket with memory buffer
               created above

the problem is that when 'bucket' is a file bucket, len will be the size
of the entire file.  so if the file is 200k, 500k, 10Mb, 1Gb, etc., there
is a malloc() for that size plus the additional bytes need for encryption.
and that is just for our copy, the ssl encryption layer mallocs for its
buffer before it is passed to mem_write() (the pbioWrite BIO callback).

patch below works towards solving this problem:
- first by breaking the buffer size fed to SSL_write into AP_IOBUFSIZE or
  less size chunks

- change pbioWrite from a memory buffer to a custom BIO BIO_s_bucket.
  this allows us to feed buckets down to the core as data is being
  written instead of filling the memory buffer first.  and making it
  possible to avoid an extra copy of the encrypted data.

it still needs to be optimized, see this comment for starters:
 * if we use heap buckets we can buffer before flushing to the core
 * but this means we have to malloc/free data in each bucket.
 * if we use transient buckets, no malloc/free for the data
 * but we have to flush right away.

in a nutshell: mallocs / less writev()s vs no mallocs / more writevs()

which is best can be debated.  but at the very least, we need to cut down
the size of buffers fed to SSL_write.  and a custom BIO will provide us
with the most optimizations options.

Index: modules/ssl/ssl_engine_io.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.44
diff -u -r1.44 ssl_engine_io.c
--- modules/ssl/ssl_engine_io.c	2001/11/19 22:37:57	1.44
+++ modules/ssl/ssl_engine_io.c	2001/11/23 21:46:51
@@ -70,6 +70,230 @@
 
 /* XXX THIS STUFF NEEDS A MAJOR CLEANUP -RSE XXX */
 
+typedef struct {
+    SSLFilterRec *frec;
+    conn_rec *c;
+    apr_bucket_brigade *bb;
+    apr_size_t length;
+    char buffer[AP_IOBUFSIZE];
+    apr_size_t blen;
+    int num_buckets;
+} BIO_bucket_t;
+
+#define BIO_bucket_ptr(bio) (BIO_bucket_t *)bio->ptr
+
+static int bucket_write(BIO *bio, const char *buf, int num);
+static int bucket_read(BIO *bio, char *buf, int size);
+static int bucket_puts(BIO *bio, const char *str);
+static int bucket_gets(BIO *bio, char *str, int size);
+static long bucket_ctrl(BIO *bio, int cmd, long arg1, void *arg2);
+static int bucket_new(BIO *bio);
+static int bucket_free(BIO *bio);
+
+static BIO_METHOD bucket_method = {
+    BIO_TYPE_MEM,
+    "APR bucket buffer",
+    bucket_write,
+    bucket_read,
+    bucket_puts,
+    bucket_gets,
+    bucket_ctrl,
+    bucket_new,
+    bucket_free,
+    NULL,
+};
+
+static BIO_METHOD *BIO_s_bucket(void)
+{
+    return &bucket_method;
+}
+
+static BIO_bucket_t *BIO_bucket_new(SSLFilterRec *frec, conn_rec *c)
+{
+    BIO_bucket_t *b = apr_palloc(c->pool, sizeof(*b));
+
+    b->frec = frec;
+    b->c = c;
+    b->bb = apr_brigade_create(c->pool);
+    b->blen = 0;
+    b->num_buckets = 0;
+
+    return b;
+}
+
+static int BIO_bucket_flush(BIO *bio)
+{
+    BIO_bucket_t *b = BIO_bucket_ptr(bio);
+
+    if (!(b->blen || b->length)) {
+        return APR_SUCCESS;
+    }
+
+    if (b->blen) {
+        apr_bucket *bucket = 
+            apr_bucket_transient_create(b->buffer,
+                                        b->blen);
+        /* we filled this buffer first so add it to the 
+         * head of the brigade
+         */
+        APR_BRIGADE_INSERT_HEAD(b->bb, bucket);
+        b->blen = 0;
+    }
+
+    b->num_buckets = 0;
+    b->length = 0;
+    APR_BRIGADE_INSERT_TAIL(b->bb, apr_bucket_flush_create());
+
+    return ap_pass_brigade(b->frec->pOutputFilter->next, b->bb);
+}
+
+static int bucket_new(BIO *bio)
+{
+    bio->shutdown = 1;
+    bio->init = 1;
+    bio->num = -1;
+    bio->ptr = NULL;
+
+    return 1;
+}
+
+static int bucket_free(BIO *bio)
+{
+    if (bio == NULL) {
+        return 0;
+    }
+
+    if (bio->shutdown) {
+        if ((bio->init) && (bio->ptr != NULL)) {
+            BIO_bucket_t *b = BIO_bucket_ptr(bio);
+            b->bb = NULL;
+            bio->ptr = NULL;
+        }
+    }
+
+    return 1;
+}
+	
+static int bucket_read(BIO *bio, char *out, int outl)
+{
+    return -1;
+}
+
+#define SSL_MAX_BUCKETS 16 /* MAX_IOVEC_TO_WRITE */
+#define SSL_USE_HEAP_BUCKETS
+
+/*
+ * if we use heap buckets we can buffer before flushing to the core
+ * but this means we have to malloc/free data in each bucket.
+ * if we use transient buckets, no malloc/free for the data
+ * but we have to flush right away.
+ */
+static int bucket_write(BIO *bio, const char *in, int inl)
+{
+    BIO_bucket_t *b = BIO_bucket_ptr(bio);
+
+    BIO_clear_retry_flags(bio);
+
+    if (!b->length && (inl < (sizeof(b->buffer) - b->blen))) {
+        /* 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(&b->buffer[b->blen], in, inl);
+        if (b->blen == 0) {
+            /* a bucket will be created in BIO_bucket_flush() */
+            b->num_buckets++;
+        }
+        b->blen += inl;
+    }
+    else {
+#ifdef SSL_USE_HEAP_BUCKETS
+        apr_bucket *bucket = apr_bucket_heap_create(in, inl, 1);
+#else
+        apr_bucket *bucket = apr_bucket_transient_create(in, inl);
+#endif
+
+        APR_BRIGADE_INSERT_TAIL(b->bb, bucket);
+        b->num_buckets++;
+        b->length += inl;
+
+#ifdef SSL_USE_HEAP_BUCKETS
+        if (b->num_buckets == SSL_MAX_BUCKETS) {
+            /* prevent core_output_filter from doing apr_brigade_split */
+            BIO_bucket_flush(bio);
+        }
+#else
+        BIO_bucket_flush(bio);
+#endif
+    }
+
+    return inl;
+}
+
+static long bucket_ctrl(BIO *bio, int cmd, long num, void *ptr)
+{
+    long ret = 1;
+
+    BIO_bucket_t *b = BIO_bucket_ptr(bio);
+
+    switch (cmd) {
+      case BIO_CTRL_RESET:
+        ret = 0; /*XXX*/
+        break;
+      case BIO_CTRL_EOF:
+        ret = 0; /*XXX*/
+        break;
+      case BIO_CTRL_INFO:
+        ret = 0; /*XXX*/
+        break;
+      case BIO_CTRL_GET_CLOSE:
+        ret = (long)bio->shutdown;
+        break;
+      case BIO_CTRL_SET_CLOSE:
+        bio->shutdown = (int)num;
+        break;
+      case BIO_CTRL_WPENDING:
+        ret = 0L;
+        break;
+      case BIO_CTRL_PENDING:
+        ret = (long)b->length;
+        break;
+      case BIO_CTRL_DUP:
+      case BIO_CTRL_FLUSH:
+        ret = (BIO_bucket_flush(bio) == APR_SUCCESS);
+        break;
+      case BIO_CTRL_PUSH:
+      case BIO_CTRL_POP:
+      case BIO_C_SET_BUF_MEM_EOF_RETURN:
+      case BIO_C_SET_BUF_MEM:
+      case BIO_C_GET_BUF_MEM_PTR:
+      default:
+        ret = 0;
+        break;
+    }
+
+    return ret;
+}
+
+static int bucket_gets(BIO *bio, char *buf, int size)
+{
+    return -1;
+}
+
+static int bucket_puts(BIO *bio, const char *str)
+{
+    /* this is never actually called */
+    int n, ret;
+
+    n = strlen(str);
+    ret = bucket_write(bio, str, n);
+
+    return ret;
+}
+
 static const char ssl_io_filter[] = "SSL/TLS Filter";
 
 static int ssl_io_hook_read(SSL *ssl, char *buf, int len)
@@ -112,13 +336,24 @@
 
 static int ssl_io_hook_write(SSL *ssl, unsigned char *buf, int len)
 {
-    int rc;
+    int total=0, rc=0, nrd;
 
     if (ssl == NULL) {
         return -1;
     }
 
-    rc = SSL_write(ssl, buf, len);
+    while (len > 0) {
+        int wlen = len > AP_IOBUFSIZE ? AP_IOBUFSIZE : len;
+        if ((nrd = SSL_write(ssl, buf, wlen)) < 0) {
+            rc = nrd;
+            break;
+        }
+        else {
+            total += nrd;
+        }
+        buf += nrd;
+        len -= nrd;
+    }
 
     if (rc < 0) {
         int ssl_err = SSL_get_error(ssl, rc);
@@ -141,50 +376,19 @@
          * XXX - Just trying to reflect the behaviour in 
          * openssl_state_machine.c [mod_tls]. TBD
          */
-        rc = 0;
+        total = 0;
     }
-    return rc;
+    return total;
 }
 
-#define BIO_mem(b) ((BUF_MEM *)b->ptr)
-
 static apr_status_t churn_output(SSLFilterRec *ctx)
 {
-    ap_filter_t *f = ctx->pOutputFilter;
-    apr_pool_t *p = f->c->pool;
-
     if (!ctx->pssl) {
         /* we've been shutdown */
         return APR_EOF;
     }
-
-    if (BIO_pending(ctx->pbioWrite)) {
-        BUF_MEM *bm = BIO_mem(ctx->pbioWrite);
-        apr_bucket_brigade *bb = apr_brigade_create(p);
-        apr_bucket *bucket; 
-
-        /*
-         * use the BIO memory buffer that has already been allocated,
-         * rather than making another copy of it.
-         * use bm directly here is *much* faster than calling BIO_read()
-         * look at crypto/bio/bss_mem.c:mem_read and you'll see why
-         */
-
-        bucket = apr_bucket_transient_create((const char *)bm->data,
-                                             bm->length);
-
-        bm->length = 0; /* reset */
-
-        APR_BRIGADE_INSERT_TAIL(bb, bucket);
-
-	/* XXX: it may be possible to not always flush */
-        bucket = apr_bucket_flush_create();
-        APR_BRIGADE_INSERT_TAIL(bb, bucket);
-
-        return ap_pass_brigade(f->next, bb);
-    }
 
-    return APR_SUCCESS;
+    return BIO_bucket_flush(ctx->pbioWrite);
 }
 
 #define bio_is_renegotiating(bio) \
@@ -583,7 +787,8 @@
     filter->b               = apr_brigade_create(c->pool);
     filter->rawb            = apr_brigade_create(c->pool);
     filter->pbioRead        = BIO_new(BIO_s_mem());
-    filter->pbioWrite       = BIO_new(BIO_s_mem());
+    filter->pbioWrite       = BIO_new(BIO_s_bucket());
+    filter->pbioWrite->ptr  = BIO_bucket_new(filter, c);
     SSL_set_bio(ssl, filter->pbioRead, filter->pbioWrite);
     filter->pssl            = ssl;
 


Mime
View raw message