From: Stefan Eissing Date: Sat, 27 Aug 2016 12:23:35 +0000 (+0000) Subject: Merge of r1757985,r1758003 from trunk X-Git-Tag: 2.4.24~283 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=89de5fac3ded566372d34731388d9b0fec165e33;p=thirdparty%2Fapache%2Fhttpd.git 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 fc47f4e9830..090262cd4da 100644 --- 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 a77ad808998..cccbbc8536f 100644 --- 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 6a1e74d3e7f..1338ba68b01 100644 --- 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 fcafb063ddd..8ccd8a3aaa7 100644 --- 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 c1de2699b32..e340c9a6788 100644 --- 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 79a2e82e82d..22f37e08578 100644 --- 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 e6214088aa7..31ae303c9d0 100644 --- 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 9b4c0175677..f457eb49ff8 100644 --- 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 7edfae754ab..4394c706f0b 100644 --- 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 75f376cf0ff..318943ae4a8 100644 --- 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 76e8780d2ac..f13366e26cd 100644 --- 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 b62b6c7a77f..e65ea7aa1f5 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.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 06410612933..65ea3d6aa4e 100644 --- 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()) {