I spent alot of time over the last several days trying to shoot down the
obscure bugs in SSL lockup. Some might have to do with the DBM and
apr_global_mutex bugs. Others weren't so obvious ("Spurious Interrupt,
perhaps one of those OpenSSL confusions?")
The answer is we rarely know which end is up with the existing input
SSL filter. We aren't tracking our own APR result codes correctly,
and nothing percolates properly. We don't look (today) correctly at
the failure and retry cases.
The patch attached is a rewrite of the SSL input filtering. Please take
a close look at the new input filter. I've collapsed two functions that
didn't need to be split so it's easier to follow, now.
Bill
? foobar.patch
Index: ssl_engine_io.c
===================================================================
RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.82
diff -u -r1.82 ssl_engine_io.c
--- ssl_engine_io.c 17 Oct 2002 13:25:08 -0000 1.82
+++ ssl_engine_io.c 30 Oct 2002 23:52:21 -0000
@@ -301,7 +301,6 @@
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;
@@ -358,6 +357,12 @@
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
* the other protocols.
@@ -366,11 +371,11 @@
BIO_bucket_flush(inbio->wbio);
}
- inbio->rc = APR_SUCCESS;
-
+ BIO_clear_retry_flags(bio);
+
/* 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) {
+ if (len >= inl) {
return len;
}
inl -= len;
@@ -379,13 +384,11 @@
while (1) {
const char *buf;
apr_size_t buf_len = 0;
+ apr_bucket *bucket;
- 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 (!inbio->bb) {
+ inbio->rc = APR_EGENERAL;
+ return -1;
}
if (APR_BRIGADE_EMPTY(inbio->bb)) {
@@ -396,21 +399,58 @@
AP_MODE_READBYTES, inbio->block,
inl);
- if ((inbio->rc != APR_SUCCESS) || APR_BRIGADE_EMPTY(inbio->bb))
- {
+ if (APR_STATUS_IS_EAGAIN(inbio->rc)
+ || APR_STATUS_IS_EINTR(inbio->rc)) {
+ break;
+ }
+
+ if (inbio->rc != APR_SUCCESS) {
+ /* Unexpected errors discard the brigade */
+ apr_brigade_cleanup(inbio->bb);
+ inbio->bb = NULL;
+ return -1;
+ }
+
+ if (APR_BRIGADE_EMPTY(inbio->bb)) {
+ /* Treat an empty brigade as a retry case */
break;
}
}
- inbio->bucket = APR_BRIGADE_FIRST(inbio->bb);
+ bucket = APR_BRIGADE_FIRST(inbio->bb);
+
+ if (APR_BUCKET_IS_EOS(bucket)) {
+ if (len) {
+ /* Leave this EOS on the brigade */
+ break;
+ }
+ /* Consume the EOS and return 0, we already
+ * reset the retry flag above
+ */
+ apr_brigade_cleanup(inbio->bb);
+ inbio->bb = NULL;
+ return 0;
+ }
- inbio->rc = apr_bucket_read(inbio->bucket,
+ inbio->rc = apr_bucket_read(bucket,
&buf, &buf_len, inbio->block);
+ if (inbio->rc == APR_EOF) {
+ /* This bucket is consumed, continue to the next */
+ apr_bucket_delete(bucket);
+ continue;
+ }
+
+ if (APR_STATUS_IS_EAGAIN(inbio->rc)
+ || APR_STATUS_IS_EINTR(inbio->rc)) {
+ break;
+ }
+
if (inbio->rc != APR_SUCCESS) {
- apr_bucket_delete(inbio->bucket);
- inbio->bucket = NULL;
- return len;
+ /* Unexpected errors discard the buffer */
+ apr_brigade_cleanup(inbio->bb);
+ inbio->bb = NULL;
+ return -1;
}
if (buf_len) {
@@ -439,20 +479,23 @@
len += buf_len;
}
}
+ }
- 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 (len == 0) {
+ /* here we presume everything is a retry case, real
+ * EOFs must be handled above
+ */
+ BIO_set_retry_read(bio);
+ }
+ else if (len > 0) {
+ /* Ignore result codes if we have some results. */
+ inbio->rc = APR_SUCCESS;
}
return len;
}
+
static BIO_METHOD bio_bucket_in_method = {
BIO_TYPE_MEM,
"APR input bucket brigade",
@@ -475,42 +518,6 @@
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;
@@ -642,8 +649,7 @@
/*
* 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.
+ * use what we have there first if any, then go for more.
*/
static apr_status_t ssl_io_input_read(ssl_io_input_ctx_t *ctx,
char *buf,
@@ -668,7 +674,7 @@
}
}
- rc = ssl_io_hook_read(ctx->frec->pssl, buf + bytes, wanted - bytes);
+ rc = SSL_read(ctx->frec->pssl, buf + bytes, wanted - bytes);
if (rc > 0) {
*len += rc;
@@ -676,16 +682,38 @@
char_buffer_write(&ctx->cbuf, buf, rc);
}
}
- else if ((rc == 0) && (errno != EINTR)) {
- /* something other than SSL_ERROR_WANT_READ */
+ else if (rc == 0) {
+ /* EOF unless EAGAIN */
+ if (APR_STATUS_IS_EAGAIN(ctx->inbio.rc)
+ || APR_STATUS_IS_EINTR(ctx->inbio.rc)) {
+ return ctx->inbio.rc;
+ }
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.
- */
- return APR_EGENERAL;
+ else /* (rc < 0) */ {
+ int ssl_err = SSL_get_error(ctx->frec->pssl, 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.)
+ */
+ return APR_EAGAIN; /* non fatal error */
+ }
+ if (ssl_err == SSL_ERROR_SSL) {
+ /*
+ * Log SSL errors
+ */
+ 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;
+ }
+ }
}
return ctx->inbio.rc;
@@ -887,7 +915,6 @@
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;
