On the 2.4.x branch:

Merged /httpd/httpd/trunk:r1791790,1792195



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

7 files changed, 112 insertions, 98 deletions

diff --git a/CHANGES b/CHANGES
index 02b6e1753a..16c927344a 100644
--- a/CHANGES
+++ b/CHANGES
@@ -2,19 +2,13 @@
 
 Changes with Apache 2.4.26
 
+  *) mod_http2: fixed possible deadlock that could occur when connections were 
+     terminated early with ongoing streams. Fixed possible hanger with timeout
+     on race when connection considers itself idle. [Stefan Eissing]  
+
   *) mod_http2: MaxKeepAliveRequests now limits the number of times a 
      slave connection gets reused. [Stefan Eissing]
 
-  *) mod_http2: client streams that lack the EOF flag get now forcefully
-     closed with a RST_STREAM (NO_ERROR) when the request has been answered.
-     [Stefan Eissing]     
-
-  *) mod_http2: only when 'HttpProtocolOptions Unsafe' is configured, will
-     control characters in response headers or trailers be forwarded to the
-     client. Otherwise, in the default configuration, a request will eiher 
-     fail with status 500 or the stream will be reset by a RST_STREAM frame. 
-     [Stefan Eissing]
-     
   *) mod_brotli: Add a new module for dynamic Brotli (RFC 7932) compression.
      [Evgeny Kotkov]
 
diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 872c67544d..fff3f5dc93 100644
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -496,6 +496,28 @@ static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool)
     }
 }
 
+static void recv_buffer_cleanup(h2_bucket_beam *beam, h2_beam_lock *bl)
+{
+    if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) {
+        apr_bucket_brigade *bb = beam->recv_buffer;
+        apr_off_t bblen = 0;
+        
+        beam->recv_buffer = NULL;
+        apr_brigade_length(bb, 0, &bblen);
+        beam->received_bytes += bblen;
+        
+        /* need to do this unlocked since bucket destroy might 
+         * call this beam again. */
+        if (bl) leave_yellow(beam, bl);
+        apr_brigade_destroy(bb);
+        if (bl) enter_yellow(beam, bl);
+        
+        if (beam->cons_ev_cb) { 
+            beam->cons_ev_cb(beam->cons_ctx, beam);
+        }
+    }
+}
+
 static apr_status_t beam_cleanup(void *data)
 {
     h2_bucket_beam *beam = data;
@@ -526,10 +548,7 @@ static apr_status_t beam_cleanup(void *data)
             pool_kill(beam, beam->recv_pool, beam_recv_cleanup);
             beam->recv_pool = NULL;
         }
