]> granicus.if.org Git - apache/commitdiff
Completely refactor the BIO-side client input handling for the SSL library.
authorWilliam A. Rowe Jr <wrowe@apache.org>
Fri, 1 Nov 2002 08:35:19 +0000 (08:35 +0000)
committerWilliam A. Rowe Jr <wrowe@apache.org>
Fri, 1 Nov 2002 08:35:19 +0000 (08:35 +0000)
  Should eliminate many false spurious interrupt detected errors.

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@97367 13f79535-47bb-0310-9956-ffa450edef68

CHANGES
modules/ssl/ssl_engine_io.c

diff --git a/CHANGES b/CHANGES
index 384f801ed2274a6cfbe1b48704e72c3e6d04d30e..022b7e0cdb89106852506c28113921285c9bc3ce 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,5 +1,9 @@
 Changes with Apache 2.0.44
 
+  *) Gracefully handly retry situations in the SSL input filter,
+     by following the SSL libraries' retry semantics.
+     [William Rowe]
+
   *) Terminate CGI scripts when the client connection drops.  This
      fix only applies to some normal paths in mod_cgi.  mod_cgid
      is still busted.  PR 8388  [Jeff Trawick]
index 45796220194bbc0e544a08a84b5fa60c7fd4ed16..1d3f0a5144aafa7912647ae200ef32ce416bbba3 100644 (file)
@@ -301,7 +301,6 @@ typedef struct {
     ap_input_mode_t mode;
     apr_read_type_e block;
     apr_bucket_brigade *bb;
-    apr_bucket *bucket;
     char_buffer_t cbuf;
 } BIO_bucket_in_t;
 
@@ -352,11 +351,86 @@ static int char_buffer_write(char_buffer_t *buffer, char *in, int inl)
  */
 #define BIO_bucket_in_ptr(bio) (BIO_bucket_in_t *)bio->ptr
 
