*) mod_http2: Fixed rare cases where a h2 worker could deadlock the main connection.

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

diff --git a/CHANGES b/CHANGES
index f73f111dbf3a5d06b7be42ea1d0f904c57457806..51e361974c29068fcd4a6020b81986630044fd00 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -1,6 +1,9 @@
                                                          -*- coding: utf-8 -*-
 Changes with Apache 2.5.1
 
+  *) mod_http2: Fixed rare cases where a h2 worker could deadlock the main connection. 
+     [Yann Ylavic, Stefan Eissing]
+
   *) mod_lua: Accept nil assignments to the exposed tables (r.subprocess_env, 
      r.headers_out, etc) to remove the key from the table. PR63971. 
      [Eric Covener]

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index 0c93d486646e52aa608beec39b22ad57159956e8..c3d590dd70412f30ac148b520a4e11aec1ca4ab9 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -75,13 +75,13 @@ apr_status_t h2_mplx_child_init(apr_pool_t *pool, server_rec *s)
 #define H2_MPLX_ENTER_ALWAYS(m)    \
     apr_thread_mutex_lock(m->lock)
 
-#define H2_MPLX_ENTER_MAYBE(m, lock)    \
-    if (lock) apr_thread_mutex_lock(m->lock)
+#define H2_MPLX_ENTER_MAYBE(m, dolock)    \
+    if (dolock) apr_thread_mutex_lock(m->lock)
 
-#define H2_MPLX_LEAVE_MAYBE(m, lock)    \
-    if (lock) apr_thread_mutex_unlock(m->lock)
+#define H2_MPLX_LEAVE_MAYBE(m, dolock)    \
+    if (dolock) apr_thread_mutex_unlock(m->lock)
 
-static void check_data_for(h2_mplx *m, h2_stream *stream, int lock);
+static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked);
 
 static void stream_output_consumed(void *ctx, 
                                    h2_bucket_beam *beam, apr_off_t length)
@@ -104,6 +104,7 @@ static void stream_joined(h2_mplx *m, h2_stream *stream)
 {
     ap_assert(!h2_task_has_started(stream->task) || stream->task->worker_done);
     
+    h2_ififo_remove(m->readyq, stream->id);
     h2_ihash_remove(m->shold, stream->id);
     h2_ihash_add(m->spurge, stream);
 }
@@ -125,14 +126,16 @@ static void stream_cleanup(h2_mplx *m, h2_stream *stream)
 
     h2_ihash_remove(m->streams, stream->id);
     h2_iq_remove(m->q, stream->id);
-    h2_ififo_remove(m->readyq, stream->id);
-    h2_ihash_add(m->shold, stream);
     
     if (!h2_task_has_started(stream->task) || stream->task->done_done) {
         stream_joined(m, stream);
     }
-    else if (stream->task) {
-        stream->task->c->aborted = 1;
+    else {
+        h2_ififo_remove(m->readyq, stream->id);
+        h2_ihash_add(m->shold, stream);
+        if (stream->task) {
+            stream->task->c->aborted = 1;
+        }
     }
 }
 
@@ -508,12 +511,11 @@ static void output_produced(void *ctx, h2_bucket_beam *beam, apr_off_t bytes)
     h2_stream *stream = ctx;
     h2_mplx *m = stream->session->mplx;
     
-    check_data_for(m, stream, 1);
+    check_data_for(m, stream, 0);
 }
 
 static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
 {
-    apr_status_t status = APR_SUCCESS;
     h2_stream *stream = h2_ihash_get(m->streams, stream_id);
     
     if (!stream || !stream->task || m->aborted) {
@@ -527,7 +529,7 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
         h2_beam_log(beam, m->c, APLOG_TRACE2, "out_open");
     }
     else {
-        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c,
+        ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
                       "h2_mplx(%s): out open", stream->task->id);
     }
     
@@ -539,8 +541,8 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
     
     /* we might see some file buckets in the output, see
      * if we have enough handles reserved. */
-    check_data_for(m, stream, 0);
-    return status;
+    check_data_for(m, stream, 1);
+    return APR_SUCCESS;
 }
 
 apr_status_t h2_mplx_out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam)
