From: Stefan Eissing Date: Mon, 23 Nov 2015 14:30:07 +0000 (+0000) Subject: fixes races during session shutdown when connection is aborted X-Git-Tag: 2.5.0-alpha~2604 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=1497a895302b918c2a945cde0b6d0c22aa48b335;p=apache fixes races during session shutdown when connection is aborted git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1715833 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/modules/http2/h2_bucket_eoc.c b/modules/http2/h2_bucket_eoc.c index 8b145cf29e..3ddb54d68a 100644 --- a/modules/http2/h2_bucket_eoc.c +++ b/modules/http2/h2_bucket_eoc.c @@ -90,10 +90,11 @@ static void bucket_destroy(void *data) if (apr_bucket_shared_destroy(h)) { h2_session *session = h->session; + apr_bucket_free(h); if (session) { h2_session_eoc_callback(session); + /* all is gone now */ } - apr_bucket_free(h); } } diff --git a/modules/http2/h2_config.c b/modules/http2/h2_config.c index 7ac4297b32..7dc0b20d20 100644 --- a/modules/http2/h2_config.c +++ b/modules/http2/h2_config.c @@ -43,7 +43,7 @@ static h2_config defconf = { H2_INITIAL_WINDOW_SIZE, /* window_size */ -1, /* min workers */ -1, /* max workers */ - 10 * 60, /* max workers idle secs */ + 10, /* max workers idle secs */ 64 * 1024, /* stream max mem size */ NULL, /* no alt-svcs */ -1, /* alt-svc max age */ diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c index d4f56c6630..6fec75ea9a 100644 --- a/modules/http2/h2_conn.c +++ b/modules/http2/h2_conn.c @@ -177,10 +177,6 @@ apr_status_t h2_conn_process(conn_rec *c, request_rec *r) ap_log_cerror( APLOG_MARK, APLOG_DEBUG, status, session->c, "h2_session(%ld): done", session->id); - h2_session_close(session); - h2_session_flush(session); - /* hereafter session might be gone */ - /* Make sure this connection gets closed properly. */ ap_update_child_status_from_conn(c->sbh, SERVER_CLOSING, c); c->keepalive = AP_CONN_CLOSE; @@ -188,6 +184,8 @@ apr_status_t h2_conn_process(conn_rec *c, request_rec *r) c->cs->state = CONN_STATE_WRITE_COMPLETION; } + h2_session_close(session); + /* hereafter session will be gone */ return status; } diff --git a/modules/http2/h2_conn_io.c b/modules/http2/h2_conn_io.c index aa8d4d5802..485a8bd47e 100644 --- a/modules/http2/h2_conn_io.c +++ b/modules/http2/h2_conn_io.c @@ -23,6 +23,7 @@ #include #include "h2_private.h" +#include "h2_bucket_eoc.h" #include "h2_config.h" #include "h2_conn_io.h" #include "h2_h2.h" @@ -44,20 +45,20 @@ #define WRITE_BUFFER_SIZE (8*WRITE_SIZE_MAX) -apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c) +apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c, apr_pool_t *pool) { h2_config *cfg = h2_config_get(c); io->connection = c; - io->input = apr_brigade_create(c->pool, c->bucket_alloc); - io->output = apr_brigade_create(c->pool, c->bucket_alloc); + io->input = apr_brigade_create(pool, c->bucket_alloc); + io->output = apr_brigade_create(pool, c->bucket_alloc); io->buflen = 0; io->is_tls = h2_h2_is_tls(c); io->buffer_output = io->is_tls; if (io->buffer_output) { io->bufsize = WRITE_BUFFER_SIZE; - io->buffer = apr_pcalloc(c->pool, io->bufsize); + io->buffer = apr_pcalloc(pool, io->bufsize); } else { io->bufsize = 0; @@ -115,6 +116,8 @@ static apr_status_t h2_conn_io_bucket_read(h2_conn_io *io, &bucket_length, block); if (status == APR_SUCCESS && bucket_length > 0) { + apr_size_t consumed = 0; + if (APLOGctrace2(io->connection)) { char buffer[32]; h2_util_hex_dump(buffer, sizeof(buffer)/sizeof(buffer[0]), @@ -124,20 +127,18 @@ static apr_status_t h2_conn_io_bucket_read(h2_conn_io *io, io->connection->id, (int)bucket_length, buffer); } - if (bucket_length > 0) { - apr_size_t consumed = 0; - status = on_read_cb(bucket_data, bucket_length, - &consumed, pdone, puser); - if (status == APR_SUCCESS && bucket_length > consumed) { - /* We have data left in the bucket. Split it. */ - status = apr_bucket_split(bucket, consumed); - } - readlen += consumed; + status = on_read_cb(bucket_data, bucket_length, &consumed, + pdone, puser); + if (status == APR_SUCCESS && bucket_length > consumed) { + /* We have data left in the bucket. Split it. */ + status = apr_bucket_split(bucket, consumed); } + readlen += consumed; } } apr_bucket_delete(bucket); } + if (readlen == 0 && status == APR_SUCCESS && block == APR_NONBLOCK_READ) { return APR_EAGAIN; } @@ -158,10 +159,10 @@ apr_status_t h2_conn_io_read(h2_conn_io *io, /* Seems something is left from a previous read, lets * satisfy our caller with the data we already have. */ status = h2_conn_io_bucket_read(io, block, on_read_cb, puser, &done); + apr_brigade_cleanup(io->input); if (status != APR_SUCCESS || done) { return status; } - apr_brigade_cleanup(io->input); } /* We only do a blocking read when we have no streams to process. So, @@ -179,6 +180,9 @@ apr_status_t h2_conn_io_read(h2_conn_io *io, ap_update_child_status(io->connection->sbh, SERVER_BUSY_READ, NULL); } + /* TODO: replace this with a connection filter itself, so that we + * no longer need to transfer incoming buckets to our own brigade. + */ status = ap_get_brigade(io->connection->input_filters, io->input, AP_MODE_READBYTES, block, 64 * 4096); @@ -379,4 +383,19 @@ apr_status_t h2_conn_io_flush(h2_conn_io *io) apr_status_t h2_conn_io_pass(h2_conn_io *io) { return h2_conn_io_flush_int(io, 0); +} + +apr_status_t h2_conn_io_close(h2_conn_io *io, void *session) +{ + apr_bucket *b; + + /* Send out anything in our buffers */ + h2_conn_io_flush_int(io, 0); + + b = h2_bucket_eoc_create(io->connection->bucket_alloc, session); + APR_BRIGADE_INSERT_TAIL(io->output, b); + b = apr_bucket_flush_create(io->connection->bucket_alloc); + APR_BRIGADE_INSERT_TAIL(io->output, b); + return ap_pass_brigade(io->connection->output_filters, io->output); + /* and all is gone */ } \ No newline at end of file diff --git a/modules/http2/h2_conn_io.h b/modules/http2/h2_conn_io.h index 4406261a33..a0dd0d0e5c 100644 --- a/modules/http2/h2_conn_io.h +++ b/modules/http2/h2_conn_io.h @@ -42,7 +42,7 @@ typedef struct { int unflushed; } h2_conn_io; -apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c); +apr_status_t h2_conn_io_init(h2_conn_io *io, conn_rec *c, apr_pool_t *pool); int h2_conn_io_is_buffered(h2_conn_io *io); @@ -65,5 +65,6 @@ apr_status_t h2_conn_io_consider_flush(h2_conn_io *io); apr_status_t h2_conn_io_pass(h2_conn_io *io); apr_status_t h2_conn_io_flush(h2_conn_io *io); +apr_status_t h2_conn_io_close(h2_conn_io *io, void *session); #endif /* defined(__mod_h2__h2_conn_io__) */ diff --git a/modules/http2/h2_io_set.c b/modules/http2/h2_io_set.c index 74ab508fef..2bb6e69469 100644 --- a/modules/http2/h2_io_set.c +++ b/modules/http2/h2_io_set.c @@ -145,37 +145,23 @@ h2_io *h2_io_set_pop_highest_prio(h2_io_set *set) return NULL; } -void h2_io_set_destroy_all(h2_io_set *sp) -{ - int i; - for (i = 0; i < sp->list->nelts; ++i) { - h2_io *io = h2_io_IDX(sp->list, i); - h2_io_destroy(io); - } - sp->list->nelts = 0; -} - -void h2_io_set_remove_all(h2_io_set *sp) -{ - sp->list->nelts = 0; -} - int h2_io_set_is_empty(h2_io_set *sp) { AP_DEBUG_ASSERT(sp); return sp->list->nelts == 0; } -void h2_io_set_iter(h2_io_set *sp, +int h2_io_set_iter(h2_io_set *sp, h2_io_set_iter_fn *iter, void *ctx) { int i; for (i = 0; i < sp->list->nelts; ++i) { h2_io *s = h2_io_IDX(sp->list, i); if (!iter(ctx, s)) { - break; + return 0; } } + return 1; } apr_size_t h2_io_set_size(h2_io_set *sp) diff --git a/modules/http2/h2_io_set.h b/modules/http2/h2_io_set.h index 5e7555af92..04ff8702ed 100644 --- a/modules/http2/h2_io_set.h +++ b/modules/http2/h2_io_set.h @@ -32,16 +32,24 @@ apr_status_t h2_io_set_add(h2_io_set *set, struct h2_io *io); h2_io *h2_io_set_get(h2_io_set *set, int stream_id); h2_io *h2_io_set_remove(h2_io_set *set, struct h2_io *io); -void h2_io_set_remove_all(h2_io_set *set); -void h2_io_set_destroy_all(h2_io_set *set); int h2_io_set_is_empty(h2_io_set *set); apr_size_t h2_io_set_size(h2_io_set *set); typedef int h2_io_set_iter_fn(void *ctx, struct h2_io *io); -void h2_io_set_iter(h2_io_set *set, - h2_io_set_iter_fn *iter, void *ctx); +/** + * Iterator over all h2_io* in the set or until a + * callback returns 0. It is not safe to add or remove + * set members during iteration. + * + * @param set the set of h2_io to iterate over + * @param iter the function to call for each io + * @param ctx user data for the callback + * @return 1 iff iteration completed for all members + */ +int h2_io_set_iter(h2_io_set *set, + h2_io_set_iter_fn *iter, void *ctx); h2_io *h2_io_set_pop_highest_prio(h2_io_set *set); diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c index 3908590985..1257ec79a3 100644 --- a/modules/http2/h2_mplx.c +++ b/modules/http2/h2_mplx.c @@ -73,6 +73,9 @@ static void have_out_data_for(h2_mplx *m, int stream_id); static void h2_mplx_destroy(h2_mplx *m) { AP_DEBUG_ASSERT(m); + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, + "h2_mplx(%ld): destroy, refs=%d", + m->id, m->refs); m->aborted = 1; if (m->ready_ios) { h2_io_set_destroy(m->ready_ios); @@ -83,15 +86,6 @@ static void h2_mplx_destroy(h2_mplx *m) m->stream_ios = NULL; } - if (m->lock) { - apr_thread_mutex_destroy(m->lock); - m->lock = NULL; - } - - if (m->spare_pool) { - apr_pool_destroy(m->spare_pool); - m->spare_pool = NULL; - } if (m->pool) { apr_pool_destroy(m->pool); } @@ -199,13 +193,62 @@ static void workers_unregister(h2_mplx *m) { h2_workers_unregister(m->workers, m); } +static void io_destroy(h2_mplx *m, h2_io *io) +{ + apr_pool_t *pool = io->pool; + + io->pool = NULL; + /* The pool is cleared/destroyed which also closes all + * allocated file handles. Give this count back to our + * file handle pool. */ + m->file_handles_allowed += io->files_handles_owned; + h2_io_set_remove(m->stream_ios, io); + h2_io_set_remove(m->ready_ios, io); + h2_io_destroy(io); + + if (pool) { + apr_pool_clear(pool); + if (m->spare_pool) { + apr_pool_destroy(m->spare_pool); + } + m->spare_pool = pool; + } +} + +static int io_stream_done(h2_mplx *m, h2_io *io, int rst_error) +{ + /* Remove io from ready set, we will never submit it */ + h2_io_set_remove(m->ready_ios, io); + if (io->task_done || h2_tq_remove(m->q, io->id)) { + /* already finished or not even started yet */ + io_destroy(m, io); + return 0; + } + else { + /* cleanup once task is done */ + io->orphaned = 1; + if (rst_error) { + h2_io_rst(io, rst_error); + } + return 1; + } +} + +static int stream_done_iter(void *ctx, h2_io *io) { + return io_stream_done((h2_mplx*)ctx, io, 0); +} + apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) { apr_status_t status; + workers_unregister(m); - status = apr_thread_mutex_lock(m->lock); if (APR_SUCCESS == status) { + while (!h2_io_set_iter(m->stream_ios, stream_done_iter, m)) { + /* iterator until all h2_io have been orphaned or destroyed */ + } + release(m, 0); while (m->refs > 0) { m->join_wait = wait; @@ -215,10 +258,11 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) apr_thread_cond_wait(wait, m->lock); } ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, - "h2_mplx(%ld): release_join -> destroy", m->id); - m->pool = NULL; - apr_thread_mutex_unlock(m->lock); + "h2_mplx(%ld): release_join -> destroy, (#ios=%ld)", + m->id, (long)h2_io_set_size(m->stream_ios)); h2_mplx_destroy(m); + /* all gone */ + /*apr_thread_mutex_unlock(m->lock);*/ } return status; } @@ -230,33 +274,8 @@ void h2_mplx_abort(h2_mplx *m) status = apr_thread_mutex_lock(m->lock); if (APR_SUCCESS == status) { m->aborted = 1; - h2_io_set_destroy_all(m->stream_ios); apr_thread_mutex_unlock(m->lock); } - workers_unregister(m); -} - - -static void io_destroy(h2_mplx *m, h2_io *io) -{ - apr_pool_t *pool = io->pool; - - io->pool = NULL; - /* The pool is cleared/destroyed which also closes all - * allocated file handles. Give this count back to our - * file handle pool. */ - m->file_handles_allowed += io->files_handles_owned; - h2_io_set_remove(m->stream_ios, io); - h2_io_set_remove(m->ready_ios, io); - h2_io_destroy(io); - - if (pool) { - apr_pool_clear(pool); - if (m->spare_pool) { - apr_pool_destroy(m->spare_pool); - } - m->spare_pool = pool; - } } apr_status_t h2_mplx_stream_done(h2_mplx *m, int stream_id, int rst_error) @@ -264,9 +283,6 @@ apr_status_t h2_mplx_stream_done(h2_mplx *m, int stream_id, int rst_error) apr_status_t status; AP_DEBUG_ASSERT(m); - if (m->aborted) { - return APR_ECONNABORTED; - } status = apr_thread_mutex_lock(m->lock); if (APR_SUCCESS == status) { h2_io *io = h2_io_set_get(m->stream_ios, stream_id); @@ -275,20 +291,7 @@ apr_status_t h2_mplx_stream_done(h2_mplx *m, int stream_id, int rst_error) * for processing, e.g. when we received all HEADERs. But when * a stream is cancelled very early, it will not exist. */ if (io) { - /* Remove io from ready set, we will never submit it */ - h2_io_set_remove(m->ready_ios, io); - if (io->task_done || h2_tq_remove(m->q, io->id)) { - /* already finished or not even started yet */ - io_destroy(m, io); - } - else { - /* cleanup once task is done */ - io->orphaned = 1; - if (rst_error) { - h2_io_rst(io, rst_error); - } - } - + io_stream_done(m, io, rst_error); } apr_thread_mutex_unlock(m->lock); diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index d70eefd296..7e4ed96e40 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -24,7 +24,6 @@ #include #include "h2_private.h" -#include "h2_bucket_eoc.h" #include "h2_bucket_eos.h" #include "h2_config.h" #include "h2_h2.h" @@ -84,11 +83,6 @@ h2_stream *h2_session_open_stream(h2_session *session, int stream_id) return stream; } -apr_status_t h2_session_flush(h2_session *session) -{ - return h2_conn_io_flush(&session->io); -} - /** * Determine the importance of streams when scheduling tasks. * - if both stream depend on the same one, compare weights @@ -612,13 +606,12 @@ static h2_session *h2_session_create_int(conn_rec *c, session->c = c; session->r = r; + session->pool = pool; apr_pool_pre_cleanup_register(pool, session, session_pool_cleanup); session->max_stream_count = h2_config_geti(config, H2_CONF_MAX_STREAMS); session->max_stream_mem = h2_config_geti(config, H2_CONF_STREAM_MAX_MEM); - session->pool = pool; - status = apr_thread_cond_create(&session->iowait, session->pool); if (status != APR_SUCCESS) { return NULL; @@ -629,7 +622,7 @@ static h2_session *h2_session_create_int(conn_rec *c, session->workers = workers; session->mplx = h2_mplx_create(c, session->pool, workers); - h2_conn_io_init(&session->io, c); + h2_conn_io_init(&session->io, c, session->pool); session->bbtmp = apr_brigade_create(session->pool, c->bucket_alloc); status = init_callbacks(c, &callbacks); @@ -703,10 +696,6 @@ static void h2_session_cleanup(h2_session *session) apr_pool_destroy(session->spare); session->spare = NULL; } - if (session->mplx) { - h2_mplx_release_and_join(session->mplx, session->iowait); - session->mplx = NULL; - } } void h2_session_destroy(h2_session *session) @@ -714,6 +703,10 @@ void h2_session_destroy(h2_session *session) AP_DEBUG_ASSERT(session); h2_session_cleanup(session); + if (session->mplx) { + h2_mplx_release_and_join(session->mplx, session->iowait); + session->mplx = NULL; + } if (session->streams) { if (!h2_stream_set_is_empty(session->streams)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c, @@ -993,10 +986,8 @@ apr_status_t h2_session_close(h2_session *session) ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0,session->c, "h2_session: closing, writing eoc"); - h2_session_cleanup(session); - return h2_conn_io_writeb(&session->io, - h2_bucket_eoc_create(session->c->bucket_alloc, - session)); + h2_session_cleanup(session); + return h2_conn_io_close(&session->io, session); } static ssize_t stream_data_cb(nghttp2_session *ng2s, diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h index 90052fc9e7..5c3be0a53b 100644 --- a/modules/http2/h2_session.h +++ b/modules/http2/h2_session.h @@ -147,12 +147,6 @@ apr_status_t h2_session_start(h2_session *session, int *rv); */ apr_status_t h2_session_abort(h2_session *session, apr_status_t reason, int rv); -/** - * Pass any buffered output data through the connection filters. - * @param session the session to flush - */ -apr_status_t h2_session_flush(h2_session *session); - /** * Called before a session gets destroyed, might flush output etc. */ diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c index b7d48a1bf6..b529db199c 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -228,8 +228,8 @@ apr_status_t h2_task_do(h2_task *task, h2_worker *worker) apr_thread_cond_signal(task->io); } - h2_mplx_task_done(task->mplx, task->stream_id); h2_worker_release_task(worker, task); + h2_mplx_task_done(task->mplx, task->stream_id); return status; } diff --git a/modules/http2/h2_version.h b/modules/http2/h2_version.h index 98a431b3bf..76dd2db0fc 100644 --- a/modules/http2/h2_version.h +++ b/modules/http2/h2_version.h @@ -20,7 +20,7 @@ * @macro * Version number of the h2 module as c string */ -#define MOD_HTTP2_VERSION "1.0.5-DEV" +#define MOD_HTTP2_VERSION "1.0.6-DEV" /** * @macro @@ -28,7 +28,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 0x010005 +#define MOD_HTTP2_VERSION_NUM 0x010006 #endif /* mod_h2_h2_version_h */ diff --git a/modules/http2/h2_worker.c b/modules/http2/h2_worker.c index b11e8549ff..3119cb081e 100644 --- a/modules/http2/h2_worker.c +++ b/modules/http2/h2_worker.c @@ -96,8 +96,9 @@ h2_worker *h2_worker_create(int id, apr_allocator_t *allocator = NULL; apr_pool_t *pool = NULL; h2_worker *w; + apr_status_t status; - apr_status_t status = apr_allocator_create(&allocator); + status = apr_allocator_create(&allocator); if (status != APR_SUCCESS) { return NULL; } @@ -126,7 +127,6 @@ h2_worker *h2_worker_create(int id, apr_pool_pre_cleanup_register(w->pool, w, cleanup_join_thread); apr_thread_create(&w->thread, attr, execute, w, w->pool); - apr_pool_create(&w->task_pool, w->pool); } return w; } @@ -167,7 +167,11 @@ h2_task *h2_worker_create_task(h2_worker *worker, h2_mplx *m, /* Create a subpool from the worker one to be used for all things * with life-time of this task execution. */ + if (!worker->task_pool) { + apr_pool_create(&worker->task_pool, worker->pool); + } task = h2_task_create(m->id, req, worker->task_pool, m, eos); + /* Link the task to the worker which provides useful things such * as mutex, a socket etc. */ task->io = worker->io;