Merge r1705194, r1705823, r1705826, r1705828, r1705833, r1706275, r1707230, r1707231...

mod_ssl: forward EOR (only) brigades to the core_output_filter().

mod_ssl: don't FLUSH output (blocking) on read.
This defeats deferred write (and pipelining), eg. check_pipeline() is not
expecting the pipe to be flushed under it.
So let OpenSSL >= 0.9.8m issue the flush when necessary (earlier versions
are known to not handle all the cases, so we keep flushing with those).

mod_ssl: follow up to r1705823.
Oups, every #if needs a #endif...

mod_ssl: pass through metadata buckets untouched in ssl_io_filter_output(),
the core output filter needs them.

Proposed by: jorton

mod_ssl: follow up to r1705194, r1705823, r1705826 and r1705828.
Add CHANGES entry, and restore ap_process_request_after_handler()'s comment
as prior to r1705194 (the change makes no sense now).

mod_ssl: follow up to r1705823.
We still need to flush in the middle of a SSL/TLS handshake.

mod_ssl: follow up to r1705823.
Flush SSL/TLS handshake data when writing (instead of before reading),
and only when necessary (openssl < 0.9.8m or proxy/client side).

mod_ssl: follow up to r1707230: fix (inverted) logic for SSL_in_connect_init().

Submitted by: ylavic
Reviewed/backported by: jim

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1715014 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 2703361a916cef548f4283aee3dc3ae61eb15bc4..870e76fbac76666ac188799f8be605b3ddd59140 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -24,6 +24,9 @@ Changes with Apache 2.4.17
      to avoid reusing it should the close be effective after some new request
      is ready to be sent.  [Yann Ylavic]
 
+  *) mod_ssl: Make the output filter more friendly with deferred write and
+     response pipelining. [Yann Ylavic, Joe Orton]
+
   *) mod_substitute: Allow to configure the patterns merge order with the new
      SubstituteInheritBefore on|off directive.  PR 57641
      [Marc.Stern <Marc.Stern approach.be>, Yann Ylavic, William Rowe]

diff --git a/STATUS b/STATUS
index a14b76a829b60f7dae0ebdabc1b1b9be6b0ef8aa..6af0176b31194d25ed3b790b6491f3516f53e6c1 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -111,18 +111,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-   * mod_ssl: Make the output filter more friendly with deferred write and
-              response pipelining.
-     trunk patch: http://svn.apache.org/r1705194
-                  http://svn.apache.org/r1705823
-                  http://svn.apache.org/r1705826
-                  http://svn.apache.org/r1705828
-                  http://svn.apache.org/r1705833
-                  http://svn.apache.org/r1706275
-                  http://svn.apache.org/r1707230
-                  http://svn.apache.org/r1707231
-     2.4.x patch: http://people.apache.org/~ylavic/httpd-2.4.x-mod_ssl-deferred_friendly-v3.patch
-     +1: ylavic, icing, jim
 
   *) synch 2.4.x with trunk
         mod_authn_anon: Constify + save a few bytes in conf pool

diff --git a/modules/ssl/ssl_engine_io.c b/modules/ssl/ssl_engine_io.c
index 1e6c61846dd2b856555c39d8536f5db1b1e2a9c3..09b768b8827e0b6a6805c6f3287ba5a6f7edb1fd 100644 (file)
--- a/modules/ssl/ssl_engine_io.c
+++ b/modules/ssl/ssl_engine_io.c
@@ -187,6 +187,7 @@ 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;
+    int need_flush;
 
     /* Abort early if the client has initiated a renegotiation. */
     if (outctx->filter_ctx->config->reneg_state == RENEG_ABORT) {
@@ -205,6 +206,26 @@ static int bio_filter_out_write(BIO *bio, const char *in, int inl)
     e = apr_bucket_transient_create(in, inl, outctx->bb->bucket_alloc);
     APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
 
+    /* In theory, OpenSSL should flush as necessary, but it is known
+     * not to do so correctly in some cases (< 0.9.8m; see PR 46952),
+     * or on the proxy/client side (after ssl23_client_hello(), e.g.
+     * ssl/proxy.t test suite).
+     *
+     * Historically, this flush call was performed only for an SSLv2
+     * connection or for a proxy connection.  Calling _out_flush can
+     * be expensive in cases where requests/reponses are pipelined,
+     * so limit the performance impact to handshake time.
+     */
+#if OPENSSL_VERSION_NUMBER < 0x0009080df
+     need_flush = !SSL_is_init_finished(outctx->filter_ctx->pssl)
+#else
+     need_flush = SSL_in_connect_init(outctx->filter_ctx->pssl);
+#endif
+    if (need_flush) {
+        e = apr_bucket_flush_create(outctx->bb->bucket_alloc);
+        APR_BRIGADE_INSERT_TAIL(outctx->bb, e);
+    }
+
     if (bio_filter_out_pass(outctx) < 0) {
         return -1;
     }
@@ -448,21 +469,6 @@ static int bio_filter_in_read(BIO *bio, char *in, int inlen)
         return -1;
     }
 
