Handle no async jobs in libssl

If the application has limited the size of the async pool using ASYNC_init_thread() then we could run out of jobs while trying to start a libssl io operation. However libssl was failing to handle this and treating it like a fatal error. It should not be fatal...we just need to retry when there are jobs available again. Reviewed-by: Richard Levitte <levitte@openssl.org>
openssl · May 5, 2016 · fc7f190 · fc7f190
1 parent 0eadff0
commit fc7f190
Show file tree

Hide file tree

Showing 6 changed files with 101 additions and 50 deletions.
diff --git a/apps/s_client.c b/apps/s_client.c
@@ -238,6 +238,7 @@ static void do_ssl_shutdown(SSL *ssl)
             case SSL_ERROR_WANT_READ:
             case SSL_ERROR_WANT_WRITE:
             case SSL_ERROR_WANT_ASYNC:
+            case SSL_ERROR_WANT_ASYNC_JOB:
                 /* We just do busy waiting. Nothing clever */
                 continue;
             }
@@ -2360,6 +2361,8 @@ int s_client_main(int argc, char **argv)
                     write_ssl = 0;
                 }
                 break;
+            case SSL_ERROR_WANT_ASYNC_JOB:
+                /* This shouldn't ever happen in s_client - treat as an error */
             case SSL_ERROR_SSL:
                 ERR_print_errors(bio_err);
                 goto shut;
@@ -2446,6 +2449,8 @@ int s_client_main(int argc, char **argv)
                 BIO_printf(bio_c_out, "closed\n");
                 ret = 0;
                 goto shut;
+            case SSL_ERROR_WANT_ASYNC_JOB:
+                /* This shouldn't ever happen in s_client. Treat as an error */
             case SSL_ERROR_SSL:
                 ERR_print_errors(bio_err);
                 goto shut;

diff --git a/apps/s_server.c b/apps/s_server.c
@@ -2421,6 +2421,10 @@ static int sv_body(int s, int stype, unsigned char *context)
                 case SSL_ERROR_WANT_X509_LOOKUP:
                     BIO_printf(bio_s_out, "Write BLOCK\n");
                     break;
+                case SSL_ERROR_WANT_ASYNC_JOB:
+                    /*
+                     * This shouldn't ever happen in s_server. Treat as an error
+                     */
                 case SSL_ERROR_SYSCALL:
                 case SSL_ERROR_SSL:
                     BIO_printf(bio_s_out, "ERROR\n");
@@ -2495,6 +2499,10 @@ static int sv_body(int s, int stype, unsigned char *context)
                 case SSL_ERROR_WANT_READ:
                     BIO_printf(bio_s_out, "Read BLOCK\n");
                     break;
+                case SSL_ERROR_WANT_ASYNC_JOB:
+                    /*
+                     * This shouldn't ever happen in s_server. Treat as an error
+                     */
                 case SSL_ERROR_SYSCALL:
                 case SSL_ERROR_SSL:
                     BIO_printf(bio_s_out, "ERROR\n");

diff --git a/doc/ssl/SSL_get_error.pod b/doc/ssl/SSL_get_error.pod
@@ -99,6 +99,16 @@ L<SSL_get_async_wait_fd(3)>. The TLS/SSL I/O function should be called again
 later. The function B<must> be called from the same thread that the original
 call was made from.
 
+=item SSL_ERROR_WANT_ASYNC_JOB
+
+The asynchronous job could not be started because there were no async jobs
+available in the pool (see ASYNC_init_thread(3)). This will only occur if the
+mode has been set to SSL_MODE_ASYNC using L<SSL_CTX_set_mode(3)> or
+L<SSL_set_mode(3)> and a maximum limit has been set on the async job pool
+through a call to L<ASYNC_init_thread(3)>. The application should retry the
+operation after a currently executing asynchronous operation for the current
+thread has completed.
+
 =item SSL_ERROR_SYSCALL
 
 Some I/O error occurred.  The OpenSSL error queue may contain more

diff --git a/doc/ssl/SSL_want.pod b/doc/ssl/SSL_want.pod
@@ -2,7 +2,9 @@
 
 =head1 NAME
 
-SSL_want, SSL_want_nothing, SSL_want_read, SSL_want_write, SSL_want_x509_lookup - obtain state information TLS/SSL I/O operation
+SSL_want, SSL_want_nothing, SSL_want_read, SSL_want_write, SSL_want_x509_lookup,
+SSL_want_async, SSL_want_async_job - obtain state information TLS/SSL I/O
+operation
 
 =head1 SYNOPSIS
 
