Bug #46685 [Com]: SSL code should use select

Edit report at http://bugs.php.net/bug.php?id=46685&edit=1 ID: 46685 Comment by: voltara at gmail dot com Reported by: scott dot php at scottrix dot co dot uk Summary: SSL code should use select Status: Assigned Type: Bug Package: OpenSSL related Operating System: * PHP Version: 5.*, 6 Assigned To: rasmus New Comment: I have a patch against 5.3.2, but cannot post it because of the error message "Please do not SPAM our bug system." It's the same code, but in the form of a diff applies cleanly. Previous Comments: ------------------------------------------------------------------------ [2010-07-21 22:19:21] askalski at gmail dot com I made a somewhat different patch. As far as I can tell, the busy-waiting only happens during the call to php_openssl_enable_crypto, where the socket is forced nonblocking to support the connect timeout. My patch exposes the WANT_READ/WANT_WRITE error code through the return value of handle_ssl_error(), and adds a select() to the SSL_connect loop, using a deadline-based timeout calculation. The diff is against 5.2.9, which is the version I need to fix. It seems to apply cleanly against 5.2.13, but I haven't tested it. (I would need to create a separate patch for 5.3, though.) diff --git a/ext/openssl/xp_ssl.c b/ext/openssl/xp_ssl.c index c31a505..e0a72b1 100644 --- a/ext/openssl/xp_ssl.c +++ b/ext/openssl/xp_ssl.c @@ -92,19 +92,25 @@ static int handle_ssl_error(php_stream *stream, int nr_bytes, zend_bool is_init char *ebuf = NULL, *wptr = NULL; size_t ebuf_size = 0; unsigned long code, ecode; - int retry = 1; + int retry = 0; switch(err) { case SSL_ERROR_ZERO_RETURN: /* SSL terminated (but socket may still be active) */ - retry = 0; break; case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: /* re-negotiation, or perhaps the SSL layer needs more * packets: retry in next iteration */ errno = EAGAIN; - retry = is_init ? 1 : sslsock->s.is_blocked; + if (is_init || sslsock->s.is_blocked) { + /* Return the actual error code, rather than just "1". The SSL_connect + * call is non-blocking for connect_timeout support ("blocking with timeout" + * from the outside perspective.) The caller needs to know whether to + * 'select' on read or write. + */ + retry = err; + } break; case SSL_ERROR_SYSCALL: if (ERR_peek_error() == 0) { @@ -115,7 +121,6 @@ static int handle_ssl_error(php_stream *stream, int nr_bytes, zend_bool is_init } SSL_set_shutdown(sslsock->ssl_handle, SSL_SENT_SHUTDOWN|SSL_RECEIVED_SHUTDOWN); stream->eof = 1; - retry = 0; } else { char *estr = php_socket_strerror(php_socket_errno(), NULL, 0); @@ -123,7 +128,6 @@ static int handle_ssl_error(php_stream *stream, int nr_bytes, zend_bool is_init "SSL: %s", estr); efree(estr); - retry = 0; } break; } @@ -137,7 +141,6 @@ static int handle_ssl_error(php_stream *stream, int nr_bytes, zend_bool is_init switch (ERR_GET_REASON(ecode)) { case SSL_R_NO_SHARED_CIPHER: php_error_docref(NULL TSRMLS_CC, E_WARNING, "SSL_R_NO_SHARED_CIPHER: no suitable shared cipher could be used. This could be because the server is missing an SSL certificate (local_cert context option)"); - retry = 0; break; default: @@ -175,7 +178,6 @@ static int handle_ssl_error(php_stream *stream, int nr_bytes, zend_bool is_init } } - retry = 0; errno = 0; } return retry; @@ -391,12 +393,9 @@ static inline int php_openssl_enable_crypto(php_stream *stream, php_stream_xport_crypto_param *cparam TSRMLS_DC) { - int n, retry = 1; + int n, retry; if (cparam->inputs.activate && !sslsock->ssl_active) { - float timeout = sslsock->connect_timeout.tv_sec + sslsock->connect_timeout.tv_usec / 1000000; - int blocked = sslsock->s.is_blocked; - if (!sslsock->state_set) { if (sslsock->is_client) { SSL_set_connect_state(sslsock->ssl_handle); @@ -406,36 +405,85 @@ static inline int php_openssl_enable_crypto(php_stream *stream, sslsock->state_set = 1; } - if (sslsock->is_client && SUCCESS == php_set_sock_blocking(sslsock->s.socket, 0 TSRMLS_CC)) { - sslsock->s.is_blocked = 0; - } - do { - if (sslsock->is_client) { - struct timeval tvs, tve; - struct timezone tz; + if (sslsock->is_client) { + int blocked = sslsock->s.is_blocked; + struct timeval deadline, timeout; + + /* temporarily force non-blocking, to support connection timeout */ + if (sslsock->s.is_blocked && SUCCESS == php_set_sock_blocking(sslsock->s.socket, 0 TSRMLS_CC)) { + sslsock->s.is_blocked = 0; + } + + /* set a deadline for completing negotiation, based on connect_timeout */ + gettimeofday(&deadline, NULL); + deadline.tv_sec += sslsock->connect_timeout.tv_sec; + deadline.tv_usec += sslsock->connect_timeout.tv_usec; + if (deadline.tv_usec > 1000000) { + deadline.tv_usec -= 1000000; + deadline.tv_sec++; + } + + while ((n = SSL_connect(sslsock->ssl_handle)) <= 0 && + (retry = handle_ssl_error(stream, n, 1 TSRMLS_CC))) + { + fd_set fds; + + FD_ZERO(&fds); + FD_SET(sslsock->s.socket, &fds); + + /* calculate select timeout based on deadline vs current time */ + gettimeofday(&timeout, NULL); + timeout.tv_sec = deadline.tv_sec - timeout.tv_sec; + timeout.tv_usec = deadline.tv_usec - timeout.tv_usec; + if (timeout.tv_usec < 0) { + timeout.tv_usec += 1000000; + timeout.tv_sec--; + } + + /* if there's any time left, select() on read or write */ + if (timeout.tv_sec >= 0) { + if (retry == SSL_ERROR_WANT_READ) { + n = select(sslsock->s.socket + 1, &fds, NULL, NULL, &timeout); + } + else { + n = select(sslsock->s.socket + 1, NULL, &fds, NULL, &timeout); + } + } + else { + n = 0; + } + + /* handle errors */ + if (n <= 0) { + if (n == 0) { + errno = ETIMEDOUT; + php_error_docref(NULL TSRMLS_CC, E_WARNING, "SSL: connection timeout"); + } + else { + char *estr = php_socket_strerror(php_socket_errno(), NULL, 0); + php_error_docref(NULL TSRMLS_CC, E_WARNING, + "SSL: %s", estr); + efree(estr); + } - gettimeofday(&tvs, &tz); - n = SSL_connect(sslsock->ssl_handle); - gettimeofday(&tve, &tz); + if (sslsock->s.is_blocked != blocked && SUCCESS == php_set_sock_blocking(sslsock->s.socket, blocked TSRMLS_CC)) { + sslsock->s.is_blocked = blocked; + } - timeout -= (tve.tv_sec + (float) tve.tv_usec / 1000000) - (tvs.tv_sec + (float) tvs.tv_usec / 1000000); - if (timeout < 0) { - php_error_docref(NULL TSRMLS_CC, E_WARNING, "SSL: connection timeout"); return -1; } - } else { - n = SSL_accept(sslsock->ssl_handle); } - if (n <= 0) { - retry = handle_ssl_error(stream, n, 1 TSRMLS_CC); - } else { - break; + if (sslsock->s.is_blocked != blocked && SUCCESS == php_set_sock_blocking(sslsock->s.socket, blocked TSRMLS_CC)) { + sslsock->s.is_blocked = blocked; } - } while (retry); + } + else { + n = SSL_accept(sslsock->ssl_handle); - if (sslsock->is_client && sslsock->s.is_blocked != blocked && SUCCESS == php_set_sock_blocking(sslsock->s.socket, blocked TSRMLS_CC)) { - sslsock->s.is_blocked = blocked; + if (n <= 0) { + handle_ssl_error(stream, n, 1 TSRMLS_CC); + } } if (n == 1) { ------------------------------------------------------------------------ [2010-07-21 16:56:39] askalski at gmail dot com Any progress on this ticket? We've begun encountering this bug somewhat regularly, whenever one of our third-party SOAP services has problems. The symptom is our entire cluster of PHP servers redlines the CPU for the duration of the SOAP outage. Do you still need a test case reduction? I have reproduced the issue in the latest PHP 5.2 and 5.3 releases. In the meantime, I am going to look over the original submitter's proposed patch, and use that to fix our immediate problem. ------------------------------------------------------------------------ [2009-11-27 18:44:53] pajoye@php.net assigned to Rasmus, looks like he is willing to figure out the cause and improve this patch. ------------------------------------------------------------------------ [2008-11-26 13:36:31] scott dot php at scottrix dot co dot uk Seems to me you don't understand, I could easily have found this with a code walk and would not have any test case. The way you are using the SSL libraries is (IMO and the openssl examples) wrong. Doesn't matter if it has any effects on running scripts, it is still wrong. I DO understand that you will want a test to show it has been fixed, but this isn't possible with all coding problems as I'm sure you know, the main thing from a change like this is that it doesn't break any of the existing tests and regression tests that you already have. (or anybody else code that is out there....) The only test I have that shows the problem is an strace on a running script. I don't have permission to release that script and you don't have the server pages and software that the script runs against, so would be useless anyway. I would try to create one, but I personally don't know PHP so can't. Given the problem I would imagine that any PHP script that is talking SSL and having to wait for data to come back would have the same problem (viewable by strace output). Anyway, if you NEED more to be able to apply a patch and test it, then I guess you can close this bug. The important thing for me is that I have tried to get someone upstream interested in writing better code, and that it is searchable so that others that experience the problem can find it and try my fix. ------------------------------------------------------------------------ [2008-11-26 13:19:34] pajoye@php.net You get it wrong. A patch can and will be applied when tests are provided as well. The goal is not only to be able to reproduce a possible issue, but also to be sure about the patch is trying to fix. You do not want to provide them and I'm not sure your patch is the right to do it (as already said, that does not mean it is wrong). So this bug has now in a "no feedback" status, until you have figured out that what I asked is relevent. That's a relatively simple process, isn't it? ------------------------------------------------------------------------ The remainder of the comments for this report are too long. To view the rest of the comments, please view the bug report online at http://bugs.php.net/bug.php?id=46685 -- Edit this bug report at http://bugs.php.net/bug.php?id=46685&edit=1