-    /* In theory, OpenSSL should flush as necessary, but it is known
-     * not to do so correctly in some cases; see PR 46952.
-     *
-     * Historically, this flush call was performed only for an SSLv2
-     * connection or for a proxy connection.  Calling _out_flush
-     * should be very cheap in cases where it is unnecessary (and no
-     * output is buffered) so the performance impact of doing it
-     * unconditionally should be minimal.
-     */
-    if (bio_filter_out_flush(inctx->bio_out) < 0) {
-        bio_filter_out_ctx_t *outctx = inctx->bio_out->ptr;
-        inctx->rc = outctx->rc;
-        return -1;
-    }
-
     BIO_clear_retry_flags(bio);
 
     if (!inctx->bb) {
@@ -1632,49 +1638,30 @@ static apr_status_t ssl_io_filter_output(ap_filter_t *f,
         return ssl_io_filter_error(f, bb, status);
     }
 
-    while (!APR_BRIGADE_EMPTY(bb)) {
+    while (!APR_BRIGADE_EMPTY(bb) && status == APR_SUCCESS) {
         apr_bucket *bucket = APR_BRIGADE_FIRST(bb);
 
-        /* If it is a flush or EOS, we need to pass this down.
-         * These types do not require translation by OpenSSL.
-         */
-        if (APR_BUCKET_IS_EOS(bucket) || APR_BUCKET_IS_FLUSH(bucket)) {
-            if (bio_filter_out_flush(filter_ctx->pbioWrite) < 0) {
-                status = outctx->rc;
-                break;
-            }
-
-            if (APR_BUCKET_IS_EOS(bucket)) {
-                /*
-                 * By definition, nothing can come after EOS.
-                 * which also means we can pass the rest of this brigade
-                 * without creating a new one since it only contains the
-                 * EOS bucket.
-                 */
-
-                if ((status = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
-                    return status;
-                }
-                break;
-            }
-            else {
-                /* bio_filter_out_flush() already passed down a flush bucket
-                 * if there was any data to be flushed.
-                 */
-                apr_bucket_delete(bucket);
+        if (APR_BUCKET_IS_METADATA(bucket)) {
+            /* Pass through metadata buckets untouched.  EOC is
+             * special; terminate the SSL layer first. */
+            if (AP_BUCKET_IS_EOC(bucket)) {
+                ssl_filter_io_shutdown(filter_ctx, f->c, 0);
             }
-        }
-        else if (AP_BUCKET_IS_EOC(bucket)) {
-            /* The EOC bucket indicates connection closure, so SSL
-             * shutdown must now be performed.  */
-            ssl_filter_io_shutdown(filter_ctx, f->c, 0);
-            if ((status = ap_pass_brigade(f->next, bb)) != APR_SUCCESS) {
-                return status;
-            }
-            break;
+            AP_DEBUG_ASSERT(APR_BRIGADE_EMPTY(outctx->bb));
+
+            /* Metadata buckets are passed one per brigade; it might
+             * be more efficient (but also more complex) to use
+             * outctx->bb as a true buffer and interleave these with
+             * data buckets. */
+            APR_BUCKET_REMOVE(bucket);
+            APR_BRIGADE_INSERT_HEAD(outctx->bb, bucket);
+            status = ap_pass_brigade(f->next, outctx->bb);
+            if (status == APR_SUCCESS && f->c->aborted)
+                status = APR_ECONNRESET;
+            apr_brigade_cleanup(outctx->bb);
         }
         else {
-            /* filter output */
+            /* Filter a data bucket. */
             const char *data;
             apr_size_t len;
 
@@ -1687,7 +1674,9 @@ static apr_status_t ssl_io_filter_output(ap_filter_t *f,
                     break;
                 }
                 rblock = APR_BLOCK_READ;
-                continue; /* and try again with a blocking read. */
+                /* and try again with a blocking read. */
+                status = APR_SUCCESS;
+                continue;
             }
 
             rblock = APR_NONBLOCK_READ;
@@ -1698,11 +1687,8 @@ static apr_status_t ssl_io_filter_output(ap_filter_t *f,
 
             status = ssl_filter_write(f, data, len);
             apr_bucket_delete(bucket);
-
-            if (status != APR_SUCCESS) {
-                break;
-            }
         }
+
     }
 
     return status;