*) mod_http2: fixed a bug that could lead to a crash in main connection

     output handling. This occured only when the last request on a HTTP/2
     connection had been processed and the session decided to shut down.
     This could lead to an attempt to send a final GOAWAY while the previous
     write was still in progress. See PR 66646.

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

diff --git a/changes-entries/h2_pr66646.txt b/changes-entries/h2_pr66646.txt
new file mode 100644 (file)
index 0000000..6bf23cf
--- /dev/null
+++ b/changes-entries/h2_pr66646.txt
@@ -0,0 +1,6 @@
+  *) mod_http2: fixed a bug that could lead to a crash in main connection
+     output handling. This occured only when the last request on a HTTP/2
+     connection had been processed and the session decided to shut down.
+     This could lead to an attempt to send a final GOAWAY while the previous
+     write was still in progress. See PR 66646.
+     [Stefan Eissing]

diff --git a/modules/http2/h2_c1_io.c b/modules/http2/h2_c1_io.c
index ade8836635887b4d477ed0ed1b515600ba1df611..c43fa1b801c1840cf4864256de0ae16a6d7fff1b 100644 (file)
--- a/modules/http2/h2_c1_io.c
+++ b/modules/http2/h2_c1_io.c
@@ -260,9 +260,22 @@ static apr_status_t read_to_scratch(h2_c1_io *io, apr_bucket *b)
 static apr_status_t pass_output(h2_c1_io *io, int flush)
 {
     conn_rec *c = io->session->c1;
-    apr_off_t bblen;
+    apr_off_t bblen = 0;
     apr_status_t rv;
-    
+
+    if (io->is_passing) {
+        /* recursive call, may be triggered by an H2EOS bucket
+         * being destroyed and triggering sending more data? */
+        AP_DEBUG_ASSERT(0);
+        ap_log_cerror(APLOG_MARK, APLOG_ERR, rv, c, APLOGNO()
+                      "h2_c1_io(%ld): recursive call of h2_c1_io_pass. "
+                      "Denied to prevent output corruption. This "
+                      "points to a bug in the HTTP/2 implementation.",
+                      c->id);
+        return APR_EGENERAL;
+    }
+    io->is_passing = 1;
+
     append_scratch(io);
     if (flush) {
         if (!APR_BUCKET_IS_FLUSH(APR_BRIGADE_LAST(io->output))) {
@@ -271,13 +284,14 @@ static apr_status_t pass_output(h2_c1_io *io, int flush)
         }
     }
     if (APR_BRIGADE_EMPTY(io->output)) {
-        return APR_SUCCESS;
+        rv = APR_SUCCESS;
+        goto cleanup;
     }
     
     io->unflushed = !APR_BUCKET_IS_FLUSH(APR_BRIGADE_LAST(io->output));
     apr_brigade_length(io->output, 0, &bblen);
     C1_IO_BB_LOG(c, 0, APLOG_TRACE2, "out", io->output);
-    
+
     rv = ap_pass_brigade(c->output_filters, io->output);
     if (APR_SUCCESS != rv) goto cleanup;
 
@@ -309,6 +323,7 @@ cleanup:
                       c->id, (long)bblen);
     }
     apr_brigade_cleanup(io->output);
+    io->is_passing = 0;
     return rv;
 }
 

