> Am 15.10.2018 um 23:16 schrieb Rainer Jung <rainer.j...@kippdata.de>: > > Adjusted SSL_read() rc value 0 handling applied in r1843954 to trunk. I'll > let it sit there until tomorrow for comments and then suggest for backport.
Thanks, Rainer! The h2 test suite runs as smoothly as it did before on my machine with OpenSSL 1.1.1 in trunk. I am all for backporting and testing von 2.4.x on this. -Stefan > > Am 15.10.2018 um 12:55 schrieb Rainer Jung: >> Am 15.10.2018 um 10:02 schrieb Stefan Eissing: >>> >>> >>>> Am 14.10.2018 um 00:46 schrieb Rainer Jung <rainer.j...@kippdata.de>: >>>> >>>> It seems the h2 failure only happens when building httpd against OpenSSL >>>> 1.1.1 (independent of TLS version used). I did a quick check with an httpd >>>> build against 1.1.0i and there the same vhost of the test framework >>>> instance worked with the same clients, that failed for 1.1.1. >>>> >>>> The client side OpenSSL version seems not to matter. >>>> >>>> When using 1.1.1 in the server even browsers seem to fail with h2. >>>> >>>> Anyone who can successfully use h2 with 2.4.36 build against OpenSSL 1.1.1? >>> >>> Me, and I got reports from others. >>> >>>> When comparing trace logs between the 1.1.1 server and the 1.1.0i server, >>>> one can see, that a failing OpenSSL read (0 bytes) results in APR_EOF >>>> (70014) for 1.1.1 but in errno 11 (EAGAIN) for 1.1.0i. I wonder whether >>>> this is due to the new SSL_MODE_AUTO_RETRY and maybe the following change: >>>> >>>> +#if OPENSSL_VERSION_NUMBER >= 0x1010100fL >>>> + /* For OpenSSL >=1.1.1, disable auto-retry mode so it's possible >>>> + * to consume handshake records without blocking for app-data. >>>> + * https://github.com/openssl/openssl/issues/7178 */ >>>> + SSL_CTX_clear_mode(ctx, SSL_MODE_AUTO_RETRY); >>>> +#endif >>> >>> Hmm, just read the ticket made by Joe regarding this change. >>> >>> I try to summarize my understanding: >>> >>> - this has nothing to do with HTTP/2 in particular. It's only triggered by >>> the h2 calling sequence. >>> - SSL_read() only returns transport data, but sometimes reads (part of) TLS >>> meta data, such as handshake messages. >>> - When it reads this meta data (and has processed it), it can either do >>> another read directly after or return with >>> its equivalent of EAGAIN. By default, it does the first. >>> - The change in handshake handling between TLSv1.2 and TLSv1.3 led, in >>> Joe's testing, to the case where SSL_read()'s >>> internal re-read() on the socket blocked, as the client was not sending >>> anything. >>> - So, after discussion on the openssl issue tracker, Joe changed the >>> OpenSSL behaviour of this by inserting the code above. >>> - Now, when SSL_read() is called, processes some meta data, it will not >>> read() on the socket again, but return a read of length 0. >>> - mod_ssl interprets this as EOF: >>> ssl_engine_io.c, lines 673.. >>> >>> else if (rc == 0) { >>> /* If EAGAIN, we will loop given a blocking read, >>> * otherwise consider ourselves at EOF. >>> */ >>> >>> and returns APR_EOF to h2 which then shuts the connection down. >>> >>> To my understanding, mod_ssl's ssl_engine_io.c, ssl_io_input_read() has the >>> special case handling that: >>> - if it gets a SSL_ERROR_WANT_READ from SSL_read(), it remembers that, >>> calls again >>> - if SSL_read() == 0, non-blocking, it >>> a) on having seen SSL_ERROR_WANT_READ before, return APR_EGAIN >>> b) otherwise, return APR_EOF >>> >>> OpenSSL's documentation of SSL_read() now states: >>> >>>> Old documentation indicated a difference between 0 and -1, and that -1 was >>>> retry-able. >>>> You should instead call SSL_get_error() to find out if it's retry-able. >>> >>> So, we should change our logic here. Anyone having the courage to do so? ;-) >> Thanks for the further investigations. >> I'm currently testing the following patch which looks OK wrt. test suite >> results. Need to run more combinations (OpenSSL version client versus >> server) though. Server with 1.1.1 and with 1.0.2p both look OK (including >> the h2 tests). Maybe some cases could be folded together or be dropped, but >> I tried to make the logic changes not to big. The SSL_ERROR_ZERO_RETURN part >> is new, because without that we get an ssl:info log line AH01992 with error >> 6 (SSL_ERROR_ZERO_RETURN) at the end of the response (at least with 1.1.1). >> --- modules/ssl/ssl_engine_io.c.orig 2018-08-15 17:01:08.000000000 +0200 >> +++ modules/ssl/ssl_engine_io.c 2018-10-15 11:46:01.258042000 +0200 >> @@ -680,35 +680,32 @@ >> } >> return inctx->rc; >> } >> - else if (rc == 0) { >> - /* If EAGAIN, we will loop given a blocking read, >> - * otherwise consider ourselves at EOF. >> - */ >> - if (APR_STATUS_IS_EAGAIN(inctx->rc) >> - || APR_STATUS_IS_EINTR(inctx->rc)) { >> - /* Already read something, return APR_SUCCESS instead. >> - * On win32 in particular, but perhaps on other kernels, >> - * a blocking call isn't 'always' blocking. >> + else /* (rc <= 0) */ { >> + if (rc == 0) { >> + /* If EAGAIN, we will loop given a blocking read, >> + * otherwise consider ourselves at EOF. >> */ >> - if (*len > 0) { >> - inctx->rc = APR_SUCCESS; >> - break; >> - } >> - if (inctx->block == APR_NONBLOCK_READ) { >> - break; >> - } >> - } >> - else { >> - if (*len > 0) { >> - inctx->rc = APR_SUCCESS; >> + if (APR_STATUS_IS_EAGAIN(inctx->rc) >> + || APR_STATUS_IS_EINTR(inctx->rc)) { >> + /* Already read something, return APR_SUCCESS instead. >> + * On win32 in particular, but perhaps on other kernels, >> + * a blocking call isn't 'always' blocking. >> + */ >> + if (*len > 0) { >> + inctx->rc = APR_SUCCESS; >> + break; >> + } >> + if (inctx->block == APR_NONBLOCK_READ) { >> + break; >> + } >> } >> else { >> - inctx->rc = APR_EOF; >> + if (*len > 0) { >> + inctx->rc = APR_SUCCESS; >> + break; >> + } >> } >> - break; >> } >> - } >> - else /* (rc < 0) */ { >> int ssl_err = SSL_get_error(inctx->filter_ctx->pssl, rc); >> conn_rec *c = >> (conn_rec*)SSL_get_app_data(inctx->filter_ctx->pssl); >> @@ -754,6 +751,10 @@ >> "SSL input filter read failed."); >> } >> } >> + else if (rc == 0 && ssl_err == SSL_ERROR_ZERO_RETURN) { >> + inctx->rc = APR_EOF; >> + break; >> + } >> else /* if (ssl_err == SSL_ERROR_SSL) */ { >> /* >> * Log SSL errors and any unexpected conditions. >> @@ -763,6 +764,10 @@ >> ssl_log_ssl_error(SSLLOG_MARK, APLOG_INFO, >> mySrvFromConn(c)); >> } >> + if (rc == 0) { >> + inctx->rc = APR_EOF; >> + break; >> + } >> if (inctx->rc == APR_SUCCESS) { >> inctx->rc = APR_EGENERAL; >> } >> Regards, >> Rainer