@@ -582,7 +584,7 @@ static apr_status_t out_close(h2_mplx *m, h2_task *task)
     status = h2_beam_close(task->output.beam);
     h2_beam_log(task->output.beam, m->c, APLOG_TRACE2, "out_close");
     output_consumed_signal(m, task);
-    check_data_for(m, stream, 0);
+    check_data_for(m, stream, 1);
     return status;
 }
 
@@ -616,15 +618,23 @@ apr_status_t h2_mplx_out_trywait(h2_mplx *m, apr_interval_time_t timeout,
     return status;
 }
 
-static void check_data_for(h2_mplx *m, h2_stream *stream, int lock)
+static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked)
 {
+    /* If m->lock is already held, we must release during h2_ififo_push()
+     * which can wait on its not_full condition, causing a deadlock because
+     * no one would then be able to acquire m->lock to empty the fifo.
+     */
+    H2_MPLX_LEAVE_MAYBE(m, mplx_is_locked);
     if (h2_ififo_push(m->readyq, stream->id) == APR_SUCCESS) {
+        H2_MPLX_ENTER_ALWAYS(m);
         apr_atomic_set32(&m->event_pending, 1);
-        H2_MPLX_ENTER_MAYBE(m, lock);
         if (m->added_output) {
             apr_thread_cond_signal(m->added_output);
         }
-        H2_MPLX_LEAVE_MAYBE(m, lock);
+        H2_MPLX_LEAVE_MAYBE(m, !mplx_is_locked);
+    }
+    else {
+        H2_MPLX_ENTER_MAYBE(m, mplx_is_locked);
     }
 }
 
@@ -677,7 +687,7 @@ apr_status_t h2_mplx_process(h2_mplx *m, struct h2_stream *stream,
         h2_ihash_add(m->streams, stream);
         if (h2_stream_is_ready(stream)) {
             /* already have a response */
-            check_data_for(m, stream, 0);
+            check_data_for(m, stream, 1);
             ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
                           H2_STRM_MSG(stream, "process, add to readyq")); 
         }
@@ -808,7 +818,7 @@ static void task_done(h2_mplx *m, h2_task *task)
             }
 
             /* more data will not arrive, resume the stream */
-            check_data_for(m, stream, 0);            
+            check_data_for(m, stream, 1);            
         }
     }
     else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) {
@@ -1060,7 +1070,7 @@ apr_status_t h2_mplx_idle(h2_mplx *m)
                                   h2_beam_is_closed(stream->output),
                                   (long)h2_beam_get_buffered(stream->output));
                     h2_ihash_add(m->streams, stream);
-                    check_data_for(m, stream, 0);
+                    check_data_for(m, stream, 1);
                     stream->out_checked = 1;
                     status = APR_EAGAIN;
                 }
