From 5bdb7b75121bf7dc37bfc3f81c59e44013e1c4de Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Wed, 10 Jan 2018 21:55:08 +0000 Subject: [PATCH] Merge r1818804, r1818951, r1818958, r1818960, r1819027, r1819214, r1820035 from trunk: mpm_event: close connections not reported as handled by any module. This avoids losing track of them and leaking scoreboard entries. PR 61551. mpm_event: follow up to r1818804. Address corner case where connection is aborted due to ap_run_pre_connection() failure, and update comment about ap_run_process_connection() expected return status and state. mpm_event: follow up to r1818804 and r1818951. Align comment and fix typos. mpm_event: follow up to r1818804. Allow DONE as a successful ap_run_process_connection() return value, for instance h2_conn_run() and h2_task_process_conn() uses it, third-party modules may too... mpm_event: follow up to r1818804 and r1818951. Be more correct in comment about CONN_STATE_WRITE_COMPLETION. We currently have/need no state to simply wait for readability on a socket, so the previous comment was misleading. Write completion can't be used for a simple "wait for read event and come back to process_connection hooks". mpm_event: follow up to r1818804 and r1818960. Align mod_http2 with expected returned state from process_connection hooks in async MPMs. When the master connection is handled, enter CONN_STATE_LINGER in any case. Add missing APLOGNO Submitted by: ylavic, jailletc36 Reviewed by: ylavic, icing, covener git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1820796 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 4 ++ modules/http2/h2_conn.c | 21 ++--------- modules/http2/h2_h2.c | 5 ++- modules/http2/h2_switch.c | 5 +-- server/mpm/event/event.c | 78 ++++++++++++++++++++++++++++++--------- 5 files changed, 73 insertions(+), 40 deletions(-) diff --git a/CHANGES b/CHANGES index 1d528ea7eb..f1fe3cc8a0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,10 @@ -*- coding: utf-8 -*- Changes with Apache 2.4.30 + *) mpm_event: close connections not reported as handled by any module to + avoid losing track of them and leaking scoreboard entries. PR 61551. + [Yann Ylavic] + *) core: A signal received while stopping could have crashed the main process. PR 61558. [Yann Ylavic] diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c index 53497d03ef..f37d951727 100644 --- a/modules/http2/h2_conn.c +++ b/modules/http2/h2_conn.c @@ -253,25 +253,12 @@ apr_status_t h2_conn_run(struct h2_ctx *ctx, conn_rec *c) } while (!async_mpm && c->keepalive == AP_CONN_KEEPALIVE && mpm_state != AP_MPMQ_STOPPING); - + if (c->cs) { - switch (session->state) { - case H2_SESSION_ST_INIT: - case H2_SESSION_ST_CLEANUP: - case H2_SESSION_ST_DONE: - case H2_SESSION_ST_IDLE: - c->cs->state = CONN_STATE_WRITE_COMPLETION; - break; - case H2_SESSION_ST_BUSY: - case H2_SESSION_ST_WAIT: - default: - c->cs->state = CONN_STATE_HANDLER; - break; - - } + c->cs->state = CONN_STATE_LINGER; } - - return DONE; + + return APR_SUCCESS; } apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c) diff --git a/modules/http2/h2_h2.c b/modules/http2/h2_h2.c index 8487e413f4..f206beb788 100644 --- a/modules/http2/h2_h2.c +++ b/modules/http2/h2_h2.c @@ -669,10 +669,11 @@ int h2_h2_process_conn(conn_rec* c) ap_log_cerror(APLOG_MARK, APLOG_TRACE1, status, c, "conn_setup"); if (status != APR_SUCCESS) { h2_ctx_clear(c); - return status; + return !OK; } } - return h2_conn_run(ctx, c); + h2_conn_run(ctx, c); + return OK; } ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, c, "h2_h2, declined"); diff --git a/modules/http2/h2_switch.c b/modules/http2/h2_switch.c index 3a8567eb1b..ef6ae29e08 100644 --- a/modules/http2/h2_switch.c +++ b/modules/http2/h2_switch.c @@ -185,13 +185,12 @@ static int h2_protocol_switch(conn_rec *c, request_rec *r, server_rec *s, ap_log_rerror(APLOG_MARK, APLOG_DEBUG, status, r, APLOGNO(03088) "session setup"); h2_ctx_clear(c); - return status; + return !OK; } h2_conn_run(ctx, c); - return DONE; } - return DONE; + return OK; } return DECLINED; diff --git a/server/mpm/event/event.c b/server/mpm/event/event.c index 2efde3982d..54c9256074 100644 --- a/server/mpm/event/event.c +++ b/server/mpm/event/event.c @@ -993,14 +993,12 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t * soc c->current_thread = thd; /* Subsequent request on a conn, and thread number is part of ID */ c->id = conn_id; - - if (c->aborted) { - cs->pub.state = CONN_STATE_LINGER; - } } - if (cs->pub.state == CONN_STATE_LINGER) { + rc = OK; + if (c->aborted || cs->pub.state == CONN_STATE_LINGER) { /* do lingering close below */ + cs->pub.state = CONN_STATE_LINGER; } else if (c->clogging_input_filters) { /* Since we have an input filter which 'clogs' the input stream, @@ -1010,20 +1008,54 @@ static void process_socket(apr_thread_t *thd, apr_pool_t * p, apr_socket_t * soc * otherwise write, should set the sense appropriately. */ apr_atomic_inc32(&clogged_count); - ap_run_process_connection(c); - if (cs->pub.state != CONN_STATE_SUSPENDED) { - cs->pub.state = CONN_STATE_LINGER; - } + rc = ap_run_process_connection(c); apr_atomic_dec32(&clogged_count); + if (rc == DONE) { + rc = OK; + } } else if (cs->pub.state == CONN_STATE_READ_REQUEST_LINE) { read_request: - ap_run_process_connection(c); - - /* state will be updated upon return - * fall thru to either wait for readability/timeout or - * do lingering close - */ + rc = ap_run_process_connection(c); + if (rc == DONE) { + rc = OK; + } + } + /* + * The process_connection hooks above should set the connection state + * appropriately upon return, for event MPM to either: + * - do lingering close (CONN_STATE_LINGER), + * - wait for readability of the next request with respect to the keepalive + * timeout (CONN_STATE_CHECK_REQUEST_LINE_READABLE), + * - keep flushing the output filters stack in nonblocking mode, and then + * if required wait for read/write-ability of the underlying socket with + * respect to its own timeout (CONN_STATE_WRITE_COMPLETION); since write + * completion at some point may require reads (e.g. SSL_ERROR_WANT_READ), + * an output filter can set the sense to CONN_SENSE_WANT_READ at any time + * for event MPM to do the right thing, + * - suspend the connection (SUSPENDED) such that it now interracts with + * the MPM through suspend/resume_connection() hooks, and/or registered + * poll callbacks (PT_USER), and/or registered timed callbacks triggered + * by timer events. + * If a process_connection hook returns an error or no hook sets the state + * to one of the above expected value, we forcibly close the connection w/ + * CONN_STATE_LINGER. This covers the cases where no process_connection + * hook executes (DECLINED), or one returns OK w/o touching the state (i.e. + * CONN_STATE_READ_REQUEST_LINE remains after the call) which can happen + * with third-party modules not updated to work specifically with event MPM + * while this was expected to do lingering close unconditionally with + * worker or prefork MPMs for instance. + */ + if (rc != OK || (cs->pub.state != CONN_STATE_LINGER + && cs->pub.state != CONN_STATE_WRITE_COMPLETION + && cs->pub.state != CONN_STATE_CHECK_REQUEST_LINE_READABLE + && cs->pub.state != CONN_STATE_SUSPENDED)) { + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, c, APLOGNO(10111) + "process_socket: connection processing %s: closing", + rc ? apr_psprintf(c->pool, "returned error %i", rc) + : apr_psprintf(c->pool, "unexpected state %i", + (int)cs->pub.state)); + cs->pub.state = CONN_STATE_LINGER; } if (cs->pub.state == CONN_STATE_WRITE_COMPLETION) { @@ -1046,10 +1078,20 @@ read_request: */ cs->queue_timestamp = apr_time_now(); notify_suspend(cs); - cs->pfd.reqevents = ( - cs->pub.sense == CONN_SENSE_WANT_READ ? APR_POLLIN : - APR_POLLOUT) | APR_POLLHUP | APR_POLLERR; + + if (cs->pub.sense == CONN_SENSE_WANT_READ) { + cs->pfd.reqevents = APR_POLLIN; + } + else { + cs->pfd.reqevents = APR_POLLOUT; + } + /* POLLHUP/ERR are usually returned event only (ignored here), but + * some pollset backends may require them in reqevents to do the + * right thing, so it shouldn't hurt. + */ + cs->pfd.reqevents |= APR_POLLHUP | APR_POLLERR; cs->pub.sense = CONN_SENSE_DEFAULT; + apr_thread_mutex_lock(timeout_mutex); TO_QUEUE_APPEND(cs->sc->wc_q, cs); rc = apr_pollset_add(event_pollset, &cs->pfd); -- 2.40.0