diff options
author | Stefan Eissing <icing@apache.org> | 2017-04-21 12:21:31 +0000 |
---|---|---|
committer | Stefan Eissing <icing@apache.org> | 2017-04-21 12:21:31 +0000 |
commit | 013958a3d3c26967da9e859818f2765b9de35d04 (patch) | |
tree | 12fc008c429b1a49f9637a6e6168d47216ec6d54 | |
parent | 38e269322b2e88d8c52f9aeb1db01ffbcecdcd13 (diff) | |
download | httpd-013958a3d3c26967da9e859818f2765b9de35d04.tar.gz |
On the trunk:
mod_http2: fixed possible deadlock that could occur when connections were
terminated early with ongoing streams. Fixed possible hanger with timeout
on race when connection considers itself idle.
git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1792195 13f79535-47bb-0310-9956-ffa450edef68
-rw-r--r-- | CHANGES | 4 | ||||
-rw-r--r-- | modules/http2/h2_bucket_beam.c | 35 | ||||
-rw-r--r-- | modules/http2/h2_mplx.c | 114 | ||||
-rw-r--r-- | modules/http2/h2_session.c | 9 | ||||
-rw-r--r-- | modules/http2/h2_util.c | 33 | ||||
-rw-r--r-- | modules/http2/h2_util.h | 1 |
6 files changed, 110 insertions, 86 deletions
@@ -1,6 +1,10 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) mod_http2: fixed possible deadlock that could occur when connections were + terminated early with ongoing streams. Fixed possible hanger with timeout + on race when connection considers itself idle. [Stefan Eissing] + *) Introduce request taint checking framework to prevent privilege hijacking through .htaccess. [Nick Kew] diff --git a/modules/http2/h2_bucket_beam.c b/modules/http2/h2_bucket_beam.c index 872c67544d..fff3f5dc93 100644 --- a/modules/http2/h2_bucket_beam.c +++ b/modules/http2/h2_bucket_beam.c @@ -496,6 +496,28 @@ static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool) } } +static void recv_buffer_cleanup(h2_bucket_beam *beam, h2_beam_lock *bl) +{ + if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) { + apr_bucket_brigade *bb = beam->recv_buffer; + apr_off_t bblen = 0; + + beam->recv_buffer = NULL; + apr_brigade_length(bb, 0, &bblen); + beam->received_bytes += bblen; + + /* need to do this unlocked since bucket destroy might + * call this beam again. */ + if (bl) leave_yellow(beam, bl); + apr_brigade_destroy(bb); + if (bl) enter_yellow(beam, bl); + + if (beam->cons_ev_cb) { + beam->cons_ev_cb(beam->cons_ctx, beam); + } + } +} + static apr_status_t beam_cleanup(void *data) { h2_bucket_beam *beam = data; @@ -526,10 +548,7 @@ static apr_status_t beam_cleanup(void *data) pool_kill(beam, beam->recv_pool, beam_recv_cleanup); beam->recv_pool = NULL; } - if (beam->recv_buffer) { - apr_brigade_destroy(beam->recv_buffer); - beam->recv_buffer = NULL; - } + recv_buffer_cleanup(beam, NULL); } else { beam->recv_buffer = NULL; @@ -697,9 +716,7 @@ apr_status_t h2_beam_leave(h2_bucket_beam *beam) h2_beam_lock bl; if (enter_yellow(beam, &bl) == APR_SUCCESS) { - if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) { - apr_brigade_cleanup(beam->recv_buffer); - } + recv_buffer_cleanup(beam, &bl); beam->aborted = 1; beam_close(beam); leave_yellow(beam, &bl); @@ -932,9 +949,7 @@ apr_status_t h2_beam_receive(h2_bucket_beam *beam, if (enter_yellow(beam, &bl) == APR_SUCCESS) { transfer: if (beam->aborted) { - if (beam->recv_buffer && !APR_BRIGADE_EMPTY(beam->recv_buffer)) { - apr_brigade_cleanup(beam->recv_buffer); - } + recv_buffer_cleanup(beam, &bl); status = APR_ECONNABORTED; goto leave; } diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c index e0704b7a88..0520dc5914 100644 --- a/modules/http2/h2_mplx.c +++ b/modules/http2/h2_mplx.c @@ -378,8 +378,10 @@ static int report_stream_iter(void *ctx, void *val) { h2_stream *stream = val; h2_task *task = stream->task; ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, - H2_STRM_MSG(stream, "started=%d, scheduled=%d, ready=%d"), - !!stream->task, stream->scheduled, h2_stream_is_ready(stream)); + H2_STRM_MSG(stream, "started=%d, scheduled=%d, ready=%d, " + "out_buffer=%ld"), + !!stream->task, stream->scheduled, h2_stream_is_ready(stream), + (long)h2_beam_get_buffered(stream->output)); if (task) { ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, /* NO APLOGNO */ H2_STRM_MSG(stream, "->03198: %s %s %s" @@ -850,9 +852,6 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) /* stream not cleaned up, stay around */ ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, H2_STRM_MSG(stream, "task_done, stream open")); - /* more data will not arrive, resume the stream */ - check_data_for(m, stream, 0); - if (stream->input) { h2_beam_leave(stream->input); h2_beam_mutex_disable(stream->input); @@ -860,6 +859,9 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) if (stream->output) { h2_beam_mutex_disable(stream->output); } + + /* more data will not arrive, resume the stream */ + check_data_for(m, stream, 0); } } else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) { @@ -1001,43 +1003,75 @@ apr_status_t h2_mplx_idle(h2_mplx *m) H2_MPLX_ENTER(m); scount = h2_ihash_count(m->streams); - if (scount > 0 && m->tasks_active) { - /* If we have streams in connection state 'IDLE', meaning - * all streams are ready to sent data out, but lack - * WINDOW_UPDATEs. - * - * This is ok, unless we have streams that still occupy - * h2 workers. As worker threads are a scarce resource, - * we need to take measures that we do not get DoSed. - * - * This is what we call an 'idle block'. Limit the amount - * of busy workers we allow for this connection until it - * well behaves. - */ - now = apr_time_now(); - m->last_idle_block = now; - if (m->limit_active > 2 - && now - m->last_limit_change >= m->limit_change_interval) { - if (m->limit_active > 16) { - m->limit_active = 16; - } - else if (m->limit_active > 8) { - m->limit_active = 8; - } - else if (m->limit_active > 4) { - m->limit_active = 4; + if (scount > 0) { + if (m->tasks_active) { + /* If we have streams in connection state 'IDLE', meaning + * all streams are ready to sent data out, but lack + * WINDOW_UPDATEs. + * + * This is ok, unless we have streams that still occupy + * h2 workers. As worker threads are a scarce resource, + * we need to take measures that we do not get DoSed. + * + * This is what we call an 'idle block'. Limit the amount + * of busy workers we allow for this connection until it + * well behaves. + */ + now = apr_time_now(); + m->last_idle_block = now; + if (m->limit_active > 2 + && now - m->last_limit_change >= m->limit_change_interval) { + if (m->limit_active > 16) { + m->limit_active = 16; + } + else if (m->limit_active > 8) { + m->limit_active = 8; + } + else if (m->limit_active > 4) { + m->limit_active = 4; + } + else if (m->limit_active > 2) { + m->limit_active = 2; + } + m->last_limit_change = now; + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, + "h2_mplx(%ld): decrease worker limit to %d", + m->id, m->limit_active); } - else if (m->limit_active > 2) { - m->limit_active = 2; + + if (m->tasks_active > m->limit_active) { + status = unschedule_slow_tasks(m); } - m->last_limit_change = now; + } + else if (!h2_iq_empty(m->q)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, - "h2_mplx(%ld): decrease worker limit to %d", - m->id, m->limit_active); + "h2_mplx(%ld): idle, but %d streams to process", + m->id, (int)h2_iq_count(m->q)); + status = APR_EAGAIN; } - - if (m->tasks_active > m->limit_active) { - status = unschedule_slow_tasks(m); + else { + /* idle, have streams, but no tasks active. what are we waiting for? + * WINDOW_UPDATEs from client? */ + h2_stream *stream = NULL; + + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, + "h2_mplx(%ld): idle, no tasks ongoing, %d streams", + m->id, (int)h2_ihash_count(m->streams)); + h2_ihash_shift(m->streams, (void**)&stream, 1); + if (stream && stream->output) { + /* FIXME: this looks like a race between the session thinking + * it is idle and the EOF on a stream not being sent. + * Signal to caller to leave IDLE state. + */ + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, + H2_STRM_MSG(stream, "output closed=%d, mplx idle" + ", out has %ld bytes buffered"), + h2_beam_is_closed(stream->output), + (long)h2_beam_get_buffered(stream->output)); + h2_ihash_add(m->streams, stream); + check_data_for(m, stream, 0); + status = APR_EAGAIN; + } } } register_if_needed(m); @@ -1231,8 +1265,8 @@ int h2_mplx_awaits_data(h2_mplx *m) if (h2_ihash_empty(m->streams)) { waiting = 0; } - if ((h2_fifo_count(m->readyq) == 0) - && h2_iq_empty(m->q) && !m->tasks_active) { + else if (!m->tasks_active && !h2_fifo_count(m->readyq) + && h2_iq_empty(m->q)) { waiting = 0; } diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index adece255ee..264513f0f3 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -1050,7 +1050,8 @@ static ssize_t stream_data_cb(nghttp2_session *ng2s, break; case APR_ECONNRESET: - return 0; + case APR_ECONNABORTED: + return NGHTTP2_ERR_CALLBACK_FAILURE; case APR_EAGAIN: /* If there is no data available, our session will automatically @@ -2083,7 +2084,11 @@ apr_status_t h2_session_process(h2_session *session, int async) * That gives us the chance to check for MPMQ_STOPPING often. */ status = h2_mplx_idle(session->mplx); - if (status != APR_SUCCESS) { + if (status == APR_EAGAIN) { + dispatch_event(session, H2_SESSION_EV_DATA_READ, 0, NULL); + break; + } + else if (status != APR_SUCCESS) { dispatch_event(session, H2_SESSION_EV_CONN_ERROR, H2_ERR_ENHANCE_YOUR_CALM, "less is more"); } diff --git a/modules/http2/h2_util.c b/modules/http2/h2_util.c index f4c3aab3a9..01b506ac85 100644 --- a/modules/http2/h2_util.c +++ b/modules/http2/h2_util.c @@ -372,39 +372,6 @@ size_t h2_ihash_shift(h2_ihash_t *ih, void **buffer, size_t max) return ctx.len; } -typedef struct { - h2_ihash_t *ih; - int *buffer; - size_t max; - size_t len; -} icollect_ctx; - -static int icollect_iter(void *x, void *val) -{ - icollect_ctx *ctx = x; - if (ctx->len < ctx->max) { - ctx->buffer[ctx->len++] = *((int*)((char *)val + ctx->ih->ioff)); - return 1; - } - return 0; -} - -size_t h2_ihash_ishift(h2_ihash_t *ih, int *buffer, size_t max) -{ - icollect_ctx ctx; - size_t i; - - ctx.ih = ih; - ctx.buffer = buffer; - ctx.max = max; - ctx.len = 0; - h2_ihash_iter(ih, icollect_iter, &ctx); - for (i = 0; i < ctx.len; ++i) { - h2_ihash_remove(ih, buffer[i]); - } - return ctx.len; -} - /******************************************************************************* * iqueue - sorted list of int ******************************************************************************/ diff --git a/modules/http2/h2_util.h b/modules/http2/h2_util.h index 4b901bfe68..35163716ad 100644 --- a/modules/http2/h2_util.h +++ b/modules/http2/h2_util.h @@ -68,7 +68,6 @@ void h2_ihash_remove_val(h2_ihash_t *ih, void *val); void h2_ihash_clear(h2_ihash_t *ih); size_t h2_ihash_shift(h2_ihash_t *ih, void **buffer, size_t max); -size_t h2_ihash_ishift(h2_ihash_t *ih, int *buffer, size_t max); /******************************************************************************* * iqueue - sorted list of int with user defined ordering |