php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #46685 SSL code should use select
Submitted: 2008-11-26 10:48 UTC Modified: 2011-03-18 10:41 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:1 of 1 (100.0%)
Same Version:0 (0.0%)
Same OS:0 (0.0%)
From: scott dot php at scottrix dot co dot uk Assigned:
Status: Duplicate Package: OpenSSL related
PHP Version: 5.*, 6 OS: *
Private report: No CVE-ID: None
 [2008-11-26 10:48 UTC] scott dot php at scottrix dot co dot uk
Description:
------------
We have had some problems with CPU usage using PHP test scripts on some systems at work.  Looking into the strace logs it appeared to be in a tight loop doing lots of reads mostly getting an EAGAIN error.  Looking into the problem further it seems there are two locations in the file ext/openssl/xp_ssl.c that were causing these "loops".

at line 223 (approx):

     int retry = 1;

     do {
        nr_bytes = SSL_read(sslsock->ssl_handle, buf, count);
        if (nr_bytes <= 0) {
           retry = handle_ssl_error(stream, nr_bytes, 0 TSRMLS_CC);
           stream->eof = (retry == 0 && errno != EAGAIN &&      SSL_pending(sslsock->ssl_handle));
        } else {
           /* we got the data */
           break;
        }
     } while (retry);


and the loop around the lines (429):

         if (n <= 0) {
            retry = handle_ssl_error(stream, n, 1 TSRMLS_CC);
         } else {
            break;
         }
 
They both involve SSL connections and I understand that one needs to be very careful using select in this area since the socket is a layer away from the SSL reading etc.  However, the openssl code does come with some examples on how to do this and I have created a patch that seemed to help a lot in our case that I thought someone who knows more about php internals would like to look at and maybe incorporate to some extent into future releases.  I will hopefully be able to attach the patch once I have submitted the bug.


Patches

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2008-11-26 10:50 UTC] scott dot php at scottrix dot co dot uk
OK, so I couldn't find an add attachment button, so here is the patch.

-----------------------------------------
--- ext/openssl/xp_ssl.c.orig 2008-11-18 11:34:50.000000000 +0000
+++ ext/openssl/xp_ssl.c   2008-11-18 11:34:40.000000000 +0000
@@ -223,15 +223,31 @@
      int retry = 1;

      do {
-        nr_bytes = SSL_read(sslsock->ssl_handle, buf, count);
+        int ssl_read = 0;
+        if (! SSL_pending(sslsock->ssl_handle) ) {
+           int ssl_fd = SSL_get_fd(sslsock->ssl_handle);
+           struct timeval tv = { 1 , 0};
+           fd_set fds;
+           int ret;
+
+           FD_ZERO(&fds);
+           FD_SET(ssl_fd,&fds);
+           ret = select((ssl_fd+1),&fds,NULL,NULL,&tv);
+           if ((ret > 0) && FD_ISSET(ssl_fd,&fds)) ssl_read = 1;
+        }
+        else ssl_read = 1;
+
+        if (ssl_read) {
+           nr_bytes = SSL_read(sslsock->ssl_handle, buf, count);

-        if (nr_bytes <= 0) {
-           retry = handle_ssl_error(stream, nr_bytes, 0 TSRMLS_CC);
-           stream->eof = (retry == 0 && errno != EAGAIN && !SSL_pending(sslsock->ssl_handle));
+           if (nr_bytes <= 0) {
+              retry = handle_ssl_error(stream, nr_bytes, 0 TSRMLS_CC);
+              stream->eof = (retry == 0 && errno != EAGAIN && !SSL_pending(sslsock->ssl_handle));

-        } else {
-           /* we got the data */
-           break;
+           } else {
+              /* we got the data */
+              break;
+           }
         }
      } while (retry);
   }
@@ -429,6 +445,16 @@

         if (n <= 0) {
            retry = handle_ssl_error(stream, n, 1 TSRMLS_CC);
+           if (retry) {
+              int ssl_fd = SSL_get_fd(sslsock->ssl_handle);
+              struct timeval tv = { 1 , 0};
+              fd_set fds;
+              int ret;
+
+              FD_ZERO(&fds);
+              FD_SET(ssl_fd,&fds);
+              ret = select((ssl_fd+1),&fds,NULL,NULL,&tv);
+           }
         } else {
            break;
         }
-----------------------------------------
 [2008-11-26 10:56 UTC] pajoye@php.net
What are you trying to do, in userland? Any sample scripts?

PHP can do multiplexing already and should work with ssl streams as well.

 [2008-11-26 11:07 UTC] scott dot php at scottrix dot co dot uk
The script is running tests on our webpages over ssl.  So it is spending time waiting to receive data from the webserver.  The php source code for this waiting for data from the server (over SSL) employs a busy loop on a nonblocking socket (hence the EAGAIN read errors in strace output).
 [2008-11-26 11:12 UTC] pajoye@php.net
You can do non blocking operations in php as well. 

Sorry to insist, but please provide a reproduce case (a script). I'm not saying that your patch is wrong or not necessary, but it may be possible already without changes.
 [2008-11-26 12:22 UTC] scott dot php at scottrix dot co dot uk
I don't see how the script is relevant.  You should never sit in a busy loop like this no matter what the php script is doing.
 [2008-11-26 13:07 UTC] pajoye@php.net
your choice >  no feedback.
 [2008-11-26 13:13 UTC] scott dot php at scottrix dot co dot uk
I didn't submit the patch to help me, I already have the fix for the problem.  If you aren't interested then that is fine.  Just thought I'd try and give something back to you guys.  Next time I won't bother.
 [2008-11-26 13:19 UTC] 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?
 [2008-11-26 13:36 UTC] 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.
 [2009-11-27 18:44 UTC] pajoye@php.net
assigned to Rasmus, looks like he is willing to figure out the cause and improve this patch.
 [2010-07-21 16:56 UTC] 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.
 [2010-07-21 22:19 UTC] 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 22:53 UTC] voltara at gmail dot com
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.
 [2011-03-18 10:41 UTC] cataphract@php.net
-Status: Assigned +Status: Duplicate -Assigned To: rasmus +Assigned To:
 [2011-03-18 10:41 UTC] cataphract@php.net
This was fixed in 5.3.6 (see bug #53592)
 [2011-03-18 10:41 UTC] cataphract@php.net
This was fixed in 5.3.6 (see bug #53592)
 
PHP Copyright © 2001-2020 The PHP Group
All rights reserved.
Last updated: Tue Feb 18 01:01:26 2020 UTC