On Wed, Jul 3, 2019 at 6:38 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 07/03/2019 03:46 PM, ic...@apache.org wrote:
> >
> > - if (pending == OK) {
> > + if (pending == OK || (pending == DECLINED &&
> > + cs->pub.sense == CONN_SENSE_WANT_READ)) {
>
> Sorry for being devils advocate here and I may be completely off, but are we
> sure that
> cs->pub.sense is still CONN_SENSE_WANT_READ when we get here in all cases?
If CONN_SENSE_WANT_READ is set by the core or a module, it won't
change "inside" the MPM until after READ/WRITE_COMPLETION has been
handled, i.e. precisely in this "if {" block where it's reset to
default.
> If my memory is correct the cs->pub.sense with CONN_SENSE_WANT_READ /
> CONN_SENSE_WANT_WRITE
> was introduced as HTTP state might be writing or reading, but the underlying
> SSL needs just the
> opposite due to the state the SSL protocol is in.
Frankly I don't see how the CONN_SENSE_WANT_READ set in
ssl_filter_write (i.e. ssl_io_filter_output) can ever reach MPM event
in any meaningful way. The mod_ssl output filter runs quite late in
the connection processing, well after the handshake anyway, and any
error raised from there leads to the connection to be closed (EAGAIN
is not something we handle in the output filtering stack).
If we wanted the MPM to wait for readability during the SSL handshake
then ssl_hook_process_connection would have to return something else
than DECLINED in any case, e.g. OK when its ap_get_brigade() =>
ssl_io_filter_input() => ssl_io_filter_handshake() => SSL_accept()
returns EAGAIN (with CONN_STATE_WRITE_COMPLETION and
CONN_SENSE_WANT_READ/WRITE set appropriately, see attached trunk/2.4.x
patches).
Thus for me the current CONN_SENSE_WANT_READ is useless so this change
(r1862475) can't break it (and looks like the right thing to do to
handle CONN_SENSE_WANT_READ in MPM event).
> To be honest I did not have the time to dig deeper, but without further
> research it might happen
> that mod_ssl flips this around to CONN_SENSE_WANT_WRITE.
> More than happy to be proven wrong :-)
I'm not 100% sure to be right but it looks like CONN_SENSE_* semantics
have gone with the different places we have handled SSL handshake in
time..
Regards,
Yann.
Index: modules/ssl/mod_ssl.c
===================================================================
--- modules/ssl/mod_ssl.c (revision 1862525)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -705,18 +705,32 @@ static int ssl_hook_process_connection(conn_rec* c
SSLConnRec *sslconn = myConnConfig(c);
if (sslconn && !sslconn->disabled) {
+ apr_bucket_brigade* temp;
+ apr_status_t rv;
+
/* On an active SSL connection, let the input filters initialize
* themselves which triggers the handshake, which again triggers
* all kinds of useful things such as SNI and ALPN.
*/
- apr_bucket_brigade* temp;
-
temp = apr_brigade_create(c->pool, c->bucket_alloc);
- ap_get_brigade(c->input_filters, temp,
- AP_MODE_INIT, APR_BLOCK_READ, 0);
+ rv = ap_get_brigade(c->input_filters, temp,
+ AP_MODE_INIT, APR_BLOCK_READ, 0);
apr_brigade_destroy(temp);
+
+ if (rv != APR_SUCCESS) {
+ if (!APR_STATUS_IS_EAGAIN(rv)) {
+ return AP_FILTER_ERROR;
+ }
+ /* Let the MPM call us back again when the connection is ready
+ * to read/write (according to CONN_SENSE_WANT_READ/WRITE set
+ * by ssl_io_filter_input().
+ */
+ c->cs->state = CONN_STATE_WRITE_COMPLETION;
+ return OK;
+ }
}
-
+
+ /* SSL connection set up (handshaken), let others do their work. */
return DECLINED;
}
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1862525)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -943,15 +943,13 @@ static apr_status_t ssl_filter_write(ap_filter_t *
else if (ssl_err == SSL_ERROR_WANT_READ) {
/*
* If OpenSSL wants to read during write, and we were
- * nonblocking, set the sense explicitly to read and
- * report as an EAGAIN.
+ * nonblocking, then error out.
*
* (This is usually the case when the client forces an SSL
- * renegotiation which is handled implicitly by OpenSSL.)
+ * renegotiation which is handled implicitly by OpenSSL,
+ * but which we disallow.)
*/
- outctx->c->cs->sense = CONN_SENSE_WANT_READ;
- outctx->rc = APR_EAGAIN;
- ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, outctx->c,
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, outctx->c,
"Want read during nonblocking write");
}
else if (ssl_err == SSL_ERROR_SYSCALL) {
@@ -1422,9 +1420,20 @@ static apr_status_t ssl_io_filter_handshake(ssl_fi
* borrowed from openssl_state_machine.c [mod_tls].
* TBD.
*/
+ outctx->c->cs->sense = CONN_SENSE_WANT_READ;
outctx->rc = APR_EAGAIN;
return APR_EAGAIN;
}
+ else if (ssl_err == SSL_ERROR_WANT_WRITE) {
+ /*
+ * This is in addition to what was present earlier. It is
+ * borrowed from openssl_state_machine.c [mod_tls].
+ * TBD.
+ */
+ outctx->c->cs->sense = CONN_SENSE_WANT_WRITE;
+ outctx->rc = APR_EAGAIN;
+ return APR_EAGAIN;
+ }
else if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
ERR_GET_REASON(ERR_peek_error()) == SSL_R_HTTP_REQUEST) {
/*
@@ -1815,6 +1824,7 @@ static apr_status_t ssl_io_filter_output(ap_filter
bio_filter_out_ctx_t *outctx;
apr_bucket *flush_upto = NULL;
apr_read_type_e rblock = APR_NONBLOCK_READ;
+ apr_status_t rv;
if (f->c->aborted) {
apr_brigade_cleanup(bb);
@@ -1899,13 +1909,24 @@ static apr_status_t ssl_io_filter_output(ap_filter
}
status = ssl_filter_write(f, data, len);
+ if (APR_STATUS_IS_EAGAIN(status)) {
+ if (bio_filter_out_flush(filter_ctx->pbioWrite) < 0) {
+ status = outctx->rc;
+ break;
+ }
+ /* and try again.. */
+ status = APR_SUCCESS;
+ continue;
+ }
+
apr_bucket_delete(bucket);
}
}
+ rv = ap_filter_setaside_brigade(f, bb);
if (status == APR_SUCCESS) {
- status = ap_filter_setaside_brigade(f, bb);
+ status = rv;
}
return status;
}
Index: modules/ssl/mod_ssl.c
===================================================================
--- modules/ssl/mod_ssl.c (revision 1862498)
+++ modules/ssl/mod_ssl.c (working copy)
@@ -679,18 +679,32 @@ static int ssl_hook_process_connection(conn_rec* c
SSLConnRec *sslconn = myConnConfig(c);
if (sslconn && !sslconn->disabled) {
+ apr_bucket_brigade* temp;
+ apr_status_t rv;
+
/* On an active SSL connection, let the input filters initialize
* themselves which triggers the handshake, which again triggers
* all kinds of useful things such as SNI and ALPN.
*/
- apr_bucket_brigade* temp;
-
temp = apr_brigade_create(c->pool, c->bucket_alloc);
- ap_get_brigade(c->input_filters, temp,
- AP_MODE_INIT, APR_BLOCK_READ, 0);
+ rv = ap_get_brigade(c->input_filters, temp,
+ AP_MODE_INIT, APR_BLOCK_READ, 0);
apr_brigade_destroy(temp);
+
+ if (rv != APR_SUCCESS) {
+ if (!APR_STATUS_IS_EAGAIN(rv)) {
+ return AP_FILTER_ERROR;
+ }
+ /* Let the MPM call us back again when the connection is ready
+ * to read/write (according to CONN_SENSE_WANT_READ/WRITE set
+ * by ssl_io_filter_input().
+ */
+ c->cs->state = CONN_STATE_WRITE_COMPLETION;
+ return OK;
+ }
}
-
+
+ /* SSL connection set up (handshaken), let others do their work. */
return DECLINED;
}
Index: modules/ssl/ssl_engine_io.c
===================================================================
--- modules/ssl/ssl_engine_io.c (revision 1862498)
+++ modules/ssl/ssl_engine_io.c (working copy)
@@ -872,15 +872,13 @@ static apr_status_t ssl_filter_write(ap_filter_t *
else if (ssl_err == SSL_ERROR_WANT_READ) {
/*
* If OpenSSL wants to read during write, and we were
- * nonblocking, set the sense explicitly to read and
- * report as an EAGAIN.
+ * nonblocking, then error out.
*
* (This is usually the case when the client forces an SSL
- * renegotiation which is handled implicitly by OpenSSL.)
+ * renegotiation which is handled implicitly by OpenSSL,
+ * but which we disallow.)
*/
- outctx->c->cs->sense = CONN_SENSE_WANT_READ;
- outctx->rc = APR_EAGAIN;
- ap_log_cerror(APLOG_MARK, APLOG_TRACE6, 0, outctx->c,
+ ap_log_cerror(APLOG_MARK, APLOG_INFO, 0, outctx->c,
"Want read during nonblocking write");
}
else if (ssl_err == SSL_ERROR_SYSCALL) {
@@ -1351,9 +1349,20 @@ static apr_status_t ssl_io_filter_handshake(ssl_fi
* borrowed from openssl_state_machine.c [mod_tls].
* TBD.
*/
+ outctx->c->cs->sense = CONN_SENSE_WANT_READ;
outctx->rc = APR_EAGAIN;
return APR_EAGAIN;
}
+ else if (ssl_err == SSL_ERROR_WANT_WRITE) {
+ /*
+ * This is in addition to what was present earlier. It is
+ * borrowed from openssl_state_machine.c [mod_tls].
+ * TBD.
+ */
+ outctx->c->cs->sense = CONN_SENSE_WANT_WRITE;
+ outctx->rc = APR_EAGAIN;
+ return APR_EAGAIN;
+ }
else if (ERR_GET_LIB(ERR_peek_error()) == ERR_LIB_SSL &&
ERR_GET_REASON(ERR_peek_error()) == SSL_R_HTTP_REQUEST) {
/*
@@ -1807,6 +1816,16 @@ static apr_status_t ssl_io_filter_output(ap_filter
}
status = ssl_filter_write(f, data, len);
+ if (APR_STATUS_IS_EAGAIN(status)) {
+ if (bio_filter_out_flush(filter_ctx->pbioWrite) < 0) {
+ status = outctx->rc;
+ break;
+ }
+ /* and try again.. */
+ status = APR_SUCCESS;
+ continue;
+ }
+
apr_bucket_delete(bucket);
}
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1862498)
+++ server/mpm/event/event.c (working copy)
@@ -1113,7 +1113,8 @@ read_request:
"network write failure in core output filter");
cs->pub.state = CONN_STATE_LINGER;
}
- else if (c->data_in_output_filters) {
+ else if (c->data_in_output_filters
+ || cs->pub.sense == CONN_SENSE_WANT_READ) {
/* Still in WRITE_COMPLETION_STATE:
* Set a write timeout for this connection, and let the
* event thread poll for writeability.
