diff --git a/ChangeLog b/ChangeLog index 870ba5a9..3928cd9d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,10 @@ +v1.8.11 +-------------------------------------------------------------------------------- + * regression fix on bugzilla PR 59348, on graceful restart, ongoing + streams are finished normally before the final GOAWAY is sent. + * do not attempt to generated a response on an already aborted slave connection + * fix potential double cleanup of bucket beam memory + v1.8.10 -------------------------------------------------------------------------------- * fixes bugzilla PR60599, sending proper response for conditional requests diff --git a/configure.ac b/configure.ac index 7839081e..681a34cf 100644 --- a/configure.ac +++ b/configure.ac @@ -14,7 +14,7 @@ # AC_PREREQ([2.69]) -AC_INIT([mod_http2], [1.8.10], [stefan.eissing@greenbytes.de]) +AC_INIT([mod_http2], [1.8.11], [stefan.eissing@greenbytes.de]) LT_PREREQ([2.2.6]) LT_INIT() diff --git a/mod_http2/h2_bucket_beam.c b/mod_http2/h2_bucket_beam.c index 25c2187b..7c06e792 100644 --- a/mod_http2/h2_bucket_beam.c +++ b/mod_http2/h2_bucket_beam.c @@ -429,6 +429,25 @@ static apr_status_t beam_close(h2_bucket_beam *beam) static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool); static void beam_set_recv_pool(h2_bucket_beam *beam, apr_pool_t *pool); +static int pool_register(h2_bucket_beam *beam, apr_pool_t *pool, + apr_status_t (*cleanup)(void *)) +{ + if (pool && pool != beam->pool) { + apr_pool_pre_cleanup_register(pool, beam, cleanup); + return 1; + } + return 0; +} + +static int pool_kill(h2_bucket_beam *beam, apr_pool_t *pool, + apr_status_t (*cleanup)(void *)) { + if (pool && pool != beam->pool) { + apr_pool_cleanup_kill(pool, beam, cleanup); + return 1; + } + return 0; +} + static apr_status_t beam_recv_cleanup(void *data) { h2_bucket_beam *beam = data; @@ -440,16 +459,16 @@ static apr_status_t beam_recv_cleanup(void *data) static void beam_set_recv_pool(h2_bucket_beam *beam, apr_pool_t *pool) { - /* if the beam owner is the sender, monitor receiver pool lifetime */ - if (beam->owner == H2_BEAM_OWNER_SEND && beam->recv_pool != pool) { - if (beam->recv_pool) { - apr_pool_cleanup_kill(beam->recv_pool, beam, beam_recv_cleanup); - } - beam->recv_pool = pool; - if (beam->recv_pool) { - apr_pool_pre_cleanup_register(beam->recv_pool, beam, beam_recv_cleanup); - } + if (beam->recv_pool == pool || + (beam->recv_pool && pool + && apr_pool_is_ancestor(beam->recv_pool, pool))) { + /* when receiver same or sub-pool of existing, stick + * to the the pool we already have. */ + return; } + pool_kill(beam, beam->recv_pool, beam_recv_cleanup); + beam->recv_pool = pool; + pool_register(beam, beam->recv_pool, beam_recv_cleanup); } static apr_status_t beam_send_cleanup(void *data) @@ -473,65 +492,68 @@ static apr_status_t beam_send_cleanup(void *data) static void beam_set_send_pool(h2_bucket_beam *beam, apr_pool_t *pool) { - /* if the beam owner is the receiver, monitor sender pool lifetime */ - if (beam->owner == H2_BEAM_OWNER_RECV && beam->send_pool != pool) { - if (beam->send_pool && pool - && apr_pool_is_ancestor(beam->send_pool, pool)) { - /* when sender uses sub-pools to transmit data, stick - * to the lifetime of the pool we already have. */ - return; - } - if (beam->send_pool) { - apr_pool_cleanup_kill(beam->send_pool, beam, beam_send_cleanup); - } - beam->send_pool = pool; - if (beam->send_pool) { - apr_pool_pre_cleanup_register(beam->send_pool, beam, beam_send_cleanup); - } + if (beam->send_pool == pool || + (beam->send_pool && pool + && apr_pool_is_ancestor(beam->send_pool, pool))) { + /* when sender is same or sub-pool of existing, stick + * to the the pool we already have. */ + return; } + pool_kill(beam, beam->send_pool, beam_send_cleanup); + beam->send_pool = pool; + pool_register(beam, beam->send_pool, beam_send_cleanup); } static apr_status_t beam_cleanup(void *data) { h2_bucket_beam *beam = data; apr_status_t status = APR_SUCCESS; - /* owner of the beam is going away, depending on its role, cleanup - * strategies differ. */ - beam_close(beam); - switch (beam->owner) { - case H2_BEAM_OWNER_SEND: - status = beam_send_cleanup(beam); - beam->recv_buffer = NULL; + int safe_send = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_SEND); + int safe_recv = !beam->m_enter || (beam->owner == H2_BEAM_OWNER_RECV); + + /* + * Owner of the beam is going away, depending on which side it owns, + * cleanup strategies will differ with multi-thread protection + * still in place (beam->m_enter). + * + * In general, receiver holds references to memory from sender. + * Clean up receiver first, if safe, then cleanup sender, if safe. + */ + + /* When modify send is not safe, this means we still have multi-thread + * protection and the owner is receiving the buckets. If the sending + * side has not gone away, this means we could have dangling buckets + * in our lists that never get destroyed. This should not happen. */ + ap_assert(safe_send || !beam->send_pool); + if (!H2_BLIST_EMPTY(&beam->send_list)) { + ap_assert(beam->send_pool); + } + + if (safe_recv) { + if (beam->recv_pool) { + pool_kill(beam, beam->recv_pool, beam_recv_cleanup); beam->recv_pool = NULL; - break; - case H2_BEAM_OWNER_RECV: - if (beam->recv_buffer) { - apr_brigade_destroy(beam->recv_buffer); - } + } + if (beam->recv_buffer) { + apr_brigade_destroy(beam->recv_buffer); beam->recv_buffer = NULL; - beam->recv_pool = NULL; - if (!H2_BLIST_EMPTY(&beam->send_list)) { - ap_assert(beam->send_pool); - } - if (beam->send_pool) { - /* sender has not cleaned up, its pool still lives. - * this is normal if the sender uses cleanup via a bucket - * such as the BUCKET_EOR for requests. In that case, the - * beam should have lost its mutex protection, meaning - * it is no longer used multi-threaded and we can safely - * purge all remaining sender buckets. */ - apr_pool_cleanup_kill(beam->send_pool, beam, beam_send_cleanup); - ap_assert(!beam->m_enter); - beam_send_cleanup(beam); - } - ap_assert(H2_BPROXY_LIST_EMPTY(&beam->proxies)); - ap_assert(H2_BLIST_EMPTY(&beam->send_list)); - ap_assert(H2_BLIST_EMPTY(&beam->hold_list)); - ap_assert(H2_BLIST_EMPTY(&beam->purge_list)); - break; - default: - ap_assert(NULL); - break; + } + } + else { + beam->recv_buffer = NULL; + beam->recv_pool = NULL; + } + + if (safe_send && beam->send_pool) { + pool_kill(beam, beam->send_pool, beam_send_cleanup); + status = beam_send_cleanup(beam); + } + + if (safe_recv) { + ap_assert(H2_BPROXY_LIST_EMPTY(&beam->proxies)); + ap_assert(H2_BLIST_EMPTY(&beam->send_list)); + ap_assert(H2_BLIST_EMPTY(&beam->hold_list)); + ap_assert(H2_BLIST_EMPTY(&beam->purge_list)); } return status; } diff --git a/mod_http2/h2_from_h1.c b/mod_http2/h2_from_h1.c index d3f07a1b..a8f50987 100644 --- a/mod_http2/h2_from_h1.c +++ b/mod_http2/h2_from_h1.c @@ -517,7 +517,7 @@ apr_status_t h2_filter_headers_out(ap_filter_t *f, apr_bucket_brigade *bb) ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, f->c, "h2_task(%s): output_filter called", task->id); - if (!task->output.sent_response) { + if (!task->output.sent_response && !f->c->aborted) { /* check, if we need to send the response now. Until we actually * see a DATA bucket or some EOS/EOR, we do not do so. */ for (b = APR_BRIGADE_FIRST(bb); diff --git a/mod_http2/h2_session.c b/mod_http2/h2_session.c index 3dccdeec..565207b4 100644 --- a/mod_http2/h2_session.c +++ b/mod_http2/h2_session.c @@ -2252,8 +2252,8 @@ apr_status_t h2_session_process(h2_session *session, int async) } else if (APR_STATUS_IS_TIMEUP(status)) { /* go back to checking all inputs again */ - transit(session, "wait cycle", session->local.accepting? - H2_SESSION_ST_BUSY : H2_SESSION_ST_DONE); + transit(session, "wait cycle", session->local.shutdown? + H2_SESSION_ST_DONE : H2_SESSION_ST_BUSY); } else if (APR_STATUS_IS_ECONNRESET(status) || APR_STATUS_IS_ECONNABORTED(status)) { diff --git a/mod_http2/h2_version.h b/mod_http2/h2_version.h index e57a2ece..aee50c93 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.8.10-git" +#define MOD_HTTP2_VERSION "1.8.11-git" /** * @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 0x01080a +#define MOD_HTTP2_VERSION_NUM 0x01080b #endif /* mod_h2_h2_version_h */