@@ -1112,7 +1122,7 @@ apr_status_t h2_mplx_dispatch_master_events(h2_mplx *m,
 
 apr_status_t h2_mplx_keep_active(h2_mplx *m, h2_stream *stream)
 {
-    check_data_for(m, stream, 1);
+    check_data_for(m, stream, 0);
     return APR_SUCCESS;
 }
 

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 4fae148cf04683007efd2912e7dfc262eb69a7cb..a5cc306e365f307d5a6fdf498bf794ad065db073 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -2141,7 +2141,7 @@ apr_status_t h2_session_process(h2_session *session, int async)
                 break;
                 
             case H2_SESSION_ST_IDLE:
-                if (session->idle_until && (apr_time_now() + session->idle_delay) > session->idle_until) {
+                if (session->idle_until && (now + session->idle_delay) > session->idle_until) {
                     ap_log_cerror( APLOG_MARK, APLOG_TRACE1, status, c,
                                   H2_SSSN_MSG(session, "idle, timeout reached, closing"));
                     if (session->idle_delay) {

diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c
index 0059edf121db696f60e81746071b63b35aaa71d8..895e0ffe52cfa38e63a79f2374993f63fd09724c 100644 (file)
--- a/modules/http2/h2_util.c
+++ b/modules/http2/h2_util.c
@@ -638,15 +638,6 @@ apr_status_t h2_fifo_term(h2_fifo *fifo)
     apr_status_t rv;
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         fifo->aborted = 1;
-        apr_thread_mutex_unlock(fifo->lock);
-    }
-    return rv;
-}
-
-apr_status_t h2_fifo_interrupt(h2_fifo *fifo)
-{
-    apr_status_t rv;
-    if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         apr_thread_cond_broadcast(fifo->not_empty);
         apr_thread_cond_broadcast(fifo->not_full);
         apr_thread_mutex_unlock(fifo->lock);
@@ -710,10 +701,6 @@ static apr_status_t fifo_push(h2_fifo *fifo, void *elem, int block)
 {
     apr_status_t rv;
     
-    if (fifo->aborted) {
-        return APR_EOF;
-    }
-
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         rv = fifo_push_int(fifo, elem, block);
         apr_thread_mutex_unlock(fifo->lock);
@@ -754,10 +741,6 @@ static apr_status_t fifo_pull(h2_fifo *fifo, void **pelem, int block)
 {
     apr_status_t rv;
     
-    if (fifo->aborted) {
-        return APR_EOF;
-    }
-    
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         rv = pull_head(fifo, pelem, block);
         apr_thread_mutex_unlock(fifo->lock);
@@ -946,15 +929,6 @@ apr_status_t h2_ififo_term(h2_ififo *fifo)
     apr_status_t rv;
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         fifo->aborted = 1;
-        apr_thread_mutex_unlock(fifo->lock);
-    }
-    return rv;
-}
-
-apr_status_t h2_ififo_interrupt(h2_ififo *fifo)
-{
-    apr_status_t rv;
-    if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         apr_thread_cond_broadcast(fifo->not_empty);
         apr_thread_cond_broadcast(fifo->not_full);
         apr_thread_mutex_unlock(fifo->lock);
@@ -1018,10 +992,6 @@ static apr_status_t ififo_push(h2_ififo *fifo, int id, int block)
 {
     apr_status_t rv;
     
-    if (fifo->aborted) {
-        return APR_EOF;
-    }
-
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         rv = ififo_push_int(fifo, id, block);
         apr_thread_mutex_unlock(fifo->lock);
@@ -1062,10 +1032,6 @@ static apr_status_t ififo_pull(h2_ififo *fifo, int *pi, int block)
 {
     apr_status_t rv;
     
-    if (fifo->aborted) {
-        return APR_EOF;
-    }
-    
     if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
         rv = ipull_head(fifo, pi, block);
         apr_thread_mutex_unlock(fifo->lock);
@@ -1088,10 +1054,6 @@ static apr_status_t ififo_peek(h2_ififo *fifo, h2_ififo_peek_fn *fn, void *ctx,
     apr_status_t rv;
     int id;
     
-    if (fifo->aborted) {
-        return APR_EOF;
-    }
-    
     if (APR_SUCCESS == (rv = apr_thread_mutex_lock(fifo->lock))) {
         if (APR_SUCCESS == (rv = ipull_head(fifo, &id, block))) {
             switch (fn(id, ctx)) {
@@ -1117,39 +1079,40 @@ apr_status_t h2_ififo_try_peek(h2_ififo *fifo, h2_ififo_peek_fn *fn, void *ctx)
     return ififo_peek(fifo, fn, ctx, 0);
 }
 
-apr_status_t h2_ififo_remove(h2_ififo *fifo, int id)
+static apr_status_t ififo_remove(h2_ififo *fifo, int id)
 {
-    apr_status_t rv;
+    int rc, i;
     
     if (fifo->aborted) {
         return APR_EOF;
     }
 
-    if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
-        int i, rc;
-        int e;
-        
-        rc = 0;
-        for (i = 0; i < fifo->count; ++i) {
-            e = fifo->elems[inth_index(fifo, i)];
-            if (e == id) {
-                ++rc;
-            }
-            else if (rc) {
-                fifo->elems[inth_index(fifo, i-rc)] = e;
-            }
-        }
-        if (rc) {
-            fifo->count -= rc;
-            if (fifo->count + rc == fifo->nelems) {
-                apr_thread_cond_broadcast(fifo->not_full);
-            }
-            rv = APR_SUCCESS;
+    rc = 0;
+    for (i = 0; i < fifo->count; ++i) {
+        int e = fifo->elems[inth_index(fifo, i)];
+        if (e == id) {
+            ++rc;
         }
-        else {
-            rv = APR_EAGAIN;
+        else if (rc) {
+            fifo->elems[inth_index(fifo, i-rc)] = e;
         }
-        
+    }
+    if (!rc) {
+        return APR_EAGAIN;
+    }
+    fifo->count -= rc;
+    if (fifo->count + rc == fifo->nelems) {
+        apr_thread_cond_broadcast(fifo->not_full);
+    }
+    return APR_SUCCESS;
+}
+
+apr_status_t h2_ififo_remove(h2_ififo *fifo, int id)
+{
+    apr_status_t rv;
+    
+    if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) {
+        rv = ififo_remove(fifo, id);
         apr_thread_mutex_unlock(fifo->lock);
     }
     return rv;

diff --git a/modules/http2/h2_util.h b/modules/http2/h2_util.h
index e24fc6046b7cd455ed3b36246326f8d17bed7bca..6c676eb4b19d7d86138eb24980d8b9ce36a94de3 100644 (file)
--- a/modules/http2/h2_util.h
+++ b/modules/http2/h2_util.h
@@ -209,7 +209,6 @@ apr_status_t h2_fifo_create(h2_fifo **pfifo, apr_pool_t *pool, int capacity);
 apr_status_t h2_fifo_set_create(h2_fifo **pfifo, apr_pool_t *pool, int capacity);
 
 apr_status_t h2_fifo_term(h2_fifo *fifo);
-apr_status_t h2_fifo_interrupt(h2_fifo *fifo);
 
 int h2_fifo_count(h2_fifo *fifo);
 
@@ -280,7 +279,6 @@ apr_status_t h2_ififo_create(h2_ififo **pfifo, apr_pool_t *pool, int capacity);
 apr_status_t h2_ififo_set_create(h2_ififo **pfifo, apr_pool_t *pool, int capacity);
 
 apr_status_t h2_ififo_term(h2_ififo *fifo);
-apr_status_t h2_ififo_interrupt(h2_ififo *fifo);
 
 int h2_ififo_count(h2_ififo *fifo);
 

diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index ac7db08b91f0c936ffeca4f238d0c1bf590aa37c..0b73b3b46cca49c25761369c375b77d1658851fc 100644 (file)
--- a/modules/http2/h2_version.h
+++ b/modules/http2/h2_version.h
@@ -27,7 +27,7 @@
  * @macro
  * Version number of the http2 module as c string
  */
-#define MOD_HTTP2_VERSION "1.15.4"
+#define MOD_HTTP2_VERSION "1.15.5"
 
 /**
  * @macro
@@ -35,7 +35,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 0x010f04
+#define MOD_HTTP2_VERSION_NUM 0x010f05
 
 
 #endif /* mod_h2_h2_version_h */

diff --git a/modules/http2/h2_workers.c b/modules/http2/h2_workers.c
index 699f533f804813ae73ab47672b7641b58f27110a..52f1a70369dcebdd24c3096eac5dc1cd6f8a72b0 100644 (file)
--- a/modules/http2/h2_workers.c
+++ b/modules/http2/h2_workers.c
@@ -269,7 +269,6 @@ static apr_status_t workers_pool_cleanup(void *data)
         }
 
         h2_fifo_term(workers->mplxs);
-        h2_fifo_interrupt(workers->mplxs);
 
         cleanup_zombies(workers);
     }

diff --git a/modules/http2/mod_proxy_http2.c b/modules/http2/mod_proxy_http2.c
index 220870799021000b1a5b78f79903efb6dcd8077b..83ae431c87594ee91802e795cd12550589ada914 100644 (file)
--- a/modules/http2/mod_proxy_http2.c
+++ b/modules/http2/mod_proxy_http2.c
@@ -403,6 +403,14 @@ run_connect:
          */
         apr_table_setn(ctx->p_conn->connection->notes,
                        "proxy-request-alpn-protos", "h2");
+        if (ctx->p_conn->ssl_hostname) {
+            ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, ctx->owner,
+                          "set SNI to %s for (%s)",
+                          ctx->p_conn->ssl_hostname,
+                          ctx->p_conn->hostname);
+            apr_table_setn(ctx->p_conn->connection->notes,
+                           "proxy-request-hostname", ctx->p_conn->ssl_hostname);
+        }
     }
 
     if (ctx->master->aborted) goto cleanup;