Merge r1871810 from trunk:

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

Submitted by: icing
Reviewed by: icing, jim, steffenal

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

diff --git a/CHANGES b/CHANGES
index ded0ecce34c9e471839d64d63b36ef5619be5dce..e12ef38950142ce11dfb895e4962bfa9f017f757 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -5,6 +5,9 @@ Changes with Apache 2.4.42
      r:notes_table, r:subprocess_env_table as read-only native table alternatives
      that can be iterated over. [Eric Covener]
 
+  *) 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/STATUS b/STATUS
index 991ed406b23d59b4dfea0b6c870b2cc6b3443988..a610953ed8910bb40060965bc00c1a5e84746134 100644 (file)
--- a/STATUS
+++ b/STATUS
@@ -132,10 +132,6 @@ RELEASE SHOWSTOPPERS:
 PATCHES ACCEPTED TO BACKPORT FROM TRUNK:
   [ start all new proposals below, under PATCHES PROPOSED. ]
 
-  *) mod_http2: Fixed rare cases where a h2 worker could deadlock the main connection. 
-     trunk patch: http://svn.apache.org/r1871810
-     2.4.x patch: svn merge -c 1871810 ^/httpd/httpd/trunk
-     +1: icing, jim, steffenal(tested with 1.15.5-git)
 
 PATCHES PROPOSED TO BACKPORT FROM TRUNK:
   [ New proposals should be added at the end of the list ]

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index 9b504a58e076b451d1a3a1cac5fb071ebf04be67..2877063f0fc8da4563d194b972d26d3ee66eb2b3 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;
+        }
     }
 }
 
@@ -509,12 +512,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) {
@@ -528,7 +530,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);
     }
     
@@ -540,8 +542,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)
@@ -583,7 +585,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;
 }
 
@@ -617,15 +619,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);
     }
 }
 
@@ -678,7 +688,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")); 
         }
@@ -809,7 +819,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) {
@@ -1062,7 +1072,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;
                 }
@@ -1114,7 +1124,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 d2b63b56414e4f27aff6e27ca0b7e24dca21302e..34a478aea97a147ec4e42c2547dde860ba12f6ee 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 9dacd8bf2d69481fd0200dc3dbfa21ec568010b8..06110f5b529a731592c55b2d90ceb1c867a86abf 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 1eb262d28834446917b970a7cc2bb51fb6606cad..0e2ae5786ce2798cbc5a32180df63df1bbdd72ab 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 930cbde9c22ecc13b9572a55a2ba5e1e18b33df2..1254e9124192f54bc5baedce0960eed9363e7872 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,6 +35,6 @@
  * 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 1a3e34f3db2994aa24e68c46cd57306f8ac768e3..03c77edf8ad230700c126974c0b54d1c5fb1b80b 100644 (file)
--- a/modules/http2/mod_proxy_http2.c
+++ b/modules/http2/mod_proxy_http2.c
@@ -401,6 +401,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;