Merge of r1757985,r1758003 from trunk

mod_http2: fixed bug in stream shutdown, support for nghttp2 invalid header callback from 1.14.0 and onwards.

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

diff --git a/CHANGES b/CHANGES
index fc47f4e9830497d7449750560aa616298a185c3f..090262cd4dabd9f3437dc265201b56a4885d2b2f 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -2,6 +2,14 @@
 
 Changes with Apache 2.4.24
 
+  *) mod_http2: if configured with nghttp2 1.14.0 and onward, invalid request
+     headers will immediately reset the stream with a PROTOCOL error. Feature
+     logged by module on startup as 'INVHD' in info message.
+     [Stefan Eissing]
+     
+  *) mod_http2: fixed handling of stream buffers during shutdown.
+     [Stefan Eissing]
+     
   *) mod_reqtimeout: Fix body timeout disabling for CONNECT requests to avoid
      triggering mod_proxy_connect's AH01018 once the tunnel is established.
      [Yann Ylavic]

diff --git a/modules/http2/config2.m4 b/modules/http2/config2.m4
index a77ad8089980c12aead7b6e3b56cf316692d0f87..cccbbc8536f5aadd34e7cdcb36a7d7161794b2a4 100644 (file)
--- a/modules/http2/config2.m4
+++ b/modules/http2/config2.m4
@@ -154,6 +154,9 @@ dnl # nghttp2 >= 1.3.0: access to stream weights
 dnl # nghttp2 >= 1.5.0: changing stream priorities
       AC_CHECK_FUNCS([nghttp2_session_change_stream_priority], 
         [APR_ADDTO(MOD_CPPFLAGS, ["-DH2_NG2_CHANGE_PRIO"])], [])
+dnl # nghttp2 >= 1.14.0: invalid header callback
+      AC_CHECK_FUNCS([nghttp2_session_callbacks_set_on_invalid_header_callback], 
+        [APR_ADDTO(MOD_CPPFLAGS, ["-DH2_NG2_INVALID_HEADER_CB"])], [])
     else
       AC_MSG_WARN([nghttp2 version is too old])
     fi

diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c
index 6a1e74d3e7fb5d3c532c8d18cd055c4b1227952e..1338ba68b011f3b24f6cf5495f13deefa015aa3f 100644 (file)
--- a/modules/http2/h2_bucket_beam.c
+++ b/modules/http2/h2_bucket_beam.c
@@ -550,6 +550,11 @@ apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block,
         if (clear_buffers) {
             r_purge_reds(beam);
             h2_blist_cleanup(&beam->red);
+            if (!bl.mutex && beam->green) {
+                /* not protected, may process green in red call */
+                apr_brigade_destroy(beam->green);
+                beam->green = NULL;
+            }
         }
         beam_close(beam);
         
@@ -984,6 +989,18 @@ int h2_beam_empty(h2_bucket_beam *beam)
     return empty;
 }
 
+int h2_beam_holds_proxies(h2_bucket_beam *beam)
+{
+    int has_proxies = 1;
+    h2_beam_lock bl;
+    
+    if (enter_yellow(beam, &bl) == APR_SUCCESS) {
+        has_proxies = !H2_BPROXY_LIST_EMPTY(&beam->proxies);
+        leave_yellow(beam, &bl);
+    }
+    return has_proxies;
+}
+
 int h2_beam_closed(h2_bucket_beam *beam)
 {
     return beam->closed;

diff --git a/modules/http2/h2_bucket_beam.h b/modules/http2/h2_bucket_beam.h
index fcafb063ddd441efcab6c34afcaae0c04b77e8c3..8ccd8a3aaa7f1675334744e192d8899dde237583 100644 (file)
--- a/modules/http2/h2_bucket_beam.h
+++ b/modules/http2/h2_bucket_beam.h
@@ -272,6 +272,13 @@ int h2_beam_closed(h2_bucket_beam *beam);
  */
 int h2_beam_empty(h2_bucket_beam *beam);
 
+/**
+ * Determine if beam has handed out proxy buckets that are not destroyed. 
+ * 
+ * Call from red or green side.
+ */
+int h2_beam_holds_proxies(h2_bucket_beam *beam);
+
 /**
  * Abort the beam. Will cleanup any buffered buckets and answer all send
  * and receives with APR_ECONNABORTED.

diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c
index c1de2699b32023cba24564b2360837db63e48d58..e340c9a6788915adf0e4169784b909273874ce1c 100644 (file)
--- a/modules/http2/h2_mplx.c
+++ b/modules/http2/h2_mplx.c
@@ -197,15 +197,19 @@ static int purge_stream(void *ctx, void *val)
     h2_mplx *m = ctx;
     h2_stream *stream = val;
     int stream_id = stream->id;
-    h2_task *task = h2_ihash_get(m->tasks, stream_id);
+    h2_task *task;
+
+    /* stream_cleanup clears all buffers and destroys any buckets
+     * that might hold references into task space. Needs to be done
+     * before task destruction, otherwise it will complain. */
+    h2_stream_cleanup(stream);
     
-    h2_ihash_remove(m->spurge, stream_id);
-    h2_stream_destroy(stream);
+    task = h2_ihash_get(m->tasks, stream_id);    
     if (task) {
         task_destroy(m, task, 1);
     }
-    /* FIXME: task_destroy() might in some twisted way place the
-     * stream in the spurge hash again. Remove it last. */
+    
+    h2_stream_destroy(stream);
     h2_ihash_remove(m->spurge, stream_id);
     return 0;
 }
