Skip to content

Commit

Permalink
* Fixed rare cases where a h2 worker could deadlock the main connect…
Browse files Browse the repository at this point in the history 
…ion. All the good

   bits in this thanks to Yann Ylavic.
  • Loading branch information
Stefan Eissing committed Dec 19, 2019
1 parent 34e0f64 commit c2c2000
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 94 deletions.
9 changes: 7 additions & 2 deletions ChangeLog
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion configure.ac
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
#

AC_PREREQ([2.69])
AC_INIT([mod_http2], [1.15.4], [[email protected]])
AC_INIT([mod_http2], [1.15.5], [[email protected]])

LT_PREREQ([2.2.6])
LT_INIT()
Expand Down
54 changes: 32 additions & 22 deletions mod_http2/h2_mplx.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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);
}
Expand All @@ -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;
}
}
}

Expand Down Expand Up @@ -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) {
Expand All @@ -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);
}

Expand All @@ -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)
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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"));
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
2 changes: 1 addition & 1 deletion mod_http2/h2_session.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
89 changes: 26 additions & 63 deletions mod_http2/h2_util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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)) {
Expand All @@ -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;
Expand Down
2 changes: 0 additions & 2 deletions mod_http2/h2_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);

Expand Down
4 changes: 2 additions & 2 deletions mod_http2/h2_version.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,15 @@
* @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
* Numerical representation of the version number of the http2 module
* 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 */
1 change: 0 additions & 1 deletion mod_http2/h2_workers.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down

0 comments on commit c2c2000

Please sign in to comment.