From: Stefan Eissing Date: Tue, 7 Jun 2016 11:29:51 +0000 (+0000) Subject: Merge r1746988,r1747170 from trunk: X-Git-Tag: 2.4.21~68 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=f1fe0105a99e41d456d4faa7c88956f00e2bcd2d;p=apache Merge r1746988,r1747170 from trunk: mod_http2: backport of 1.5.7 git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1747194 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/CHANGES b/CHANGES index 75c02610d9..a714286d99 100644 --- a/CHANGES +++ b/CHANGES @@ -2,6 +2,9 @@ Changes with Apache 2.4.21 + *) mod_http2: fixed a write after free when streams/connections + were aborted before tasks returned. [Stefan Eissing] + *) mod_ssl: Correct the interaction between SSLProxyCheckPeerCN and newer SSLProxyCheckPeerName directives since release 2.4.5, such that disabling either disables both, and that enabling either triggers the new, more diff --git a/modules/http2/h2_bucket_eos.c b/modules/http2/h2_bucket_eos.c index 9953ca15f9..28c34fdc31 100644 --- a/modules/http2/h2_bucket_eos.c +++ b/modules/http2/h2_bucket_eos.c @@ -38,10 +38,8 @@ static apr_status_t bucket_cleanup(void *data) h2_stream **pstream = data; if (*pstream) { - /* - * If bucket_destroy is called after us, this prevents - * bucket_destroy from trying to destroy the pool again. - */ + /* If bucket_destroy is called after us, this prevents + * bucket_destroy from trying to destroy the stream again. */ *pstream = NULL; } return APR_SUCCESS; @@ -92,10 +90,13 @@ static void bucket_destroy(void *data) if (apr_bucket_shared_destroy(h)) { h2_stream *stream = h->stream; + if (stream && stream->pool) { + apr_pool_cleanup_kill(stream->pool, &h->stream, bucket_cleanup); + } + apr_bucket_free(h); if (stream) { h2_stream_eos_destroy(stream); } - apr_bucket_free(h); } } diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c index f7b30fffad..c09e45ac02 100644 --- a/modules/http2/h2_mplx.c +++ b/modules/http2/h2_mplx.c @@ -200,7 +200,7 @@ static int purge_stream(void *ctx, void *val) h2_ihash_remove(m->spurge, stream->id); h2_stream_destroy(stream); if (task) { - task_destroy(m, task, 1); + task_destroy(m, task, 0); } return 0; } @@ -349,10 +349,12 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master) ap_log_cerror(APLOG_MARK, APLOG_TRACE3, 0, m->c, "h2_task(%s): destroy", task->id); /* cleanup any buffered input */ - status = h2_task_shutdown(task, 0); - if (status != APR_SUCCESS){ - ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, APLOGNO(03385) - "h2_task(%s): shutdown", task->id); + if (task->input.beam) { + status = h2_beam_shutdown(task->input.beam, APR_NONBLOCK_READ); + if (status != APR_SUCCESS){ + ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, APLOGNO(03385) + "h2_task(%s): input shutdown", task->id); + } } if (called_from_master) { @@ -446,10 +448,8 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error) if (rst_error) { h2_task_rst(task, rst_error); } - /* FIXME: this should work, but does not h2_ihash_add(m->shold, stream); - return;*/ - task->input.beam = NULL; + return; } else { /* already finished */ @@ -500,6 +500,12 @@ static int task_abort_connection(void *ctx, void *val) if (task->c) { task->c->aborted = 1; } + if (task->input.beam) { + h2_beam_abort(task->input.beam); + } + if (task->output.beam) { + h2_beam_abort(task->output.beam); + } return 1; } @@ -603,7 +609,7 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) AP_DEBUG_ASSERT(h2_ihash_empty(m->shold)); if (!h2_ihash_empty(m->spurge)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, - "h2_mplx(%ld): release_join %d streams to purge", + "h2_mplx(%ld): 3. release_join %d streams to purge", m->id, (int)h2_ihash_count(m->spurge)); purge_streams(m); } @@ -1028,6 +1034,7 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) * parent pool / allocator) */ h2_ihash_remove(m->shold, stream->id); h2_ihash_add(m->spurge, stream); + task_destroy(m, task, 0); } else { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, diff --git a/modules/http2/h2_stream.c b/modules/http2/h2_stream.c index acd5072c63..3beab4b310 100644 --- a/modules/http2/h2_stream.c +++ b/modules/http2/h2_stream.c @@ -240,7 +240,7 @@ void h2_stream_destroy(h2_stream *stream) void h2_stream_eos_destroy(h2_stream *stream) { h2_session_stream_done(stream->session, stream); - /* stream is gone */ + /* stream possibly destroyed */ } apr_pool_t *h2_stream_detach_pool(h2_stream *stream) diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c index f505e85927..8cf1ce1ee5 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -88,7 +88,7 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r, { apr_status_t status = APR_SUCCESS; apr_bucket_brigade *bb = task->input.bb; - apr_table_t *t = task->request->trailers; + 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); @@ -114,7 +114,7 @@ static apr_status_t input_append_eos(h2_task *task, request_rec *r) { apr_status_t status = APR_SUCCESS; apr_bucket_brigade *bb = task->input.bb; - apr_table_t *t = task->request->trailers; + apr_table_t *t = task->request? task->request->trailers : NULL; if (task->input.chunked) { if (t && !apr_is_empty_table(t)) { @@ -151,13 +151,14 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, return ap_get_brigade(f->c->input_filters, bb, mode, block, readbytes); } - if (f->c->aborted) { + if (f->c->aborted || !task->request) { return APR_ECONNABORTED; } if (!task->input.bb) { if (!task->input.eos_written) { input_append_eos(task, f->r); + return APR_SUCCESS; } return APR_EOF; } @@ -172,11 +173,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, } } - while (APR_BRIGADE_EMPTY(task->input.bb)) { - if (task->input.eos_written) { - return APR_EOF; - } - + while (APR_BRIGADE_EMPTY(task->input.bb) && !task->input.eos) { /* Get more input data for our request. */ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c, "h2_task(%s): get more data from mplx, block=%d, " @@ -193,7 +190,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, status = APR_EOF; } - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c, + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, f->c, "h2_task(%s): read returned", task->id); if (APR_STATUS_IS_EAGAIN(status) && (mode == AP_MODE_GETLINE || block == APR_BLOCK_READ)) { @@ -202,7 +199,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, * upload 100k test on test-ser.example.org hangs */ status = APR_SUCCESS; } - else if (APR_STATUS_IS_EOF(status) && !task->input.eos_written) { + else if (APR_STATUS_IS_EOF(status)) { task->input.eos = 1; } else if (status != APR_SUCCESS) { @@ -251,8 +248,13 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, } } - if (!task->input.eos_written && task->input.eos) { - input_append_eos(task, f->r); + if (task->input.eos) { + if (!task->input.eos_written) { + input_append_eos(task, f->r); + } + if (APR_BRIGADE_EMPTY(task->input.bb)) { + return APR_EOF; + } } h2_util_bb_log(f->c, task->stream_id, APLOG_TRACE2, @@ -556,23 +558,6 @@ void h2_task_rst(h2_task *task, int error) } } -apr_status_t h2_task_shutdown(h2_task *task, int block) -{ - if (task->output.beam) { - apr_status_t status; - status = h2_beam_shutdown(task->output.beam, APR_NONBLOCK_READ); - if (block && status == APR_EAGAIN) { - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, task->c, - "h2_task(%s): output shutdown waiting", task->id); - status = h2_beam_shutdown(task->output.beam, APR_BLOCK_READ); - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, task->c, - "h2_task(%s): output shutdown done", task->id); - } - return status; - } - return APR_SUCCESS; -} - /******************************************************************************* * Register various hooks */ diff --git a/modules/http2/h2_task.h b/modules/http2/h2_task.h index dda8a562bb..010005a374 100644 --- a/modules/http2/h2_task.h +++ b/modules/http2/h2_task.h @@ -114,11 +114,6 @@ int h2_task_can_redo(h2_task *task); */ void h2_task_rst(h2_task *task, int error); -/** - * Shuts all input/output down. Clears any buckets buffered and closes. - */ -apr_status_t h2_task_shutdown(h2_task *task, int block); - void h2_task_register_hooks(void); /* * One time, post config intialization. diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h index 020827ccc3..d5125b2826 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.5.6" +#define MOD_HTTP2_VERSION "1.5.7" /** * @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 0x010506 +#define MOD_HTTP2_VERSION_NUM 0x010507 #endif /* mod_h2_h2_version_h */