@@ -373,7 +377,10 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master)
         if (status != APR_SUCCESS){
             ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, 
                           APLOGNO(03385) "h2_task(%s): output shutdown "
-                          "incomplete", task->id);
+                          "incomplete, beam empty=%d, holds proxies=%d", 
+                          task->id,
+                          h2_beam_empty(task->output.beam),
+                          h2_beam_holds_proxies(task->output.beam));
         }
     }
     

diff --git a/modules/http2/h2_proxy_session.c b/modules/http2/h2_proxy_session.c
index 79a2e82e82d0d9363e31b8d29ab46bd0c42c5581..22f37e08578e20b2aff6ee90be23819565338493 100644 (file)
--- a/modules/http2/h2_proxy_session.c
+++ b/modules/http2/h2_proxy_session.c
@@ -94,7 +94,7 @@ static int proxy_pass_brigade(apr_bucket_alloc_t *bucket_alloc,
      * issues in case of error returned below. */
     apr_brigade_cleanup(bb);
     if (status != APR_SUCCESS) {
-        ap_log_cerror(APLOG_MARK, APLOG_ERR, status, origin, APLOGNO(03357)
+        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, status, origin, APLOGNO(03357)
                       "pass output failed to %pI (%s)",
                       p_conn->addr, p_conn->hostname);
     }

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index e6214088aa73a197af035f76ae2c6bc59b37efef..31ae303c9d0fc274b1ca261d7dd9c1fcc17e5c3e 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -604,6 +604,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, session->c,
                       "h2_stream(%ld-%d): send_data_cb, reading stream",
                       session->id, (int)stream_id);
+        apr_brigade_cleanup(session->bbtmp);
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     else if (len != length) {
@@ -611,6 +612,7 @@ static int on_send_data_cb(nghttp2_session *ngh2,
                       "h2_stream(%ld-%d): send_data_cb, wanted %ld bytes, "
                       "got %ld from stream",
                       session->id, (int)stream_id, (long)length, (long)len);
+        apr_brigade_cleanup(session->bbtmp);
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
     
@@ -621,8 +623,8 @@ static int on_send_data_cb(nghttp2_session *ngh2,
     }
     
     status = h2_conn_io_pass(&session->io, session->bbtmp);
-        
     apr_brigade_cleanup(session->bbtmp);
+    
     if (status == APR_SUCCESS) {
         stream->out_data_frames++;
         stream->out_data_octets += length;
@@ -655,6 +657,27 @@ static int on_frame_send_cb(nghttp2_session *ngh2,
     return 0;
 }
 
+#ifdef H2_NG2_INVALID_HEADER_CB
+static int on_invalid_header_cb(nghttp2_session *ngh2, 
+                                const nghttp2_frame *frame, 
+                                const uint8_t *name, size_t namelen, 
+                                const uint8_t *value, size_t valuelen, 
+                                uint8_t flags, void *user_data)
+{
+    h2_session *session = user_data;
+    if (APLOGcdebug(session->c)) {
+        ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03456)
+                      "h2_session(%ld-%d): denying stream with invalid header "
+                      "'%s: %s'", session->id, (int)frame->hd.stream_id,
+                      apr_pstrndup(session->pool, (const char *)name, namelen),
+                      apr_pstrndup(session->pool, (const char *)value, valuelen));
+    }
+    return nghttp2_submit_rst_stream(session->ngh2, NGHTTP2_FLAG_NONE,
+                                     frame->hd.stream_id, 
+                                     NGHTTP2_PROTOCOL_ERROR);
+}
+#endif
+
 #define NGH2_SET_CALLBACK(callbacks, name, fn)\
 nghttp2_session_callbacks_set_##name##_callback(callbacks, fn)
 
