Merge r1746988,r1747170 from trunk:

mod_http2: backport of 1.5.7

git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1747194 13f79535-47bb-0310-9956-ffa450edef68

diff --git a/CHANGES b/CHANGES
index 75c02610d9509ff84011c76ef6e8e7edc2a85f6f..a714286d99ab15a35bf2077eacd516b274415f3d 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,9 @@
 
 Changes with Apache 2.4.21
 
+  *) mod_http2: fixed a write after free when streams/connections
+     were aborted before tasks returned. [Stefan Eissing] 
+  
   *) mod_ssl: Correct the interaction between SSLProxyCheckPeerCN and newer
      SSLProxyCheckPeerName directives since release 2.4.5, such that disabling
      either disables both, and that enabling either triggers the new, more

diff --git a/modules/http2/h2_bucket_eos.c b/modules/http2/h2_bucket_eos.c
index 9953ca15f9403d29d21b1e4e47fa87d58cc40b62..28c34fdc310ab2809d27fbc242ef089418bd6765 100644 (file)
--- a/modules/http2/h2_bucket_eos.c
+++ b/modules/http2/h2_bucket_eos.c
@@ -38,10 +38,8 @@ static apr_status_t bucket_cleanup(void *data)
     h2_stream **pstream = data;
 
     if (*pstream) {
-        /*
-         * If bucket_destroy is called after us, this prevents
-         * bucket_destroy from trying to destroy the pool again.
-         */
+        /* If bucket_destroy is called after us, this prevents
+         * bucket_destroy from trying to destroy the stream again. */
         *pstream = NULL;
     }
     return APR_SUCCESS;
@@ -92,10 +90,13 @@ static void bucket_destroy(void *data)
 
     if (apr_bucket_shared_destroy(h)) {
         h2_stream *stream = h->stream;
+        if (stream && stream->pool) {
+            apr_pool_cleanup_kill(stream->pool, &h->stream, bucket_cleanup);
+        }
+        apr_bucket_free(h);
         if (stream) {
             h2_stream_eos_destroy(stream);
         }
-        apr_bucket_free(h);
     }
 }
 

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index f7b30fffad51e201c74275b353095e071f15d628..c09e45ac02ac52a33fda112a73358dc44fc2486f 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -200,7 +200,7 @@ static int purge_stream(void *ctx, void *val)
     h2_ihash_remove(m->spurge, stream->id);
     h2_stream_destroy(stream);
     if (task) {
-        task_destroy(m, task, 1);
+        task_destroy(m, task, 0);
     }
     return 0;
 }
@@ -349,10 +349,12 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master)
     ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, m->c, 
                   "h2_task(%s): destroy", task->id);
     /* cleanup any buffered input */
-    status = h2_task_shutdown(task, 0);
-    if (status != APR_SUCCESS){
-        ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, APLOGNO(03385) 
-                      "h2_task(%s): shutdown", task->id);
+    if (task->input.beam) {
+        status = h2_beam_shutdown(task->input.beam, APR_NONBLOCK_READ);
+        if (status != APR_SUCCESS){
+            ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, APLOGNO(03385) 
+                          "h2_task(%s): input shutdown", task->id);
+        }
     }
     
     if (called_from_master) {
@@ -446,10 +448,8 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error)
             if (rst_error) {
                 h2_task_rst(task, rst_error);
             }
-            /* FIXME: this should work, but does not 
             h2_ihash_add(m->shold, stream);
-            return;*/
-            task->input.beam = NULL;
+            return;
         }
         else {
             /* already finished */
@@ -500,6 +500,12 @@ static int task_abort_connection(void *ctx, void *val)
     if (task->c) {
         task->c->aborted = 1;
     }
+    if (task->input.beam) {
+        h2_beam_abort(task->input.beam);
+    }
+    if (task->output.beam) {
+        h2_beam_abort(task->output.beam);
+    }
     return 1;
 }
 
@@ -603,7 +609,7 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait)
         AP_DEBUG_ASSERT(h2_ihash_empty(m->shold));
         if (!h2_ihash_empty(m->spurge)) {
             ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
-                          "h2_mplx(%ld): release_join %d streams to purge", 
+                          "h2_mplx(%ld): 3. release_join %d streams to purge", 
                           m->id, (int)h2_ihash_count(m->spurge));
             purge_streams(m);
         }
@@ -1028,6 +1034,7 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
                  * parent pool / allocator) */
                 h2_ihash_remove(m->shold, stream->id);
                 h2_ihash_add(m->spurge, stream);