diff --git a/modules/http2/h2_c1_io.h b/modules/http2/h2_c1_io.h
index d891ffb44dfd7a7e269a9ee0b96fefb70c8816a7..c4dac38e1ee4b3e68b68174c53e00c16251e129c 100644 (file)
--- a/modules/http2/h2_c1_io.h
+++ b/modules/http2/h2_c1_io.h
@@ -44,7 +44,8 @@ typedef struct {
     apr_off_t buffered_len;
     apr_off_t flush_threshold;
     unsigned int is_flushed : 1;
-    
+    unsigned int is_passing : 1;
+
     char *scratch;
     apr_size_t ssize;
     apr_size_t slen;

diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c
index 1f37a36bf72fd50c1f01d4fbbc23b653b55eccc2..603e47ce85e1f85e0abb2b2e4891f47efb23eebb 100644 (file)
--- a/modules/http2/h2_session.c
+++ b/modules/http2/h2_session.c
@@ -1642,10 +1642,6 @@ static void on_stream_state_enter(void *ctx, h2_stream *stream)
             h2_mplx_c1_stream_cleanup(session->mplx, stream, &session->open_streams);
             ++session->streams_done;
             update_child_status(session, SERVER_BUSY_WRITE, "done", stream);
-            if (session->open_streams == 0) {
-                h2_session_dispatch_event(session, H2_SESSION_EV_NO_MORE_STREAMS,
-                                          0, "stream done");
-            }
             break;
         default:
             break;

diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c
index d8b582ac408e74bfbf7b7c573bfa19ae1797bcee..635caf7cb3c70790b08839e443573e11be94a314 100644 (file)
--- a/modules/http2/h2_stream.c
+++ b/modules/http2/h2_stream.c
@@ -166,6 +166,7 @@ static int on_frame_recv(h2_stream_state_t state, int frame_type)
 
 static int on_event(h2_stream* stream, h2_stream_event_t ev)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (stream->monitor && stream->monitor->on_event) {
         stream->monitor->on_event(stream->monitor->ctx, stream, ev);
     }
@@ -392,6 +393,7 @@ void h2_stream_dispatch(h2_stream *stream, h2_stream_event_t ev)
 {
     int new_state;
     
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, stream->session->c1,
                   H2_STRM_MSG(stream, "dispatch event %d"), ev);
     new_state = on_event(stream, ev);
@@ -425,6 +427,7 @@ apr_status_t h2_stream_send_frame(h2_stream *stream, int ftype, int flags, size_
     apr_status_t status = APR_SUCCESS;
     int new_state, eos = 0;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     new_state = on_frame_send(stream->state, ftype);
     if (new_state < 0) {
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1,
@@ -474,6 +477,7 @@ apr_status_t h2_stream_recv_frame(h2_stream *stream, int ftype, int flags, size_
     apr_status_t status = APR_SUCCESS;
     int new_state, eos = 0;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     new_state = on_frame_recv(stream->state, ftype);
     if (new_state < 0) {
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1,
@@ -528,6 +532,7 @@ apr_status_t h2_stream_recv_DATA(h2_stream *stream, uint8_t flags,
     h2_session *session = stream->session;
     apr_status_t status = APR_SUCCESS;
     
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     stream->in_data_frames++;
     if (len > 0) {
         if (APLOGctrace3(session->c1)) {
@@ -548,11 +553,38 @@ apr_status_t h2_stream_recv_DATA(h2_stream *stream, uint8_t flags,
     return status;
 }
 
+#ifdef AP_DEBUG
+static apr_status_t stream_pool_destroy(void *data)
+{
+    h2_stream *stream = data;
+    switch (stream->magic) {
+    case H2_STRM_MAGIC_OK:
+        ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, stream->session->c1,
+                      H2_STRM_MSG(stream, "was not destroyed explicitly"));
+        AP_DEBUG_ASSERT(0);
+        break;
+    case H2_STRM_MAGIC_SDEL:
+        /* stream has been explicitly destroyed, as it should */
+        H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_PDEL);
+        break;
+    case H2_STRM_MAGIC_PDEL:
+        ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, stream->session->c1,
+                      H2_STRM_MSG(stream, "already pool destroyed"));
+        AP_DEBUG_ASSERT(0);
+        break;
+    default:
+        AP_DEBUG_ASSERT(0);
+    }
+    return APR_SUCCESS;
+}
+#endif
+
 h2_stream *h2_stream_create(int id, apr_pool_t *pool, h2_session *session,
                             h2_stream_monitor *monitor, int initiated_on)
 {
     h2_stream *stream = apr_pcalloc(pool, sizeof(h2_stream));
-    
+
+    H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_OK);
     stream->id           = id;
     stream->initiated_on = initiated_on;
     stream->created      = apr_time_now();
@@ -560,6 +592,12 @@ h2_stream *h2_stream_create(int id, apr_pool_t *pool, h2_session *session,
     stream->pool         = pool;
     stream->session      = session;
     stream->monitor      = monitor;
+#ifdef AP_DEBUG
+    if (id) { /* stream 0 has special lifetime */
+        apr_pool_cleanup_register(pool, stream, stream_pool_destroy,
+                                  apr_pool_cleanup_null);
+    }
+#endif
 
 #ifdef H2_NG2_LOCAL_WIN_SIZE
     if (id) {
@@ -581,6 +619,7 @@ void h2_stream_cleanup(h2_stream *stream)
      * end of the in/out notifications get closed.
      */
     ap_assert(stream);
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (stream->out_buffer) {
         apr_brigade_cleanup(stream->out_buffer);
     }
@@ -589,13 +628,16 @@ void h2_stream_cleanup(h2_stream *stream)
 void h2_stream_destroy(h2_stream *stream)
 {
     ap_assert(stream);
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, stream->session->c1,
                   H2_STRM_MSG(stream, "destroy"));
+    H2_STRM_ASSIGN_MAGIC(stream, H2_STRM_MAGIC_SDEL);
     apr_pool_destroy(stream->pool);
 }
 
 void h2_stream_rst(h2_stream *stream, int error_code)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     stream->rst_error = error_code;
     if (stream->c2) {
         h2_c2_abort(stream->c2, stream->session->c1);
@@ -611,6 +653,7 @@ apr_status_t h2_stream_set_request_rec(h2_stream *stream,
     h2_request *req;
     apr_status_t status;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_assert(stream->request == NULL);
     ap_assert(stream->rtmp == NULL);
     if (stream->rst_error) {
@@ -632,6 +675,7 @@ apr_status_t h2_stream_set_request_rec(h2_stream *stream,
 
 void h2_stream_set_request(h2_stream *stream, const h2_request *r)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_assert(stream->request == NULL);
     ap_assert(stream->rtmp == NULL);
     stream->rtmp = h2_request_clone(stream->pool, r);
@@ -691,6 +735,7 @@ apr_status_t h2_stream_add_header(h2_stream *stream,
     int error = 0, was_added = 0;
     apr_status_t status = APR_SUCCESS;
     
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (stream->response) {
         return APR_EINVAL;    
     }
@@ -797,6 +842,7 @@ apr_status_t h2_stream_end_headers(h2_stream *stream, int eos, size_t raw_bytes)
     int is_http_or_https;
     h2_request *req = stream->rtmp;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     status = h2_request_end_headers(req, stream->pool, raw_bytes);
     if (APR_SUCCESS != status || req->http_status != H2_HTTP_STATUS_UNSET) {
         goto cleanup;
@@ -1045,6 +1091,7 @@ apr_status_t h2_stream_read_to(h2_stream *stream, apr_bucket_brigade *bb,
 {
     apr_status_t rv = APR_SUCCESS;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (stream->rst_error) {
         return APR_ECONNRESET;
     }
@@ -1139,6 +1186,7 @@ apr_status_t h2_stream_submit_pushes(h2_stream *stream, h2_headers *response)
     apr_array_header_t *pushes;
     int i;
     
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     pushes = h2_push_collect_update(stream, stream->request, response);
     if (pushes && !apr_is_empty_array(pushes)) {
         ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, stream->session->c1,
@@ -1158,6 +1206,7 @@ apr_status_t h2_stream_submit_pushes(h2_stream *stream, h2_headers *response)
 
 apr_table_t *h2_stream_get_trailers(h2_stream *stream)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     return NULL;
 }
 
@@ -1169,6 +1218,7 @@ const h2_priority *h2_stream_get_priority(h2_stream *stream,
                                           h2_headers *response)
 #endif
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (response && stream->initiated_on) {
         const char *ctype = apr_table_get(response->headers, "content-type");
         if (ctype) {
@@ -1182,6 +1232,7 @@ const h2_priority *h2_stream_get_priority(h2_stream *stream,
 int h2_stream_is_ready(h2_stream *stream)
 {
     /* Have we sent a response or do we have the response in our buffer? */
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (stream->response) {
         return 1;
     }
@@ -1193,11 +1244,13 @@ int h2_stream_is_ready(h2_stream *stream)
 
 int h2_stream_is_at(const h2_stream *stream, h2_stream_state_t state)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     return stream->state == state;
 }
 
 int h2_stream_is_at_or_past(const h2_stream *stream, h2_stream_state_t state)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     switch (state) {
         case H2_SS_IDLE:
             return 1; /* by definition */
@@ -1220,6 +1273,7 @@ apr_status_t h2_stream_in_consumed(h2_stream *stream, apr_off_t amount)
 {
     h2_session *session = stream->session;
     
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (amount > 0) {
         apr_off_t consumed = amount;
         
@@ -1345,6 +1399,7 @@ static ssize_t stream_data_cb(nghttp2_session *ng2s,
                       H2_SSSN_STRM_MSG(session, stream_id, "data_cb, stream not found"));
         return NGHTTP2_ERR_CALLBACK_FAILURE;
     }
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (!stream->output || !stream->response || !stream->out_buffer) {
         return NGHTTP2_ERR_DEFERRED;
     }
@@ -1476,6 +1531,7 @@ static apr_status_t stream_do_response(h2_stream *stream)
 #endif
     nghttp2_data_provider provider, *pprovider = NULL;
 
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_assert(!stream->response);
     ap_assert(stream->out_buffer);
 
@@ -1666,6 +1722,7 @@ void h2_stream_on_output_change(h2_stream *stream)
 
     /* stream->pout_recv_write signalled a change. Check what has happend, read
      * from it and act on seeing a response/data. */
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     if (!stream->output) {
         /* c2 has not assigned the output beam to the stream (yet). */
         ap_log_cerror(APLOG_MARK, APLOG_WARNING, 0, c1,
@@ -1714,6 +1771,7 @@ void h2_stream_on_output_change(h2_stream *stream)
 
 void h2_stream_on_input_change(h2_stream *stream)
 {
+    H2_STRM_ASSERT_MAGIC(stream, H2_STRM_MAGIC_OK);
     ap_assert(stream->input);
     h2_beam_report_consumption(stream->input);
     if (h2_stream_is_at(stream, H2_SS_CLOSED_L)

diff --git a/modules/http2/h2_stream.h b/modules/http2/h2_stream.h
index 695d56ac5e1cf0797ff16fc4c7000cb29b32dc07..638dbdac0d158aa841adfb7c3e3d4e6d00f6e605 100644 (file)
--- a/modules/http2/h2_stream.h
+++ b/modules/http2/h2_stream.h
@@ -63,7 +63,22 @@ typedef struct h2_stream_monitor {
                                              trigger a state change */
 } h2_stream_monitor;
 
+#ifdef AP_DEBUG
+#define H2_STRM_MAGIC_OK     0x5354524d
+#define H2_STRM_MAGIC_SDEL   0x5344454c
+#define H2_STRM_MAGIC_PDEL   0x5044454c
+
+#define H2_STRM_ASSIGN_MAGIC(s,m)  ((s)->magic = m)
+#define H2_STRM_ASSERT_MAGIC(s,m)  ap_assert((s)->magic == m)
+#else
+#define H2_STRM_ASSIGN_MAGIC(s,m)  ((void)0)
+#define H2_STRM_ASSERT_MAGIC(s,m)  ((void)0)
+#endif
+
 struct h2_stream {
+#ifdef AP_DEBUG
+    uint32_t magic;
+#endif
     int id;                     /* http2 stream identifier */
     int initiated_on;           /* initiating stream id (PUSH) or 0 */
     apr_pool_t *pool;           /* the memory pool for this stream */

diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h
index aca4ffe0d572e86c5aa4f51117e84d1e149b69fe..7de3144ec7f96109ee1079cb8c536dcd539bb730 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 "2.0.18-git"
+#define MOD_HTTP2_VERSION "2.0.19-git"
 
 /**
  * @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 0x020012
+#define MOD_HTTP2_VERSION_NUM 0x020013
 
 
 #endif /* mod_h2_h2_version_h */