From f7debe933d1a1554f8fc0bf431bff8736e486527 Mon Sep 17 00:00:00 2001 From: Jim Jagielski Date: Wed, 18 Nov 2015 16:14:36 +0000 Subject: [PATCH] Merge r1705194, r1705823, r1705826, r1705828, r1705833, r1706275, r1707230, r1707231 from trunk: 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 --- CHANGES | 3 ++ STATUS | 12 ----- modules/ssl/ssl_engine_io.c | 102 ++++++++++++++++-------------------- 3 files changed, 47 insertions(+), 70 deletions(-) diff --git a/CHANGES b/CHANGES index 2703361a91..870e76fbac 100644 --- 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 , Yann Ylavic, William Rowe] diff --git a/STATUS b/STATUS index a14b76a829..6af0176b31 100644 --- 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 1e6c61846d..09b768b882 100644 --- 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; -- 2.40.0