@@ -13,6 +15,8 @@ SSL_want, SSL_want_nothing, SSL_want_read, SSL_want_write, SSL_want_x509_lookup
  int SSL_want_read(const SSL *ssl);
  int SSL_want_write(const SSL *ssl);
  int SSL_want_x509_lookup(const SSL *ssl);
+ int SSL_want_async(const SSL *ssl);
+ int SSL_want_async_job(const SSL *ssl);
 
 =head1 DESCRIPTION
 
@@ -65,10 +69,23 @@ SSL_CTX_set_client_cert_cb() has asked to be called again.
 A call to L<SSL_get_error(3)> should return
 SSL_ERROR_WANT_X509_LOOKUP.
 
+=item SSL_ASYNC_PAUSED
+
+An asynchronous operation partially completed and was then paused. See
+L<SSL_get_all_async_fds(3)>. A call to L<SSL_get_error(3)> should return
+SSL_ERROR_WANT_ASYNC.
+
+=item SSL_ASYNC_NO_JOBS
+
+The asynchronous job could not be started because there were no async jobs
+available in the pool (see ASYNC_init_thread(3)). A call to L<SSL_get_error(3)>
+should return SSL_ERROR_WANT_ASYNC_JOB.
+
 =back
 
-SSL_want_nothing(), SSL_want_read(), SSL_want_write(), SSL_want_x509_lookup()
-return 1, when the corresponding condition is true or 0 otherwise.
+SSL_want_nothing(), SSL_want_read(), SSL_want_write(), SSL_want_x509_lookup(),
+SSL_want_async() and SSL_want_async_job() return 1, when the corresponding
+condition is true or 0 otherwise.
 
 =head1 SEE ALSO
 

diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
@@ -884,13 +884,15 @@ __owur int SSL_extension_supported(unsigned int ext_type);
 # define SSL_READING            3
 # define SSL_X509_LOOKUP        4
 # define SSL_ASYNC_PAUSED       5
+# define SSL_ASYNC_NO_JOBS      6
 
 /* These will only be used when doing non-blocking IO */
 # define SSL_want_nothing(s)     (SSL_want(s) == SSL_NOTHING)
 # define SSL_want_read(s)        (SSL_want(s) == SSL_READING)
 # define SSL_want_write(s)       (SSL_want(s) == SSL_WRITING)
 # define SSL_want_x509_lookup(s) (SSL_want(s) == SSL_X509_LOOKUP)
 # define SSL_want_async(s)       (SSL_want(s) == SSL_ASYNC_PAUSED)
+# define SSL_want_async_job(s)   (SSL_want(s) == SSL_ASYNC_NO_JOBS)
 
 # define SSL_MAC_FLAG_READ_MAC_STREAM 1
 # define SSL_MAC_FLAG_WRITE_MAC_STREAM 2
@@ -1122,6 +1124,7 @@ DECLARE_PEM_rw(SSL_SESSION, SSL_SESSION)
 # define SSL_ERROR_WANT_CONNECT          7
 # define SSL_ERROR_WANT_ACCEPT           8
 # define SSL_ERROR_WANT_ASYNC            9
+# define SSL_ERROR_WANT_ASYNC_JOB       10
 # define SSL_CTRL_SET_TMP_DH                     3
 # define SSL_CTRL_SET_TMP_ECDH                   4
 # define SSL_CTRL_SET_TMP_DH_CB                  6

diff --git a/ssl/ssl_lib.c b/ssl/ssl_lib.c
@@ -1516,6 +1516,9 @@ static int ssl_start_async_job(SSL *s, struct ssl_async_args *args,
     case ASYNC_PAUSE:
         s->rwstate = SSL_ASYNC_PAUSED;
         return -1;
+    case ASYNC_NO_JOBS:
+        s->rwstate = SSL_ASYNC_NO_JOBS;
+        return -1;
     case ASYNC_FINISH:
         s->job = NULL;
         return ret;
@@ -2939,56 +2942,61 @@ int SSL_get_error(const SSL *s, int i)
             return (SSL_ERROR_SSL);
     }
 
