From 14fdb48ebfe6ea1a3a55c6ff39584f53cc9e8bfc Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Thu, 9 Jun 2016 10:38:10 +0000 Subject: [PATCH] Merge of r1747531 from trunk: mod_http2: improved cleanup of connection/streams/tasks to always have deterministic order regardless of event initiating it. Addresses reported crashes due to memory read after free issues. git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1747532 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 6 ++++-- modules/http2/h2_bucket_beam.c | 16 ++++++++++++---- modules/http2/h2_bucket_beam.h | 11 ++++++----- modules/http2/h2_mplx.c | 25 ++++++++++++------------- modules/http2/h2_stream.c | 4 ++-- modules/http2/h2_version.h | 4 ++-- 6 files changed, 38 insertions(+), 28 deletions(-) diff --git a/CHANGES b/CHANGES index a714286d99..ec9512bbd6 100644 --- a/CHANGES +++ b/CHANGES @@ -2,8 +2,10 @@ Changes with Apache 2.4.21 - *) mod_http2: fixed a write after free when streams/connections - were aborted before tasks returned. [Stefan Eissing] + *) mod_http2: improved cleanup of connection/streams/tasks to always + have deterministic order regardless of event initiating it. Addresses + reported crashes due to memory read after free issues. + [Stefan Eissing] *) mod_ssl: Correct the interaction between SSLProxyCheckPeerCN and newer SSLProxyCheckPeerName directives since release 2.4.5, such that disabling diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c index 6b32e9beab..cf2cb84dad 100644 --- a/modules/http2/h2_bucket_beam.c +++ b/modules/http2/h2_bucket_beam.c @@ -407,6 +407,12 @@ static apr_status_t beam_cleanup(void *data) r_purge_reds(beam); h2_blist_cleanup(&beam->red); report_consumption(beam, 0); + while (!H2_BPROXY_LIST_EMPTY(&beam->proxies)) { + h2_beam_proxy *proxy = H2_BPROXY_LIST_FIRST(&beam->proxies); + H2_BPROXY_REMOVE(proxy); + proxy->beam = NULL; + proxy->bred = NULL; + } h2_blist_cleanup(&beam->purge); h2_blist_cleanup(&beam->hold); @@ -534,16 +540,18 @@ apr_status_t h2_beam_close(h2_bucket_beam *beam) return beam->aborted? APR_ECONNABORTED : APR_SUCCESS; } -apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block) +apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block, + int clear_buffers) { apr_status_t status; h2_beam_lock bl; if ((status = enter_yellow(beam, &bl)) == APR_SUCCESS) { - r_purge_reds(beam); - h2_blist_cleanup(&beam->red); + if (clear_buffers) { + r_purge_reds(beam); + h2_blist_cleanup(&beam->red); + } beam_close(beam); - report_consumption(beam, 0); while (status == APR_SUCCESS && (!H2_BPROXY_LIST_EMPTY(&beam->proxies) diff --git a/modules/http2/h2_bucket_beam.h b/modules/http2/h2_bucket_beam.h index a5c7458a11..5c5d65de2e 100644 --- a/modules/http2/h2_bucket_beam.h +++ b/modules/http2/h2_bucket_beam.h @@ -282,16 +282,17 @@ void h2_beam_abort(h2_bucket_beam *beam); apr_status_t h2_beam_close(h2_bucket_beam *beam); /** - * Empty any buffered data and return APR_SUCCESS when all buckets - * in transit have been handled. When called with APR_BLOCK_READ and - * with a mutex installed, will wait until this is the case. Otherwise - * APR_EAGAIN is returned. + * Return APR_SUCCESS when all buckets in transit have been handled. + * When called with APR_BLOCK_READ and a mutex set, will wait until the green + * side has consumed all data. Otherwise APR_EAGAIN is returned. + * With clear_buffers set, any queued data is discarded. * If a timeout is set on the beam, waiting might also time out and * return APR_ETIMEUP. * * Call from the red side only. */ -apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block); +apr_status_t h2_beam_shutdown(h2_bucket_beam *beam, apr_read_type_e block, + int clear_buffers); void h2_beam_mutex_set(h2_bucket_beam *beam, h2_beam_mutex_enter m_enter, diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c index c09e45ac02..9b2cb956a1 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, 0); + task_destroy(m, task, 1); } return 0; } @@ -348,15 +348,6 @@ 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 */ - 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) { /* Process outstanding events before destruction */ h2_stream *stream = h2_ihash_get(m->streams, task->stream_id); @@ -372,6 +363,12 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master) m->tx_handles_reserved += h2_beam_get_files_beamed(task->output.beam); h2_beam_on_produced(task->output.beam, NULL, NULL); + status = h2_beam_shutdown(task->output.beam, APR_NONBLOCK_READ, 1); + if (status != APR_SUCCESS){ + ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, + APLOGNO(03385) "h2_task(%s): output shutdown " + "incomplete", task->id); + } } slave = task->c; @@ -438,6 +435,8 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error) h2_ihash_remove(m->streams, stream->id); if (stream->input) { m->tx_handles_reserved += h2_beam_get_files_beamed(stream->input); + h2_beam_on_consumed(stream->input, NULL, NULL); + h2_beam_mutex_set(stream->input, NULL, NULL, NULL); } h2_stream_cleanup(stream); @@ -651,6 +650,7 @@ apr_status_t h2_mplx_stream_done(h2_mplx *m, h2_stream *stream) "h2_mplx(%ld-%d): marking stream as done.", m->id, stream->id); stream_done(m, stream, stream->rst_error); + purge_streams(m); leave_mutex(m, acquired); } return status; @@ -766,6 +766,7 @@ apr_status_t h2_mplx_out_trywait(h2_mplx *m, apr_interval_time_t timeout, status = APR_SUCCESS; } else { + purge_streams(m); m->added_output = iowait; status = apr_thread_cond_timedwait(m->added_output, m->lock, timeout); if (APLOGctrace2(m->c)) { @@ -1021,20 +1022,18 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) } } else { - /* stream done, was it placed in hold? */ + /* stream no longer active, was it placed in hold? */ stream = h2_ihash_get(m->shold, task->stream_id); if (stream) { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, "h2_mplx(%s): task_done, stream in hold", task->id); - stream->response = NULL; /* ref from task memory */ /* We cannot destroy the stream here since this is * called from a worker thread and freeing memory pools * is only safe in the only thread using it (and its * 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 3beab4b310..a7a6764192 100644 --- a/modules/http2/h2_stream.c +++ b/modules/http2/h2_stream.c @@ -213,12 +213,12 @@ void h2_stream_cleanup(h2_stream *stream) } if (stream->input) { apr_status_t status; - status = h2_beam_shutdown(stream->input, APR_NONBLOCK_READ); + status = h2_beam_shutdown(stream->input, APR_NONBLOCK_READ, 1); if (status == APR_EAGAIN) { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, stream->session->c, "h2_stream(%ld-%d): wait on input shutdown", stream->session->id, stream->id); - status = h2_beam_shutdown(stream->input, APR_BLOCK_READ); + status = h2_beam_shutdown(stream->input, APR_BLOCK_READ, 1); ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, stream->session->c, "h2_stream(%ld-%d): input shutdown returned", stream->session->id, stream->id); diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h index d5125b2826..8fac3beb1f 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.7" +#define MOD_HTTP2_VERSION "1.5.8" /** * @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 0x010507 +#define MOD_HTTP2_VERSION_NUM 0x010508 #endif /* mod_h2_h2_version_h */ -- 2.50.0