+                task_destroy(m, task, 0);
             }
             else {
                 ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index acd5072c639b12b56baadc14fdc10fdda7254237..3beab4b310c9dcd4db9cf7ebf2e92d025b7e3e41 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -240,7 +240,7 @@ void h2_stream_destroy(h2_stream *stream)
 void h2_stream_eos_destroy(h2_stream *stream)
 {
     h2_session_stream_done(stream->session, stream);
-    /* stream is gone */
+    /* stream possibly destroyed */
 }
 
 apr_pool_t *h2_stream_detach_pool(h2_stream *stream)

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index f505e85927f05429f052efff50882c0e305e3211..8cf1ce1ee57b6f95324e98424a320681f6f1b76b 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -88,7 +88,7 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r,
 {
     apr_status_t status = APR_SUCCESS;
     apr_bucket_brigade *bb = task->input.bb;
-    apr_table_t *t = task->request->trailers;
+    apr_table_t *t = task->request? task->request->trailers : NULL;
 
     if (task->input.chunked) {
         task->input.tmp = apr_brigade_split_ex(bb, b, task->input.tmp);
@@ -114,7 +114,7 @@ static apr_status_t input_append_eos(h2_task *task, request_rec *r)
 {
     apr_status_t status = APR_SUCCESS;
     apr_bucket_brigade *bb = task->input.bb;
-    apr_table_t *t = task->request->trailers;
+    apr_table_t *t = task->request? task->request->trailers : NULL;
 
     if (task->input.chunked) {
         if (t && !apr_is_empty_table(t)) {
@@ -151,13 +151,14 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f,
         return ap_get_brigade(f->c->input_filters, bb, mode, block, readbytes);
     }
     
-    if (f->c->aborted) {
+    if (f->c->aborted || !task->request) {
         return APR_ECONNABORTED;
     }
     
     if (!task->input.bb) {
         if (!task->input.eos_written) {
             input_append_eos(task, f->r);
+            return APR_SUCCESS;
         }
         return APR_EOF;
     }
@@ -172,11 +173,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f,
         } 
     }
     
-    while (APR_BRIGADE_EMPTY(task->input.bb)) {
-        if (task->input.eos_written) {
-            return APR_EOF;
-        }
-        
+    while (APR_BRIGADE_EMPTY(task->input.bb) && !task->input.eos) {
         /* Get more input data for our request. */
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c,
                       "h2_task(%s): get more data from mplx, block=%d, "
@@ -193,7 +190,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f,
             status = APR_EOF;
         }
         
-        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c,
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, f->c,
                       "h2_task(%s): read returned", task->id);
         if (APR_STATUS_IS_EAGAIN(status) 
             && (mode == AP_MODE_GETLINE || block == APR_BLOCK_READ)) {
@@ -202,7 +199,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f,
              * upload 100k test on test-ser.example.org hangs */
             status = APR_SUCCESS;
         }
-        else if (APR_STATUS_IS_EOF(status) && !task->input.eos_written) {
+        else if (APR_STATUS_IS_EOF(status)) {
             task->input.eos = 1;
         }
         else if (status != APR_SUCCESS) {
@@ -251,8 +248,13 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f,
         }
     }
     
-    if (!task->input.eos_written && task->input.eos) {
-        input_append_eos(task, f->r);
+    if (task->input.eos) {
+        if (!task->input.eos_written) {
+            input_append_eos(task, f->r);
+        }
+        if (APR_BRIGADE_EMPTY(task->input.bb)) {
+            return APR_EOF;
+        }
     }
 
     h2_util_bb_log(f->c, task->stream_id, APLOG_TRACE2, 
@@ -556,23 +558,6 @@ void h2_task_rst(h2_task *task, int error)
     }
 }
 
-apr_status_t h2_task_shutdown(h2_task *task, int block)
-{
-    if (task->output.beam) {
-        apr_status_t status;
-        status = h2_beam_shutdown(task->output.beam, APR_NONBLOCK_READ);
-        if (block && status == APR_EAGAIN) {
-            ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, task->c,
-                          "h2_task(%s): output shutdown waiting", task->id);
-            status = h2_beam_shutdown(task->output.beam, APR_BLOCK_READ);
-            ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, task->c,
-                          "h2_task(%s): output shutdown done", task->id);
-        }
-        return status;
-    }
-    return APR_SUCCESS;
-}
-
 /*******************************************************************************
  * Register various hooks
  */

diff --git a/modules/http2/h2_task.h b/modules/http2/h2_task.h
index dda8a562bb412610debf4f35e0f6488866a33b00..010005a374e0aba20e4b80cdd5ca43f343d74e7b 100644 (file)
--- a/modules/http2/h2_task.h
+++ b/modules/http2/h2_task.h
@@ -114,11 +114,6 @@ int h2_task_can_redo(h2_task *task);
  */
 void h2_task_rst(h2_task *task, int error);
 
-/**
- * Shuts all input/output down. Clears any buckets buffered and closes.
- */
-apr_status_t h2_task_shutdown(h2_task *task, int block);
-
 void h2_task_register_hooks(void);
 /*
  * One time, post config intialization.

diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index 020827ccc330b1dfed3c93acf533fe7034d64575..d5125b2826a076470eb3ba85212262b0e884bbca 100644 (file)
--- a/modules/http2/h2_version.h
+++ b/modules/http2/h2_version.h
@@ -26,7 +26,7 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.5.6"
+#define MOD_HTTP2_VERSION "1.5.7"
 
 /**
  * @macro
@@ -34,7 +34,7 @@
  * release. This is a 24 bit number with 8 bits for major number, 8 bits
  * for minor and 8 bits for patch. Version 1.2.3 becomes 0x010203.
  */
-#define MOD_HTTP2_VERSION_NUM 0x010506
+#define MOD_HTTP2_VERSION_NUM 0x010507
 
 
 #endif /* mod_h2_h2_version_h */