Re: cvs commit: httpd-2.0/modules/ssl mod_ssl.c mod_ssl.h ssl_engine_io.c ssl_engine_kernel.c

[Justin Erenkrantz <je...@apache.org>]

--On Tuesday, November 05, 2002 20:47:01 +0000 wrowe@apache.org wrote:

>   Index: ssl_engine_io.c
>   ===================================================================
>   RCS file: /home/cvs/httpd-2.0/modules/ssl/ssl_engine_io.c,v
>   retrieving revision 1.95
>   retrieving revision 1.96
>   diff -u -r1.95 -r1.96
>   --- ssl_engine_io.c	5 Nov 2002 06:38:41 -0000	1.95
>   +++ ssl_engine_io.c	5 Nov 2002 20:47:01 -0000	1.96
>   @@ -73,15 +73,15 @@
>     * remember what is in this file.  So, first, a quick overview.
>     *
>     * In this file, you will find:
>   - * - ssl_io_filter_Input    (Apache input filter)
>   - * - ssl_io_filter_Output   (Apache output filter)
>   + * - ssl_io_filter_input    (Apache input filter)
>   + * - ssl_io_filter_output   (Apache output filter)

Woo-hoo!

>   @@ -129,7 +129,15 @@
>     */
>
>    typedef struct {
>   -    SSLFilterRec *filter_ctx;
>   +    SSL                *pssl;
>   +    BIO                *pbioRead;
>   +    BIO                *pbioWrite;
>   +    ap_filter_t        *pInputFilter;
>   +    ap_filter_t        *pOutputFilter;
>   +} ssl_filter_ctx_t;
>   +
>   +typedef struct {
>   +    ssl_filter_ctx_t *filter_ctx;
>        conn_rec *c;
>        apr_bucket_brigade *bb;
>        apr_size_t length;
>   @@ -138,7 +146,7 @@
>        apr_status_t rc;
>    } bio_filter_out_ctx_t;
>
>   -static bio_filter_out_ctx_t *bio_filter_out_ctx_new(SSLFilterRec
> *filter_ctx,   +static bio_filter_out_ctx_t
> *bio_filter_out_ctx_new(ssl_filter_ctx_t *filter_ctx,
> conn_rec *c)    {
>        bio_filter_out_ctx_t *outctx = apr_palloc(c->pool,
> sizeof(*outctx));   @@ -348,7 +356,7 @@
>        char_buffer_t cbuf;
>        apr_pool_t *pool;
>        char buffer[AP_IOBUFSIZE];
>   -    SSLFilterRec *filter_ctx;
>   +    ssl_filter_ctx_t *filter_ctx;
>    } bio_filter_in_ctx_t;
>
>    /*
>   @@ -887,6 +754,74 @@
>        return APR_SUCCESS;
>    }
>
>   +
>   +static apr_status_t ssl_filter_write(ap_filter_t *f,
>   +                                     const char *data,
>   +                                     apr_size_t len)
>   +{
>   +    ssl_filter_ctx_t *filter_ctx = f->ctx;
>   +    bio_filter_out_ctx_t *outctx =
>   +           (bio_filter_out_ctx_t *)(filter_ctx->pbioWrite->ptr);
>   +    int res;
>   +
>   +    /* write SSL */
>   +    if (filter_ctx->pssl == NULL) {
>   +        return APR_EGENERAL;
>   +    }
>   +
>   +    res = SSL_write(filter_ctx->pssl, (unsigned char *)data, len);
>   +
>   +    if (res < 0) {
>   +        int ssl_err = SSL_get_error(filter_ctx->pssl, res);
>   +
>   +        if (ssl_err == SSL_ERROR_WANT_WRITE) {
>   +            /*
>   +             * If OpenSSL wants to write more, and we were nonblocking,
>   +             * report as an EAGAIN.  Otherwise loop, pushing more
>   +             * data at the network filter.
>   +             *
>   +             * (This is usually the case when the client forces an SSL
>   +             * renegotation which is handled implicitly by OpenSSL.)
>   +             */
>   +            outctx->rc = APR_EAGAIN;
>   +        }
>   +        else if (ssl_err == SSL_ERROR_SYSCALL) {
>   +            conn_rec *c =
> (conn_rec*)SSL_get_app_data(outctx->filter_ctx->pssl);
>   +    ap_log_error(APLOG_MARK, APLOG_ERR, outctx->rc, c->base_server,
>   +    "SSL filter out error writing data");

Can't we get the conn_rec from outctx->filter_ctx->pOutputFilter->c rather 
than trying to be cute and calling SSL_get_app_data?  This pattern seems to 
be happening a lot in this code and this seems like a better and much 
faster approach.

>   +        }
>   +        else /* if (ssl_err == SSL_ERROR_SSL) */ {
>   +            /*
>   +             * Log SSL errors
>   +             */
>   +            conn_rec *c = (conn_rec
> *)SSL_get_app_data(filter_ctx->pssl);   +
> ap_log_error(APLOG_MARK, APLOG_ERR, outctx->rc, c->base_server,   +
> "SSL library out error writing data");
>   +            ssl_log_ssl_error(APLOG_MARK, APLOG_ERR, c->base_server);
>   +
>   +        }
>   +        if (outctx->rc == APR_SUCCESS) {
>   +            outctx->rc = APR_EGENERAL;
>   +        }
>   +    }
>   +    else if ((apr_size_t)res != len) {

Just as a note that might want to enable partial write support in OpenSSL. 
Not sure, but it might perform better?  So, we may want to remove the res 
!= len check at some point.  -- justin