-        if (beam->recv_buffer) {
-            apr_brigade_destroy(beam->recv_buffer);
-            beam->recv_buffer = NULL;
-        }
+        recv_buffer_cleanup(beam, NULL);
     }
     else {
         beam->recv_buffer = NULL;
@@ -697,9 +716,7 @@ apr_status_t h2_beam_leave(h2_bucket_beam *beam)
     h2_beam_lock bl;
     
     if (enter_yellow(beam, &bl) == APR_SUCCESS) {
-        if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) {
-            apr_brigade_cleanup(beam->recv_buffer);
-        }
+        recv_buffer_cleanup(beam, &bl);
         beam->aborted = 1;
         beam_close(beam);
         leave_yellow(beam, &bl);
@@ -932,9 +949,7 @@ apr_status_t h2_beam_receive(h2_bucket_beam *beam,
     if (enter_yellow(beam, &bl) == APR_SUCCESS) {
 transfer:
         if (beam->aborted) {
-            if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) {
-                apr_brigade_cleanup(beam->recv_buffer);
-            }
+            recv_buffer_cleanup(beam, &bl);
             status = APR_ECONNABORTED;
             goto leave;
         }
diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index e0704b7a88..0520dc5914 100644
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -378,8 +378,10 @@ static int report_stream_iter(void *ctx, void *val) {
     h2_stream *stream = val;
     h2_task *task = stream->task;
     ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
-                  H2_STRM_MSG(stream, "started=%d, scheduled=%d, ready=%d"), 
-                  !!stream->task, stream->scheduled, h2_stream_is_ready(stream));
+                  H2_STRM_MSG(stream, "started=%d, scheduled=%d, ready=%d, "
+                              "out_buffer=%ld"), 
+                  !!stream->task, stream->scheduled, h2_stream_is_ready(stream),
+                  (long)h2_beam_get_buffered(stream->output));
     if (task) {
         ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, /* NO APLOGNO */
                       H2_STRM_MSG(stream, "->03198: %s %s %s"
@@ -850,9 +852,6 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
             /* stream not cleaned up, stay around */
             ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
                           H2_STRM_MSG(stream, "task_done, stream open")); 
-            /* more data will not arrive, resume the stream */
-            check_data_for(m, stream, 0);
-            
             if (stream->input) {
                 h2_beam_leave(stream->input);
                 h2_beam_mutex_disable(stream->input);
@@ -860,6 +859,9 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn)
             if (stream->output) {
                 h2_beam_mutex_disable(stream->output);
             }
+
+            /* more data will not arrive, resume the stream */
+            check_data_for(m, stream, 0);            
         }
     }
     else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) {
@@ -1001,43 +1003,75 @@ apr_status_t h2_mplx_idle(h2_mplx *m)
     H2_MPLX_ENTER(m);
 
     scount = h2_ihash_count(m->streams);
-    if (scount > 0 && m->tasks_active) {
-        /* If we have streams in connection state 'IDLE', meaning
-         * all streams are ready to sent data out, but lack
-         * WINDOW_UPDATEs. 
-         * 
-         * This is ok, unless we have streams that still occupy
-         * h2 workers. As worker threads are a scarce resource, 
-         * we need to take measures that we do not get DoSed.
-         * 
-         * This is what we call an 'idle block'. Limit the amount 
-         * of busy workers we allow for this connection until it
-         * well behaves.
-         */
-        now = apr_time_now();
-        m->last_idle_block = now;
-        if (m->limit_active > 2 
-            && now - m->last_limit_change >= m->limit_change_interval) {
-            if (m->limit_active > 16) {
-                m->limit_active = 16;
-            }
-            else if (m->limit_active > 8) {
-                m->limit_active = 8;
-            }
-            else if (m->limit_active > 4) {
-                m->limit_active = 4;
+    if (scount > 0) {
+        if (m->tasks_active) {
+            /* If we have streams in connection state 'IDLE', meaning
+             * all streams are ready to sent data out, but lack
+             * WINDOW_UPDATEs. 
+             * 
+             * This is ok, unless we have streams that still occupy
+             * h2 workers. As worker threads are a scarce resource, 
+             * we need to take measures that we do not get DoSed.
+             * 
+             * This is what we call an 'idle block'. Limit the amount 
+             * of busy workers we allow for this connection until it
+             * well behaves.
+             */
+            now = apr_time_now();
+            m->last_idle_block = now;
+            if (m->limit_active > 2 
+                && now - m->last_limit_change >= m->limit_change_interval) {
+                if (m->limit_active > 16) {
+                    m->limit_active = 16;
+                }
+                else if (m->limit_active > 8) {
+                    m->limit_active = 8;
+                }
+                else if (m->limit_active > 4) {
+                    m->limit_active = 4;
+                }
+                else if (m->limit_active > 2) {
+                    m->limit_active = 2;
+                }
+                m->last_limit_change = now;
+                ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
+                              "h2_mplx(%ld): decrease worker limit to %d",
+                              m->id, m->limit_active);
             }
-            else if (m->limit_active > 2) {
-                m->limit_active = 2;
+            
+            if (m->tasks_active > m->limit_active) {
+                status = unschedule_slow_tasks(m);
             }
-            m->last_limit_change = now;
+        }
+        else if (!h2_iq_empty(m->q)) {
             ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
-                          "h2_mplx(%ld): decrease worker limit to %d",
-                          m->id, m->limit_active);
+                          "h2_mplx(%ld): idle, but %d streams to process",
+                          m->id, (int)h2_iq_count(m->q));
+            status = APR_EAGAIN;
         }
-        
-        if (m->tasks_active > m->limit_active) {
-            status = unschedule_slow_tasks(m);
+        else {
+            /* idle, have streams, but no tasks active. what are we waiting for?
+             * WINDOW_UPDATEs from client? */
+            h2_stream *stream = NULL;
+            
+            ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c,
+                          "h2_mplx(%ld): idle, no tasks ongoing, %d streams",
+                          m->id, (int)h2_ihash_count(m->streams));
+            h2_ihash_shift(m->streams, (void**)&stream, 1);
+            if (stream && stream->output) {
+                /* FIXME: this looks like a race between the session thinking
+                 * it is idle and the EOF on a stream not being sent.
+                 * Signal to caller to leave IDLE state.
+                 */
+                ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c,
+                              H2_STRM_MSG(stream, "output closed=%d, mplx idle"
+                              ", out has %ld bytes buffered"),
+                              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);
+                status = APR_EAGAIN;
+            }
         }
     }
     register_if_needed(m);
@@ -1231,8 +1265,8 @@ int h2_mplx_awaits_data(h2_mplx *m)
     if (h2_ihash_empty(m->streams)) {
         waiting = 0;
     }
-    if ((h2_fifo_count(m->readyq) == 0) 
-        && h2_iq_empty(m->q) && !m->tasks_active) {
+    else if (!m->tasks_active && !h2_fifo_count(m->readyq)
+             && h2_iq_empty(m->q)) {
         waiting = 0;
     }
 
diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index b22f7135b1..bc24bb41cd 100644
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -1050,7 +1050,8 @@ static ssize_t stream_data_cb(nghttp2_session *ng2s,
             break;
             
         case APR_ECONNRESET:
-            return 0;
+        case APR_ECONNABORTED:
+            return NGHTTP2_ERR_CALLBACK_FAILURE;
             
         case APR_EAGAIN:
             /* If there is no data available, our session will automatically
@@ -2083,7 +2084,11 @@ apr_status_t h2_session_process(h2_session *session, int async)
                      * That gives us the chance to check for MPMQ_STOPPING often. 
                      */
                     status = h2_mplx_idle(session->mplx);
-                    if (status != APR_SUCCESS) {
+                    if (status == APR_EAGAIN) {
+                        dispatch_event(session, H2_SESSION_EV_DATA_READ, 0, NULL);
+                        break;
+                    }
+                    else if (status != APR_SUCCESS) {
                         dispatch_event(session, H2_SESSION_EV_CONN_ERROR, 
                                        H2_ERR_ENHANCE_YOUR_CALM, "less is more");
                     }
diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c
index f4c3aab3a9..01b506ac85 100644
--- a/modules/http2/h2_util.c
+++ b/modules/http2/h2_util.c
@@ -372,39 +372,6 @@ size_t h2_ihash_shift(h2_ihash_t *ih, void **buffer, size_t max)
     return ctx.len;
 }
 
-typedef struct {
-    h2_ihash_t *ih;
-    int *buffer;
-    size_t max;
-    size_t len;
-} icollect_ctx;
-
-static int icollect_iter(void *x, void *val)
-{
-    icollect_ctx *ctx = x;
-    if (ctx->len < ctx->max) {
-        ctx->buffer[ctx->len++] = *((int*)((char *)val + ctx->ih->ioff));
-        return 1;
-    }
-    return 0;
-}
-
-size_t h2_ihash_ishift(h2_ihash_t *ih, int *buffer, size_t max)
-{
-    icollect_ctx ctx;
-    size_t i;
-    
-    ctx.ih = ih;
-    ctx.buffer = buffer;
-    ctx.max = max;
-    ctx.len = 0;
-    h2_ihash_iter(ih, icollect_iter, &ctx);
-    for (i = 0; i < ctx.len; ++i) {
-        h2_ihash_remove(ih, buffer[i]);
-    }
-    return ctx.len;
-}
-
 /*******************************************************************************
  * iqueue - sorted list of int
  ******************************************************************************/
diff --git a/modules/http2/h2_util.h b/modules/http2/h2_util.h
index 4b901bfe68..35163716ad 100644
--- a/modules/http2/h2_util.h
+++ b/modules/http2/h2_util.h
@@ -68,7 +68,6 @@ void h2_ihash_remove_val(h2_ihash_t *ih, void *val);
 void h2_ihash_clear(h2_ihash_t *ih);
 
 size_t h2_ihash_shift(h2_ihash_t *ih, void **buffer, size_t max);
-size_t h2_ihash_ishift(h2_ihash_t *ih, int *buffer, size_t max);
 
 /*******************************************************************************
  * iqueue - sorted list of int with user defined ordering
diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index 72ac248c8d..5452296e80 100644
--- 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.10.2"
+#define MOD_HTTP2_VERSION "1.10.3"
 
 /**
  * @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 0x010a02
+#define MOD_HTTP2_VERSION_NUM 0x010a03
 
 
 #endif /* mod_h2_h2_version_h */