-    if ((i < 0) && SSL_want_read(s)) {
-        bio = SSL_get_rbio(s);
-        if (BIO_should_read(bio))
-            return (SSL_ERROR_WANT_READ);
-        else if (BIO_should_write(bio))
-            /*
-             * This one doesn't make too much sense ... We never try to write
-             * to the rbio, and an application program where rbio and wbio
-             * are separate couldn't even know what it should wait for.
-             * However if we ever set s->rwstate incorrectly (so that we have
-             * SSL_want_read(s) instead of SSL_want_write(s)) and rbio and
-             * wbio *are* the same, this test works around that bug; so it
-             * might be safer to keep it.
-             */
-            return (SSL_ERROR_WANT_WRITE);
-        else if (BIO_should_io_special(bio)) {
-            reason = BIO_get_retry_reason(bio);
-            if (reason == BIO_RR_CONNECT)
-                return (SSL_ERROR_WANT_CONNECT);
-            else if (reason == BIO_RR_ACCEPT)
-                return (SSL_ERROR_WANT_ACCEPT);
-            else
-                return (SSL_ERROR_SYSCALL); /* unknown */
+    if (i < 0) {
+        if (SSL_want_read(s)) {
+            bio = SSL_get_rbio(s);
+            if (BIO_should_read(bio))
+                return (SSL_ERROR_WANT_READ);
+            else if (BIO_should_write(bio))
+                /*
+                 * This one doesn't make too much sense ... We never try to write
+                 * to the rbio, and an application program where rbio and wbio
+                 * are separate couldn't even know what it should wait for.
+                 * However if we ever set s->rwstate incorrectly (so that we have
+                 * SSL_want_read(s) instead of SSL_want_write(s)) and rbio and
+                 * wbio *are* the same, this test works around that bug; so it
+                 * might be safer to keep it.
+                 */
+                return (SSL_ERROR_WANT_WRITE);
+            else if (BIO_should_io_special(bio)) {
+                reason = BIO_get_retry_reason(bio);
+                if (reason == BIO_RR_CONNECT)
+                    return (SSL_ERROR_WANT_CONNECT);
+                else if (reason == BIO_RR_ACCEPT)
+                    return (SSL_ERROR_WANT_ACCEPT);
+                else
+                    return (SSL_ERROR_SYSCALL); /* unknown */
+            }
         }
-    }
 
-    if ((i < 0) && SSL_want_write(s)) {
-        bio = SSL_get_wbio(s);
-        if (BIO_should_write(bio))
-            return (SSL_ERROR_WANT_WRITE);
-        else if (BIO_should_read(bio))
-            /*
-             * See above (SSL_want_read(s) with BIO_should_write(bio))
-             */
-            return (SSL_ERROR_WANT_READ);
-        else if (BIO_should_io_special(bio)) {
-            reason = BIO_get_retry_reason(bio);
-            if (reason == BIO_RR_CONNECT)
-                return (SSL_ERROR_WANT_CONNECT);
-            else if (reason == BIO_RR_ACCEPT)
-                return (SSL_ERROR_WANT_ACCEPT);
-            else
-                return (SSL_ERROR_SYSCALL);
+        if (SSL_want_write(s)) {
+            bio = SSL_get_wbio(s);
+            if (BIO_should_write(bio))
+                return (SSL_ERROR_WANT_WRITE);
+            else if (BIO_should_read(bio))
+                /*
+                 * See above (SSL_want_read(s) with BIO_should_write(bio))
+                 */
+                return (SSL_ERROR_WANT_READ);
+            else if (BIO_should_io_special(bio)) {
+                reason = BIO_get_retry_reason(bio);
+                if (reason == BIO_RR_CONNECT)
+                    return (SSL_ERROR_WANT_CONNECT);
+                else if (reason == BIO_RR_ACCEPT)
+                    return (SSL_ERROR_WANT_ACCEPT);
+                else
+                    return (SSL_ERROR_SYSCALL);
+            }
+        }
+        if (SSL_want_x509_lookup(s)) {
+            return (SSL_ERROR_WANT_X509_LOOKUP);
+        }
+        if (SSL_want_async(s)) {
+            return SSL_ERROR_WANT_ASYNC;
+        }
+        if (SSL_want_async_job(s)) {
+            return SSL_ERROR_WANT_ASYNC_JOB;
         }
-    }
-    if ((i < 0) && SSL_want_x509_lookup(s)) {
-        return (SSL_ERROR_WANT_X509_LOOKUP);
-    }
-    if ((i < 0) && SSL_want_async(s)) {
-        return SSL_ERROR_WANT_ASYNC;
     }
 
     if (i == 0) {