- eliminated the use of ssl_log - it used to cause seg faults during cleanup

since the conn_rec will no longer be valid.
- eliminated the "for (;;)" processing loop in ssl_io_filter_Output() -
we'll have to do that in churn_output() if required, so that any remaining
OpenSSL data (if available) is transferred before we call the
CloseConnection.
- Any remaining data in SSL should be cleaned up ideally in the
APR_BUCKET_IS_EOS() processing stage itself, as we close the SSL connection
here.

Submitted by:	Madhusudan Mathihalli <madhusudan_mathihalli@hp.com>
Reviewed by:	William Rowe

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

diff --git a/ssl_engine_io.c b/ssl_engine_io.c
index 8e03e52597c58964cf109981359c28da371b916c..bdc0a2c8944be29d26ab7a1d3dc7b17e7eba70a3 100644 (file)
--- a/ssl_engine_io.c
+++ b/ssl_engine_io.c
@@ -323,17 +323,6 @@ apr_status_t ssl_io_filter_Output(ap_filter_t *f,apr_bucket_brigade *pbbIn)
 {
     SSLFilterRec *pRec=f->ctx;
     apr_bucket *pbktIn;
-    conn_rec *c = SSL_get_app_data (pRec->pssl);
-
-    if (!c) {
-        /* if this happens we have already called ssl_hook_CloseConnection
-         * if we dont return here, this routine will segv
-         * XXX: this doesnt seem right, ssl_hook_CloseConnection probably 
-         * is being called to early, but as the README:TODO says:
-         * "Cleanup ssl_engine_io.c !!"
-         */
-        return APR_EOF;
-    }
 
     APR_BRIGADE_FOREACH(pbktIn,pbbIn) {
        const char *data;
@@ -341,44 +330,23 @@ apr_status_t ssl_io_filter_Output(ap_filter_t *f,apr_bucket_brigade *pbbIn)
        apr_status_t ret;
 
        if(APR_BUCKET_IS_EOS(pbktIn)) {
-           /* XXX: demote to debug */
-            ssl_log(c->base_server, SSL_LOG_INFO, "EOS in output");
+           if ((ret = churn_output(pRec)) != APR_SUCCESS)
+            {
+                ap_log_error(
+                    APLOG_MARK,APLOG_ERR,ret,NULL, "Error in churn_output");
+               return ret;
+            }
 
-            if (ssl_hook_CloseConnection (pRec) != APR_SUCCESS)
-                ssl_log(c->base_server, SSL_LOG_INFO,
+            if ((ret = ssl_hook_CloseConnection (pRec)) != APR_SUCCESS)
+                ap_log_error(APLOG_MARK,APLOG_ERR,ret,NULL,
                     "Error in ssl_hook_CloseConnection");
-#if 0
-           /* XXX: dubious - does this always terminate? Does it return the right thing? */
-           for( ; ; ) {
-               ret=churn_output(pRec);
-               if(ret != APR_SUCCESS)
-                   return ret;
-                /* XXX - Verify if passing &len is okay for churn - TBD */
-                len = 0;
-               ret=churn(pRec,APR_NONBLOCK_READ,&len);
-               if(ret != APR_SUCCESS) {
-                   if(ret == APR_EOF)
-                       return APR_SUCCESS;
-                   else
-                       return ret;
-               }
-           }
-#endif
            break;
        }
 
        if(APR_BUCKET_IS_FLUSH(pbktIn)) {
-           /* assume that churn will flush (or already has) if there's output */
-            /* XXX - Verify if passing &len is okay for churn - TBD */
-            ssl_log(c->base_server, SSL_LOG_INFO, "FLUSH in output");
-            len = 0;
-           ret=churn(pRec,APR_NONBLOCK_READ,&len);
-           if(ret != APR_SUCCESS)
-               return ret;
            continue;
        }
 
-        ssl_log(c->base_server, SSL_LOG_INFO, "DATA in output");
        /* read filter */
        apr_bucket_read(pbktIn,&data,&len,APR_BLOCK_READ);
 
@@ -386,7 +354,6 @@ apr_status_t ssl_io_filter_Output(ap_filter_t *f,apr_bucket_brigade *pbbIn)
         n = ssl_io_hook_write(pRec->pssl, (unsigned char *)data, len);
         assert (n == len);
 
-
        /* churn the state machine */
        ret=churn_output(pRec);
        if(ret != APR_SUCCESS)
@@ -421,6 +388,12 @@ apr_status_t ssl_io_filter_Input(ap_filter_t *f,apr_bucket_brigade *pbbOut,
     return APR_SUCCESS;
 }
 
+apr_status_t ssl_io_filter_cleanup (void *data)
+{
+    SSL *ssl = (SSL *)data;
+    return APR_SUCCESS;
+}
+
 void ssl_io_filter_init(conn_rec *c, SSL *ssl)
 {
     SSLFilterRec *filter;
@@ -434,6 +407,10 @@ void ssl_io_filter_init(conn_rec *c, SSL *ssl)
     filter->pbioWrite       = BIO_new(BIO_s_mem());
     SSL_set_bio(ssl, filter->pbioRead, filter->pbioWrite);
     filter->pssl            = ssl;
+
+    apr_pool_cleanup_register(c->pool, (void*)ssl,
+                              ssl_io_filter_cleanup, apr_pool_cleanup_null);
+
     return;
 }
 

diff --git a/ssl_engine_kernel.c b/ssl_engine_kernel.c
index 7bbf9cb0ef4be6f13d27b740d0f4dac0d7ef2a6f..1357071b82b19119d7430d78d5cd65c54051e71f 100644 (file)
--- a/ssl_engine_kernel.c
+++ b/ssl_engine_kernel.c
@@ -397,7 +397,9 @@ apr_status_t ssl_hook_CloseConnection(SSLFilterRec *filter)
      * calls of Apache it would lead to an I/O error in the browser due
      * to the fact that the SSL layer was already removed by us.
      */
+#if 0 /* XXX We've flush the OpenSSL buffer and not connection buffer - TBD */
     ap_flush_conn(conn);
+#endif
 
     /*
      * Now close the SSL layer of the connection. We've to take