Re: [PATCH 60223] non-empty OpenSSL error queue preventing non-blocking reads through mod_ssl
On 11/11/2016 11:20 AM, Jacob Champion wrote: I suspect that these failures have nothing to do with this patch, but I just want to confirm that before it goes into trunk. Yeah, the failures show up with or without the patch, so I added a couple comments and committed. The SSL failures in the trunk test suite are going to make this difficult to test fully; has anyone seen them before or already have a fix tucked away privately? --Jacob
Re: [PATCH 60223] non-empty OpenSSL error queue preventing non-blocking reads through mod_ssl
On 11/11/2016 10:15 AM, William A Rowe Jr wrote: The patch looks reasonable to me. As you point out, fixing this in APR might be worthwhile, but doesn't protect mod_ssl from other OpenSSL consuming logic such as OpenLDAP auth, database connections or other plug-ins that may leave a lingering SSL_error state lying about. Looks good to me too -- thanks for the bump, Paul. I haven't committed this yet because of some SSL-related test failures in our suite when running against httpd-trunk: - mod_session_crypto appears to be segfaulting the server for some reason - mod_proxy tests hang the suite when run in -ssl test mode, and httpd complains about malformed request lines I suspect that these failures have nothing to do with this patch, but I just want to confirm that before it goes into trunk. --Jacob
Re: [PATCH 60223] non-empty OpenSSL error queue preventing non-blocking reads through mod_ssl
On Fri, Nov 11, 2016 at 9:01 AM, Paul Spanglerwrote: > On 10/17/2016 2:04 PM, Paul Spangler wrote: > >> Hello, >> >> Due to the way OpenSSL stores errors in a per-thread queue, functions >> such as SSL_read followed by SSL_get_error may not produce the desired >> result if the error queue is not empty prior to calling SSL_read[1]. For >> example, a non-blocking read reports that no data is available by >> setting up SSL_get_error to return SSL_ERROR_WANT_READ. But if an error >> is already present in the queue, SSL_get_error sees that error instead >> and returns SSL_ERROR_SSL. >> >> I found at least one case where the error queue may be non-empty prior >> to a non-blocking read[2] that involves combining mod_session_crypto >> (which leaves the error queue non-empty) and the third-party >> mod_websocket (which uses non-blocking reads), resulting in the >> connection being closed. I included a potential patch to mod_ssl for >> consideration on the bug report that simply clears the error queue prior >> to any of the three SSL_* calls that mod_ssl makes. An ideal fix might >> be to keep the error queue empty at all times (i.e. patch the APR crypto >> library), but I propose that this patch is more robust in a modular >> environment. >> >> Thanks for your consideration. >> >> [1] >> https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html#DESCRIPTION >> [2] https://bz.apache.org/bugzilla/show_bug.cgi?id=60223 >> >> Anyone have any thoughts on this small patch? It addresses an issue with > OpenSSL's per-thread state causing connections to fail. > > > Regards, > Paul Spangler > LabVIEW R > National Instruments > The patch looks reasonable to me. As you point out, fixing this in APR might be worthwhile, but doesn't protect mod_ssl from other OpenSSL consuming logic such as OpenLDAP auth, database connections or other plug-ins that may leave a lingering SSL_error state lying about.
Re: [PATCH 60223] non-empty OpenSSL error queue preventing non-blocking reads through mod_ssl
On 10/17/2016 2:04 PM, Paul Spangler wrote: Hello, Due to the way OpenSSL stores errors in a per-thread queue, functions such as SSL_read followed by SSL_get_error may not produce the desired result if the error queue is not empty prior to calling SSL_read[1]. For example, a non-blocking read reports that no data is available by setting up SSL_get_error to return SSL_ERROR_WANT_READ. But if an error is already present in the queue, SSL_get_error sees that error instead and returns SSL_ERROR_SSL. I found at least one case where the error queue may be non-empty prior to a non-blocking read[2] that involves combining mod_session_crypto (which leaves the error queue non-empty) and the third-party mod_websocket (which uses non-blocking reads), resulting in the connection being closed. I included a potential patch to mod_ssl for consideration on the bug report that simply clears the error queue prior to any of the three SSL_* calls that mod_ssl makes. An ideal fix might be to keep the error queue empty at all times (i.e. patch the APR crypto library), but I propose that this patch is more robust in a modular environment. Thanks for your consideration. [1] https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html#DESCRIPTION [2] https://bz.apache.org/bugzilla/show_bug.cgi?id=60223 Anyone have any thoughts on this small patch? It addresses an issue with OpenSSL's per-thread state causing connections to fail. Regards, Paul Spangler LabVIEW R National Instruments
[PATCH 60223] non-empty OpenSSL error queue preventing non-blocking reads through mod_ssl
Hello, Due to the way OpenSSL stores errors in a per-thread queue, functions such as SSL_read followed by SSL_get_error may not produce the desired result if the error queue is not empty prior to calling SSL_read[1]. For example, a non-blocking read reports that no data is available by setting up SSL_get_error to return SSL_ERROR_WANT_READ. But if an error is already present in the queue, SSL_get_error sees that error instead and returns SSL_ERROR_SSL. I found at least one case where the error queue may be non-empty prior to a non-blocking read[2] that involves combining mod_session_crypto (which leaves the error queue non-empty) and the third-party mod_websocket (which uses non-blocking reads), resulting in the connection being closed. I included a potential patch to mod_ssl for consideration on the bug report that simply clears the error queue prior to any of the three SSL_* calls that mod_ssl makes. An ideal fix might be to keep the error queue empty at all times (i.e. patch the APR crypto library), but I propose that this patch is more robust in a modular environment. Thanks for your consideration. [1] https://www.openssl.org/docs/manmaster/ssl/SSL_get_error.html#DESCRIPTION [2] https://bz.apache.org/bugzilla/show_bug.cgi?id=60223 Regards, Paul Spangler LabVIEW R National Instruments