Folks, once again, please take a look at the attached code. This time
around, it actually works ;-)
This is correct only for the input BIO layer. The output BIO layer still
needs alot of work.
Although I merged SSL_read from two into a single function (against
other's preferences), it's necessary because the error cases were
impossible to grok. I did split out the bucket_read into a brigade_consume
function so that half, at least, is much easier to follow.
brigade_consume is much like brigade_flatten. However, they aren't
identical, because you can't do non-blocking reads with brigade_flatten.
The changes to the SSL input effectively force some data to be returned
if the filter read was blocking. However, we will not attempt to pull more
information off the wire than is available.
The biggest issue throughout the old code is exception handling. Of
course the existing code 'worked' in a way, as long as the flow wasn't
interrupted and no other errors occured.
I've tested some POST requests, but if someone can put this on
the httpd-test/perl-framework and exercise it, I'd really appreciate it.
Now I'll get to work on the write side of the equation.
Bill
Index: modules/ssl/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
--- modules/ssl/ssl_engine_io.c 17 Oct 2002 13:25:08 -0000 1.82
+++ modules/ssl/ssl_engine_io.c 1 Nov 2002 00:29:38 -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;
@@ -352,11 +351,86 @@
*/
#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 @@
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 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 @@
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 @@
}
}
- 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 @@
* 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, \
@@ -793,6 +844,8 @@
}
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 @@
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;
