diff --git a/ChangeLog b/ChangeLog index a2caca6b..10eaf748 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,5 +1,10 @@ - v1.15.4 - -------------------------------------------------------------------------------- +v1.15.5 +-------------------------------------------------------------------------------- + * Fixed rare cases where a h2 worker could deadlock the main connection. All the good + bits in this thanks to Yann Ylavic. + +v1.15.4 +-------------------------------------------------------------------------------- * Fixed interaction with mod_reqtimeout. mod-h2 was knocking it out completely, however it allow configuration of the TLS handshake timeout and that is very useful indeed. Also, fixed a stupid mistake of mine that made `H2Direct` always `on`, irregardless diff --git a/configure.ac b/configure.ac index c2c9ca0d..dbca577e 100644 --- a/configure.ac +++ b/configure.ac @@ -14,7 +14,7 @@ # AC_PREREQ([2.69]) -AC_INIT([mod_http2], [1.15.4], [stefan.eissing@greenbytes.de]) +AC_INIT([mod_http2], [1.15.5], [stefan.eissing@greenbytes.de]) LT_PREREQ([2.2.6]) LT_INIT() diff --git a/mod_http2/h2_mplx.c b/mod_http2/h2_mplx.c index 0c93d486..c3d590dd 100644 --- a/mod_http2/h2_mplx.c +++ b/mod_http2/h2_mplx.c @@ -75,13 +75,13 @@ apr_status_t h2_mplx_child_init(apr_pool_t *pool, server_rec *s) #define H2_MPLX_ENTER_ALWAYS(m) \ apr_thread_mutex_lock(m->lock) -#define H2_MPLX_ENTER_MAYBE(m, lock) \ - if (lock) apr_thread_mutex_lock(m->lock) +#define H2_MPLX_ENTER_MAYBE(m, dolock) \ + if (dolock) apr_thread_mutex_lock(m->lock) -#define H2_MPLX_LEAVE_MAYBE(m, lock) \ - if (lock) apr_thread_mutex_unlock(m->lock) +#define H2_MPLX_LEAVE_MAYBE(m, dolock) \ + if (dolock) apr_thread_mutex_unlock(m->lock) -static void check_data_for(h2_mplx *m, h2_stream *stream, int lock); +static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked); static void stream_output_consumed(void *ctx, h2_bucket_beam *beam, apr_off_t length) @@ -104,6 +104,7 @@ static void stream_joined(h2_mplx *m, h2_stream *stream) { ap_assert(!h2_task_has_started(stream->task) || stream->task->worker_done); + h2_ififo_remove(m->readyq, stream->id); h2_ihash_remove(m->shold, stream->id); h2_ihash_add(m->spurge, stream); } @@ -125,14 +126,16 @@ static void stream_cleanup(h2_mplx *m, h2_stream *stream) h2_ihash_remove(m->streams, stream->id); h2_iq_remove(m->q, stream->id); - h2_ififo_remove(m->readyq, stream->id); - h2_ihash_add(m->shold, stream); if (!h2_task_has_started(stream->task) || stream->task->done_done) { stream_joined(m, stream); } - else if (stream->task) { - stream->task->c->aborted = 1; + else { + h2_ififo_remove(m->readyq, stream->id); + h2_ihash_add(m->shold, stream); + if (stream->task) { + stream->task->c->aborted = 1; + } } } @@ -508,12 +511,11 @@ static void output_produced(void *ctx, h2_bucket_beam *beam, apr_off_t bytes) h2_stream *stream = ctx; h2_mplx *m = stream->session->mplx; - check_data_for(m, stream, 1); + check_data_for(m, stream, 0); } static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam) { - apr_status_t status = APR_SUCCESS; h2_stream *stream = h2_ihash_get(m->streams, stream_id); if (!stream || !stream->task || m->aborted) { @@ -527,7 +529,7 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam) h2_beam_log(beam, m->c, APLOG_TRACE2, "out_open"); } else { - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, m->c, + ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, "h2_mplx(%s): out open", stream->task->id); } @@ -539,8 +541,8 @@ static apr_status_t out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam) /* we might see some file buckets in the output, see * if we have enough handles reserved. */ - check_data_for(m, stream, 0); - return status; + check_data_for(m, stream, 1); + return APR_SUCCESS; } apr_status_t h2_mplx_out_open(h2_mplx *m, int stream_id, h2_bucket_beam *beam) @@ -582,7 +584,7 @@ static apr_status_t out_close(h2_mplx *m, h2_task *task) status = h2_beam_close(task->output.beam); h2_beam_log(task->output.beam, m->c, APLOG_TRACE2, "out_close"); output_consumed_signal(m, task); - check_data_for(m, stream, 0); + check_data_for(m, stream, 1); return status; } @@ -616,15 +618,23 @@ apr_status_t h2_mplx_out_trywait(h2_mplx *m, apr_interval_time_t timeout, return status; } -static void check_data_for(h2_mplx *m, h2_stream *stream, int lock) +static void check_data_for(h2_mplx *m, h2_stream *stream, int mplx_is_locked) { + /* If m->lock is already held, we must release during h2_ififo_push() + * which can wait on its not_full condition, causing a deadlock because + * no one would then be able to acquire m->lock to empty the fifo. + */ + H2_MPLX_LEAVE_MAYBE(m, mplx_is_locked); if (h2_ififo_push(m->readyq, stream->id) == APR_SUCCESS) { + H2_MPLX_ENTER_ALWAYS(m); apr_atomic_set32(&m->event_pending, 1); - H2_MPLX_ENTER_MAYBE(m, lock); if (m->added_output) { apr_thread_cond_signal(m->added_output); } - H2_MPLX_LEAVE_MAYBE(m, lock); + H2_MPLX_LEAVE_MAYBE(m, !mplx_is_locked); + } + else { + H2_MPLX_ENTER_MAYBE(m, mplx_is_locked); } } @@ -677,7 +687,7 @@ apr_status_t h2_mplx_process(h2_mplx *m, struct h2_stream *stream, h2_ihash_add(m->streams, stream); if (h2_stream_is_ready(stream)) { /* already have a response */ - check_data_for(m, stream, 0); + check_data_for(m, stream, 1); ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, H2_STRM_MSG(stream, "process, add to readyq")); } @@ -808,7 +818,7 @@ static void task_done(h2_mplx *m, h2_task *task) } /* more data will not arrive, resume the stream */ - check_data_for(m, stream, 0); + check_data_for(m, stream, 1); } } else if ((stream = h2_ihash_get(m->shold, task->stream_id)) != NULL) { @@ -1060,7 +1070,7 @@ apr_status_t h2_mplx_idle(h2_mplx *m) 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); + check_data_for(m, stream, 1); stream->out_checked = 1; status = APR_EAGAIN; } @@ -1112,7 +1122,7 @@ apr_status_t h2_mplx_dispatch_master_events(h2_mplx *m, apr_status_t h2_mplx_keep_active(h2_mplx *m, h2_stream *stream) { - check_data_for(m, stream, 1); + check_data_for(m, stream, 0); return APR_SUCCESS; } diff --git a/mod_http2/h2_session.c b/mod_http2/h2_session.c index 4fae148c..a5cc306e 100644 --- a/mod_http2/h2_session.c +++ b/mod_http2/h2_session.c @@ -2141,7 +2141,7 @@ apr_status_t h2_session_process(h2_session *session, int async) break; case H2_SESSION_ST_IDLE: - if (session->idle_until && (apr_time_now() + session->idle_delay) > session->idle_until) { + if (session->idle_until && (now + session->idle_delay) > session->idle_until) { ap_log_cerror( APLOG_MARK, APLOG_TRACE1, status, c, H2_SSSN_MSG(session, "idle, timeout reached, closing")); if (session->idle_delay) { diff --git a/mod_http2/h2_util.c b/mod_http2/h2_util.c index 0059edf1..895e0ffe 100644 --- a/mod_http2/h2_util.c +++ b/mod_http2/h2_util.c @@ -638,15 +638,6 @@ apr_status_t h2_fifo_term(h2_fifo *fifo) apr_status_t rv; if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { fifo->aborted = 1; - apr_thread_mutex_unlock(fifo->lock); - } - return rv; -} - -apr_status_t h2_fifo_interrupt(h2_fifo *fifo) -{ - apr_status_t rv; - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { apr_thread_cond_broadcast(fifo->not_empty); apr_thread_cond_broadcast(fifo->not_full); apr_thread_mutex_unlock(fifo->lock); @@ -710,10 +701,6 @@ static apr_status_t fifo_push(h2_fifo *fifo, void *elem, int block) { apr_status_t rv; - if (fifo->aborted) { - return APR_EOF; - } - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { rv = fifo_push_int(fifo, elem, block); apr_thread_mutex_unlock(fifo->lock); @@ -754,10 +741,6 @@ static apr_status_t fifo_pull(h2_fifo *fifo, void **pelem, int block) { apr_status_t rv; - if (fifo->aborted) { - return APR_EOF; - } - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { rv = pull_head(fifo, pelem, block); apr_thread_mutex_unlock(fifo->lock); @@ -946,15 +929,6 @@ apr_status_t h2_ififo_term(h2_ififo *fifo) apr_status_t rv; if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { fifo->aborted = 1; - apr_thread_mutex_unlock(fifo->lock); - } - return rv; -} - -apr_status_t h2_ififo_interrupt(h2_ififo *fifo) -{ - apr_status_t rv; - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { apr_thread_cond_broadcast(fifo->not_empty); apr_thread_cond_broadcast(fifo->not_full); apr_thread_mutex_unlock(fifo->lock); @@ -1018,10 +992,6 @@ static apr_status_t ififo_push(h2_ififo *fifo, int id, int block) { apr_status_t rv; - if (fifo->aborted) { - return APR_EOF; - } - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { rv = ififo_push_int(fifo, id, block); apr_thread_mutex_unlock(fifo->lock); @@ -1062,10 +1032,6 @@ static apr_status_t ififo_pull(h2_ififo *fifo, int *pi, int block) { apr_status_t rv; - if (fifo->aborted) { - return APR_EOF; - } - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { rv = ipull_head(fifo, pi, block); apr_thread_mutex_unlock(fifo->lock); @@ -1088,10 +1054,6 @@ static apr_status_t ififo_peek(h2_ififo *fifo, h2_ififo_peek_fn *fn, void *ctx, apr_status_t rv; int id; - if (fifo->aborted) { - return APR_EOF; - } - if (APR_SUCCESS == (rv = apr_thread_mutex_lock(fifo->lock))) { if (APR_SUCCESS == (rv = ipull_head(fifo, &id, block))) { switch (fn(id, ctx)) { @@ -1117,39 +1079,40 @@ apr_status_t h2_ififo_try_peek(h2_ififo *fifo, h2_ififo_peek_fn *fn, void *ctx) return ififo_peek(fifo, fn, ctx, 0); } -apr_status_t h2_ififo_remove(h2_ififo *fifo, int id) +static apr_status_t ififo_remove(h2_ififo *fifo, int id) { - apr_status_t rv; + int rc, i; if (fifo->aborted) { return APR_EOF; } - if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { - int i, rc; - int e; - - rc = 0; - for (i = 0; i < fifo->count; ++i) { - e = fifo->elems[inth_index(fifo, i)]; - if (e == id) { - ++rc; - } - else if (rc) { - fifo->elems[inth_index(fifo, i-rc)] = e; - } - } - if (rc) { - fifo->count -= rc; - if (fifo->count + rc == fifo->nelems) { - apr_thread_cond_broadcast(fifo->not_full); - } - rv = APR_SUCCESS; + rc = 0; + for (i = 0; i < fifo->count; ++i) { + int e = fifo->elems[inth_index(fifo, i)]; + if (e == id) { + ++rc; } - else { - rv = APR_EAGAIN; + else if (rc) { + fifo->elems[inth_index(fifo, i-rc)] = e; } - + } + if (!rc) { + return APR_EAGAIN; + } + fifo->count -= rc; + if (fifo->count + rc == fifo->nelems) { + apr_thread_cond_broadcast(fifo->not_full); + } + return APR_SUCCESS; +} + +apr_status_t h2_ififo_remove(h2_ififo *fifo, int id) +{ + apr_status_t rv; + + if ((rv = apr_thread_mutex_lock(fifo->lock)) == APR_SUCCESS) { + rv = ififo_remove(fifo, id); apr_thread_mutex_unlock(fifo->lock); } return rv; diff --git a/mod_http2/h2_util.h b/mod_http2/h2_util.h index e24fc604..6c676eb4 100644 --- a/mod_http2/h2_util.h +++ b/mod_http2/h2_util.h @@ -209,7 +209,6 @@ apr_status_t h2_fifo_create(h2_fifo **pfifo, apr_pool_t *pool, int capacity); apr_status_t h2_fifo_set_create(h2_fifo **pfifo, apr_pool_t *pool, int capacity); apr_status_t h2_fifo_term(h2_fifo *fifo); -apr_status_t h2_fifo_interrupt(h2_fifo *fifo); int h2_fifo_count(h2_fifo *fifo); @@ -280,7 +279,6 @@ apr_status_t h2_ififo_create(h2_ififo **pfifo, apr_pool_t *pool, int capacity); apr_status_t h2_ififo_set_create(h2_ififo **pfifo, apr_pool_t *pool, int capacity); apr_status_t h2_ififo_term(h2_ififo *fifo); -apr_status_t h2_ififo_interrupt(h2_ififo *fifo); int h2_ififo_count(h2_ififo *fifo); diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h index 5a52670d..7dbd37da 100644 --- a/mod_http2/h2_version.h +++ b/mod_http2/h2_version.h @@ -27,7 +27,7 @@ * @macro * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "1.15.4-git" +#define MOD_HTTP2_VERSION "1.15.5-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 0x010f04 +#define MOD_HTTP2_VERSION_NUM 0x010f05 #endif /* mod_h2_h2_version_h */ diff --git a/mod_http2/h2_workers.c b/mod_http2/h2_workers.c index 699f533f..52f1a703 100644 --- a/mod_http2/h2_workers.c +++ b/mod_http2/h2_workers.c @@ -269,7 +269,6 @@ static apr_status_t workers_pool_cleanup(void *data) } h2_fifo_term(workers->mplxs); - h2_fifo_interrupt(workers->mplxs); cleanup_zombies(workers); }