diff --git a/ChangeLog b/ChangeLog index dbbd845a..c2753ae9 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,15 @@ +v1.5.7 +-------------------------------------------------------------------------------- + * fixed read after free on certain conditions when clients abort connections + * fixed connection shutdown when stream tasks block on reads + +v1.5.6 +-------------------------------------------------------------------------------- + * fixed possible null pointer reference in new bucket beams during read on an + aborted beam + * improved event handling on master connection (idle/submit/wait) to increase + performance + v1.5.5 -------------------------------------------------------------------------------- * Fix async write issue that sometimes led to selection of wrong timeout diff --git a/configure.ac b/configure.ac index 4829ef7e..a16ec649 100644 --- a/configure.ac +++ b/configure.ac @@ -14,7 +14,7 @@ # AC_PREREQ([2.69]) -AC_INIT([mod_http2], [1.5.6], [stefan.eissing@greenbytes.de]) +AC_INIT([mod_http2], [1.5.7], [stefan.eissing@greenbytes.de]) LT_PREREQ([2.2.6]) LT_INIT() diff --git a/mod_http2/h2_bucket_eos.c b/mod_http2/h2_bucket_eos.c index 9953ca15..28c34fdc 100644 --- a/mod_http2/h2_bucket_eos.c +++ b/mod_http2/h2_bucket_eos.c @@ -38,10 +38,8 @@ static apr_status_t bucket_cleanup(void *data) h2_stream **pstream = data; if (*pstream) { - /* - * If bucket_destroy is called after us, this prevents - * bucket_destroy from trying to destroy the pool again. - */ + /* If bucket_destroy is called after us, this prevents + * bucket_destroy from trying to destroy the stream again. */ *pstream = NULL; } return APR_SUCCESS; @@ -92,10 +90,13 @@ static void bucket_destroy(void *data) if (apr_bucket_shared_destroy(h)) { h2_stream *stream = h->stream; + if (stream && stream->pool) { + apr_pool_cleanup_kill(stream->pool, &h->stream, bucket_cleanup); + } + apr_bucket_free(h); if (stream) { h2_stream_eos_destroy(stream); } - apr_bucket_free(h); } } diff --git a/mod_http2/h2_mplx.c b/mod_http2/h2_mplx.c index f7b30fff..c09e45ac 100644 --- a/mod_http2/h2_mplx.c +++ b/mod_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, 1); + task_destroy(m, task, 0); } return 0; } @@ -349,10 +349,12 @@ 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 */ - status = h2_task_shutdown(task, 0); - if (status != APR_SUCCESS){ - ap_log_cerror(APLOG_MARK, APLOG_WARNING, status, m->c, APLOGNO(03385) - "h2_task(%s): shutdown", task->id); + 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) { @@ -446,10 +448,8 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error) if (rst_error) { h2_task_rst(task, rst_error); } - /* FIXME: this should work, but does not h2_ihash_add(m->shold, stream); - return;*/ - task->input.beam = NULL; + return; } else { /* already finished */ @@ -500,6 +500,12 @@ static int task_abort_connection(void *ctx, void *val) if (task->c) { task->c->aborted = 1; } + if (task->input.beam) { + h2_beam_abort(task->input.beam); + } + if (task->output.beam) { + h2_beam_abort(task->output.beam); + } return 1; } @@ -603,7 +609,7 @@ apr_status_t h2_mplx_release_and_join(h2_mplx *m, apr_thread_cond_t *wait) AP_DEBUG_ASSERT(h2_ihash_empty(m->shold)); if (!h2_ihash_empty(m->spurge)) { ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, m->c, - "h2_mplx(%ld): release_join %d streams to purge", + "h2_mplx(%ld): 3. release_join %d streams to purge", m->id, (int)h2_ihash_count(m->spurge)); purge_streams(m); } @@ -1028,6 +1034,7 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) * 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/mod_http2/h2_mplx.h b/mod_http2/h2_mplx.h index e92a5ea3..f2f60fc9 100644 --- a/mod_http2/h2_mplx.h +++ b/mod_http2/h2_mplx.h @@ -167,7 +167,7 @@ apr_uint32_t h2_mplx_shutdown(h2_mplx *m); * Notifies mplx that a stream has finished processing. * * @param m the mplx itself - * @param stream_id the id of the stream being done + * @param stream the id of the stream being done * @param rst_error if != 0, the stream was reset with the error given * */ @@ -188,7 +188,7 @@ apr_status_t h2_mplx_out_trywait(h2_mplx *m, apr_interval_time_t timeout, * Process a stream request. * * @param m the multiplexer - * @param stream_id the identifier of the stream + * @param stream the identifier of the stream * @param r the request to be processed * @param cmp the stream priority compare function * @param ctx context data for the compare function diff --git a/mod_http2/h2_push.h b/mod_http2/h2_push.h index d3519dcb..62f5a0a7 100644 --- a/mod_http2/h2_push.h +++ b/mod_http2/h2_push.h @@ -63,7 +63,7 @@ apr_array_header_t *h2_push_collect(apr_pool_t *p, /** * Create a new push diary for the given maximum number of entries. * - * @oaram p the pool to use + * @param p the pool to use * @param N the max number of entries, rounded up to 2^x * @return the created diary, might be NULL of max_entries is 0 */ diff --git a/mod_http2/h2_stream.c b/mod_http2/h2_stream.c index acd5072c..3beab4b3 100644 --- a/mod_http2/h2_stream.c +++ b/mod_http2/h2_stream.c @@ -240,7 +240,7 @@ void h2_stream_destroy(h2_stream *stream) void h2_stream_eos_destroy(h2_stream *stream) { h2_session_stream_done(stream->session, stream); - /* stream is gone */ + /* stream possibly destroyed */ } apr_pool_t *h2_stream_detach_pool(h2_stream *stream) diff --git a/mod_http2/h2_stream.h b/mod_http2/h2_stream.h index dd2c1933..f80f8115 100644 --- a/mod_http2/h2_stream.h +++ b/mod_http2/h2_stream.h @@ -103,7 +103,7 @@ void h2_stream_cleanup(h2_stream *stream); * destruction to take the pool with it. * * @param stream the stream to detach the pool from - * @param the detached memmory pool or NULL if stream no longer has one + * @result the detached memory pool or NULL if stream no longer has one */ apr_pool_t *h2_stream_detach_pool(h2_stream *stream); @@ -153,7 +153,7 @@ apr_status_t h2_stream_write_data(h2_stream *stream, * @param stream the stream to reset * @param error_code the HTTP/2 error code */ -void h2_stream_rst(h2_stream *streamm, int error_code); +void h2_stream_rst(h2_stream *stream, int error_code); /** * Schedule the stream for execution. All header information must be @@ -182,7 +182,7 @@ struct h2_response *h2_stream_get_response(h2_stream *stream); * the stream response has been collected. * * @param stream the stream to set the response for - * @param resonse the response data for the stream + * @param response the response data for the stream * @param bb bucket brigade with output data for the stream. Optional, * may be incomplete. */ diff --git a/mod_http2/h2_task.c b/mod_http2/h2_task.c index f505e859..8cf1ce1e 100644 --- a/mod_http2/h2_task.c +++ b/mod_http2/h2_task.c @@ -88,7 +88,7 @@ static apr_status_t input_handle_eos(h2_task *task, request_rec *r, { apr_status_t status = APR_SUCCESS; apr_bucket_brigade *bb = task->input.bb; - apr_table_t *t = task->request->trailers; + apr_table_t *t = task->request? task->request->trailers : NULL; if (task->input.chunked) { task->input.tmp = apr_brigade_split_ex(bb, b, task->input.tmp); @@ -114,7 +114,7 @@ static apr_status_t input_append_eos(h2_task *task, request_rec *r) { apr_status_t status = APR_SUCCESS; apr_bucket_brigade *bb = task->input.bb; - apr_table_t *t = task->request->trailers; + apr_table_t *t = task->request? task->request->trailers : NULL; if (task->input.chunked) { if (t && !apr_is_empty_table(t)) { @@ -151,13 +151,14 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, return ap_get_brigade(f->c->input_filters, bb, mode, block, readbytes); } - if (f->c->aborted) { + if (f->c->aborted || !task->request) { return APR_ECONNABORTED; } if (!task->input.bb) { if (!task->input.eos_written) { input_append_eos(task, f->r); + return APR_SUCCESS; } return APR_EOF; } @@ -172,11 +173,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, } } - while (APR_BRIGADE_EMPTY(task->input.bb)) { - if (task->input.eos_written) { - return APR_EOF; - } - + while (APR_BRIGADE_EMPTY(task->input.bb) && !task->input.eos) { /* Get more input data for our request. */ ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c, "h2_task(%s): get more data from mplx, block=%d, " @@ -193,7 +190,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, status = APR_EOF; } - ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, f->c, + ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, f->c, "h2_task(%s): read returned", task->id); if (APR_STATUS_IS_EAGAIN(status) && (mode == AP_MODE_GETLINE || block == APR_BLOCK_READ)) { @@ -202,7 +199,7 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, * upload 100k test on test-ser.example.org hangs */ status = APR_SUCCESS; } - else if (APR_STATUS_IS_EOF(status) && !task->input.eos_written) { + else if (APR_STATUS_IS_EOF(status)) { task->input.eos = 1; } else if (status != APR_SUCCESS) { @@ -251,8 +248,13 @@ static apr_status_t input_read(h2_task *task, ap_filter_t* f, } } - if (!task->input.eos_written && task->input.eos) { - input_append_eos(task, f->r); + if (task->input.eos) { + if (!task->input.eos_written) { + input_append_eos(task, f->r); + } + if (APR_BRIGADE_EMPTY(task->input.bb)) { + return APR_EOF; + } } h2_util_bb_log(f->c, task->stream_id, APLOG_TRACE2, @@ -556,23 +558,6 @@ void h2_task_rst(h2_task *task, int error) } } -apr_status_t h2_task_shutdown(h2_task *task, int block) -{ - if (task->output.beam) { - apr_status_t status; - status = h2_beam_shutdown(task->output.beam, APR_NONBLOCK_READ); - if (block && status == APR_EAGAIN) { - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, task->c, - "h2_task(%s): output shutdown waiting", task->id); - status = h2_beam_shutdown(task->output.beam, APR_BLOCK_READ); - ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, task->c, - "h2_task(%s): output shutdown done", task->id); - } - return status; - } - return APR_SUCCESS; -} - /******************************************************************************* * Register various hooks */ diff --git a/mod_http2/h2_task.h b/mod_http2/h2_task.h index dda8a562..010005a3 100644 --- a/mod_http2/h2_task.h +++ b/mod_http2/h2_task.h @@ -114,11 +114,6 @@ int h2_task_can_redo(h2_task *task); */ void h2_task_rst(h2_task *task, int error); -/** - * Shuts all input/output down. Clears any buckets buffered and closes. - */ -apr_status_t h2_task_shutdown(h2_task *task, int block); - void h2_task_register_hooks(void); /* * One time, post config intialization. diff --git a/mod_http2/h2_util.h b/mod_http2/h2_util.h index 99724d7a..61ffdbcb 100644 --- a/mod_http2/h2_util.h +++ b/mod_http2/h2_util.h @@ -58,7 +58,7 @@ void *h2_ihash_get(h2_ihash_t *ih, int id); * @param ih the hash to iterate over * @param fn the function to invoke on each member * @param ctx user supplied data passed into each iteration call - * @param 0 if one iteration returned 0, otherwise != 0 + * @return 0 if one iteration returned 0, otherwise != 0 */ int h2_ihash_iter(h2_ihash_t *ih, h2_ihash_iter_t *fn, void *ctx); @@ -141,14 +141,14 @@ int h2_iq_empty(h2_iqueue *q); int h2_iq_count(h2_iqueue *q); /** - * Add a stream idto the queue. + * Add a stream id to the queue. * * @param q the queue to append the task to * @param sid the stream id to add * @param cmp the comparator for sorting * @param ctx user data for comparator */ -void h2_iq_add(h2_iqueue *q, int i, h2_iq_cmp *cmp, void *ctx); +void h2_iq_add(h2_iqueue *q, int sid, h2_iq_cmp *cmp, void *ctx); /** * Remove the stream id from the queue. Return != 0 iff task @@ -157,7 +157,7 @@ void h2_iq_add(h2_iqueue *q, int i, h2_iq_cmp *cmp, void *ctx); * @param sid the stream id to remove * @return != 0 iff task was found in queue */ -int h2_iq_remove(h2_iqueue *q, int i); +int h2_iq_remove(h2_iqueue *q, int sid); /** * Remove all entries in the queue. @@ -374,19 +374,19 @@ apr_size_t h2_util_bb_print(char *buffer, apr_size_t bmax, * Logs the bucket brigade (which bucket types with what length) * to the log at the given level. * @param c the connection to log for - * @param stream_id the stream identifier this brigade belongs to + * @param sid the stream identifier this brigade belongs to * @param level the log level (as in APLOG_*) * @param tag a short message text about the context * @param bb the brigade to log */ -#define h2_util_bb_log(c, i, level, tag, bb) \ +#define h2_util_bb_log(c, sid, level, tag, bb) \ do { \ char buffer[4 * 1024]; \ const char *line = "(null)"; \ apr_size_t len, bmax = sizeof(buffer)/sizeof(buffer[0]); \ len = h2_util_bb_print(buffer, bmax, (tag), "", (bb)); \ ap_log_cerror(APLOG_MARK, level, 0, (c), "bb_dump(%ld-%d): %s", \ - (c)->id, (int)(i), (len? buffer : line)); \ + (c)->id, (int)(sid), (len? buffer : line)); \ } while(0) diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h index 020827cc..d5125b28 100644 --- a/mod_http2/h2_version.h +++ b/mod_http2/h2_version.h @@ -26,7 +26,7 @@ * @macro * Version number of the http2 module as c string */ -#define MOD_HTTP2_VERSION "1.5.6" +#define MOD_HTTP2_VERSION "1.5.7" /** * @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 0x010506 +#define MOD_HTTP2_VERSION_NUM 0x010507 #endif /* mod_h2_h2_version_h */