+static apr_status_t brigade_consume(apr_bucket_brigade *bb,
+                                    apr_read_type_e block,
+                                    char *c, apr_size_t *len)
+{
+    apr_size_t actual = 0;
+    while (!APR_BRIGADE_EMPTY(bb)) {
+        apr_bucket *b = APR_BRIGADE_FIRST(bb);
+        const char *str;
+        apr_status_t status;
+        apr_size_t str_len;
+        apr_size_t consume;
+
+        if (APR_BUCKET_IS_EOS(b)) {
+            *len = 0;
+            return APR_EOF;
+        }
+
+        status = apr_bucket_read(b, &str, &str_len, block);
+        
+        if (status != APR_SUCCESS) {
+            if (status == APR_EOF) {
+                /* This stream bucket was consumed */
+                APR_BUCKET_REMOVE(b);
+                apr_bucket_destroy(b);
+                continue;
+            }
+
+            *len = actual;
+            return status;
+        }
+
+        if (str_len > 0) {
+            /* Do not block once some data has been consumed */
+            block = APR_NONBLOCK_READ;
+
+            /* Assure we don't overflow. */
+            consume = (str_len + actual > *len) ? *len - actual : str_len;
+
+            memcpy(c, str, consume);
+
+            c += consume;
+            actual += consume;
+        }
+
+        if (b->start >= 0) {
+            if (consume >= b->length) {
+                /* This physical bucket was consumed */
+                APR_BUCKET_REMOVE(b);
+                apr_bucket_destroy(b);
+            }
+            else {
+                /* Only part of this physical bucket was consumed */
+                b->start += consume;
+                b->length -= consume;
+            }
+        }
+
+        /* This could probably be actual == *len, but be safe from stray
+         * photons. */
+        if (actual >= *len) {
+            break;
+        }
+    }
+
+    *len = actual;
+    return APR_SUCCESS;
+}
+
 static int bio_bucket_in_read(BIO *bio, char *in, int inl)
 {
     BIO_bucket_in_t *inbio = BIO_bucket_in_ptr(bio);
+    apr_read_type_e block = inbio->block;
     SSLConnRec *sslconn = myConnConfig(inbio->f->c);
-    int len = 0;
+
+    inbio->rc = APR_SUCCESS;
+
+    /* OpenSSL catches this case, so should we. */
+    if (!in)
+        return 0;
 
     /* XXX: flush here only required for SSLv2;
      * OpenSSL calls BIO_flush() at the appropriate times for
@@ -366,93 +440,64 @@ static int bio_bucket_in_read(BIO *bio, char *in, int inl)
         BIO_bucket_flush(inbio->wbio);
     }
 
-    inbio->rc = APR_SUCCESS;
-    
-    /* first use data already read from socket if any */
-    if ((len = char_buffer_read(&inbio->cbuf, in, inl))) {
-        if ((len <= inl) || inbio->mode == AP_MODE_GETLINE) {
-            return len;
-        }
-        inl -= len;
-    }
+    BIO_clear_retry_flags(bio);
 
-    while (1) {
-        const char *buf;
-        apr_size_t buf_len = 0;
+    if (!inbio->bb) {
+        inbio->rc = APR_EOF;
+        return -1;
+    }
 
-        if (inbio->bucket) {
-            /* all of the data in this bucket has been read,
-             * so we can delete it now.
-             */
-            apr_bucket_delete(inbio->bucket);
-            inbio->bucket = NULL;
-        }
+    if (APR_BRIGADE_EMPTY(inbio->bb)) {
 
-        if (APR_BRIGADE_EMPTY(inbio->bb)) {
-            /* We will always call with READBYTES even if the user wants
-             * GETLINE.
-             */
-            inbio->rc = ap_get_brigade(inbio->f->next, inbio->bb,
-                                       AP_MODE_READBYTES, inbio->block, 
-                                       inl);
+        inbio->rc = ap_get_brigade(inbio->f->next, inbio->bb,
+                                   AP_MODE_READBYTES, block, 
+                                   inl);
 
-            if ((inbio->rc != APR_SUCCESS) || APR_BRIGADE_EMPTY(inbio->bb))
-            {
-                break;
-            }
+        /* Not a problem, there was simply no data ready yet.
+         */
+        if (APR_STATUS_IS_EAGAIN(inbio->rc) || APR_STATUS_IS_EINTR(inbio->rc)
+               || (inbio->rc == APR_SUCCESS && APR_BRIGADE_EMPTY(inbio->bb))) {
+            BIO_set_retry_read(bio);
+            return 0;
         }
 
-        inbio->bucket = APR_BRIGADE_FIRST(inbio->bb);
-
-        inbio->rc = apr_bucket_read(inbio->bucket,
-                                    &buf, &buf_len, inbio->block);
-
         if (inbio->rc != APR_SUCCESS) {
-            apr_bucket_delete(inbio->bucket);
-            inbio->bucket = NULL;
-            return len;
+            /* Unexpected errors discard the brigade */
+            apr_brigade_cleanup(inbio->bb);
+            inbio->bb = NULL;
+            return -1;
         }
+    }
 
-        if (buf_len) {
-            /* Protected against len > MAX_INT 
-             */
-            if ((len + (int)buf_len) >= inl || (int)buf_len < 0) {
-                /* we have enough to fill the buffer.
-                 * append if we have already written to the buffer.
-                 */
-                int nibble = inl - len;
-                char *value = (char *)buf+nibble;
-
-                int length = buf_len - nibble;
-                memcpy(in + len, buf, nibble);
+    inbio->rc = brigade_consume(inbio->bb, block, in, &inl);
 
-                char_buffer_write(&inbio->cbuf, value, length);
-                len += nibble;
+    if (inbio->rc == APR_SUCCESS) {
+        return inl;
+    }
 
-                break;
-            }
-            else {
-                /* not enough data,
-                 * save what we have and try to read more.
-                 */
-                memcpy(in + len, buf, buf_len);
-                len += buf_len;
-            }
-        }
+    if (APR_STATUS_IS_EAGAIN(inbio->rc) 
+            || APR_STATUS_IS_EINTR(inbio->rc)) {
+        BIO_set_retry_read(bio);
+        return inl;
+    }
+        
+    /* Unexpected errors and APR_EOF clean out the brigade.
+     * Subsequent calls will return APR_EOF.
+     */
+    apr_brigade_cleanup(inbio->bb);
+    inbio->bb = NULL;
 
-        if (inbio->mode == AP_MODE_GETLINE) {
-            /* only read from the socket once in getline mode.
-             * since callers buffer size is likely much larger than
-             * the request headers.  caller can always come back for more
-             * if first read didn't get all the headers.
-             */
-            break;
-        }
+    if ((inbio->rc == APR_EOF) && inl) {
+        /* Provide the results of this read pass,
+         * without resetting the BIO retry_read flag
+         */
+        return inl;
     }
 
-    return len;
+    return -1;
 }
 
+
 static BIO_METHOD bio_bucket_in_method = {
     BIO_TYPE_MEM,
     "APR input bucket brigade",
@@ -475,42 +520,6 @@ static BIO_METHOD *BIO_s_in_bucket(void)
 
 static const char ssl_io_filter[] = "SSL/TLS Filter";
 
-static int ssl_io_hook_read(SSL *ssl, char *buf, int len)
-{
-    int rc;
-
-    if (ssl == NULL) {
-        return -1;
-    }
-
-    rc = SSL_read(ssl, buf, len);
-
-    if (rc <= 0) {
-        int ssl_err = SSL_get_error(ssl, rc);
-
-        if (ssl_err == SSL_ERROR_WANT_READ) {
-            /*
-             * Simulate an EINTR in case OpenSSL wants to read more.
-             * (This is usually the case when the client forces an SSL
-             * renegotation which is handled implicitly by OpenSSL.)
-             */
-            errno = EINTR;
-            rc = 0; /* non fatal error */
-        }
-        else if (ssl_err == SSL_ERROR_SSL) {
-            /*
-             * Log SSL errors
-             */
-            conn_rec *c = (conn_rec *)SSL_get_app_data(ssl);
-            ap_log_error(APLOG_MARK, APLOG_ERR, 0, c->base_server,
-                    "SSL error on reading data");
-            ssl_log_ssl_error(APLOG_MARK, APLOG_ERR, c->base_server);
-        }
-    }
-
-    return rc;
-}
-
 static int ssl_io_hook_write(SSL *ssl, unsigned char *buf, int len)
 {
     int rc;
@@ -640,11 +649,6 @@ static apr_status_t ssl_io_filter_Output(ap_filter_t *f,
     return status;
 }
 
-/*
- * ctx->cbuf is leftover plaintext from ssl_io_input_getline,
- * use what we have there first if any,
- * then go for more by calling ssl_io_hook_read.
- */
 static apr_status_t ssl_io_input_read(ssl_io_input_ctx_t *ctx,
                                       char *buf,
                                       apr_size_t *len)
@@ -668,26 +672,75 @@ static apr_status_t ssl_io_input_read(ssl_io_input_ctx_t *ctx,
         }
     }
 
-    rc = ssl_io_hook_read(ctx->frec->pssl, buf + bytes, wanted - bytes);
+    while (1) {
 
-    if (rc > 0) {
-        *len += rc;
-        if (ctx->inbio.mode == AP_MODE_SPECULATIVE) {
-            char_buffer_write(&ctx->cbuf, buf, rc);
-        }
-    }
-    else if ((rc == 0) && (errno != EINTR)) {
-        /* something other than SSL_ERROR_WANT_READ */
-        return APR_EOF;
-    }
-    else if ((rc == -1) && (ctx->inbio.rc == APR_SUCCESS)) {
-        /*
-         * bucket read from socket was successful,
-         * but there was an error within the ssl runtime.
+        /* SSL_read may not read because we haven't taken enough data
+         * from the stack.  This is where we want to consider all of
+         * the blocking and SPECULATIVE semantics
          */
-        return APR_EGENERAL;
-    }
+        rc = SSL_read(ctx->frec->pssl, buf + bytes, wanted - bytes);
 
+        if (rc > 0) {
+            *len += rc;
+            if (ctx->inbio.mode == AP_MODE_SPECULATIVE) {
+                char_buffer_write(&ctx->cbuf, buf, rc);
+            }
+            return ctx->inbio.rc;
+        }
+        else if (rc == 0) {
+            /* If EAGAIN, we will loop given a blocking read,
+             * otherwise consider ourselves at EOF.
+             */
+            if (APR_STATUS_IS_EAGAIN(ctx->inbio.rc)
+                    || APR_STATUS_IS_EINTR(ctx->inbio.rc)) {
+                if (ctx->inbio.block == APR_NONBLOCK_READ) {
+                    break;
+                }
+            }
+            else {
+                ctx->inbio.rc = APR_EOF;
+                break;
+            }
+        }
+        else /* (rc < 0) */ {
+            int ssl_err = SSL_get_error(ctx->frec->pssl, rc);
+
+            if (ssl_err == SSL_ERROR_WANT_READ) {
+                /*
+                 * If OpenSSL wants to read more, and we were nonblocking,
+                 * report as an EAGAIN.  Otherwise loop, pulling more
+                 * data from network filter.
+                 *
+                 * (This is usually the case when the client forces an SSL
+                 * renegotation which is handled implicitly by OpenSSL.)
+                 */
+                if (ctx->inbio.block == APR_NONBLOCK_READ) {
+                    ctx->inbio.rc = APR_EAGAIN;
+                    break; /* non fatal error */
+                }
+            }
+            if (ssl_err == SSL_ERROR_SYSCALL) {
+                conn_rec *c = (conn_rec *)SSL_get_app_data(ctx->frec->pssl);
+                ap_log_error(APLOG_MARK, APLOG_ERR, ctx->inbio.rc, c->base_server,
+                            "SSL filter error reading data");
+                break;
+            }
+            else if (ssl_err == SSL_ERROR_SSL) {
+                /*
+                 * Log SSL errors and any unexpected conditions.
+                 */
+                conn_rec *c = (conn_rec *)SSL_get_app_data(ctx->frec->pssl);
+                ap_log_error(APLOG_MARK, APLOG_ERR, ctx->inbio.rc, c->base_server,
+                            "SSL library error reading data");
+                ssl_log_ssl_error(APLOG_MARK, APLOG_ERR, c->base_server);
+
+                if (ctx->inbio.rc == APR_SUCCESS) {
+                    ctx->inbio.rc = APR_EGENERAL;
+                }
+                break;
+            }
+        }
+    }
     return ctx->inbio.rc;
 }
 
@@ -745,13 +798,11 @@ static apr_status_t ssl_io_input_getline(ssl_io_input_ctx_t *ctx,
  * we use a flag in the conn_rec->conn_vector now.  The fake request just
  * gets the request back to the Apache core so that a response can be sent.
  *
- * We should probably use a 0.9 request, but the BIO bucket code is calling
- * socket_bucket_read one extra time with all 0.9 requests from the client.
- * Until that is resolved, continue to use a 1.0 request, just like we
- * always have.
+ * To avoid calling back for more data from the socket, use an HTTP/0.9
+ * request, and tack on an EOS bucket.
  */
 #define HTTP_ON_HTTPS_PORT \
-    "GET / HTTP/1.0"
+    "GET /" CRLF
 
 #define HTTP_ON_HTTPS_PORT_BUCKET(alloc) \
     apr_bucket_immortal_create(HTTP_ON_HTTPS_PORT, \
@@ -792,6 +843,8 @@ static apr_status_t ssl_io_filter_error(ap_filter_t *f,
         return status;
     }
 
+    APR_BRIGADE_INSERT_TAIL(bb, bucket);
+    bucket = apr_bucket_eos_create(f->c->bucket_alloc);
     APR_BRIGADE_INSERT_TAIL(bb, bucket);
 
     return APR_SUCCESS;
@@ -887,7 +940,6 @@ static void ssl_io_input_add_filter(SSLFilterRec *frec, conn_rec *c,
     ctx->inbio.wbio = frec->pbioWrite;
     ctx->inbio.f = frec->pInputFilter;
     ctx->inbio.bb = apr_brigade_create(c->pool, c->bucket_alloc);
-    ctx->inbio.bucket = NULL;
     ctx->inbio.cbuf.length = 0;
 
     ctx->cbuf.length = 0;