@@ -677,7 +700,9 @@ static apr_status_t init_callbacks(conn_rec *c, nghttp2_session_callbacks **pcb)
     NGH2_SET_CALLBACK(*pcb, on_header, on_header_cb);
     NGH2_SET_CALLBACK(*pcb, send_data, on_send_data_cb);
     NGH2_SET_CALLBACK(*pcb, on_frame_send, on_frame_send_cb);
-
+#ifdef H2_NG2_INVALID_HEADER_CB
+    NGH2_SET_CALLBACK(*pcb, on_invalid_header, on_invalid_header_cb);
+#endif
     return APR_SUCCESS;
 }
 
@@ -726,7 +751,7 @@ static apr_status_t h2_session_shutdown_notice(h2_session *session)
     if (status == APR_SUCCESS) {
         status = h2_conn_io_flush(&session->io);
     }
-    ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO()
+    ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, session->c, APLOGNO(03457)
                   "session(%ld): sent shutdown notice", session->id);
     return status;
 }
@@ -775,6 +800,7 @@ static apr_status_t h2_session_shutdown(h2_session *session, int error,
     dispatch_event(session, H2_SESSION_EV_LOCAL_GOAWAY, error, msg);
     
     if (force_close) {
+        apr_brigade_cleanup(session->bbtmp);
         h2_mplx_abort(session->mplx);
     }
     
@@ -1987,6 +2013,7 @@ static void dispatch_event(h2_session *session, h2_session_event_t ev,
     }
     
     if (session->state == H2_SESSION_ST_DONE) {
+        apr_brigade_cleanup(session->bbtmp);
         h2_mplx_abort(session->mplx);
     }
 }

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index 9b4c017567726d79d5a69af7c467ee5f7d5eb93c..f457eb49ff8e725c41a4aef3ae182309c855f1c1 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -429,6 +429,7 @@ apr_status_t h2_stream_write_data(h2_stream *stream,
 {
     conn_rec *c = stream->session->c;
     apr_status_t status = APR_SUCCESS;
+    apr_bucket_brigade *tmp;
     
     AP_DEBUG_ASSERT(stream);
     if (!stream->input) {
@@ -461,18 +462,15 @@ apr_status_t h2_stream_write_data(h2_stream *stream,
         }
     }
     
-    if (!stream->tmp) {
-        stream->tmp = apr_brigade_create(stream->pool, c->bucket_alloc);
-    }
-    apr_brigade_write(stream->tmp, NULL, NULL, data, len);
+    tmp = apr_brigade_create(stream->pool, c->bucket_alloc);
+    apr_brigade_write(tmp, NULL, NULL, data, len);
     if (eos) {
-        APR_BRIGADE_INSERT_TAIL(stream->tmp, 
-                                apr_bucket_eos_create(c->bucket_alloc)); 
+        APR_BRIGADE_INSERT_TAIL(tmp, apr_bucket_eos_create(c->bucket_alloc)); 
         close_input(stream);
     }
+    status = h2_beam_send(stream->input, tmp, APR_BLOCK_READ);
+    apr_brigade_destroy(tmp);
     
-    status = h2_beam_send(stream->input, stream->tmp, APR_BLOCK_READ);
-    apr_brigade_cleanup(stream->tmp);
     stream->in_data_frames++;
     stream->in_data_octets += len;
     

diff --git a/modules/http2/h2_stream.h b/modules/http2/h2_stream.h
index 7edfae754ab6b3141389ba770ad8fee829af4ebd..4394c706f0bfb083afd4a81647e50e049d60f4f6 100644 (file)
--- a/modules/http2/h2_stream.h
+++ b/modules/http2/h2_stream.h
@@ -56,7 +56,6 @@ struct h2_stream {
     struct h2_response *last_sent;
     struct h2_bucket_beam *output;
     apr_bucket_brigade *buffer;
-    apr_bucket_brigade *tmp;
     apr_array_header_t *files;  /* apr_file_t* we collected during I/O */
 
     int rst_error;              /* stream error for RST_STREAM */

diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c
index 75f376cf0ff4c6e549f98548a0cdef21d2ea80e0..318943ae4a83f929a7f7f8849deb44b71f926889 100644 (file)
--- a/modules/http2/h2_task.c
+++ b/modules/http2/h2_task.c
@@ -92,7 +92,7 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r,
     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);
+        apr_bucket_brigade *tmp = apr_brigade_split_ex(bb, b, NULL);
         if (t && !apr_is_empty_table(t)) {
             status = apr_brigade_puts(bb, NULL, NULL, "0\r\n");
             apr_table_do(input_ser_header, task, t, NULL);
@@ -101,7 +101,8 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r,
         else {
             status = apr_brigade_puts(bb, NULL, NULL, "0\r\n\r\n");
         }
-        APR_BRIGADE_CONCAT(bb, task->input.tmp);
+        APR_BRIGADE_CONCAT(bb, tmp);
+        apr_brigade_destroy(tmp);
     }
     else if (r && t && !apr_is_empty_table(t)){
         /* trailers passed in directly. */

diff --git a/modules/http2/h2_task.h b/modules/http2/h2_task.h
index 76e8780d2ac06d20203450f367953b2d406f6338..f13366e26cdf79a67ed7e8138ea177e4a4ee0247 100644 (file)
--- a/modules/http2/h2_task.h
+++ b/modules/http2/h2_task.h
@@ -61,7 +61,6 @@ struct h2_task {
     struct {
         struct h2_bucket_beam *beam;
         apr_bucket_brigade *bb;
-        apr_bucket_brigade *tmp;
         apr_read_type_e block;
         unsigned int chunked : 1;
         unsigned int eos : 1;

diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index b62b6c7a77f746acbc3d5f3cff0529b75d23e389..e65ea7aa1f57bb89d5cfd9af72296b9c39ee9cdc 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.6.0"
+#define MOD_HTTP2_VERSION "1.6.1"
 
 /**
  * @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 0x010600
+#define MOD_HTTP2_VERSION_NUM 0x010601
 
 
 #endif /* mod_h2_h2_version_h */

diff --git a/modules/http2/mod_http2.c b/modules/http2/mod_http2.c
index 06410612933ad3d9ab5daf0b961637ba7daabc97..65ea3d6aa4e294a317b352279e83264a6ada23d3 100644 (file)
--- a/modules/http2/mod_http2.c
+++ b/modules/http2/mod_http2.c
@@ -60,6 +60,7 @@ static int h2_h2_fixups(request_rec *r);
 typedef struct {
     unsigned int change_prio : 1;
     unsigned int sha256 : 1;
+    unsigned int inv_headers : 1;
 } features;
 
 static features myfeats;
@@ -84,16 +85,17 @@ static int h2_post_config(apr_pool_t *p, apr_pool_t *plog,
     const char *mod_h2_init_key = "mod_http2_init_counter";
     nghttp2_info *ngh2;
     apr_status_t status;
-    const char *sep = "";
     
     (void)plog;(void)ptemp;
 #ifdef H2_NG2_CHANGE_PRIO
     myfeats.change_prio = 1;
-    sep = "+";
 #endif
 #ifdef H2_OPENSSL
     myfeats.sha256 = 1;
 #endif
+#ifdef H2_NG2_INVALID_HEADER_CB
+    myfeats.inv_headers = 1;
+#endif
     
     apr_pool_userdata_get(&data, mod_h2_init_key, s->process->pool);
     if ( data == NULL ) {
@@ -108,8 +110,9 @@ static int h2_post_config(apr_pool_t *p, apr_pool_t *plog,
     ap_log_error( APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(03090)
                  "mod_http2 (v%s, feats=%s%s%s, nghttp2 %s), initializing...",
                  MOD_HTTP2_VERSION, 
-                 myfeats.change_prio? "CHPRIO" : "", sep, 
-                 myfeats.sha256?      "SHA256" : "",
+                 myfeats.change_prio? "CHPRIO"  : "", 
+                 myfeats.sha256?      "+SHA256" : "",
+                 myfeats.inv_headers? "+INVHD"  : "",
                  ngh2?                ngh2->version_str : "unknown");
     
     switch (h2_conn_mpm_type()) {